Skip to content
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.

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

RpcV2 Protocol tests #1911

Closed
wants to merge 8 commits into from

Conversation

adwsingh
Copy link
Contributor

@adwsingh adwsingh commented Aug 7, 2023

Issue #, if available:

Description of changes:

Draft PR, I am going to add more protocol tests. Looking for early feedback.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@adwsingh adwsingh force-pushed the smithy-rpc-v2 branch 2 times, most recently from 22fe304 to e84fe57 Compare August 8, 2023 20:14
headers: {
"smithy-protocol": "rpc-v2-cbor",
"Accept": "application/cbor",
"Content-Type": "application/cbor"
Copy link
Contributor

Choose a reason for hiding this comment

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

Content-Type should be forbidden here; the operation has no input.

Copy link
Contributor

Choose a reason for hiding this comment

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

This remains unanswered.

documentation: "Servers should allow the Accept header to be set to the default content-type.",
headers: {
"smithy-protocol": "rpc-v2-cbor",
"Accept": "application/cbor",
Copy link
Contributor

Choose a reason for hiding this comment

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

The server can't really honor sending a CBOR response here right? The operation has no output; the payload must be empty.

Copy link
Contributor

Choose a reason for hiding this comment

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

This remains unanswered.

Comment on lines +47 to +48
"Accept": "application/cbor",
"Content-Type": "application/cbor"
Copy link
Contributor

@david-perez david-perez Aug 18, 2023

Choose a reason for hiding this comment

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

Same comments apply here.

Copy link
Contributor

Choose a reason for hiding this comment

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

This remains unanswered.

}
method: "POST",
uri: "/service/aws.protocoltests.rpcv2.RpcV2Protocol/operation/NoInputOutput",
body: "v/8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't expect a body here right?

Requests for operations with no defined input type MUST NOT contain bodies in their HTTP requests. The Content-Type for the serialization format MUST NOT be set.

Copy link
Contributor

Choose a reason for hiding this comment

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

This remains unanswered.

@adwsingh adwsingh force-pushed the smithy-rpc-v2 branch 3 times, most recently from 5019a16 to ac60ffb Compare November 2, 2023 02:10
@adwsingh adwsingh marked this pull request as ready for review November 2, 2023 02:11
@adwsingh adwsingh requested a review from a team as a code owner November 2, 2023 02:11
crisidev and others added 6 commits January 2, 2024 13:22
* Add Smithy RPC v2 trait definition and implementation

Signed-off-by: Bigo <[email protected]>

* Add support for wire formats

* Address PR comments

* Rename SmithyRpcV2* to just Rpcv2

* Finish refactor to just have Rpcv2

* Address PR comments. Remove the validator for now

* Address comments from PR

---------

Signed-off-by: Bigo <[email protected]>
* Implement Rpcv2TraitValidator and tests

* Add missing license

* Update smithy-protocols-traits/src/main/java/software/amazon/smithy/protocols/traits/Rpcv2TraitValidator.java

Co-authored-by: Kevin Stich <[email protected]>

* Address comments from PR

* Make sure format is always set

* Fix typo

* Fix comment

* Fix comment

* Fix trailing whitespace

---------

Co-authored-by: Kevin Stich <[email protected]>
This commit refactors the trait implementation to be specific to the
CBOR wire format instead of taking a collection of possible wire
formats, simplifying the process of resolving the protocol to use
for generated code.

It also moves the package to `smithy-protocol-traits`, removing the
pluralization on `protocols`.
@adwsingh adwsingh reopened this Jan 4, 2024
Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

I did another pass against my implementation. I found a few bugs that I have now fixed. As of writing, against the latest pushed commit, smithy-lang/smithy-rs@636bba9 in my PR, the following 10 tests fail:

failures:
    operation::server_no_input_output_test::no_input_server_allows_accept_request
    operation::server_no_input_output_test::no_input_server_allows_empty_cbor_request
    operation::server_no_input_output_test::no_input_server_ingores_unexpected_fields_request
    operation::server_no_input_output_test::no_output_response
    operation::server_rpc_v2_cbor_maps_test::rpc_v2_cbor_deserializes_dense_set_map_response
    operation::server_rpc_v2_cbor_maps_test::rpc_v2_cbor_deserializes_null_map_values_response
    operation::server_rpc_v2_cbor_maps_test::rpc_v2_cbor_serializes_dense_set_map_request
    operation::server_rpc_v2_cbor_maps_test::rpc_v2_cbor_serializes_null_map_values_request
    operation::server_simple_scalar_properties_test::rpc_v2_cbor_server_doesnt_de_serialize_null_structure_values_request
    operation::server_simple_scalar_properties_test::rpc_v2_cbor_simple_scalar_properties_response

Most fail due to minor mistakes in the tests' definitions; I've commented on each one indicating where I think they're wrong. However, in general, in the spec:

  • we need to be clear on what to accept/reject when (1) the modeled operation has no input/output, (2) the input/output shapes are unit shapes, and (3) the input/output shapes are empty. My preference would be to be as strict as possible and only accept empty payloads for (1) and (2), and and empty map for (3).
  • we need to be clear on when to set the application/cbor media type in Accept and Content-Type. My preference would be to be as strict as possible and only set it when we should only accept or return a non-empty HTTP body, respectively.

In terms of test suite completeness, I think we still have a long way to go. I haven't thought exhaustively about what's missing, but off the top of my head, we need tests for (in order of importance):

I would also like to ask the Smithy team that they consider encoding bodies in a text format that is amenable for review: #2108

"smithy-protocol": "rpc-v2-cbor",
"Content-Type": "application/cbor"
},
body: "v2ZfX3R5cGV4J2F3cy5wcm90b2NvbHRlc3RzLnJwY3YyI0ludmFsaWRHcmVldGluZ2dNZXNzYWdlYkhp/w==",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
body: "v2ZfX3R5cGV4J2F3cy5wcm90b2NvbHRlc3RzLnJwY3YyI0ludmFsaWRHcmVldGluZ2dNZXNzYWdlYkhp/w==",
body: "v2ZfX3R5cGV4K2F3cy5wcm90b2NvbHRlc3RzLnJwY3YyQ2JvciNJbnZhbGlkR3JlZXRpbmdnTWVzc2FnZWJIaf8=",
 == hint:
