Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[bugfix] Don't inherit encodedName #3430

Merged
merged 7 commits into from
Jan 2, 2024
Merged

[bugfix] Don't inherit encodedName #3430

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
6d265bb
Don't inherit encodedName
kciesielski Dec 27, 2023
e3eb05f
Test subschemas of parent schema as well
kciesielski Dec 27, 2023
a2f6ad1
Fix subtype schema name derivation for Scala 2
kciesielski Dec 27, 2023
0065e51
Add comments to clarify on how type annotations are not inherited for
kciesielski Dec 28, 2023
bed2a3a
Inherit encodedName for fields
kciesielski Dec 28, 2023
fabe301
Apply for Pickler as well
kciesielski Dec 28, 2023
2990258
Remove debug println
kciesielski Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 6 additions & 7 deletions core/src/main/scala-2/sttp/tapir/generic/auto/SchemaMagnoliaDerivation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,14 +15,14 @@ trait SchemaMagnoliaDerivation {

def join[T](ctx: ReadOnlyCaseClass[Schema, T])(implicit genericDerivationConfig: Configuration): Schema[T] = {
val annotations = mergeAnnotations(ctx.annotations, ctx.inheritedAnnotations)
withCache(ctx.typeName, annotations) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so ... we aren't using the merged annotations for caching and generating the schema name, but we are using them for other purposes? maybe add a comment to line 25 below that naming has to be explicitly declared as an annotation, and isn't inherited?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and that's only for names of types, not for fields, right? :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we are using them for other purposes (enrichSchema). I added a comment and also explicitly moved the mergedAnnotatations call to enrichSchema for better clarity.
However, good question about field names. With my changes they will also stop inheriting encodedName. How can one generate such a case? Adding a val to the parent trait doesn't put a field on a case class anyway.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm can't you have sth as follows?

sealed trait X {
  @encodedName("yy")
  val y: String
}

case class XX(y: String) extends X

I guess the encoded name should be inherited then

Copy link
Member Author

@kciesielski kciesielski Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I tried it with @description but it wasn't working at first, turns out I needed a clean 😁
I updated the PR to ensure it will work for @encodedName as well :)

withCache(ctx.typeName, ctx.annotations) {
val result =
if (ctx.isValueClass) {
require(ctx.parameters.nonEmpty, s"Cannot derive schema for generic value class: ${ctx.typeName.owner}")
val valueSchema = ctx.parameters.head.typeclass
Schema[T](schemaType = valueSchema.schemaType.asInstanceOf[SchemaType[T]], format = valueSchema.format)
} else {
Schema[T](schemaType = productSchemaType(ctx), name = Some(typeNameToSchemaName(ctx.typeName, annotations)))
Schema[T](schemaType = productSchemaType(ctx), name = Some(typeNameToSchemaName(ctx.typeName, ctx.annotations)))
}
enrichSchema(result, annotations)
}
Expand All @@ -33,7 +33,7 @@ trait SchemaMagnoliaDerivation {
ctx.parameters.map { p =>
val annotations = mergeAnnotations(p.annotations, p.inheritedAnnotations)
val pSchema = enrichSchema(p.typeclass, annotations)
val encodedName = getEncodedName(annotations).getOrElse(genericDerivationConfig.toEncodedName(p.label))
val encodedName = getEncodedName(p.annotations).getOrElse(genericDerivationConfig.toEncodedName(p.label))

SProductField[T, p.PType](FieldName(p.label, encodedName), pSchema, t => Some(p.dereference(t)))
}.toList
Expand All @@ -51,7 +51,7 @@ trait SchemaMagnoliaDerivation {
}

private def subtypeNameToSchemaName(subtype: Subtype[Typeclass, _]): Schema.SName =
typeNameToSchemaName(subtype.typeName, mergeAnnotations(subtype.annotations, subtype.inheritedAnnotations))
typeNameToSchemaName(subtype.typeName, subtype.annotations)

private def getEncodedName(annotations: Seq[Any]): Option[String] =
annotations.collectFirst { case ann: Schema.annotations.encodedName => ann.name }
Expand All @@ -73,8 +73,7 @@ trait SchemaMagnoliaDerivation {
}

def split[T](ctx: SealedTrait[Schema, T])(implicit genericDerivationConfig: Configuration): Schema[T] = {
val annotations = mergeAnnotations(ctx.annotations, ctx.inheritedAnnotations)
withCache(ctx.typeName, annotations) {
withCache(ctx.typeName, ctx.annotations) {
val subtypesByName =
ctx.subtypes
.map(s => subtypeNameToSchemaName(s) -> s.typeclass.asInstanceOf[Typeclass[T]])
Expand All @@ -101,7 +100,7 @@ trait SchemaMagnoliaDerivation {
)
case None => baseCoproduct
}
Schema(schemaType = coproduct, name = Some(typeNameToSchemaName(ctx.typeName, annotations)))
Schema(schemaType = coproduct, name = Some(typeNameToSchemaName(ctx.typeName, ctx.annotations)))
}
}

Expand Down
12 changes: 6 additions & 6 deletions core/src/main/scala-3/sttp/tapir/generic/auto/SchemaMagnoliaDerivation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@ trait SchemaMagnoliaDerivation {

override def join[T](ctx: CaseClass[Schema, T]): Schema[T] = {
val annotations = mergeAnnotations(ctx.annotations, ctx.inheritedAnnotations)
withCache(ctx.typeInfo, annotations) {
withCache(ctx.typeInfo, ctx.annotations) {
val result =
if (ctx.isValueClass) {
require(ctx.params.nonEmpty, s"Cannot derive schema for generic value class: ${ctx.typeInfo.owner}")
val valueSchema = ctx.params.head.typeclass
Schema[T](schemaType = valueSchema.schemaType.asInstanceOf[SchemaType[T]], format = valueSchema.format)
} else {
Schema[T](schemaType = productSchemaType(ctx), name = Some(typeNameToSchemaName(ctx.typeInfo, annotations)))
Schema[T](schemaType = productSchemaType(ctx), name = Some(typeNameToSchemaName(ctx.typeInfo, ctx.annotations)))
}
enrichSchema(result, annotations)
}
Expand Down Expand Up @@ -54,7 +54,7 @@ trait SchemaMagnoliaDerivation {
}

private def subtypeNameToSchemaName(subtype: SealedTrait.Subtype[Typeclass, _, ?]): Schema.SName =
typeNameToSchemaName(subtype.typeInfo, mergeAnnotations(subtype.annotations, subtype.inheritedAnnotations))
typeNameToSchemaName(subtype.typeInfo, subtype.annotations)

private def getEncodedName(annotations: Seq[Any]): Option[String] =
annotations.collectFirst { case ann: Schema.annotations.encodedName => ann.name }
Expand All @@ -77,11 +77,11 @@ trait SchemaMagnoliaDerivation {

override def split[T](ctx: SealedTrait[Schema, T]): Schema[T] = {
val annotations = mergeAnnotations(ctx.annotations, ctx.inheritedAnnotations)
withCache(ctx.typeInfo, annotations) {
withCache(ctx.typeInfo, ctx.annotations) {
val subtypesByName =
ctx.subtypes.toList
.map(s =>
typeNameToSchemaName(s.typeInfo, mergeAnnotations(s.annotations, s.inheritedAnnotations)) -> s.typeclass
typeNameToSchemaName(s.typeInfo, s.annotations) -> s.typeclass
.asInstanceOf[Typeclass[T]]
)
.toListMap
Expand All @@ -101,7 +101,7 @@ trait SchemaMagnoliaDerivation {
case None => baseCoproduct
}

Schema(schemaType = coproduct, name = Some(typeNameToSchemaName(ctx.typeInfo, annotations)))
Schema(schemaType = coproduct, name = Some(typeNameToSchemaName(ctx.typeInfo, ctx.annotations)))
}
}

Expand Down
13 changes: 13 additions & 0 deletions core/src/test/scala/sttp/tapir/SchemaMacroTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,6 +214,19 @@ class SchemaMacroTest extends AnyFlatSpec with Matchers with TableDrivenProperty
)
}

it should "Not propagate encodedName to subtypes of a sealed trait" in {
val parentSchema = Schema.derived[Hericium]
val child1Schema = Schema.derived[Hericium.Erinaceus]
val child2Schema = Schema.derived[Hericium.Botryoides]

parentSchema.name.map(_.fullName) shouldBe Some("CustomHericium")
parentSchema.schemaType.asInstanceOf[SCoproduct[Hericium]].subtypes.flatMap(_.name.map(_.fullName)) should contain allOf (
"sttp.tapir.SchemaMacroTestData.Hericium.Abietis", "sttp.tapir.SchemaMacroTestData.Hericium.Botryoides", "CustomErinaceus"
)
child1Schema.name.map(_.fullName) shouldBe Some("CustomErinaceus")
child2Schema.name.map(_.fullName) shouldBe Some("sttp.tapir.SchemaMacroTestData.Hericium.Botryoides")
}

it should "add discriminator based on a trait method" in {
val sUser = Schema.derived[User]
val sOrganization = Schema.derived[Organization]
Expand Down
11 changes: 11 additions & 0 deletions core/src/test/scala/sttp/tapir/SchemaMacroTestData.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,4 +62,15 @@ object SchemaMacroTestData {
case object B extends Letters
case object C extends Letters
}

@encodedName("CustomHericium")
sealed trait Hericium

object Hericium {

@encodedName("CustomErinaceus")
final case class Erinaceus(e: String) extends Hericium
final case class Abietis(a: Int) extends Hericium
final case class Botryoides(b: Boolean) extends Hericium
}
}
2 changes: 1 addition & 1 deletion json/pickler/src/main/scala/sttp/tapir/json/pickler/SchemaDerivation.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ private class SchemaDerivation(genericDerivationConfig: Expr[Configuration])(usi
// skip inherited annotations if defined at the top-level
topLevel ++ inherited.filterNot(i => topLevel.exists(t => t.tpe <:< i.tpe))

def encodedName: Option[Expr[String]] = all
def encodedName: Option[Expr[String]] = topLevel
.map(_.asExpr)
.collectFirst { case '{ $en: Schema.annotations.encodedName } => en }
.map(en => '{ $en.name })
Expand Down
17 changes: 15 additions & 2 deletions json/pickler/src/test/scala/sttp/tapir/json/pickler/SchemaDerivationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,9 @@ package sttp.tapir.json.pickler

import org.scalatest.flatspec.AsyncFlatSpec
import org.scalatest.matchers.should.Matchers
import sttp.tapir.Schema.annotations._
import sttp.tapir.Schema.annotations.*
import sttp.tapir.Schema.{SName, schemaForBoolean}
import sttp.tapir.SchemaMacroTestData.{Cat, Dog, Hamster, Pet}
import sttp.tapir.SchemaMacroTestData.*
import sttp.tapir.SchemaType._
import sttp.tapir.TestUtil.field
import sttp.tapir.{AttributeKey, FieldName, Schema, SchemaType, Validator}
Expand Down Expand Up @@ -137,6 +137,19 @@ class SchemaGenericAutoTest extends AsyncFlatSpec with Matchers {
)(identity)
}

it should "Not propagate encodedName to subtypes of a sealed trait" in {
val parentSchema = implicitlySchema[Hericium]
val child1Schema = implicitlySchema[Hericium.Erinaceus]
val child2Schema = implicitlySchema[Hericium.Botryoides]

parentSchema.name.map(_.fullName) shouldBe Some("CustomHericium")
parentSchema.schemaType.asInstanceOf[SCoproduct[Hericium]].subtypes.flatMap(_.name.map(_.fullName)) should contain allOf (
"sttp.tapir.SchemaMacroTestData.Hericium.Abietis", "sttp.tapir.SchemaMacroTestData.Hericium.Botryoides", "CustomErinaceus"
)
child1Schema.name.map(_.fullName) shouldBe Some("CustomErinaceus")
child2Schema.name.map(_.fullName) shouldBe Some("sttp.tapir.SchemaMacroTestData.Hericium.Botryoides")
}

ignore should "add meta-data to schema from annotations" in { // TODO https://github.com/softwaremill/tapir/issues/3167
val schema = implicitlySchema[I]
schema shouldBe Schema[I](
Expand Down