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 all 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
15 changes: 7 additions & 8 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 @@ -14,17 +14,17 @@ trait SchemaMagnoliaDerivation {
type Typeclass[T] = Schema[T]

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)))
// Not using inherited annotations when generating type name, we don't want @encodedName to be inherited for types
Schema[T](schemaType = productSchemaType(ctx), name = Some(typeNameToSchemaName(ctx.typeName, ctx.annotations)))
}
enrichSchema(result, annotations)
enrichSchema(result, mergeAnnotations(ctx.annotations, ctx.inheritedAnnotations))
}
}

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
16 changes: 8 additions & 8 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 @@ -17,17 +17,17 @@ trait SchemaMagnoliaDerivation {
type Typeclass[T] = Schema[T]

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)))
// Not using inherited annotations when generating type name, we don't want @encodedName to be inherited for types
Schema[T](schemaType = productSchemaType(ctx), name = Some(typeNameToSchemaName(ctx.typeInfo, ctx.annotations)))
}
enrichSchema(result, annotations)
enrichSchema(result, mergeAnnotations(ctx.annotations, ctx.inheritedAnnotations))
}
}

Expand All @@ -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
21 changes: 20 additions & 1 deletion core/src/test/scala/sttp/tapir/SchemaMacroTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package sttp.tapir

import org.scalatest.Assertions
import org.scalatest.Inside
import org.scalatest.flatspec.AnyFlatSpec
import org.scalatest.matchers.should.Matchers
import org.scalatest.prop.TableDrivenPropertyChecks
Expand All @@ -11,7 +12,7 @@ import sttp.tapir.generic.Configuration
import sttp.tapir.generic.D
import sttp.tapir.generic.auto._

class SchemaMacroTest extends AnyFlatSpec with Matchers with TableDrivenPropertyChecks {
class SchemaMacroTest extends AnyFlatSpec with Matchers with TableDrivenPropertyChecks with Inside {
import SchemaMacroTestData._

behavior of "apply modification"
Expand Down Expand Up @@ -214,6 +215,24 @@ class SchemaMacroTest extends AnyFlatSpec with Matchers with TableDrivenProperty
)
}

it should "Not propagate type encodedName to subtypes of a sealed trait, but keep inheritance for fields" 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")
inside(child2Schema.schemaType.asInstanceOf[SProduct[Hericium.Botryoides]].fields.find(_.name.encodedName == "customCommonField")) {
case Some(field) =>
field.schema.name.map(_.fullName) shouldBe None
field.schema.description shouldBe Some("A common field")
}
}

it should "add discriminator based on a trait method" in {
val sUser = Schema.derived[User]
val sOrganization = Schema.derived[Organization]
Expand Down
15 changes: 15 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,19 @@ object SchemaMacroTestData {
case object B extends Letters
case object C extends Letters
}

@encodedName("CustomHericium")
sealed trait Hericium {
@encodedName("customCommonField")
@description("A common field")
val commonField: Int
}

object Hericium {

@encodedName("CustomErinaceus")
final case class Erinaceus(commonField: Int, e: String) extends Hericium
final case class Abietis(commonField: Int, a: Int) extends Hericium
final case class Botryoides(commonField: Int, b: Boolean) extends Hericium
}
}
8 changes: 6 additions & 2 deletions 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 @@ -94,7 +94,7 @@ private class SchemaDerivation(genericDerivationConfig: Expr[Configuration])(usi
finally deriveInProgress.remove(cacheKey)

private def typeNameToSchemaName(typeInfo: TypeInfo, annotations: Annotations): Expr[Schema.SName] =
val encodedName: Option[Expr[String]] = annotations.encodedName
val encodedName: Option[Expr[String]] = annotations.topLevelEncodedName

encodedName match
case None =>
Expand Down Expand Up @@ -151,7 +151,11 @@ 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 topLevelEncodedName: Option[Expr[String]] = findEncodedName(topLevel)

def encodedName: Option[Expr[String]] = findEncodedName(all)

private def findEncodedName(terms: List[Term]): Option[Expr[String]] = terms
.map(_.asExpr)
.collectFirst { case '{ $en: Schema.annotations.encodedName } => en }
.map(en => '{ $en.name })
Expand Down
25 changes: 22 additions & 3 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,16 +2,17 @@ package sttp.tapir.json.pickler

import org.scalatest.flatspec.AsyncFlatSpec
import org.scalatest.matchers.should.Matchers
import sttp.tapir.Schema.annotations._
import org.scalatest.Inside
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}

import java.math.{BigDecimal => JBigDecimal, BigInteger => JBigInteger}

class SchemaGenericAutoTest extends AsyncFlatSpec with Matchers {
class SchemaGenericAutoTest extends AsyncFlatSpec with Matchers with Inside {
import SchemaGenericAutoTest._

import generic.auto._
Expand Down Expand Up @@ -137,6 +138,24 @@ class SchemaGenericAutoTest extends AsyncFlatSpec with Matchers {
)(identity)
}

it should "Not propagate type encodedName to subtypes of a sealed trait, but keep inheritance for fields" 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")
inside(child2Schema.schemaType.asInstanceOf[SProduct[Hericium.Botryoides]].fields.find(_.name.encodedName == "customCommonField")) {
case Some(field) =>
field.schema.name.map(_.fullName) shouldBe None
field.schema.description shouldBe Some("A common field")
}
}

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