Rework Pickler for coproducts and enums #3222

kciesielski · 2023-10-03T09:29:36Z

This PR's main intention was to fix handling for enumerations which are represented by sealed hierarchies. However, it has become a wider rework for enums and coproducts, which introduces quite a few changes and fixes:

Enumerations are now understood as sealed hierarchies with case objects only, or native enum with parameterless cases. Both cases are encoded in the same way and have consistent schemas.
Instead of sttp.tapir.generic.Configuration Pickler API now uses sttp.tapir.pickler.PicklerConfiguration. It allows to have specific defaults different than Configuration.default (we'd like to have discriminator = "$type", not None as a default).
Coproduct schemas are consistent with PicklerConfiguration, which wasn't the case before.

adamw · 2023-10-03T12:12:58Z

json/pickler/src/test/scala/sttp/tapir/json/pickler/PicklerTest.scala

@@ -523,6 +502,21 @@ class PicklerTest extends AnyFlatSpec with Matchers {
    codec.decode(encoded) shouldBe Value(inputObj)
  }

+  it should "handle sealed hierarchies consisting of objects only" in {


maybe we should somehow separate tests per-feature, here: for enumerations (direct, wrapped, customised)

adamw · 2023-10-03T12:13:17Z

json/pickler/src/test/scala/sttp/tapir/json/pickler/Fixtures.scala

@@ -79,3 +79,14 @@ object Fixtures:
  }

  case class StatusResponse(status: Status)
+
+  case class SealedVariantContainer(v: SealedVariant)


maybe also introduce a test case with enums (parameterless and not)

(unless we already don't have one :) )

We have such tests

Parameterless enums

tapir/json/pickler/src/test/scala/sttp/tapir/json/pickler/PicklerTest.scala

Line 448 in 3d981a7

it should "support simple enums" in {

With parameters

tapir/json/pickler/src/test/scala/sttp/tapir/json/pickler/PicklerTest.scala

Line 478 in 3d981a7

it should "support sealed hierarchies looking like enums" in {

Ah ok, sorry, I didn't unwrap enough context :) But maybe if they will be in separate files it will be easier to see what's already there

(btw. open api docs tests are grouped in a similar way - each test has its own test classes in the companion object etc.)

adamw · 2023-10-03T12:17:37Z

it could also be good to explicitly mention in the documentation how we handle enumerations - how to customise, what's the default encoding (and when it is applied)

- The entire enum codec building and customization logic is now in CreateDerivedEnumerationPickler

adamw · 2023-10-04T10:42:03Z

doc/endpoint/pickler.md

@@ -87,9 +87,10 @@ import sttp.tapir.generic.Configuration
 given customConfiguration: Configuration = Configuration.default.withSnakeCaseMemberNames
 ```

-## Enums / sealed traits / coproducts
+## Sealed traits / coproducts


I guess we can keep the enum in the title, that's where I would look for info on how to serialize enums :)

adamw · 2023-10-04T10:42:31Z

doc/endpoint/pickler.md


-Pickler derivation for coproduct types (enums / sealed hierarchies) works automatically, by adding an `$type` discriminator field with the full class name. This is the default behavior of uPickle, but it can be overridden either by changing the discriminator field name, or by using custom logic to get field value from base trait.
+Pickler derivation for coproduct types (enums with parameters / sealed hierarchies) works automatically, by adding a `$type` discriminator field with the full class name. This is the default behavior of uPickle, but it can be overridden either by changing the discriminator field name, or by using custom logic to get field value from base trait.


Hm will that match the generated docs - that is, do we provide a pickler-specific Configuration which matches this behavior?

Good catch! This would almost certainly diverge in the schema, I'll add some tests and fixes accordingly.

@adamw I'm implementing a fix for this and it raises a question: Should we really encode/decode default discriminator with full type name? This would make us compatible with uPickle default logic.

{"field": {"$type": "my.app.Green", "intensity": 100}}

However, the implementation of Configuration.default is

implicit val default: Configuration = Configuration(Predef.identity, None, shortIdentitySubtypeTransformation)

so it expects shortIdentitySubtypeTransformation, like Green instead of my.app.Green.

I think it's more important that we are consistent with passed configuration, and follow with Green. If you agree, then here's another question: are we still consistent with passed configuration if its discriminator is None and we generate a $type field? I think we can treat this as OK, because None doesn't have to mean "no discriminator", and making it mean "use default" still makes sense.

I think that pickler has to provide its own configuration - setting the discriminator field to $type. Question then is, how to sensibly provide such an implicit which has higher priority than the default one (should be easy, I think almost everything has higher priority than companion object implicits), but also in a way that's overridable by the user with a custom configuration. Can this be done at all?

As for whether to use short or long names - that's a good question ;) I think we should pick a sensible default, not necessarily follow current tapir's or upickle defaults. Full names seem to expose too much implementation details (the package name) to me, so I think short names are the better option. As for the discriminator field, $type seems to be as good as any.

adamw · 2023-10-04T10:44:07Z

doc/endpoint/pickler.md

@@ -136,14 +137,23 @@ Schemas generated by picklers can be customized using annotations, just like wit

 ## Enumerations

-Scala 3 `enums`, where all cases are parameterless, are treated as an enumeration (not as a coproduct / sealed hierarchy). They are also automatically handled by `Pickler.derived[T]`: enum values are encoded as simple strings representing the type name. For example:
+Tapir schemas and JSON codecs treats following cases as "enumerations":


adamw · 2023-10-04T10:48:10Z

json/pickler/src/main/scala/sttp/tapir/json/pickler/CreateDerivedEnumerationPickler.scala

+      case _: EmptyTuple.type => Nil
+    }
+
+  /** Enumeration cases and case objects in an enumeration need special writers and readers, which are generated here, instead of being


thanks, much clearer now :)

kciesielski · 2023-10-05T13:17:29Z

Converting back to draft, as I'm reworking quite a lot around coproducts and configuration as well.

adamw · 2023-10-06T06:28:55Z

Done! :)

Rework handling for enumeration

9a8f84f

kciesielski added the Pickler label Oct 3, 2023

kciesielski requested a review from adamw October 3, 2023 10:46

kciesielski marked this pull request as ready for review October 3, 2023 10:46

kciesielski added a commit that referenced this pull request Oct 3, 2023

Apply changes from #3222

4a522b7

adamw reviewed Oct 3, 2023

View reviewed changes

kciesielski added 4 commits October 3, 2023 15:15

Remove EnumValueDiscriminator

0e8fad8

- The entire enum codec building and customization logic is now in CreateDerivedEnumerationPickler

Test signatures

b76356e

Split tests into smaller suites

4ca7130

Extend documentation about enumerations

718232a

kciesielski requested a review from adamw October 4, 2023 09:45

adamw reviewed Oct 4, 2023

View reviewed changes

kciesielski added 3 commits October 4, 2023 12:48

Restore section name

c0d32a2

Rework coproducts and configuration

5222bf3

Add missing file

f7be39b

kciesielski marked this pull request as draft October 5, 2023 13:17

kciesielski added 4 commits October 5, 2023 18:07

Adjust documentation

f3792c3

Organize imports

23384c9

Restore original file state

6368534

Refactor handling default discriminator in writers

7088450

kciesielski changed the title ~~Fix handling for case-object enumerations~~ Rework Pickler for coproducts and enums Oct 5, 2023

kciesielski marked this pull request as ready for review October 5, 2023 17:48

adamw merged commit 561c80f into master Oct 6, 2023
13 checks passed

mergify bot deleted the pickler-enum-case-objects branch October 6, 2023 06:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework Pickler for coproducts and enums #3222

Rework Pickler for coproducts and enums #3222

kciesielski commented Oct 3, 2023 •

edited

Loading

adamw Oct 3, 2023

adamw Oct 3, 2023

adamw Oct 3, 2023

kciesielski Oct 3, 2023

adamw Oct 3, 2023

adamw Oct 3, 2023

adamw commented Oct 3, 2023

adamw Oct 4, 2023

adamw Oct 4, 2023

kciesielski Oct 4, 2023

kciesielski Oct 4, 2023

adamw Oct 4, 2023

adamw Oct 4, 2023

adamw Oct 4, 2023

kciesielski commented Oct 5, 2023

adamw commented Oct 6, 2023


		Pickler derivation for coproduct types (enums / sealed hierarchies) works automatically, by adding an `$type` discriminator field with the full class name. This is the default behavior of uPickle, but it can be overridden either by changing the discriminator field name, or by using custom logic to get field value from base trait.
		Pickler derivation for coproduct types (enums with parameters / sealed hierarchies) works automatically, by adding a `$type` discriminator field with the full class name. This is the default behavior of uPickle, but it can be overridden either by changing the discriminator field name, or by using custom logic to get field value from base trait.

Rework Pickler for coproducts and enums #3222

Rework Pickler for coproducts and enums #3222

Conversation

kciesielski commented Oct 3, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamw commented Oct 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kciesielski commented Oct 5, 2023

adamw commented Oct 6, 2023

kciesielski commented Oct 3, 2023 •

edited

Loading