Repository navigation
[scala-http4s-server] Use the JSON name for the discriminator and fail decoding on unknown values - #25308
Conversation
…l decoding on unknown values
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="modules/openapi-generator/src/main/resources/scala-http4s-server/types.mustache">
<violation number="1" location="modules/openapi-generator/src/main/resources/scala-http4s-server/types.mustache:56">
P2: The new JSON key interpolation still HTML-escapes raw discriminator names, so valid keys such as `a&b` become `a&b` in generated codecs. Use the generator’s JSON/Scala-string-safe raw interpolation for all discriminator key occurrences, including the mapped branch.</violation>
<violation number="2" location="modules/openapi-generator/src/main/resources/scala-http4s-server/types.mustache:67">
P3: The fallback `DecodingFailure` uses `cursor.history`, which is the parent object's history and omits the discriminator field. Use `cursor.downField("{{discriminator.propertyBaseName}}").history` (or the equivalent mapped-section name) so the error path points at the discriminator.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
View guided diff | Re-trigger cubic
| implicit val {{#lambda.camelcase}}{{classname}}{{/lambda.camelcase}}Encoder: Encoder[{{classname}}] = Encoder.instance { | ||
| {{#oneOf}} | ||
| case {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}: {{.}} => {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}.asJson.mapObject(("{{discriminator.propertyName}}" -> "{{.}}".asJson) +: _) | ||
| case {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}: {{.}} => {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}.asJson.mapObject(("{{discriminator.propertyBaseName}}" -> "{{.}}".asJson) +: _) |
There was a problem hiding this comment.
P2: The new JSON key interpolation still HTML-escapes raw discriminator names, so valid keys such as a&b become a&b in generated codecs. Use the generator’s JSON/Scala-string-safe raw interpolation for all discriminator key occurrences, including the mapped branch.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At modules/openapi-generator/src/main/resources/scala-http4s-server/types.mustache, line 56:
<comment>The new JSON key interpolation still HTML-escapes raw discriminator names, so valid keys such as `a&b` become `a&b` in generated codecs. Use the generator’s JSON/Scala-string-safe raw interpolation for all discriminator key occurrences, including the mapped branch.</comment>
<file context>
@@ -53,16 +53,18 @@ object {{classname}} {
implicit val {{#lambda.camelcase}}{{classname}}{{/lambda.camelcase}}Encoder: Encoder[{{classname}}] = Encoder.instance {
{{#oneOf}}
- case {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}: {{.}} => {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}.asJson.mapObject(("{{discriminator.propertyName}}" -> "{{.}}".asJson) +: _)
+ case {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}: {{.}} => {{#lambda.camelcase}}{{.}}{{/lambda.camelcase}}.asJson.mapObject(("{{discriminator.propertyBaseName}}" -> "{{.}}".asJson) +: _)
{{/oneOf}}
}
</file context>
There was a problem hiding this comment.
This applies to discriminator handling in general, not just this change. The client template (scala-http4s/model.mustache, #23853) uses the same {{discriminator.propertyBaseName}}, and both templates already HTML-escape the discriminator values ("{{.}}", "{{mappingName}}") the same way. Switching to triple braces wouldn't fix it either, since a " or \ in the name would then break the Scala string literal. A proper fix would escape keys and values as Scala string literals in both templates, which belongs in a separate PR. This one fixes the reserved-word case (type), which is the one that breaks decoding in practice.
| cursor.as[{{.}}] | ||
| {{/oneOf}} | ||
| case discriminatorValue => | ||
| Left(io.circe.DecodingFailure(s"Unknown discriminator value: $discriminatorValue", cursor.history)) |
There was a problem hiding this comment.
P3: The fallback DecodingFailure uses cursor.history, which is the parent object's history and omits the discriminator field. Use cursor.downField("{{discriminator.propertyBaseName}}").history (or the equivalent mapped-section name) so the error path points at the discriminator.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At modules/openapi-generator/src/main/resources/scala-http4s-server/types.mustache, line 67:
<comment>The fallback `DecodingFailure` uses `cursor.history`, which is the parent object's history and omits the discriminator field. Use `cursor.downField("{{discriminator.propertyBaseName}}").history` (or the equivalent mapped-section name) so the error path points at the discriminator.</comment>
<file context>
@@ -53,16 +53,18 @@ object {{classname}} {
cursor.as[{{.}}]
{{/oneOf}}
+ case discriminatorValue =>
+ Left(io.circe.DecodingFailure(s"Unknown discriminator value: $discriminatorValue", cursor.history))
}
}
</file context>
| Left(io.circe.DecodingFailure(s"Unknown discriminator value: $discriminatorValue", cursor.history)) | |
| Left(io.circe.DecodingFailure(s"Unknown discriminator value: $discriminatorValue", cursor.downField("{{discriminator.propertyBaseName}}").history)) |
There was a problem hiding this comment.
This is intentional: the default case matches the one in the client template (scala-http4s/model.mustache), which also uses cursor.history, so client and server report unknown discriminator values the same way. Pointing the history at the discriminator field would be a small improvement, but it should change both templates together, so I'd leave it for a follow-up.
|
tested locally and the fix (diff) looks good. thanks for the contribution. |
This fixes two problems with the discriminator codecs in
scala-http4s-server/types.mustache. Both branches are affected: with and without discriminator mapping.1. The JSON key is the Scala identifier instead of the property name. The template uses
propertyName. That's the Scala identifier, which is backtick-escaped for reserved words and then HTML-escaped by Mustache. With a discriminator ontype, the encoder writes the key`type`and the decoder looks it up withcursor.downField("`type`"), so the polymorphic decoders never match a spec-conformant body. The template now usespropertyBaseName. #23853 made the same change in thescala-http4sclient template.2. An unknown discriminator value throws a
MatchError. TheflatMapover the discriminator value has no default case, so an unexpected value throws, and the server returns a 500 instead of rejecting the request body. Both decoders now end with the same default case the client template uses:DecodingFailureis fully qualified so the sharedimport io.circe.{ Decoder, Encoder }stays unchanged for traits without a discriminator.Tests. I added
ScalaHttp4sServerCodegenTestwith a spec that has twooneOfunions discriminated ontype, one with mapping and one without. The test checks that the keys are"type"in the encoders and decoders, that no HTML-encoded backticks are left, and that both decoders have the default case. It fails on master and passes with this change. I also compiled the server generated from that spec with sbt.The petstore sample has no discriminator, so
./bin/generate-samples.sh bin/configs/scala-http4s-server.yamlproduces no changes.PR checklist
(Ran the
modules/openapi-generatorbuild with its Scala http4s codegen tests and regenerated thescala-http4s-serversample, the only one using this template. No generator options changed, so the generator docs are unaffected.)@clasnake (2017/07), @shijinkui (2018/01), @ramzimaalej (2018/03), @chameleon82 (2020/03), @Bouillie (2020/04) @Fish86 (2023/06)
Summary by cubic
Fixes two problems with discriminator codecs in
scala-http4s-server/types.mustache: the JSON key now uses the property name instead of the escaped Scala identifier, and unknown discriminator values return aDecodingFailureinstead of throwing aMatchError.propertyBaseName, so reserved words liketypeare no longer emitted as HTML-escaped backtick identifiers.DecodingFailure.ScalaHttp4sServerCodegenTestwith a spec covering discriminatedoneOfunions on the reserved wordtype, with and without mapping.Written for commit 0fb9214. Summary will update on new commits.