Added support for modeling union types. #3124

millems · 2022-03-24T23:10:00Z

Service models marked as a union type have additional helper methods to take advantage of the knowledge that just one member is expected to be set.

More information: https://github.com/aws/aws-sdk-java-v2/tree/master/docs/design/core/tagged-unions

dagnir · 2022-03-25T18:47:44Z

...gen/src/test/resources/software/amazon/awssdk/codegen/poet/model/alltypesunionstructure.java

+
+        private Type type = Type.UNKNOWN_TO_SDK_VERSION;
+
+        private Set<Type> setTypes = EnumSet.noneOf(Type.class);


Any reason we need to track multiple types? Spec requirement?

Wondering if we can't just do something like

private Object unionValue; private Type;

The AllTypesUnionStructure constructor would need to be changed as well I suppose

To support backwards-compatibility we'd need to support multiple fields being populated, and the SEP specifies that making an existing structure into a union is backwards-compatible.

It's true that things could be simplified if we made switching between union/structures backwards incompatible, because we could guarantee only one field is populated.

I see! I thought adding union to an existing structure would not be backwards compatible so thought this would a safe change.

Digging into the exact wording, I suppose it's less strict than that:

When adding tagged unions to existing SDKs, maintaining backward compatibility comes first. When existing SDKs start to see structure shapes marked with the union trait, they MUST continue to expose the shape as if it is a normal structure; however, if possible, these SDKs SHOULD also expose the shape through a tagged union abstraction. New SDKs or SDKs that are building a new major version do not have such limitations and are not required to expose tagged unions as a kind of subtype of a structure.

dagnir · 2022-03-25T18:51:44Z

...gen/src/test/resources/software/amazon/awssdk/codegen/poet/model/alltypesunionstructure.java

+
+        private AllTypesUnionStructure allTypesUnionStructure;
+
+        private Type type = Type.UNKNOWN_TO_SDK_VERSION;


Is there a use case for distinguishing between "type is unknown" and "no value set"?

Not to my knowledge. WDYT?

I can't really thing of a good use case for a specific "no value set" Type value, but I think intuitively, it might more sense to just set this to null if no members are set?

For responses, UNKNOWN_TO_SDK_VERSION actually makes sense for zero members because it likely means that the service responded with a type that doesn't exist in the union (so we populated nothing into the structure, because we didn't check that new field). Upgrading the SDK would likely fix the problem, because then the SDK would know about the new type.

For requests, it's a bit more murky, but there's no concept of 'request' and 'response' structures, so we can't meaningfully make a distinction.

More than one member being populated I could see being the thing we might want to differentiate on (return null for the type instead of UNKNOWN_TO_SDK_VERSION), but I didn't want the customer to have to worry about the two different cases for responses, because the "more than one" shouldn't happen.

Ah good point about forwards compatibility on response; though I think null type in that situation would still suffice no? Since null would mean none of the members are set.

For clarity I'm advocating for

null -> no member set

UNKNOWN_TO_SDK_VERSION -> multiple members set

We use UNKNOWN_TO_SDK_VERSION in enums already to mean "a member we're not aware of, but upgrading your SDK version might help with that", so we'd probably not want to use the same enum name to mean something different ("multiple members").

I like having an enum value to represent "a member we're not aware of", because switching on a null value doesn't work, and we want to ENCOURAGE customers to implement code to handle this case, not the other way around.

If we want to differentiate, we could do

UNKNOWN_TO_SDK_VERSION -> no member set

null -> multiple members set

That would preserve the existing meaning of "UNKNOWN_TO_SDK_VERSION" to be "the SDK doesn't know, but upgrading might help", and "null" to mean "the SDK doesn't know, and upgrading wouldn't help".

Again, all of this is about responses. For requests, I agree that these values are not optimal, but customers populated these values so it's less common that they'd be asserting on the type.

The only other option I can think of would be to let the SDK explicitly tell the union type "the service returned something I don't know" in the unmarshallers to meaningfully set UNKNOWN_TO_SDK_VERSION, and have every other case (0 members on requests, 2+ members on requests, 2+ members on response) have a null type().

