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

Bump smithy version to 1.52 #3887

Merged

landonxjames merged 14 commits into main from landonxjames/smithy152

Oct 24, 2024

Contributor

landonxjames commented Oct 18, 2024 •

edited

Loading

Motivation and Context

Bumping smithy to 1.52 for an httpChecksum trait bugfix

A few fixes had to be made to protocol tests for this release:

Update document to initialize as empty {} instead of null
Update our header serialization to serialize present but empty headers to "" (Note that this change does not apply to server header serialization)
Add accept: application/cbor header to add rpcV2Cbor requests
Skip two broken tests. These tests were fixed in Fix name of XML root nodes in tests smithy#2423 and that change was included in the 1.52 release notes, but the actual change did not make it into the build artifact. The changes should make it into the 1.52.1 release in the next few days: Bump version to 1.52.1 smithy#2428

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

landonxjames added 5 commits

October 18, 2024 15:56


          Bump smithy version to 1.52

19d76ca


          Merge branch 'main' into landonxjames/smithy152

200335b


          Fix null document in protocol test

e98d3ed

Also update ddb model in protocol tests to match current since it was
old and throwing various warnings.


          Revert ddb update, it broke something

894cf94


          Update header serialization to include empty headers

14ae7dd

landonxjames force-pushed the landonxjames/smithy152 branch from 2a7b98b to 14ae7dd Compare

October 20, 2024 04:40


          Add accept header to RpcV2Cbor

d43e580

landonxjames force-pushed the landonxjames/smithy152 branch from 81198f9 to d43e580 Compare

October 20, 2024 21:15

smithy-lang deleted a comment from github-actions bot

smithy-lang deleted a comment from github-actions bot

smithy-lang deleted a comment from github-actions bot

smithy-lang deleted a comment from github-actions bot

landonxjames force-pushed the landonxjames/smithy152 branch from f42809f to 423c8ba Compare

October 20, 2024 21:44

github-actions bot commented Oct 20, 2024

A new generated diff is ready to view.

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

A new doc preview is ready to view.


          Remove unnecessary reference in header serializer

7657f1b

landonxjames force-pushed the landonxjames/smithy152 branch from 46e5dd2 to 7657f1b Compare

October 20, 2024 23:14


          Update comment

f0bc029

github-actions bot commented Oct 20, 2024

A new generated diff is ready to view.

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

A new doc preview is ready to view.


          Add broken tests for smithy 1.52

7dcedeb

github-actions bot commented Oct 21, 2024

A new generated diff is ready to view.

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

A new doc preview is ready to view.

landonxjames added 3 commits

October 21, 2024 16:07


          Update header codegen to not user unnecessary ref

29413cc


          Separate empty header logic for client/server

037972f


          Merge branch 'main' into landonxjames/smithy152

47247ac

github-actions bot commented Oct 22, 2024

A new generated diff is ready to view.

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

A new doc preview is ready to view.

landonxjames marked this pull request as ready for review

October 22, 2024 20:55

landonxjames requested review from a team as code owners

October 22, 2024 20:55

landonxjames added the needs-server-review label

ysaito1001 reviewed

View reviewed changes

...tlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt Outdated Show resolved Hide resolved


          Merge branch 'main' into landonxjames/smithy152

f96ff63

github-actions bot commented Oct 23, 2024

A new generated diff is ready to view.

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

A new doc preview is ready to view.


          Update header serializiation with conditionalBlock

c2ecd75

github-actions bot commented Oct 23, 2024

A new generated diff is ready to view.

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

A new doc preview is ready to view.

ysaito1001 approved these changes

View reviewed changes

...tlin/software/amazon/smithy/rust/codegen/core/smithy/generators/http/HttpBindingGenerator.kt

Comment on lines +632 to +636

+                          // Empty vec in header is serialized as an empty string
+                          if ${context.valueExpression.name}.is_empty() {
+                              builder = builder.header("$headerName", "");
+                          } else {""",
+                          "}", conditional = serializeEmptyHeaders,

Contributor

ysaito1001 Oct 23, 2024

Nice!

david-perez approved these changes

View reviewed changes

...software/amazon/smithy/rust/codegen/client/smithy/generators/http/RequestBindingGenerator.kt

    
            @@ -91,7 +91,7 @@ class RequestBindingGenerator(
          
                   */

                  fun renderUpdateHttpBuilder(implBlockWriter: RustWriter) {

                      uriBase(implBlockWriter)

                      val addHeadersFn = httpBindingGenerator.generateAddHeadersFn(operationShape)

                      val addHeadersFn = httpBindingGenerator.generateAddHeadersFn(operationShape, serializeEmptyHeaders = true)

Contributor

david-perez Oct 24, 2024

Why does this only affect clients?

Contributor Author

landonxjames Oct 24, 2024 •

edited

Loading

The updated tests for this only apply to clients: https://github.com/smithy-lang/smithy/pull/2403/files

There are similar tests for the server, but they still indicate that empty headers should not be serialized by the server: https://github.com/smithy-lang/smithy/blob/main/smithy-aws-protocol-tests/model/restJson1/http-headers.smithy#L455

I asked internally about this discrepancy and there didn't seem to be much of a reason for it, so it might be updated in the future to align the two.

Contributor Author

landonxjames Oct 24, 2024

I made a PR to smithy to update the server tests to align: smithy-lang/smithy#2433

landonxjames added this pull request to the merge queue

landonxjames mentioned this pull request

Update NullAndEmptyHeadersServer tests for restJson1 and restXml smithy-lang/smithy#2433

Merged

Merged via the queue into main with commit 8d8e7ab

44 checks passed

landonxjames deleted the landonxjames/smithy152 branch

October 24, 2024 16:22

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-server-review