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

Support optional URI elements #102

Closed
rhernandez35 opened this issue Jan 5, 2021 · 4 comments
Closed

Support optional URI elements #102

rhernandez35 opened this issue Jan 5, 2021 · 4 comments

Comments

@rhernandez35
Copy link
Contributor

rhernandez35 commented Jan 5, 2021

Currently, the Rust code generator assumes that all members with the httpLabel trait are required. This isn't always true. For example, S3's GetObjectRequest structure binds partNumber to a URI element, but it's necessarily optional since it's only accepted when retrieving a single part of a multi-part object. As is, the generated code forces this field to be specified in all requests.

I'd say "just examine the member for the required trait," but I sadly don't see many models using it. FWIW, The AWS service models bundled with botocore appear to correctly model all required members regardless of where they appear on the wire. Are these service definitions hand-maintained?

@rcoh
Copy link
Collaborator

rcoh commented Jan 13, 2021

Interesting. The smithy spec mandates that things marked as httpLabel are required:

httpLabel members MUST be marked as required trait.

From https://awslabs.github.io/smithy/spec/http.html#httplabel-trait

Part number is actually in httpQuery:

                "PartNumber": {
                    "target": "com.amazonaws.s3#PartNumber",
                    "traits": {
                        "smithy.api#documentation": "<p>Part number of the object being read. This is a positive integer between 1 and 10,000.\n         Effectively performs a 'ranged' GET request for the part specified. Useful for downloading\n         just a part of an object.</p>",
                        "smithy.api#httpQuery": "partNumber"
                    }
                },

Is that modeled correctly?

@rcoh
Copy link
Collaborator

rcoh commented Jan 13, 2021

The underlying field is unboxed which is why it isn't modeled as Option<u32>

@rcoh
Copy link
Collaborator

rcoh commented Jan 13, 2021

The actual issue here is that the Http protocol binding traits send 0 in the query param when the field is unset. Although this behavior is technically within spec, in the case of GetObject (as well as many other APIs) sending 0 is actually an error.

@rcoh
Copy link
Collaborator

rcoh commented Jan 13, 2021

Closed and opened #108

@rcoh rcoh closed this as completed Jan 13, 2021
milesziemer added a commit to milesziemer/smithy-rs that referenced this issue Nov 21, 2023
Removes logic from RustWriter that was used to not serialize
zero values, ie. 0 for numbers and false for booleans, if
that was also the default value for the member. With this change,
the only time the default value for a member is not serialized is
if the member is also `@clientOptional`.

The Blob type was also updated to implement std::Default, since
SymbolVisitor treats `""` as a RustDefault.

It _seems_ like this logic was originally added to fix issues with
[S3](smithy-lang#102) which
are technically modeling issues and have since been fixed.
However, this change impacts all the protocols and will cause
many members to now be sent over the wire that previously weren't.
@milesziemer milesziemer mentioned this issue Nov 21, 2023
4 tasks
milesziemer added a commit to milesziemer/smithy-rs that referenced this issue Nov 21, 2023
Removes logic from RustWriter that was used to not serialize
zero values, ie. 0 for numbers and false for booleans, if
that was also the default value for the member. With this change,
the only time the default value for a member is not serialized is
if the member is also `@clientOptional`.

The Blob type was also updated to implement std::Default, since
SymbolVisitor treats `""` as a RustDefault.

It _seems_ like this logic was originally added to fix issues with
[S3](smithy-lang#102) which
are technically modeling issues and have since been fixed.
However, this change impacts all the protocols and will cause
many members to now be sent over the wire that previously weren't.
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

No branches or pull requests

2 participants