Fix enumeration does not handle customise annotation - #5481
Conversation
adamw
left a comment
There was a problem hiding this comment.
Automated review by Claude (via Claude Code). 7 inline comments, each prefixed with a severity. Nothing here is blocking on its own — use your judgement.
The main one is the hand-written copy in SchemaAnnotations: it changes type inference at call sites in a way the synthetic copy does not, verified against both 2.13 and 3.3.
One note that did not fit on a diff line: the -Yretain-trees addition at build.sbt:1263 mirrors the flag the enumeratum module already needs (build.sbt:707). So the Scala 3 requirement is pre-existing, but it means a user who writes @customise and compiles without -Yretain-trees gets the annotation silently ignored — worth documenting alongside this fix.
The versionedScalaJvmSourceDirectories change looks correct: docs/openapi-docs/src/test/scalajvm-3-2.13+ is the only such directory in the repo, and the change brings the helper in line with versionedScalaSourceDirectories.
| * `copy[T]()` in `withCustomise` can go back to `copy()`. | ||
| */ | ||
|
|
||
| def copy[T]( |
There was a problem hiding this comment.
🤖 Claude-generated review comment
high — This hand-written copy declares its own type parameter T, which shadows the class one. The compiler special-cases the synthetic case-class copy so its type parameter defaults to the class's; a hand-written one gets no such treatment, so T is inferred as Nothing whenever no argument mentions it and there is no expected type.
Checked with scala-cli on 3.3.4 and 2.13.16, same result on both:
val a: SchemaAnnotations[String] = ...
val b = a.copy(description = Some("y")) // synthetic copy: SchemaAnnotations[String]
// this copy: SchemaAnnotations[Nothing]
b.enrich(schemaForString) // Found: String, Required: NothingSchemaAnnotations is public API in sttp.tapir, so this is a silent source break for anyone calling .copy. MiMa does not catch it (binary signatures are unchanged, and MiMa is skipped for Scala 3).
Suggested fix: move _customise into the constructor parameter list and drop this copy override, accepting the binary break, rather than shadowing copy.
| .dependsOn(core, openapiDocs, tests % Test) | ||
|
|
||
| lazy val openapiDocs3 = openapiDocs.jvm(scala3).dependsOn() | ||
| lazy val openapiDocs3 = openapiDocs.jvm(scala3).dependsOn(enumeratum.jvm(scala3)) |
There was a problem hiding this comment.
🤖 Claude-generated review comment
medium — dependsOn without a configuration is a compile-scope dependency, so it lands in the published POM. This one exists only so a test can import sttp.tapir.codec.enumeratum._.
tapir-openapi-docs_3 1.11.50 has no enumeratum dependency today; after this change every Scala 3 user of tapir-openapi-docs transitively pulls tapir-enumeratum and enumeratum. (The 2.13 line already leaks this way, which is a pre-existing bug, not a precedent to follow.)
Fix: dependsOn(enumeratum.jvm(scala3) % Test).
| * sealed trait OrderStatus | ||
| * }}} | ||
| * | ||
| * `f` is applied after the metadata annotations ([[description]], [[encodedExample]], [[default]], [[format]], [[deprecated]], |
There was a problem hiding this comment.
🤖 Claude-generated review comment
medium — "applied after the metadata annotations ... so it can override what they set" is only true for SchemaAnnotations.enrich. It is false for the auto/magnolia path, which is the main documented use of @customise: SchemaMagnoliaDerivation.enrichSchema folds over annotations in declaration order.
@customise(_.description("a"))
@description("b")
case class Foo(x: Int)Schema.derived gives description "b"; derivedEnumeration/enumeratum gives "a".
Either correct the doc or make the two paths agree.
| * [[hidden]], [[encodedName]]), so it can override what they set. When deriving a schema for an enumeration, it is applied before | ||
| * [[validate]] and [[validateEach]] - a validator set inside `f` is therefore added to, not replaced by, those annotations. | ||
| */ | ||
| class customise(val f: Schema[Any] => Schema[Any]) extends StaticAnnotation with Serializable |
There was a problem hiding this comment.
🤖 Claude-generated review comment
medium — Changing the parameter from Schema[?] => Schema[?] to Schema[Any] => Schema[Any] is source-incompatible. Function1 is contravariant in its parameter and Schema is invariant, so Schema[?] is not a subtype of Schema[Any] and existing code stops compiling:
val f: Schema[?] => Schema[?] = ...
@customise(f) // no longer conforms
@customise((s: Schema[?]) => s.hidden(true)) // no longer conformsThe casts added in the derivation code would have worked with the old signature too. If the change is kept, it needs a release-note entry.
| sa => '{ ${ sa }.copy(validate = ${ Expr.ofList(allAnnArg(ValidateAnn).map(_.asExprOf[sttp.tapir.Validator[T]])) }) }, | ||
| sa => '{ ${ sa }.copy(validateEach = ${ Expr.ofList(allAnnArg(ValidateEachAnn).map(_.asExprOf[sttp.tapir.Validator[Any]])) }) } | ||
| sa => '{ ${ sa }.copy(validateEach = ${ Expr.ofList(allAnnArg(ValidateEachAnn).map(_.asExprOf[sttp.tapir.Validator[Any]])) }) }, | ||
| sa => firstAnnArg(CustomiseAnn).map(arg => '{ ${ sa }.withCustomise(${ arg.asExprOf[Schema[Any] => Schema[Any]] } ) }).getOrElse(sa) |
There was a problem hiding this comment.
🤖 Claude-generated review comment
low — firstAnnArg takes only the first @customise, and withCustomise overwrites rather than composes. The Scala 2 macro does the same (collectFirst, SchemaAnnotationsMacro.scala:45), but SchemaMagnoliaDerivation.enrichSchema folds over all of them.
So a type with two @customise annotations gets both applied under Schema.derived, and only the first under derivedEnumeration/enumeratum. Either collect all of them here and compose, or reject more than one.
| * When breaking binary compatibility would be an option, consider moving this into a regular parameter list with adjusting | ||
| * SchemaAnnotationsMacro(both scala 3 and 2) accordingly | ||
| */ | ||
| private var _customise: Option[Schema[Any] => Schema[Any]] = None |
There was a problem hiding this comment.
🤖 Claude-generated review comment
low — _customise is not a constructor parameter, so the generated equals, hashCode, toString, unapply and productIterator all ignore it. Two SchemaAnnotations differing only in the customise function are == with equal hash codes, and toString (what shows up in test failure output) says nothing about it. Anything caching or asserting on SchemaAnnotations treats customised and non-customised as identical.
Goes away if _customise becomes a real constructor parameter.
| val ValidateAnn = TypeTree.of[sttp.tapir.Schema.annotations.validate[_]].tpe | ||
| val ValidateEachAnn = TypeTree.of[sttp.tapir.Schema.annotations.validateEach[_]].tpe | ||
| val CustomiseAnn = TypeTree.of[sttp.tapir.Schema.annotations.customise].tpe | ||
|
|
There was a problem hiding this comment.
🤖 Claude-generated review comment
nit — Stray blank line with trailing whitespace left in the diff.
cc7162c to
889adde
Compare
Why I did it?
In order to test and fix a bug #4292
How I did it:
I prepared a new test case in VerifyYamlEnumerationTest with a user provided code
After that. I've extended SchemaAnnotations to handle customise annotation and related macros accordingly.
I've changed customise annotation param itself from Schema[?] => Schema[Any] in order to properly derive types
to avoid call site changes and keep api stable.