add discriminated union trait #30

lewisjkl · 2022-01-06T00:04:53Z

No description provided.

kubukoz · 2022-01-06T02:22:03Z

modules/protocol/resources/META-INF/smithy/smithy4s.smithy

@@ -41,3 +41,7 @@ structure UncheckedExample {
 @trait(selector: "string")
 structure uuidFormat {
 }
+
+@trait(selector: "union")
+structure discriminated {


I think this should be parameterized by the field name. Personally I've seen "@type", "type", "kind", "node_kind" and so on, so it should probably be up to the API drafter :)

That is indeed correct. See "propertyName" in openapi : https://spec.openapis.org/oas/latest.html#discriminator-object.

I think I've seen type more than anything else, but making it explicit is probably a good call.

So it could be, for instance :

structure discriminated { propertyName: String }

We don't care much about the mapping thing from openapi. By default we'll use the labels of union members, and hopefully @jsonName will be made usable on union members and we'll be using that.

Baccata · 2022-01-07T08:40:03Z

modules/core/src/smithy4s/internals/SchematicDocumentDecoder.scala

+    val decoders: DecoderMap[S] =
+      (first +: rest).map { case alt @ Alt(_, instance, inject) =>
+        val encoder = { (pp: List[PayloadPath.Segment], doc: Document) =>
+          inject(instance.get.apply(jsonLabel(alt) :: pp, doc))


extract the jsonLabel call from here so that it ain't on the hot path.

Baccata · 2022-01-07T08:40:14Z

modules/core/src/smithy4s/internals/SchematicDocumentDecoder.scala

+        jsonLabel(alt) -> encoder
+      }.toMap
+
+    Hinted[DocumentDecoder].onHintOpt[Discriminated, S] {


Baccata · 2022-01-07T08:41:05Z

modules/core/src/smithy4s/internals/SchematicDocumentDecoder.scala

+          throw new PayloadError(
+            PayloadPath(pp.reverse),
+            "Union",
+            "Expected more than single-key Json object"


Not necessarily : the alternative could be an empty structure. The if (map.size > 1) is un-necessary

Ah yeah good call, I will update and add a test case to make sure that is handled

Baccata · 2022-01-07T08:42:13Z

modules/core/src/smithy4s/internals/SchematicDocumentDecoder.scala

+        case DObject(map) if (map.size > 1) =>
+          map
+            .get(discriminated.propertyName) match {
+            case Some(value: Document.DString) =>


Baccata · 2022-01-07T20:46:03Z

modules/json/src/smithy4s/http/json/SchematicJCodec.scala

+          else {
+            in.rollbackToken()
+            in.setMark()
+            val key = if (in.skipToKey(discriminated.propertyName)) {


oh really cool ! I thought we were gonna have to do that by hand, happy it's not the case

Yeah I was super happy to learn of setMark + skipToKey + rollbackToMark.. made the backtracking a lot easier

Yeah I was aware of setMark + rollbackToMark. Kudos to @plokhotnyuk, I really appreciate jsoniter's API.

Baccata · 2022-01-07T20:47:44Z

modules/json/src/smithy4s/http/json/SchematicJCodec.scala

      }
+
+      val altCache = new PolyFunction[Alt[JCodecMake, Z, *], JCodec] {


Baccata · 2022-01-07T21:07:06Z

The implementation of the schematics look good to me. Maybe we could do with some property tests generating random schemas that have the discriminated trait, but I'm okay with us doing that later.

There's two things that I'd like to have before merging :

Some smithy validator that checks that a union with the discriminated trait only has alternatives that are structures. Alternatively, a selector on the discriminated trait that achieves the same thing.
Should we make discriminated a string instead of a struct ? I'm thinking maybe making it a struct is overkill, because besides the name of the discriminator, I don't think any other property is relevant.

lewisjkl · 2022-01-07T22:00:25Z

Some smithy validator that checks that a union with the discriminated trait only has alternatives that are structures. Alternatively, a selector on the discriminated trait that achieves the same thing.

Agreed, thanks for the reminder.

Should we make discriminated a string instead of a struct ? I'm thinking maybe making it a struct is overkill, because besides the name of the discriminator, I don't think any other property is relevant.

Yeah, I can't think of anything else that would ever be specified in there. I will update it to be a string.

Baccata

👍 pre-approving (conditioned by the addition of a validator for the discriminator trait)

Baccata · 2022-01-10T17:11:00Z

modules/protocol/src/smithy4s/api/validation/DiscriminatedUnionValidator.java

+    public List<ValidationEvent> validate(Model model) {
+        return model.getShapesWithTrait(DiscriminatedUnionTrait.class).stream().flatMap(unionShape -> {
+           return unionShape.asUnionShape().get().getAllMembers().entrySet().stream().flatMap(entry -> {
+              System.out.println(model.getShape(entry.getValue().getTarget()));


Baccata · 2022-01-10T17:12:15Z

modules/protocol/src/smithy4s/api/validation/DiscriminatedUnionValidator.java

+              if (maybeTarget.isPresent() && maybeTarget.get().isStructureShape()) { // if not defined then shape won't be structure
+                  return Stream.empty();
+              } else {
+                  return Stream.of(error(entry.getValue(), String.format("Member shape corresponding to key '%s' is not a structure shape", entry.getKey())));


Suggested change

return Stream.of(error(entry.getValue(), String.format("Member shape corresponding to key '%s' is not a structure shape", entry.getKey())));

return Stream.of(error(entry.getValue(), String.format("Target of member '%s' is not a structure shape", entry.getKey())));

Baccata

re-approving

kubukoz

The only thing that I'm kind of missing is an example in a .smithy file, but otherwise it looks good

lewisjkl · 2022-01-10T17:53:21Z

The only thing that I'm kind of missing is an example in a .smithy file, but otherwise it looks good

Good call, I will add that

lewisjkl · 2022-01-10T20:08:22Z

One thing that we will want to look at (possibly in different PR) is the OpenAPI conversion respecting the discriminated union trait. I think there should be a discriminator field in the OpenAPI representation. (https://swagger.io/docs/specification/data-models/oneof-anyof-allof-not/)

Baccata · 2022-01-10T20:38:10Z

Happy for the openapi conversion to happen in a subsequent PR

kubukoz · 2022-01-11T09:51:17Z

🎉

* add `discriminated` trait, indicating that a union should be encoded as an object with a discriminator field. The field is defined by the string value taken by the trait * adds a validator to ensure that all members of a discriminated unions target structure shapes * updates Document codecs and Json codecs to take it in consideration * adds an example

add discriminated union trait

146dee0

kubukoz requested changes Jan 6, 2022

View reviewed changes

lewisjkl added 4 commits January 6, 2022 11:40

add propertyName to discriminated union

bf4e2e7

update SchematicDocumentEncoder to handle discriminated unions

c6f80ed

update SchematicDocumentDecoder to handle discriminated unions

534ef75

DRY up SchematicDocumentDecoder

06e550c

Baccata reviewed Jan 7, 2022

View reviewed changes

lewisjkl added 3 commits January 7, 2022 10:14

clean up hot path, handle empty structure alternatives

68f8de5

update SchematicJCodec to handle discriminated unions

6f101e6

update to make sure json name trait is respected

92e10cb

Baccata reviewed Jan 7, 2022

View reviewed changes

modules/json/src/smithy4s/http/json/SchematicJCodec.scala

}

val altCache = new PolyFunction[Alt[JCodecMake, Z, *], JCodec] {

Copy link

Contributor

Baccata Jan 7, 2022

Choose a reason for hiding this comment

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

👍

lewisjkl added 2 commits January 7, 2022 15:32

update discriminated to be string trait

f05de2f

Merge remote-tracking branch 'origin/main' into discriminated-unions

3afebfa

Baccata approved these changes Jan 10, 2022

View reviewed changes

add discriminated union validator to check member types

9236bd9

Baccata reviewed Jan 10, 2022

View reviewed changes

update error message; rm println

bc60f5d

Baccata approved these changes Jan 10, 2022

View reviewed changes

kubukoz approved these changes Jan 10, 2022

View reviewed changes

add discriminated smithy example

7292006

lewisjkl marked this pull request as ready for review January 10, 2022 20:08

Baccata merged commit 43f2a1e into main Jan 11, 2022

Baccata deleted the discriminated-unions branch January 11, 2022 09:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add discriminated union trait #30

add discriminated union trait #30

lewisjkl commented Jan 6, 2022

kubukoz Jan 6, 2022

Baccata Jan 6, 2022 •

edited

Loading

Baccata Jan 7, 2022

Baccata Jan 7, 2022

Baccata Jan 7, 2022 •

edited

Loading

lewisjkl Jan 7, 2022

Baccata Jan 7, 2022

Baccata Jan 7, 2022

lewisjkl Jan 7, 2022

Baccata Jan 7, 2022

Baccata Jan 7, 2022

Baccata commented Jan 7, 2022

lewisjkl commented Jan 7, 2022

Baccata left a comment •

edited

Loading

Baccata Jan 10, 2022

Baccata Jan 10, 2022

Baccata left a comment

kubukoz left a comment

lewisjkl commented Jan 10, 2022

lewisjkl commented Jan 10, 2022

Baccata commented Jan 10, 2022

kubukoz commented Jan 11, 2022

		}

		val altCache = new PolyFunction[Alt[JCodecMake, Z, *], JCodec] {

	return Stream.of(error(entry.getValue(), String.format("Member shape corresponding to key '%s' is not a structure shape", entry.getKey())));
	return Stream.of(error(entry.getValue(), String.format("Target of member '%s' is not a structure shape", entry.getKey())));

add discriminated union trait #30

add discriminated union trait #30

Conversation

lewisjkl commented Jan 6, 2022

Choose a reason for hiding this comment

Baccata Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata Jan 7, 2022 • 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

Choose a reason for hiding this comment

Baccata commented Jan 7, 2022

lewisjkl commented Jan 7, 2022

Baccata left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Baccata left a comment

Choose a reason for hiding this comment

kubukoz left a comment

Choose a reason for hiding this comment

lewisjkl commented Jan 10, 2022

lewisjkl commented Jan 10, 2022

Baccata commented Jan 10, 2022

kubukoz commented Jan 11, 2022

Baccata Jan 6, 2022 •

edited

Loading

Baccata Jan 7, 2022 •

edited

Loading

Baccata left a comment •

edited

Loading