Upgrade Smithy to 1.16.1 #1053

jdisanti · 2022-01-08T02:49:24Z

Description

This PR upgrades Smithy from 1.13 to 1.16.1.

Smithy change log here: https://github.com/awslabs/smithy/blob/main/CHANGELOG.md

Biggest changes:

Entries in string list headers can now be quoted so that they can contain commas and escaped quotation marks
The AWS JSON protocols no longer support @jsonName

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates

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

…-smithy-116

github-actions · 2022-01-08T02:56:38Z

A new doc preview is ready to view.

github-actions · 2022-01-08T02:58:38Z

A new generated diff is ready to view.

github-actions · 2022-01-08T02:59:42Z

A new generated diff is ready to view.

github-actions · 2022-01-08T03:00:04Z

A new doc preview is ready to view.

github-actions · 2022-01-08T03:00:45Z

A new doc preview is ready to view.

github-actions · 2022-01-08T03:02:17Z

A new generated diff is ready to view.

github-actions · 2022-01-08T03:02:37Z

A new doc preview is ready to view.

github-actions · 2022-01-08T03:04:55Z

A new generated diff is ready to view.

rcoh

Overall looks great! I think we may need more tests on the quoted header parsing

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

rust-runtime/aws-smithy-http/src/header.rs

…-smithy-116

github-actions · 2022-01-11T23:02:09Z

A new doc preview is ready to view.

github-actions · 2022-01-11T23:05:21Z

A new generated diff is ready to view.

github-actions · 2022-01-12T01:18:59Z

A new doc preview is ready to view.

github-actions · 2022-01-12T01:20:47Z

A new generated diff is ready to view.

david-perez · 2022-01-12T13:41:29Z

There are 5 protocol test failures for the server.

2 of them are for missing required header: <header> (example).
3 of them are for RequestExtensions not found (example).

The 5 tests are new in Smithy 1.16 and are not regressions i.e. had they been in the current Smithy version we're using, the server would still have failed. The server is doing poorly at protocol tests; I'm working on reducing the failures.

(1) is due to us not generating the tests correctly, we're not inserting the input headers to construct the request. Adding them should make many more tests pass.

(2) is due to us only inserting RequestExtensions in responses for fallible operations (link). We should add them to all responses and rename the struct to ResponseExtensions, since we're adding them only to responses.

I've created GitHub issues to track these and I will be tackling them shortly:

Since these fixes are semantically unrelated to this PR, I suggest I fix them in separate PRs. To unblock merging this PR, you can add this patch:

