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

AIP-154: etag field_behavior #1395

Closed loeffel-io closed 3 months ago

loeffel-io commented 5 months ago

AIP-154 mentions

Etags on request methods

On a request message, the etag field should be given a behavior annotation - either REQUIRED or OPTIONAL. See AIP-203 for more information.

But there is a linter error when using REQUIRED:

- file_path: mindful/earth/user/v1/user.proto
  problems:
    - message: Delete RPCs must only require fields explicitly described in AIPs, not "etag".
      location:
        start_position:
            line_number: 282
            column_number: 3
        end_position:
            line_number: 282
            column_number: 59
        path: mindful/earth/user/v1/user.proto
      rule_id: core::0135::request-required-fields
      rule_doc_uri: https://linter.aip.dev/135/request-required-fields

message:

message DeleteUserRequest {
  // The resource name
  // Format: users/{user}
  string name = 1 [
    (google.api.resource_reference) = {type: "user.mindful.com/User"},
    (google.api.field_behavior) = REQUIRED,
    (validate.rules).string = {pattern: "^users\\/[a-f0-9]{8}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{4}-[a-f0-9]{12}"}
  ];
  // The etag of the user. Must match the server's etag.
  string etag = 2 [(google.api.field_behavior) = REQUIRED];
}
noahdietz commented 3 months ago

Yep, you are definitely right. Sorry for not seeing this bug earlier, it's been a bit crazy.

Fixing in #1414 and looking at the other RPCs

noahdietz commented 3 months ago

We had an internal bug open to look at this for Standard Get as well. I won't proceed with the change yet as there is still some debate there. This will be marked fixed when I release the ApiLinter, which will happen on Monday (no Friday releases) reviews permitting.

loeffel-io commented 3 months ago

Thanks @noahdietz 🙏