The tricky part with that is that we'd then need the unmarshallers to be union-aware (they're currently not) and we'd need some method on the union type to be able to create one that is UNKNOWN_TO_SDK_VERSION. AttributeValue.unknownType()?

I like having an enum value to represent "a member we're not aware of", because switching on a null value doesn't work, and we want to ENCOURAGE customers to implement code to handle this case, not the other way around.

Great point! Yes, completely agree with this.

That would preserve the existing meaning of "UNKNOWN_TO_SDK_VERSION" to be "the SDK doesn't know, but upgrading might help", and "null" to mean "the SDK doesn't know, and upgrading wouldn't help".

Nice I like this change

The only other option I can think of would be to let the SDK explicitly tell the union type "the service returned something I don't know" in the unmarshallers to meaningfully set UNKNOWN_TO_SDK_VERSION, and have every other case (0 members on requests, 2+ members on requests, 2+ members on response) have a null type().

The marshaller would need to check for any other members that are not modeled which creates more work done in the marshalling step. I feel like you said, that UNKNOWN_TO_SDK_VERSION is a much more efficient solution for that.

dagnir · 2022-03-25T19:29:29Z

Are there any union related protocol tests from smithy that we can port to our tests?

millems · 2022-03-28T16:41:37Z

Are there any union related protocol tests from smithy that we can port to our tests?

The tests are just around making sure we can marshal and unmarshal unions. Those should be covered by our existing test suite, since unions are just structures with some extra helper methods. Do you think it's worth adding them anyway?

dagnir · 2022-03-28T17:03:27Z

The tests are just around making sure we can marshal and unmarshal unions. Those should be covered by our existing test suite, since unions are just structures with some extra helper methods. Do you think it's worth adding them anyway?

Nope in that case I don't think it's necessary.

Service models marked as a union type have additional helper methods to take advantage of the knowledge that just one member is expected to be set. More information: #3119

…ype() == UNKNOWN_TO_SDK_VERSION.

sonarqubecloud · 2022-03-28T20:25:43Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
8 Code Smells

97.5% Coverage
0.0% Duplication

millems requested a review from a team as a code owner March 24, 2022 23:10

millems force-pushed the millem/union-type-implementation branch from e9d8170 to 3e03931 Compare March 24, 2022 23:14

dagnir reviewed Mar 25, 2022

View reviewed changes

millems force-pushed the millem/union-type-implementation branch from 3e03931 to 32c0365 Compare March 28, 2022 16:55

millems added 2 commits March 28, 2022 11:56

Added support for modeling union types.

55a778d

Service models marked as a union type have additional helper methods to take advantage of the knowledge that just one member is expected to be set. More information: #3119

Addressed comments: two values set is now type() == null instead of t…

4effe63

…ype() == UNKNOWN_TO_SDK_VERSION.

millems force-pushed the millem/union-type-implementation branch from 32c0365 to 4effe63 Compare March 28, 2022 19:14

Merge branch 'master' into millem/union-type-implementation

efb4d71

dagnir approved these changes Mar 28, 2022

View reviewed changes

millems merged commit e2a4cc1 into master Mar 28, 2022

millems deleted the millem/union-type-implementation branch March 28, 2022 20:27


		private Type type = Type.UNKNOWN_TO_SDK_VERSION;

		private Set<Type> setTypes = EnumSet.noneOf(Type.class);


		private AllTypesUnionStructure allTypesUnionStructure;

		private Type type = Type.UNKNOWN_TO_SDK_VERSION;

Added support for modeling union types. #3124

Added support for modeling union types. #3124

Uh oh!

Conversation

millems commented Mar 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

millems Mar 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dagnir commented Mar 25, 2022

Uh oh!

millems commented Mar 28, 2022

Uh oh!

dagnir commented Mar 28, 2022

Uh oh!

sonarqubecloud bot commented Mar 28, 2022

Uh oh!

Uh oh!

millems commented Mar 24, 2022 •

edited

Loading

millems Mar 28, 2022 •

edited

Loading

millems Mar 28, 2022 •

edited

Loading