Unknown enum variants removed from server #1398

82marbag · 2022-05-18T13:21:09Z

The server must have the most up to date variants and the unknown enum
variant should not be used. Clients are generated with it because they
might not have the most recent model and the server might return
an unknown variant to them.

Closes #1187

Signed-off-by: Daniele Ahmed [email protected]

Motivation and Context

Closes #1187

Description

Remove unknown in server
Implement TryFrom and the relevant Errors for enums
Refactor the code to use the server/client generators
Added and refactored tests

Testing

./gradlew :codegen-server-test:test

Checklist

I have updated CHANGELOG.next.toml if I made changes to the smithy-rs codegen or runtime crates
I have updated CHANGELOG.next.toml if I made changes to the AWS SDK, generated SDK code, or SDK 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.

github-actions · 2022-05-18T14:05:32Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	1.01%	81774.22	80952.92
Total requests	1.07%	7366691	7288801
Total errors	NaN%	0	0
Total successes	1.07%	7366691	7288801
Average latency ms	-2.44%	0.8	0.82
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-4.76%	24.42	25.64
Stdev latency ms	-5.63%	1.34	1.42
Transfer Mb	1.07%	765.77	757.67
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

crisidev · 2022-05-18T15:29:56Z

CHANGELOG.next.toml

+[[smithy-rs]]
+message = "Remove Unknown variant from enums in server"
+references = ["smithy-rs#1187"]
+meta = { "breaking" = true, "tada" = false, "bug" = false }
+author = "82marbag"


We do not publish CHANGELOG entries for server only changes. If this PR does not change how the client SDK is generated, this should go away.

crisidev · 2022-05-18T15:38:58Z

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/EnumGenerator.kt

+            if (mode == CodegenMode.Client) {
+                docs("$UnknownVariant contains new variants that have been added since this code was generated.")
+                write("$UnknownVariant(String)")
+            }


Can't this be abstracted completely inside the ServerEnumGenerator rather than having an if guard here?

Yes, I understand. They are here because the client generates the same code as the server, plus some more. The one where the server is generating actual different code is in the server only. But not to copy/paste code around, I generate the common code in the client and the additional code in the client. Moving to ServerEnumGenerator would mean to copy the same code there anyway. I am open for suggestions

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

github-actions · 2022-05-18T15:51:49Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	0.68%	61969.57	61552.21
Total requests	0.62%	5579718	5545530
Total errors	NaN%	0	0
Total successes	0.62%	5579718	5545530
Average latency ms	4.29%	0.73	0.7
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	14.96%	24.52	21.33
Stdev latency ms	7.69%	0.98	0.91
Transfer Mb	0.62%	580.01	576.46
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

github-actions · 2022-05-19T08:00:45Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	2.91%	36256.88	35232.7
Total requests	2.94%	3265061	3171704
Total errors	NaN%	0	0
Total successes	2.94%	3265061	3171704
Average latency ms	0.00%	0.92	0.92
Minimum latency ms	0.00%	0.03	0.03
Maximum latency ms	-31.47%	16.9	24.66
Stdev latency ms	17.65%	0.6	0.51
Transfer Mb	2.94%	339.4	329.7
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

david-perez · 2022-05-19T10:53:13Z

...er/src/main/kotlin/software/amazon/smithy/rust/codegen/server/smithy/ServerCodegenVisitor.kt