diff --git a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt
index aad4fdc1..0afb1004 100644
--- a/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt
+++ b/codegen-server/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/protocol/ServerProtocolTestGenerator.kt
@@ -431,6 +431,11 @@ class ServerProtocolTestGenerator(
         private val AwsQuery = "aws.protocoltests.query#AwsQuery"
         private val Ec2Query = "aws.protocoltests.ec2#AwsEc2"
         private val ExpectFail = setOf<FailingTest>(
+            FailingTest(RestJson, "RestJsonInputAndOutputWithQuotedStringHeaders", Action.Request),
+            FailingTest(RestJson, "RestJsonInputAndOutputWithQuotedStringHeaders", Action.Response),
+            FailingTest(RestJson, "RestJsonOutputUnionWithUnitMember", Action.Response),
+            FailingTest(RestJson, "RestJsonUnitInputAllowsAccept", Action.Request),
+            FailingTest(RestJson, "RestJsonUnitInputAndOutputNoOutput", Action.Response),
             FailingTest(RestJson, "RestJsonAllQueryStringTypes", Action.Request),
             FailingTest(RestJson, "RestJsonQueryStringEscaping", Action.Request),
             FailingTest(RestJson, "RestJsonSupportsNaNFloatQueryValues", Action.Request),

rcoh

LGTM! might be worth fuzzing the many_dates stuff while we're there but not urgent

rcoh · 2022-01-12T15:28:27Z

rust-runtime/aws-smithy-http/src/header.rs

                    if inner.contains("\\\"") {
                        inner = Cow::Owned(inner.replace("\\\"", "\""));
                    }
-                    let rest = then_delim(&input[(index + 1)..])?;
+                    if inner.contains("\\\\") {
+                        inner = Cow::Owned(inner.replace("\\\\", "\\"));
+                    }
+                    let rest = then_comma(&input[(index + 1)..])?;


could factor out fn replace(cow, pat, pat) -> cow

rcoh · 2022-01-12T15:30:36Z

rust-runtime/aws-smithy-http/src/header.rs

    fn read_many_strings() {
        let test_request = http::Request::builder()
            .header("Empty", "")
-            .header("Foo", "foo")
+            .header("Foo", "  foo")
+            .header("FooInQuotes", "\"  foo\"")
            .header("CommaInQuotes", "\"foo,bar\",baz")
            .header("QuoteInQuotes", "\"foo\\\",bar\",\"\\\"asdf\\\"\",baz")
+            .header(
+                "QuoteInQuotesWithSpaces",
+                "\"foo\\\",bar\", \"\\\"asdf\\\"\", baz",
+            )
            .header("JunkFollowingQuotes", "\"\\\"asdf\\\"\"baz")
            .header("EmptyQuotes", "\"\",baz")
+            .header("EscapedSlashesInQuotes", "foo, \"(foo\\\\bar)\"")
            .body(())
            .unwrap();
+        let read =


should probably also hit trailing whitespace

rcoh · 2022-01-12T15:32:33Z

rust-runtime/aws-smithy-http/fuzz/fuzz_targets/read_many_from_str.rs

+
+fuzz_target!(|data: &[u8]| {
+    if let Ok(s) = std::str::from_utf8(data) {
+        if let Ok(req) = http::Request::builder().header("test", s).body(()) {


does this ever return OK? you can make a request without a URI?

Yeah, the builder builds successfully. I verified by adding a panic! inside of the conditions, and it definitely gets triggered.

rcoh · 2022-01-12T15:32:59Z

rust-runtime/aws-smithy-http/fuzz/fuzz_targets/read_many_from_str.rs

@@ -1,3 +1,8 @@
+/*


hooray for lints

crisidev

Looks good from the server codegen perspective.

…-smithy-116

Upgrade Smithy to 1.16.1

d8b33c9

jdisanti requested review from a team as code owners January 8, 2022 02:49

jdisanti added 4 commits January 8, 2022 02:50

Update changelog

50b469e

Merge remote-tracking branch 'upstream/main' into jdisanti-upgrade-to…

41a721b

…-smithy-116

Fix ktlint

98bf201

Fix clippy

d8712e6

rcoh reviewed Jan 10, 2022

View reviewed changes

rust-runtime/aws-smithy-http/src/header.rs Outdated Show resolved Hide resolved

rcoh reviewed Jan 10, 2022

View reviewed changes

rust-runtime/aws-smithy-http/src/header.rs Outdated Show resolved Hide resolved

jdisanti added 3 commits January 11, 2022 18:30

Fix header quoting issues

02df022

Add fuzz test for read_many_from_str

d5b30de

Merge remote-tracking branch 'upstream/main' into jdisanti-upgrade-to…

be45634

…-smithy-116

Fix codegen integration tests

d9f55cf

rcoh approved these changes Jan 12, 2022

View reviewed changes

crisidev approved these changes Jan 12, 2022

View reviewed changes

Incorporate feedback

ad08f55

Merge remote-tracking branch 'upstream/main' into jdisanti-upgrade-to…

230b37c

…-smithy-116

jdisanti merged commit c25c24d into main Jan 12, 2022

jdisanti deleted the jdisanti-upgrade-to-smithy-116 branch January 12, 2022 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade Smithy to 1.16.1 #1053

Upgrade Smithy to 1.16.1 #1053

jdisanti commented Jan 8, 2022 •

edited

Loading

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

rcoh left a comment

github-actions bot commented Jan 11, 2022

github-actions bot commented Jan 11, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

david-perez commented Jan 12, 2022

rcoh left a comment

rcoh Jan 12, 2022

rcoh Jan 12, 2022

rcoh Jan 12, 2022

jdisanti Jan 12, 2022 •

edited

Loading

rcoh Jan 12, 2022

crisidev left a comment

Upgrade Smithy to 1.16.1 #1053

Upgrade Smithy to 1.16.1 #1053

Conversation

jdisanti commented Jan 8, 2022 • edited Loading

Description

Checklist

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

github-actions bot commented Jan 8, 2022

rcoh left a comment

Choose a reason for hiding this comment

github-actions bot commented Jan 11, 2022

github-actions bot commented Jan 11, 2022

github-actions bot commented Jan 12, 2022

github-actions bot commented Jan 12, 2022

david-perez commented Jan 12, 2022

rcoh left a comment

Choose a reason for hiding this comment

rcoh Jan 12, 2022

Choose a reason for hiding this comment

rcoh Jan 12, 2022

Choose a reason for hiding this comment

rcoh Jan 12, 2022

Choose a reason for hiding this comment

jdisanti Jan 12, 2022 • edited Loading

Choose a reason for hiding this comment

rcoh Jan 12, 2022

Choose a reason for hiding this comment

crisidev left a comment

Choose a reason for hiding this comment

jdisanti commented Jan 8, 2022 •

edited

Loading

jdisanti Jan 12, 2022 •

edited

Loading