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

Question concerning httpPayload and empty input/outputs in RestJson1 protocol tests #1068

Open
david-perez opened this issue Jan 31, 2022 · 2 comments
Labels
documentation This is a problem with documentation. guidance Question that needs advice or information.

Comments

@david-perez
Copy link
Contributor

Consider this protocol test:

{
    id: "RestJsonHttpWithHeadersButNoPayload",
    documentation: "Serializes an request with header members but no payload",
    protocol: restJson1,
    method: "POST",
    uri: "/payload",
    body: "{}",
    bodyMediaType: "application/json",
    headers: {
        "Content-Type": "application/json",
        "X-Amz-Test-Id": "t-12345"
    },
    requireHeaders: [
        "Content-Length"
    ],
    params: {
        testId: "t-12345"
    }
}

Where the operation's input is:

structure TestPayloadStructureInputOutput {
    @httpHeader("x-amz-test-id")
    testId: String,

    @httpPayload
    payloadConfig: PayloadConfig
}

structure PayloadConfig {
    data: Integer
}

If the server receives the HTTP request defined in the test case, I would expect it to be deserialized to the following Rust struct:

TestPayloadStructureInput {
    test_id: Some(
        "t-12345",
    ),
    payload_config: Some(
        PayloadConfig {
            data: None,
        },
    ),
}

We still received a JSON body {}, so payload_config which is bound to the HTTP body via httpPayload, should be Some. However, the JSON object contains no fields, so data is None.

However, the test expects:

TestPayloadStructureInput {
    test_id: Some(
        "t-12345",
    ),
    payload_config: None,
}

because its params attribute does not specify a value for the PayloadConfig field.

Is the test definition correct?


I think that this is at odds with another test in the test suite. According to this other protocol test:

{
    id: "RestJsonEmptyInputAndEmptyOutput",
    documentation: """
            As of January 2021, server implementations are expected to
            respond with a JSON object regardless of if the output
            parameters are empty.""",
    protocol: restJson1,
    code: 200,
    headers: {
        "Content-Type": "application/json",
    },
    body: "{}",
    bodyMediaType: "application/json"
}

, server implementations should always respond with a JSON object, even if the output parameters are empty. So how do you distinguish between the above two cases if a TestPayloadStructureOutput defined like above had a PayloadConfig bound to the body with httpPayload?

@JordonPhillips
Copy link
Contributor

The fundamental issue here is a confusion about the intent of params. The current intent is that you should only make assertions on those parameters that are actually present, so the omission of a parameter doesn't mean you should assert that it is null or missing but rather you should make no assertions about it. This is the same way that other parts of the test are handled, you wouldn't fail a test because your tooling adds a bunch of default headers for instance. Unlike those other sections, however, it isn't pointed out for params. I'll talk with the team to see if we want to change that, but in any event we should better document it.

@david-perez
Copy link
Contributor Author

david-perez commented Apr 8, 2022

The current intent is that you should only make assertions on those parameters that are actually present, so the omission of a parameter doesn't mean you should assert that it is null or missing but rather you should make no assertions about it.

If that is the intent, it's the only test that fails because of an omitted member that should not be asserted. All tests I've encountered so far are complete in the sense that the expected structure has the same schema as the modeled structure.

It would also make things harder to test in strongly typed languages like Rust, because you can't assert generated struct equality, but you would have to go member by member and check if it has been defined in the test's params.


My question about how to distinguish None from Some in restJson1 when the optional member is httpPayload-bound still remains unanswered. I've re-encountered this issue for streaming shapes, see #1179.

@milesziemer milesziemer added the guidance Question that needs advice or information. label Sep 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation This is a problem with documentation. guidance Question that needs advice or information.
Projects
None yet
Development

No branches or pull requests

3 participants