expected body in diagnostic format:
{_
    "__type": "aws.protocoltests.rpcv2#InvalidGreeting",
    "Message": "Hi",
}
actual body in diagnostic format:
{_
    "__type": "aws.protocoltests.rpcv2Cbor#InvalidGreeting",
    "Message": "Hi",
}
expected body in annotated hex:
bf                                     # map(*)
   66                                  #   text(6)
      5f5f74797065                     #     "__type"
   78 27                               #   text(39)
      6177732e70726f746f636f6c74657374 #     "aws.protocoltest"
      732e727063763223496e76616c696447 #     "s.rpcv2#InvalidG"
      72656574696e67                   #     "reeting"
   67                                  #   text(7)
      4d657373616765                   #     "Message"
   62                                  #   text(2)
      4869                             #     "Hi"
   ff                                  #   break

actual body in annotated hex:
bf                                     # map(*)
   66                                  #   text(6)
      5f5f74797065                     #     "__type"
   78 2b                               #   text(43)
      6177732e70726f746f636f6c74657374 #     "aws.protocoltest"
      732e727063763243626f7223496e7661 #     "s.rpcv2Cbor#Inva"
      6c69644772656574696e67           #     "lidGreeting"
   67                                  #   text(7)
      4d657373616765                   #     "Message"
   62                                  #   text(2)
      4869                             #     "Hi"
   ff                                  #   break

actual body in base64 (useful to update the protocol test):
v2ZfX3R5cGV4K2F3cy5wcm90b2NvbHRlc3RzLnJwY3YyQ2JvciNJbnZhbGlkR3JlZXRpbmdnTWVzc2FnZWJIaf8=

"smithy-protocol": "rpc-v2-cbor",
"Content-Type": "application/cbor"
},
body: "v2ZfX3R5cGV4JGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyI0NvbXBsZXhFcnJvcmZOZXN0ZWS/Y0Zvb2NiYXL/aFRvcExldmVsaVRvcCBsZXZlbP8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
body: "v2ZfX3R5cGV4JGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyI0NvbXBsZXhFcnJvcmZOZXN0ZWS/Y0Zvb2NiYXL/aFRvcExldmVsaVRvcCBsZXZlbP8=",
body: "v2ZfX3R5cGV4KGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyQ2JvciNDb21wbGV4RXJyb3JoVG9wTGV2ZWxpVG9wIGxldmVsZk5lc3RlZL9jRm9vY2Jhcv//",
 == hint:
expected body in diagnostic format:
{_
    "__type": "aws.protocoltests.rpcv2#ComplexError",
    "Nested": {_ "Foo": "bar"},
    "TopLevel": "Top level",
}
actual body in diagnostic format:
{_
    "__type": "aws.protocoltests.rpcv2Cbor#ComplexError",
    "TopLevel": "Top level",
    "Nested": {_ "Foo": "bar"},
}
expected body in annotated hex:
bf                                     # map(*)
   66                                  #   text(6)
      5f5f74797065                     #     "__type"
   78 24                               #   text(36)
      6177732e70726f746f636f6c74657374 #     "aws.protocoltest"
      732e727063763223436f6d706c657845 #     "s.rpcv2#ComplexE"
      72726f72                         #     "rror"
   66                                  #   text(6)
      4e6573746564                     #     "Nested"
   bf                                  #   map(*)
      63                               #     text(3)
         466f6f                        #       "Foo"
      63                               #     text(3)
         626172                        #       "bar"
      ff                               #     break
   68                                  #   text(8)
      546f704c6576656c                 #     "TopLevel"
   69                                  #   text(9)
      546f70206c6576656c               #     "Top level"
   ff                                  #   break

actual body in annotated hex:
bf                                     # map(*)
   66                                  #   text(6)
      5f5f74797065                     #     "__type"
   78 28                               #   text(40)
      6177732e70726f746f636f6c74657374 #     "aws.protocoltest"
      732e727063763243626f7223436f6d70 #     "s.rpcv2Cbor#Comp"
      6c65784572726f72                 #     "lexError"
   68                                  #   text(8)
      546f704c6576656c                 #     "TopLevel"
   69                                  #   text(9)
      546f70206c6576656c               #     "Top level"
   66                                  #   text(6)
      4e6573746564                     #     "Nested"
   bf                                  #   map(*)
      63                               #     text(3)
         466f6f                        #       "Foo"
      63                               #     text(3)
         626172                        #       "bar"
      ff                               #     break
   ff                                  #   break

actual body in base64 (useful to update the protocol test):
v2ZfX3R5cGV4KGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyQ2JvciNDb21wbGV4RXJyb3JoVG9wTGV2ZWxpVG9wIGxldmVsZk5lc3RlZL9jRm9vY2Jhcv//

protocol: rpcv2Cbor,
code: 400,
headers: {
"Content-Type": "application/x-amz-json-1.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
"Content-Type": "application/x-amz-json-1.1"
"Content-Type": "application/cbor"

headers: {
"Content-Type": "application/x-amz-json-1.1"
},
body: "v2ZfX3R5cGV4JGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyI0NvbXBsZXhFcnJvcv8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
body: "v2ZfX3R5cGV4JGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyI0NvbXBsZXhFcnJvcv8=",
body: "v2ZfX3R5cGV4KGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyQ2JvciNDb21wbGV4RXJyb3L/",
  == hint:
expected body in diagnostic format:
{_ "__type": "aws.protocoltests.rpcv2#ComplexError"}
actual body in diagnostic format:
{_ "__type": "aws.protocoltests.rpcv2Cbor#ComplexError"}
expected body in annotated hex:
bf                                     # map(*)
   66                                  #   text(6)
      5f5f74797065                     #     "__type"
   78 24                               #   text(36)
      6177732e70726f746f636f6c74657374 #     "aws.protocoltest"
      732e727063763223436f6d706c657845 #     "s.rpcv2#ComplexE"
      72726f72                         #     "rror"
   ff                                  #   break

actual body in annotated hex:
bf                                     # map(*)
   66                                  #   text(6)
      5f5f74797065                     #     "__type"
   78 28                               #   text(40)
      6177732e70726f746f636f6c74657374 #     "aws.protocoltest"
      732e727063763243626f7223436f6d70 #     "s.rpcv2Cbor#Comp"
      6c65784572726f72                 #     "lexError"
   ff                                  #   break

actual body in base64 (useful to update the protocol test):
v2ZfX3R5cGV4KGF3cy5wcm90b2NvbHRlc3RzLnJwY3YyQ2JvciNDb21wbGV4RXJyb3L/

documentation: "Servers should allow the Accept header to be set to the default content-type.",
headers: {
"smithy-protocol": "rpc-v2-cbor",
"Accept": "application/cbor",
Copy link
Contributor

Choose a reason for hiding this comment

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

This remains unanswered.

documentation: "A response that contains a dense map of sets.",
protocol: rpcv2Cbor,
code: 200,
body: "v2xzcGFyc2VTZXRNYXC/YXmfYWFhYv9heJ////8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
body: "v2xzcGFyc2VTZXRNYXC/YXmfYWFhYv9heJ////8=",
body: "v2tkZW5zZVNldE1hcKJheIBheYJhYWFi/w==",
expected body in diagnostic format:
{_ "sparseSetMap": {_ "y": [_ "a", "b"], "x": [_ ]}}
actual body in diagnostic format:
{_ "denseSetMap": {"x": [], "y": ["a", "b"]}}
expected body in annotated hex:
bf                             # map(*)
   6c                          #   text(12)
      7370617273655365744d6170 #     "sparseSetMap"
   bf                          #   map(*)
      61                       #     text(1)
         79                    #       "y"
      9f                       #     array(*)
         61                    #       text(1)
            61                 #         "a"
         61                    #       text(1)
            62                 #         "b"
         ff                    #       break
      61                       #     text(1)
         78                    #       "x"
      9f                       #     array(*)
         ff                    #       break
      ff                       #     break
   ff                          #   break

actual body in annotated hex:
bf                           # map(*)
   6b                        #   text(11)
      64656e73655365744d6170 #     "denseSetMap"
   a2                        #   map(2)
      61                     #     text(1)
         78                  #       "x"
      80                     #     array(0)
      61                     #     text(1)
         79                  #       "y"
      82                     #     array(2)
         61                  #       text(1)
            61               #         "a"
         61                  #       text(1)
            62               #         "b"
   ff                        #   break

actual body in base64 (useful to update the protocol test):
v2tkZW5zZVNldE1hcKJheIBheYJhYWFi/w==

protocol: rpcv2Cbor,
method: "POST",
uri: "/service/RpcV2Protocol/operation/RpcV2CborMaps",
body: "v2xzcGFyc2VTZXRNYXC/YXmfYWFhYv9heJ////8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

protocol: rpcv2Cbor,
method: "POST",
uri: "/service/RpcV2Protocol/operation/RpcV2CborMaps",
body: "v3BzcGFyc2VCb29sZWFuTWFwv2F49v9vc3BhcnNlTnVtYmVyTWFwv2F49v9vc3BhcnNlU3RydWN0TWFwv2F49v//",
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

},
{
id: "RpcV2CborServerDoesntDeSerializeNullStructureValues",
documentation: "RpcV2 Cbor should not deserialize null structure values",
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't the server reject the request instead of ignoring the null values on the wire? The spec says that null values MUST be omitted by clients.

"Content-Type": "application/cbor"
}
bodyMediaType: "application/cbor",
body: "v2lieXRlVmFsdWUFa2RvdWJsZVZhbHVl+z/+OVgQYk3TcWZhbHNlQm9vbGVhblZhbHVl9GpmbG9hdFZhbHVl+kDz989saW50ZWdlclZhbHVlGQEAaWxvbmdWYWx1ZRkmkWpzaG9ydFZhbHVlGSaqa3N0cmluZ1ZhbHVlZnNpbXBsZXB0cnVlQm9vbGVhblZhbHVl9f8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

This was marked as resolved but the body is still setting longValue but in params is not present.

}
method: "POST",
bodyMediaType: "application/cbor",
uri: "/service/aws.protocoltests.rpcv2Cbor.RpcV2Protocol/operation/SimpleScalarProperties",
Copy link
Contributor

Choose a reason for hiding this comment

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

Saw this commented elsewhere and marked fixed, but we still have a handful of instances of the full namespace in the path when it shouldn't be.

method: "POST",
bodyMediaType: "application/cbor",
uri: "/service/RpcV2Protocol/operation/SimpleScalarProperties",
body: "v2tkb3VibGVWYWx1Zft/+AAAAAAAAGpmbG9hdFZhbHVl+n/AAAD/"
Copy link
Contributor

@lucix-aws lucix-aws Feb 28, 2024

Choose a reason for hiding this comment

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

Worth noting that different implementations may expose different values for general NaN. We've encoded f64 NaN here as 7ff8000000000000 but go has defined it as 0x7ff8000000000001, both technically NaN but fails a bitwise comparison. Not sure if we care, though. We should probably be recommending that people do document-based equality checks for these comparisons (comparisons for this protocol in general).

]
method: "POST",
uri: "/service/aws.protocoltests.rpcv2Cbor.RpcV2Protocol/operation/EmptyInputOutput",
body: "v/8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs bodyMediaType if it's expecting a CBOR body.

]
method: "POST",
uri: "/service/aws.protocoltests.rpcv2Cbor.RpcV2Protocol/operation/OptionalInputOutput",
body: "v/8=",
Copy link
Contributor

Choose a reason for hiding this comment

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

@kstich
Copy link
Contributor

kstich commented Mar 13, 2024

Closing in favor of #2188

@kstich kstich closed this Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants