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

Confusion with the streaming example in section 13.1 of IDL v2 #1380

Closed
david-perez opened this issue Sep 2, 2022 · 7 comments · Fixed by #1458
Closed

Confusion with the streaming example in section 13.1 of IDL v2 #1380

david-perez opened this issue Sep 2, 2022 · 7 comments · Fixed by #1458
Labels
documentation This is a problem with documentation.

Comments

@david-perez
Copy link
Contributor

Link. Example as of writing:

operation StreamingOperation {
    input: StreamingOperationInput
    output: StreamingOperationOutput
}

@input
structure StreamingOperationInput {}

@output
structure StreamingOperationOutput {
    @required
    streamId: String
    output: StreamingBlob
}

@streaming
blob StreamingBlob

I'm confused because:

  1. Smithy IDL v2 introduces a change where members targeting streaming blobs now MUST have @default or @required. However, output in the example above does not have any of the two traits attached.
  2. In IDL v1 there is a restriction on the streaming trait that says:

If a service supports a protocol that supports the httpPayload trait, any member that targets a streaming blob must also be marked as @httpPayload.

However, this restriction does not appear in IDL v2. Is that intentional? The Smithy library is still enforcing it for IDL v2 models:

= Member com.amazonaws.simple#AnOperationInput$streamingBlob referencing @streaming shape com.amazonaws.simple#StreamingBlob must have the @httpPayload trait, as service com.amazonaws.simple#SimpleService has a protocol that supports @httpPayload.

  1. The example is confusing because you can't send both a string (streamId) and a streaming blob (output) in the same HTTP message body (?). I acknowledge that in another application-level protocol sending both pieces of data might be possible (which one?), but if you want to keep the example as-is, I think it would be better to call out immediately below it that it's not realizable in HTTP.
@mtdowling
Copy link
Member

mtdowling commented Sep 5, 2022

However, output in the example above does not have any of the two traits attached.

Looks like an accidental omission.

[...] However, this restriction does not appear in IDL v2. Is that intentional?

The same limitation applies, and is documented here: https://awslabs.github.io/smithy/2.0/spec/http-bindings.html#event-streams. We should make it more clear this applies to event streams and string/blob streams too.

The example is confusing because you can't send both a string (streamId) and a streaming blob (output) in the same HTTP message body (?). I acknowledge that in another application-level protocol sending both pieces of data might be possible (which one?), but if you want to keep the example as-is, I think it would be better to call out immediately below it that it's not realizable in HTTP.

RPC protocols like AWS JSON can work with this example.

@mtdowling mtdowling added the documentation This is a problem with documentation. label Sep 5, 2022
@david-perez
Copy link
Contributor Author

RPC protocols like AWS JSON can work with this example.

How? What does an HTTP body message look like in AWS JSON 1.x for that example?

@mtdowling
Copy link
Member

I'm updating the docs to address 1 and 2.

How? What does an HTTP body message look like in AWS JSON 1.x for that example?

RPC protocols use an initial event that's sent as an actual event. We don't have docs for this yet. We can track this in #1287

@david-perez
Copy link
Contributor Author

But isn't #1287 for event streams? This is for a @streaming blob shape.

@mtdowling
Copy link
Member

Oh! You're right. I completely glossed over that this targeted a blob. The example is theoretically fine as is, but doesn't work with any existing protocols, so I'll change it.

mtdowling added a commit that referenced this issue Oct 21, 2022
* Clarify that streaming trait members need httpPayload when using
  an http binding protocol.
* Clarify that servers should interpret streaming members as
  having a default value of zero bytes.
* Mention this in the v1 docs too.
* Fixed a doc linking issue for the streaming docs where we were
  using the wrong casing.

Closes #1380
Addresses #1456
mtdowling added a commit that referenced this issue Oct 22, 2022
* Clarify that streaming trait members need httpPayload when using
  an http binding protocol.
* Clarify that servers should interpret streaming members as
  having a default value of zero bytes.
* Mention this in the v1 docs too.
* Fixed a doc linking issue for the streaming docs where we were
  using the wrong casing.

Closes #1380
Addresses #1456
@david-perez
Copy link
Contributor Author

The example was changed in #1458 to:

@http(method: "GET", uri: "/streaming-operation")
operation StreamingOperation {
    input := {}
    output := {
        @required
        streamId: String

        @httpPayload
        output: StreamingBlob = ""
    }
}

@streaming
blob StreamingBlob

This is still invalid because the member targeting the streaming blob is not marked with @required or @default.

Moreover, the example is still confusing because it's irrealizable in any of the supported protocols: we can't send both a streaming blob and a string in the output payload.

@mtdowling
Copy link
Member

mtdowling commented Oct 24, 2022

The streaming blob has a default value assigned using = "" which is exactly the same as @default(""). I'll add an HTTP binding for the streamId member. (Edit) Actually, we can just remove the streamId member. I don't think it's needed for the example.

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.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants