googleapis / api-linter

A linter for APIs defined in protocol buffers.
https://linter.aip.dev/
Apache License 2.0
599 stars 144 forks source link

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

Closed SanjayVas closed 3 months ago

SanjayVas commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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 commented 3 months ago

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