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

add discriminatorMapping when deriving a schema for co-products #1838

Merged

Conversation

yurique
Copy link
Contributor

@yurique yurique commented Feb 7, 2022

Without discriminatorMapping the OpenAPI generator can't make sense of the schema and fails.


I'm not sure if adding fields to Configuration was a right call – this breaks bin-compat, but I can re-work things here to re-use toEncodedName (and deal with the $ suffix on the call site).

@adamw
Copy link
Member

adamw commented Feb 14, 2022

Could you make a PR against master? This should also fix problems with scala3 compilation


final case class Configuration(toEncodedName: String => String, discriminator: Option[String]) {
final case class Configuration(toEncodedName: String => String, discriminator: Option[String], toEncodedSubtypeName: TypeName => String) {
Copy link
Member

Choose a reason for hiding this comment

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

I think here we'd need to avoid depending on magnolia's class, as this is kind of an implementation detail. Maybe it would make sense to use SName => String here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Definitely! Also this is probably the reason CI failed here.

@@ -244,7 +244,15 @@ class SchemaGenericAutoTest extends AsyncFlatSpec with Matchers {
)
)

schemaType.asInstanceOf[SCoproduct[Entity]].discriminator shouldBe Some(SDiscriminator(FieldName("who_am_i"), Map.empty))
Copy link
Member

Choose a reason for hiding this comment

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

would be great to add a test with non-identity subtype name transformations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do!

@yurique
Copy link
Contributor Author

yurique commented Feb 14, 2022

Could you make a PR against master?

Sure! (I was secretly hoping for this to make into 0.19.x :) but we can wait for 0.20.x)

@adamw
Copy link
Member

adamw commented Feb 14, 2022

@yurique since this is binary incompatible, 0.20 is a better target :)

@yurique
Copy link
Contributor Author

yurique commented Feb 14, 2022

0.20 is a better target :)

Totally makes sense :)
I'll get to this a bit later (hopefully today).

@yurique yurique force-pushed the feature/discriminator-mapping-for-coproducts branch from 833f43f to 0a8962e Compare February 14, 2022 18:23
@yurique yurique changed the base branch from 0.19 to master February 14, 2022 18:24
@yurique
Copy link
Contributor Author

yurique commented Feb 14, 2022

This now targets master and:

  • added tests for the sub-type name transformations (added more of those)
  • configuration doesn't refer magnolia anymore

@yurique
Copy link
Contributor Author

yurique commented Feb 14, 2022

Not sure why CI is not kicking in.

@adamw
Copy link
Member

adamw commented Feb 15, 2022

Thanks! Two more things:

  • I'm not sure if toEncodedSubtypeName is the best name. What this function is doing in fact, is giving discriminator field values, given the name of the subtype. Maybe toDiscriminatorValue would be more appropriate?
  • we're missing an implementation for Scala3

@adamw
Copy link
Member

adamw commented Feb 15, 2022

(no idea why CI didn't run)

* rename toEncodedSubtypeName -> toDiscriminatorValue
@yurique
Copy link
Contributor Author

yurique commented Feb 15, 2022

@adamw

Maybe toDiscriminatorValue would be more appropriate?

I like it. Done.

we're missing an implementation for Scala3

Done.

btw, what's the role of sttp.tapir.macros.SchemaMacros? Does it need updating as well?

@adamw adamw merged commit 0b3364c into softwaremill:master Feb 15, 2022
@adamw
Copy link
Member

adamw commented Feb 15, 2022

Thanks! :)

@adamw
Copy link
Member

adamw commented Feb 15, 2022

SchemaMacros are version-specific extensions for Schema, here only the modify function, I think they are not impacted

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.

2 participants