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

Tracking issue: spurious Maybes in request/response bodies #739

Open
endgame opened this issue Dec 16, 2021 · 2 comments
Open

Tracking issue: spurious Maybes in request/response bodies #739

endgame opened this issue Dec 16, 2021 · 2 comments

Comments

@endgame
Copy link
Collaborator

endgame commented Dec 16, 2021

At some point, the generator was changed to emit Maybe values for all optional fields, even for types that had a sensible "empty" value (e.g., maps and lists). Among other benefits, this made it possible to detect empty lists and maps in DynamoDB AttributeValues (before #724 introduced an AST and solved things properly).

There's a big QoL downside to this, particularly for responses: it makes a lot of request/response fields look optional, and when handling responses you're forced to write partial code or handle errors that should never appear.

I've tried raising issues for these in botocore, but AWS service teams have a habit of not being very thorough when marking things as required in their service definitions (and changing this risks backwards-compat problems now).

Fixing these is not that difficult; it's just an additional requiredFields entry in configs/$SERVICE.json and would make a good first PR. If you make an issue or PR about one of these, please:

  1. Mention this issue;
  2. Provide proof that the field is always present (in the form of AWS documentation or a link to code in the SDK); and
  3. Tag it service config
  4. Regenerate the relevant service by running scripts/generate --commit $serviceabbrev (e.g., dynamodb)

If you have questions about a specific field and want to discuss whether or not to make a PR, please create an issue for it, mention this issue in its description, and give it the service config label.

@endgame endgame pinned this issue Dec 16, 2021
@ysangkok
Copy link
Contributor

I am a bit unsure about the Value field on the Parameter type of AWS Systems Manager (formerly known as SSM).

It is required according to docs on
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-resource-ssm-parameter.html

These docs don't mention anything about optionality, but they do mention the field and supply it in their examples:

In the Go bindings, it is a pointer, which suggests that it is optional. But it seems like everything is a pointer.
https://docs.aws.amazon.com/sdk-for-go/api/service/ssm/#Parameter

This piece of Parameter documentation claims it is not required:
https://docs.aws.amazon.com/systems-manager/latest/APIReference/API_Parameter.html

So what to do? Don't know whether I should make a PR.

@endgame
Copy link
Collaborator Author

endgame commented Dec 20, 2021

@ysangkok I moved your question to a new issue, otherwise this issue will get confusing over time. Actually, I think I'm going to lock it to prevent the from happening.

Repository owner locked and limited conversation to collaborators Dec 20, 2021
Repository owner unlocked this conversation Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants