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

Unnecessary use of Option<T> on required fields #837

Closed
allan2 opened this issue Nov 5, 2021 · 5 comments
Closed

Unnecessary use of Option<T> on required fields #837

allan2 opened this issue Nov 5, 2021 · 5 comments
Labels
breaking-change This will require a breaking change client sdk-ga

Comments

@allan2
Copy link
Contributor

allan2 commented Nov 5, 2021

An example is HostedZone, which has required fields that are wrapped in Option.

#[non_exhaustive]
pub struct HostedZone {
    pub id: Option<String>,                      // required
    pub name: Option<String>,                    // required
    pub caller_reference: Option<String>,        // required
    pub config: Option<HostedZoneConfig>,
    pub resource_record_set_count: Option<i64>,  // probably is always Some(i64)
    pub linked_service: Option<LinkedService>,
}

The Go API Reference states that those fields are required.

The JS API also has a similar typing problem when generating from Smithy . Their HostedZone has the type string | undefined for id. There is an open issue: Why is everything possibly undefined? (& other typing/doc questions)

Is there an authoritative source on whether fields are required for the AWS SDK?
It would be nice to have direct access to fields if they are always Some<T> 🙂

References
[1] HostedZone, Go API Reference
[2] HostedZone, JavaScript v3 API Reference

@jdisanti
Copy link
Collaborator

Take a look at awslabs/aws-sdk-rust#163 and feel free to join in on the conversation there.

I think the best way to tell if a field is required right now is to look at the service's API docs, which typically state the requiredness of each field. For example, for CreateHostedZone, the fields have a Required: Yes:
https://docs.aws.amazon.com/Route53/latest/APIReference/API_CreateHostedZone.html

This isn't ideal though, and we're working on improving it.

@rcoh rcoh added this to the GA milestone Dec 13, 2021
@llgerard
Copy link

Linked issue: #1095

@david-perez
Copy link
Contributor

@llgerard Just want to note that #1095 is unrelated to this issue, since #1095 is for the server implementation, while this one is for the client. The client does not have as much freedom as the server, since it needs to be able to work with older versions of the Smithy model, and the spec says that removing the required trait is a backwards-compatible change.

@jdisanti jdisanti added client breaking-change This will require a breaking change labels Sep 29, 2022
@Velfi
Copy link
Contributor

Velfi commented Oct 20, 2022

I'm going to close this in favor of aws-sdk-rust#536, which covers the same subject.

@Velfi Velfi closed this as completed Oct 20, 2022
@Velfi Velfi removed this from the SDK GA milestone Oct 20, 2022
@jdisanti
Copy link
Collaborator

It's being tracked in #1767 on the smithy-rs side.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This will require a breaking change client sdk-ga
Projects
None yet
Development

No branches or pull requests

6 participants