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.

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

merged 7 commits into from
Jan 2, 2024

Conversation

kciesielski
Copy link
Member

Fixes #3424

@kciesielski kciesielski marked this pull request as ready for review December 27, 2023 14:29
@kciesielski kciesielski requested a review from adamw December 27, 2023 14:29
@@ -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

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 :)

@adamw adamw merged commit 3081ce9 into master Jan 2, 2024
28 checks passed
@adamw adamw deleted the fix-3424-encoded-name branch January 2, 2024 11:23
@adamw
Copy link
Member

adamw commented Jan 2, 2024

Thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] encodedName of a root coproduct type is applied to sub-types
2 participants