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

fix(aip-130): identify standard and custom methods #1396

Merged

Conversation

liveFreeOrCode
Copy link
Contributor

@liveFreeOrCode liveFreeOrCode commented Jun 4, 2024

There is a problem with the Response Message Name linter of AIP-136 currently.

For reference: https://github.com/googleapis/api-linter/blob/main/rules/aip0136/response_message_name.go

This, and all linter methods in aip0136 used a method isCustomMethod. The logic of this method is incomplete/incorrect. It assumed anything with a ":" in it's URI was considered a custom method. But, according to https://google.aip.dev/130, Batch methods are also considered Standard methods. Batch methods have the following URI segments: :batchGet, :batchUpdate, :batchCreate and :batchDelete.

What happens?

service Library {
  // This fails the aip0136 response_message_name lint rule
  rpc BatchDeleteBooks(BatchDeleteBooksRequest) returns (google.protobuf.Empty) {
    option (google.api.http) = {
      post: "/v1/{parent=publishers/*}/books:batchDelete"
      body: "*"
    };
  }
}

This patch uses regex to identify standard, and therefore custom
methods can be considered anything non-standard.
@liveFreeOrCode liveFreeOrCode requested a review from a team as a code owner June 4, 2024 16:13

// Methods with no `:` in the URI are standard methods if they begin with
// one of the standard method names.
for _, prefix := range []string{"Get", "List", "Create", "Update", "Delete", "Replace"} {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Replace was maybe still in here for backwards-compatibility. But do we want that? The current rules don't advocate for Replace methods being standard methods... so why allow it in the latest version of the linter?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave it out. Thanks for calling this out.

@@ -39,14 +39,8 @@ func TestURISuffix(t *testing.T) {
{"ValidOneWord", "Translate", "/v3:translate", testutils.Problems{}},
{"ValidStdMethod", "GetBook", "/v1/{name=publishers/*/books/*}", testutils.Problems{}},
{"ValidTwoWordNoun", "WriteAudioBook", "/v1/{name=publishers/*/audioBooks/*}:write", testutils.Problems{}},
{"ValidListRevisions", "ListBookRevisions", "/v1/{name=publishers/*/books/*}:listRevisions", testutils.Problems{}},
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The History of resource revisions indicates these used to be custom methods, hinting at the fact that they are no longer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, AIP-162 is being reworked...technically there are still instances of these methods, but new ones are no longer allowed. Let's roll with this for now, I think they will still pass most custom method rules anyways.

Copy link
Collaborator

@noahdietz noahdietz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, good catch. Good reminder for me that double checking the heuristic(s) used to identify things should be reviewed regularly...while checking for : in the HTTP path, it is incomplete...


// Methods with no `:` in the URI are standard methods if they begin with
// one of the standard method names.
for _, prefix := range []string{"Get", "List", "Create", "Update", "Delete", "Replace"} {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can leave it out. Thanks for calling this out.

@@ -39,14 +39,8 @@ func TestURISuffix(t *testing.T) {
{"ValidOneWord", "Translate", "/v3:translate", testutils.Problems{}},
{"ValidStdMethod", "GetBook", "/v1/{name=publishers/*/books/*}", testutils.Problems{}},
{"ValidTwoWordNoun", "WriteAudioBook", "/v1/{name=publishers/*/audioBooks/*}:write", testutils.Problems{}},
{"ValidListRevisions", "ListBookRevisions", "/v1/{name=publishers/*/books/*}:listRevisions", testutils.Problems{}},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, AIP-162 is being reworked...technically there are still instances of these methods, but new ones are no longer allowed. Let's roll with this for now, I think they will still pass most custom method rules anyways.

@noahdietz noahdietz merged commit be41d72 into googleapis:main Jun 4, 2024
6 checks passed
@jskeet
Copy link

jskeet commented Jul 23, 2024

(User reported for spam...)

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

Successfully merging this pull request may close these issues.

3 participants