@@ -184,7 +184,7 @@ class ServerCodegenVisitor(context: PluginContext, private val codegenDecorator:
        logger.info("[rust-server-codegen] Generating an enum $shape")
        shape.getTrait<EnumTrait>()?.also { enum ->
            rustCrate.useShapeWriter(shape) { writer ->
-                EnumGenerator(model, symbolProvider, writer, shape, enum).render()
+                ServerEnumGenerator(model, symbolProvider, writer, shape, enum, codegenContext.mode, codegenContext.runtimeConfig).render()


In these cases where the number of params grows, instead of plucking everything out of codegenContext, just pass codegenContext and have the generator pull out the things it needs: model, symbolProvider, runtimeConfig etc...

...n/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerEnumGenerator.kt

david-perez · 2022-05-19T11:19:10Z

...n/kotlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerEnumGenerator.kt

+        writer.rustBlock("impl #T<$errorStruct> for #T", RuntimeType.From, ServerRuntimeType.RequestRejection(runtimeConfig)) {
+            writer.rustBlock("fn from(e: $errorStruct) -> Self") {
+                write("Self::EnumVariantNotFound(Box::new(e))")
+            }
+        }
+        writer.rustBlock("impl From<$errorStruct> for #T", RuntimeType.jsonDeserialize(runtimeConfig)) {
+            writer.rustBlock("fn from(e: $errorStruct) -> Self") {
+                write("""Self::custom(format!("unknown variant {}", e))""")
+            }
+        }
+        writer.rustBlock("impl std::error::Error for $errorStruct") {}
+        writer.rustBlock("impl std::fmt::Display for $errorStruct") {
+            writer.rustBlock("fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result") {
+                write("self.0.fmt(f)")
+            }
+        }
+    }


Elaborating on the "strive to write as much uninterrupted Rust as possible", this code is fine, but notice it's exactly the same as:

Suggested change

writer.rustBlock("impl #T<$errorStruct> for #T", RuntimeType.From, ServerRuntimeType.RequestRejection(runtimeConfig)) {

writer.rustBlock("fn from(e: $errorStruct) -> Self") {

write("Self::EnumVariantNotFound(Box::new(e))")

}

}

writer.rustBlock("impl From<$errorStruct> for #T", RuntimeType.jsonDeserialize(runtimeConfig)) {

writer.rustBlock("fn from(e: $errorStruct) -> Self") {

write("""Self::custom(format!("unknown variant {}", e))""")

}

}

writer.rustBlock("impl std::error::Error for $errorStruct") {}

writer.rustBlock("impl std::fmt::Display for $errorStruct") {

writer.rustBlock("fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result") {

write("self.0.fmt(f)")

}

}

}

writer.rustTemplate(

"""

impl #{From}<$errorStruct> for #{RequestRejection} {

fn from(e: $errorStruct) -> Self {

Self::EnumVariantNotFound(Box::new(e))

}

}

impl #{From}<$errorStruct> for #{JsonDeserialize} {

fn from(e: $errorStruct) -> Self {

Self::custom(format!("unknown variant {}", e))

}

}

impl #{StdError} for $errorStruct { }

impl std::fmt::Display for $errorStruct {

fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {

self.0.fmt(f)

}

}

""",

"From" to RuntimeType.From,

"StdError" to RuntimeType.StdError,

"RequestRejection" to ServerRuntimeType.RequestRejection(runtimeConfig),

"JsonDeserialize" to RuntimeType.jsonDeserialize(runtimeConfig),

)

which reads much nicer.

(Bonus points: we should also have a RuntimeType.Display and use it everywhere in the codebase where we've used RuntimeType.stdfmt.member("Display") or written out Display manually).

The writer.*block methods are useful when you have to loop over a collection, when you want to insert blocks conditionally, or when you need to put conditionals based on Kotlin variables inside your Rust blocks. For example, above was a good use of rustBlock, because you needed to iterate over sortedMembers.

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/SymbolVisitor.kt

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/EnumGenerator.kt

...ain/kotlin/software/amazon/smithy/rust/codegen/smithy/protocols/parse/JsonParserGenerator.kt

codegen/src/test/kotlin/software/amazon/smithy/rust/testutil/Annotations.kt

rust-runtime/aws-smithy-http-server/src/rejection.rs

The server must have the most up to date variants and the unknown enum variant should not be used. Clients are generated with it because they might not have the most recent model and the server might return an unknown variant to them. Closes #1187 Signed-off-by: Daniele Ahmed <[email protected]>

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-05-20T10:03:59Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-22.34%	38001.24	48932.66
Total requests	-22.34%	3420670	4404657
Total errors	NaN%	0	0
Total successes	-22.34%	3420670	4404657
Average latency ms	22.86%	0.86	0.7
Minimum latency ms	50.00%	0.03	0.02
Maximum latency ms	1.27%	16.8	16.59
Stdev latency ms	-18.03%	0.5	0.61
Transfer Mb	-22.34%	355.58	457.87
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-05-20T11:49:14Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-4.94%	38488.32	40486.39
Total requests	-4.92%	3465117	3644397
Total errors	NaN%	0	0
Total successes	-4.92%	3465117	3644397
Average latency ms	4.94%	0.85	0.81
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-2.98%	16.94	17.46
Stdev latency ms	0.00%	0.49	0.49
Transfer Mb	-4.92%	360.2	378.84
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

david-perez

An empty rust-runtime/.attach_pid18283 file crept into the patchset.

...tware/amazon/smithy/rust/codegen/server/smithy/protocols/ServerHttpBoundProtocolGenerator.kt

...tlin/software/amazon/smithy/rust/codegen/server/smithy/generators/ServerEnumGeneratorTest.kt

rust-runtime/aws-smithy-http-server/src/rejection.rs

...ware/amazon/smithy/rust/codegen/smithy/protocols/parse/XmlBindingTraitParserGeneratorTest.kt

Co-authored-by: david-perez <[email protected]>

Signed-off-by: Daniele Ahmed <[email protected]>

github-actions · 2022-05-20T18:08:44Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	5.30%	80440.91	76395.37
Total requests	5.20%	7240722	6882988
Total errors	NaN%	0	0
Total successes	5.20%	7240722	6882988
Average latency ms	5.56%	0.95	0.9
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	9.54%	22.84	20.85
Stdev latency ms	7.24%	1.63	1.52
Transfer Mb	5.20%	752.68	715.49
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

github-actions · 2022-05-22T20:52:31Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-7.26%	62651.57	67559
Total requests	-7.23%	5642360	6081810
Total errors	NaN%	0	0
Total successes	-7.23%	5642360	6081810
Average latency ms	-18.42%	0.62	0.76
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-7.17%	17.48	18.83
Stdev latency ms	-33.91%	0.76	1.15
Transfer Mb	-7.23%	586.53	632.21
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/Instantiator.kt

github-actions · 2022-05-23T16:07:51Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-0.70%	88147.79	88768.83
Total requests	-0.67%	7940372	7993759
Total errors	NaN%	0	0
Total successes	-0.67%	7940372	7993759
Average latency ms	0.84%	1.2	1.19
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	0.64%	28.18	28
Stdev latency ms	0.46%	2.18	2.17
Transfer Mb	-0.67%	825.41	830.96
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

github-actions · 2022-05-23T18:04:53Z

A new generated diff is ready to view.

A new doc preview is ready to view.

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

Measurement	Deviation	Current	Old
Requests/sec	-1.00%	85990.83	86861.7
Total requests	-1.00%	7747571	7825445
Total errors	NaN%	0	0
Total successes	-1.00%	7747571	7825445
Average latency ms	-0.83%	1.19	1.2
Minimum latency ms	0.00%	0.02	0.02
Maximum latency ms	-13.97%	24.45	28.42
Stdev latency ms	-0.46%	2.16	2.17
Transfer Mb	-1.00%	805.36	813.46
Connect errors	NaN%	0	0
Read errors	NaN%	0	0
Write errors	NaN%	0	0
Status errors (not 2xx/3xx)	NaN%	0	0
Timeout errors	NaN%	0	0

XML deserialization of enums is currently broken. This commit should have been done as part of #1398, but we only tested that patch using the restJson1 protocol. The converter from the unknown enum variant error into `aws_smithy_json::deserialize::Error` has been removed from `ServerEnumGenerator`, since it's protocol-specific logic. We instead use `map_err` in the protocol-specific parsers before bubbling up using `?`. Fixes #1477.

…1485) XML deserialization of enums is currently broken. This commit should have been done as part of #1398, but we only tested that patch using the restJson1 protocol. The converter from the unknown enum variant error into `aws_smithy_json::deserialize::Error` has been removed from `ServerEnumGenerator`, since it's protocol-specific logic. We instead use `map_err` in the protocol-specific parsers before bubbling up using `?`. Fixes #1477.

82marbag requested review from a team as code owners May 18, 2022 13:21

crisidev reviewed May 18, 2022

View reviewed changes

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

david-perez requested changes May 19, 2022

View reviewed changes

Daniele Ahmed added 2 commits May 20, 2022 09:39

address comments

16de0bd

Signed-off-by: Daniele Ahmed <[email protected]>

82marbag force-pushed the 1187 branch from b67d7cf to 16de0bd Compare May 20, 2022 09:41

Daniele Ahmed added 2 commits May 20, 2022 11:13

update tests

1ca2939

Signed-off-by: Daniele Ahmed <[email protected]>

update tests

5802f25

Signed-off-by: Daniele Ahmed <[email protected]>

david-perez requested changes May 20, 2022

View reviewed changes

david-perez mentioned this pull request May 20, 2022

Constraint traits uber-issue #1401

Open

23 tasks

82marbag and others added 2 commits May 20, 2022 18:46

Apply suggestions from code review

8ead3c8

Co-authored-by: david-perez <[email protected]>

address comments

4b236c0

Signed-off-by: Daniele Ahmed <[email protected]>

david-perez approved these changes May 20, 2022

View reviewed changes

david-perez added the server Rust server SDK label May 20, 2022

Merge branch 'main' into 1187

1650c44

Velfi reviewed May 23, 2022

View reviewed changes

codegen/src/main/kotlin/software/amazon/smithy/rust/codegen/smithy/generators/Instantiator.kt Show resolved Hide resolved

Velfi approved these changes May 23, 2022

View reviewed changes

Merge branch 'main' into 1187

28043d7

Merge branch 'main' into 1187

00e2e57

crisidev merged commit 35989d2 into main May 23, 2022

crisidev deleted the 1187 branch May 23, 2022 19:16

david-perez mentioned this pull request Jun 22, 2022

Bubble up unknown enum variant error when parsing XML in the server #1485

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unknown enum variants removed from server #1398

Unknown enum variants removed from server #1398

82marbag commented May 18, 2022 •

edited by david-perez

Loading

github-actions bot commented May 18, 2022

crisidev May 18, 2022

crisidev May 18, 2022

82marbag May 19, 2022

github-actions bot commented May 18, 2022

github-actions bot commented May 19, 2022

david-perez May 19, 2022

david-perez May 19, 2022

github-actions bot commented May 20, 2022

github-actions bot commented May 20, 2022

david-perez left a comment

github-actions bot commented May 20, 2022

github-actions bot commented May 22, 2022

github-actions bot commented May 23, 2022

github-actions bot commented May 23, 2022

Unknown enum variants removed from server #1398

Unknown enum variants removed from server #1398

Conversation

82marbag commented May 18, 2022 • edited by david-perez Loading

Motivation and Context

Description

Testing

Checklist

github-actions bot commented May 18, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

crisidev May 18, 2022

Choose a reason for hiding this comment

crisidev May 18, 2022

Choose a reason for hiding this comment

82marbag May 19, 2022

Choose a reason for hiding this comment

github-actions bot commented May 18, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented May 19, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

david-perez May 19, 2022

Choose a reason for hiding this comment

david-perez May 19, 2022

Choose a reason for hiding this comment

github-actions bot commented May 20, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented May 20, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

david-perez left a comment

Choose a reason for hiding this comment

github-actions bot commented May 20, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented May 22, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented May 23, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

github-actions bot commented May 23, 2022

Rust Wrk benchmark report:

Duration: 90 sec, Connections: 32, Threads: 2

82marbag commented May 18, 2022 •

edited by david-perez

Loading