`rust-server-codegen`: add support for `@httpPayload` in requests #1060

david-perez · 2022-01-11T16:57:42Z

This commit adds support for the httpPayload Smithy trait when applied
to operation input members (request deserialization).

The code to deserialize HTTP-bound data from HTTP responses in
ResponseBindingGenerator.kt has been moved into a common class,
HttpBindingGenerator.kt, since it's useful for both clients and
servers in deserializing data from HTTP requests and responses,
respectively.

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

This commit adds support for the `httpPayload` Smithy trait when applied to operation input members (request deserialization). The code to deserialize HTTP-bound data from HTTP responses in `ResponseBindingGenerator.kt` has been moved into a common class, `HttpBindingGenerator.kt`, since it's useful for both clients and servers in deserializing data from HTTP requests and responses, respectively.

github-actions · 2022-01-11T17:04:52Z

A new doc preview is ready to view.

github-actions · 2022-01-11T17:06:55Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
Server Test (ignoring whitespace)

crisidev

Minor comments. LGTM.

crisidev · 2022-01-12T11:14:49Z

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt

+                val docShapeHandler: RustWriter.(String) -> Unit = { body ->
+                    rust(
+                        "#T($body)",
+                        structuredDataParser.documentParser(operationShape),
+                    )
+                }
+                val structureShapeHandler: RustWriter.(String) -> Unit = { body ->
+                    rust("#T($body)", structuredDataParser.payloadParser(binding.member))
+                }


Out of curiosity, do you really need to explicitly set the types here? We don't really do it anywhere..

You mean the type of the function RustWriter.(String) -> Unit? Type inference fails if I omit the signature.

Yep, that's what I meant. Thanks for thee clarification.

...n/software/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpProtocolGenerator.kt

crisidev · 2022-01-12T11:18:14Z

...in/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/http/HttpBindingGenerator.kt

