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

Documentation uses the word "mandated" for requirement levels other than MUST #1425

Closed
SanjayVas opened this issue Aug 28, 2024 · 4 comments · Fixed by #1426
Closed

Documentation uses the word "mandated" for requirement levels other than MUST #1425

SanjayVas opened this issue Aug 28, 2024 · 4 comments · Fixed by #1426

Comments

@SanjayVas
Copy link

SanjayVas commented Aug 28, 2024

The term "mandated" implies a requirement level of MUST. The API linter documentation is misleading in this regard, as it uses that term even when the requirement level is SHOULD.

Example: core::0134::request-mask-required

This rule enforces that all Update standard methods have a field in the request message for the field mask, as mandated in AIP-134.

However in the AIP in question:

The method should support partial resource update

and

If partial resource update is supported, a field mask must be included.

The must here is conditional on the should above, meaning this is not actually mandated by the AIP.

@noahdietz
Copy link
Collaborator

noahdietz commented Aug 28, 2024

That's fair feedback.

Would you see the rule documentation verbiage softened e.g. as recommended in AIP-134? If so, please feel free to send a PR to update the rule doc.

The method should support partial resource update

This is a separate discussion, but having a PATCH that only supports full replace isn't a PATCH, it's a PUT without the POST-semantics if missing. So I'm wondering why we'd want to qualify inclusion of update_mask on Standard Update at all. This is probably a separate issue though. Did you have an example of a Standard Update that doesn't support partial updates? thanks!

Edit: typo

@SanjayVas
Copy link
Author

Did you have an example of a Standard Update that doesn't support partial updates? thanks!

This is very easy to consider in theory. Until/unless you add any new fields, there's no need to bother implementing partial update support (especially if you require etags). It can always be added later, meaning adding any new mutable field to the resource would require adding an update_mask field for backwards compatibility.

@SanjayVas
Copy link
Author

Note that there's not always much value in supporting update masks. For example, if the only mutable field on your resource is an unordered list. Furthermore, protobuf libraries for FieldMask don't support all of AIP-161 (e.g. wildcards and maps). This further reduces the cases where update masks are useful.

@noahdietz
Copy link
Collaborator

I see and you're right in a lot of ways, thanks for taking the time to respond. Fixing the rule docs!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants