Skip to content

Improve complex serialization support #13

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Oct 31, 2019
Merged

Improve complex serialization support #13

merged 1 commit into from
Oct 31, 2019

Conversation

kstich
Copy link
Contributor

@kstich kstich commented Oct 30, 2019 •
edited
Loading

This commit adds support for many new complex serializations. This
includes support for HTTP payloads, the prefixHeaders trait, and
nested lists, sets, maps, unions, and structures as well as their
respective HTTP binding location modifications. It also generalizes
the method for generating serde function names across the language.

This commit relies upon the changes in smithy-lang/smithy#193 which have not been published yet. To use it, you'll need to run publishToMavenLocal on an updated version of Smithy.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@kstich kstich requested a review from mtdowling October 30, 2019 20:08
@kstich kstich force-pushed the serializer_updates branch from c3f82f0 to db67f47 Compare October 30, 2019 21:04
mtdowling
mtdowling requested changes Oct 30, 2019
View reviewed changes
@kstich kstich force-pushed the serializer_updates branch from db67f47 to a366ba5 Compare October 31, 2019 16:25
@kstich kstich requested a review from mtdowling October 31, 2019 16:25
mtdowling
mtdowling requested changes Oct 31, 2019
View reviewed changes
...java/software/amazon/smithy/typescript/codegen/integration/HttpBindingProtocolGenerator.java
operationIndex.getInput(operation)
.ifPresent(input -> serializingShapes.addAll(shapeWalker.walkShapes(input).stream()
// Don't generate a sub-serializer for the actual input shape.
.filter(s -> !input.equals(s))
Copy link
Member

Choose a reason for hiding this comment

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

What if the input shape is reused in the model and not just as input? Like a batch operation? Do we need something that makes it easy to know if an input or output shape is reused in other places in the model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the input shape is reused on a batch input, it will not be the explicit input shape of the batch operation and won't be filtered in that walk.

...java/software/amazon/smithy/typescript/codegen/integration/HttpBindingProtocolGenerator.java
// This should almost certainly be abstracted out to something else, because any protocol
// with a document body is going to need to generate sub-serializers.
// Generate the serializers for shapes within the operation closure.
serializingShapes.forEach(shape -> shape.accept(new ShapeVisitor.Default<Void>() {
Copy link
Member

Choose a reason for hiding this comment

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

This anonymous visit is a lot to put into a single method. Is this something that can be easily moved into an inner class (it could be non-static if that would be useful).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think that's part of getting this abstraction right. Unsure exactly what shape it's going to take so wanted to leave that to the author, even if that's me.

...java/software/amazon/smithy/typescript/codegen/integration/HttpBindingProtocolGenerator.java

@Override
public Void listShape(ListShape shape) {
// TODO Collection cleanup point
Copy link
Member

Choose a reason for hiding this comment

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

Is this able to handle lists of lists?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes:

list FooListList {
    member: FooList
}

list FooList {
    member: Foo
}

structure Foo {
   bar: String,
   baz: String
}

would generate serializeAws_restJson1_1FooListList, serializeAws_restJson1_1FooList, and serializeAws_restJson1_1Foo.

@kstich
Improve complex serialization support
f39ab60 
This commit adds support for many new complex serializations. This
includes support for HTTP payloads, the prefixHeaders trait, and
nested lists, sets, maps, unions, and structures as well as their
respective HTTP binding location modifications. It also generalizes
the method for generating serde function names across the language.
@kstich kstich force-pushed the serializer_updates branch from a366ba5 to f39ab60 Compare October 31, 2019 17:15
@kstich kstich requested a review from mtdowling October 31, 2019 17:15
@kstich kstich merged commit 3f14659 into develop Oct 31, 2019
@mtdowling mtdowling deleted the serializer_updates branch October 31, 2019 21:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtdowling mtdowling mtdowling approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kstich @mtdowling