+/**
+ * This class generates Rust functions that deserialize data from an HTTP message.
+ * They are useful for *both*:
+ *     - servers (that deserialize HTTP requests); and


Suggested change

* - servers (that deserialize HTTP requests); and

* - servers (that deserialize HTTP requests).

All these are sentences split across bullet points. The style is:

* item 1, * item 2, * ... * item n-1; and * item n.

where and can of course be any other grammatical conjunction.

Or:

* item 1, * item 2, * ... * item n-1; and * item n if the sentence continues past the bullet points and ends in the next line.

I don't think there's an agreed upon standard, but some formal / technical writing does use this style.

I don't have strong opinions on this, I just try to be consistent.

I have no strong opinions as well. It just feels weird to me, but not a blocker. Same for the rest below.

crisidev · 2022-01-12T11:20:21Z

...in/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/http/HttpBindingGenerator.kt

+/**
+ * The type of HTTP message from which we are deserializing the HTTP-bound data.
+ * We are either:
+ *     - deserializing data from an HTTP request (we are a server); or


Suggested change

* - deserializing data from an HTTP request (we are a server); or

* - deserializing data from an HTTP request (we are a server).

crisidev · 2022-01-12T11:21:49Z

...in/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/http/HttpBindingGenerator.kt

+ * For deserialization logic that is wholly different among clients and servers, use the classes:
+ *     - [ServerRequestBindingGenerator] from the `codegen-server` project for servers; and
+ *     - [ResponseBindingGenerator] from this project for clients


Suggested change

* For deserialization logic that is wholly different among clients and servers, use the classes:

* - [ServerRequestBindingGenerator] from the `codegen-server` project for servers; and

* - [ResponseBindingGenerator] from this project for clients

* For deserialization logic that is wholly different among clients and servers, use the classes:

* - [ServerRequestBindingGenerator] from the `codegen-server` project for servers.

* - [ResponseBindingGenerator] from this project for clients.

crisidev · 2022-01-12T11:23:44Z

...in/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/http/HttpBindingGenerator.kt

+     * Generate a function to deserialize [binding] from HTTP headers
+     *
+     * The name of the resulting function is returned as a String


Suggested change

* Generate a function to deserialize [binding] from HTTP headers

*

* The name of the resulting function is returned as a String

* Generates a function to deserialize [binding] from HTTP headers.

*

* The name of the resulting function is returned as a String.

I moved ResponseBindingGenerator.kt to HttpBindingGenerator.kt; all the existing comments use the imperative. I'll add the full stops though.

crisidev · 2022-01-12T11:26:19Z

...in/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/http/HttpBindingGenerator.kt

+        // of an empty instance of the response type.
+        withBlock("(!body.is_empty()).then(||{", "}).transpose()") {
+            when (targetShape) {
+                // TODO What if targetShape is set/map/list ?


This fails the CI: https://github.com/awslabs/smithy-rs/runs/4778596089?check_suite_focus=true#step:4:73

Ah, was reading the code and forgot to test that.

Turns out that if you try to target a set/map/list, you get:

ERROR: com.amazonaws.simple#HealthcheckInputRequest$payload (ProtocolHttpPayload) @ /local/home/davidpz/workplace/smithy-ws/src/SmithyRsSource/codegen-server-test/model/simple.smithy | 36 | payload: StringList | ^ = AWS Protocols do not support applying the httpPayload trait to members that target sets, lists, or maps.

Which I didn't know, but it's called out in the protocol e.g. restJson1. We only offer support for AWS protocols:

Projection simple failed: software.amazon.smithy.codegen.core.CodegenException: No matching protocol ? service offers: []. We offer: [aws.protocols#awsJson1_0, aws.protocols#awsJson1_1, aws.protocols#awsQuery, aws.protocols#ec2Query, aws.protocols#restJson1, aws.protocols#restXml]

So I'll just leave a comment documenting this fact.

github-actions · 2022-01-12T12:43:06Z

A new doc preview is ready to view.

github-actions · 2022-01-12T12:44:05Z

A new generated diff is ready to view.

github-actions · 2022-01-12T13:53:27Z

A new doc preview is ready to view.

crisidev

LGTM

github-actions · 2022-01-12T13:55:40Z

A new generated diff is ready to view.

github-actions · 2022-01-12T14:02:26Z

A new doc preview is ready to view.

github-actions · 2022-01-12T14:04:30Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
Server Test (ignoring whitespace)

github-actions · 2022-01-12T14:32:27Z

A new doc preview is ready to view.

github-actions · 2022-01-12T14:33:17Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
Server Test (ignoring whitespace)

rcoh

httpbinding refactoring lgtm

guymguym · 2022-01-16T22:31:07Z

Hi @david-perez
Is there any overlap with my PR #1023?
Should I change anything? Thanks

david-perez · 2022-01-17T13:13:21Z

@guymguym Ah yes, the logic for setting headers that you copied over in your PR should be moved to the class HttpBindingGenerator.kt this PR introduces, since it's useful for both clients and servers. That would resolve my comment here #1023 (comment)

I also need to set headers in responses, to resolve #1071 and #1075, so I'll merge this PR and work on moving header-setting to HttpBindingGenerator.kt so that you'd only have to rebase your PR. I'll start today.

github-actions · 2022-01-17T13:21:15Z

A new doc preview is ready to view.

github-actions · 2022-01-17T13:22:15Z

A new generated diff is ready to view.

No codegen difference in the AWS SDK
Server Test (ignoring whitespace)

david-perez requested review from a team as code owners January 11, 2022 16:57

crisidev approved these changes Jan 12, 2022

View reviewed changes

david-perez added 2 commits January 12, 2022 12:55

ktlint fixes

b3ae930

fix comment style

dc72717

Document why we don't support httpPayload applied to set/map/list shapes

9e64175

crisidev approved these changes Jan 12, 2022

View reviewed changes

Merge branch 'main' into davidpz-http-payload-requests

bddf634

Merge branch 'main' into davidpz-http-payload-requests

3015ae4

rcoh approved these changes Jan 12, 2022

View reviewed changes

david-perez added the server Rust server SDK label Jan 12, 2022

Merge branch 'main' into davidpz-http-payload-requests

7470071

david-perez merged commit 8524446 into main Jan 17, 2022

david-perez deleted the davidpz-http-payload-requests branch January 17, 2022 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`rust-server-codegen`: add support for `@httpPayload` in requests #1060

`rust-server-codegen`: add support for `@httpPayload` in requests #1060

david-perez commented Jan 11, 2022

github-actions bot commented Jan 11, 2022

github-actions bot commented Jan 11, 2022

crisidev left a comment

crisidev Jan 12, 2022

david-perez Jan 12, 2022

crisidev Jan 12, 2022

crisidev Jan 12, 2022

david-perez Jan 12, 2022

crisidev Jan 12, 2022

crisidev Jan 12, 2022

crisidev Jan 12, 2022

crisidev Jan 12, 2022

david-perez Jan 12, 2022

crisidev Jan 12, 2022

david-perez Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

crisidev left a comment

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

rcoh left a comment

guymguym commented Jan 16, 2022 •

edited

Loading

david-perez commented Jan 17, 2022

github-actions bot commented Jan 17, 2022

github-actions bot commented Jan 17, 2022

	* - servers (that deserialize HTTP requests); and
	* - servers (that deserialize HTTP requests).

	* - deserializing data from an HTTP request (we are a server); or
	* - deserializing data from an HTTP request (we are a server).

rust-server-codegen: add support for @httpPayload in requests #1060

rust-server-codegen: add support for @httpPayload in requests #1060

Conversation

david-perez commented Jan 11, 2022

github-actions bot commented Jan 11, 2022

github-actions bot commented Jan 11, 2022

crisidev left a 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

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

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

crisidev left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

rcoh left a comment

Choose a reason for hiding this comment

guymguym commented Jan 16, 2022 • edited Loading

david-perez commented Jan 17, 2022

github-actions bot commented Jan 17, 2022

github-actions bot commented Jan 17, 2022

`rust-server-codegen`: add support for `@httpPayload` in requests #1060

`rust-server-codegen`: add support for `@httpPayload` in requests #1060

guymguym commented Jan 16, 2022 •

edited

Loading