core::0136::http-uri-suffix naivety

[rofrankel]

core::0136::http-uri-suffix has a Known limitations section which states:

This rule naïvely assumes that the verb is always one word (the “noun” may be any number of words; they often include adjectives). This may cause some false positives, and the rule may be disabled in these situations.

Note: Before disabling the rule, consider whether the verb is properly represented as a single word. A common occurrence here is for words like “Signup”, “Rollout”, etc., which should prefer their single-word form.

Is there a reason for this? The relevant code seems to be:

rpcSlice := strings.Split(strcase.SnakeCase(m.GetName()), "_")
want = ":" + rpcSlice[0]

It seems like it should be possible to fix this code not to have this limitation. For example, shorten rpcSlice to exclude the last element if it matches the resource name (and rpcSlice has length > 1), and then set want to a joined/camelCased rpcSlice.

I can send a PR, but first want to check whether there's a reason it doesn't already work like this.

Thanks!

[noahdietz]

Good question, I don't have the historical examples. Since Custom Methods are kind of the catch-all for non-standard methods, RPC names can be almost as permissive making it tough to assume much - hence the admittedly naive parsing.

I'm hesitant to try and change things now without having a good way to experiment with the change to see how many more things it catches vs. how many more false positives it emits.

Can you give me an example of where the current logic is complaining about something it shouldn't be? Or that you perceive as possible and would be covered by the proposed logic?

[rofrankel]

Yep, it rejects a custom method named SlowBurn with the verb slowBurn:

  - message: Custom method should have a URI suffix matching the method name, such
      as ":slow".
    location:
      start_position:
        line_number: 207
        column_number: 5
      end_position:
        line_number: 209
        column_number: 6
    rule_id: core::0136::http-uri-suffix
    rule_doc_uri: https://linter.aip.dev/136/http-uri-suffix

Here is the relevant definition:

  // Burn the book slowly, rotisserie style.
  //
  // Takes longer than just throwing it in a bonfire, but so much more satisfying.
  rpc SlowBurnBook(SlowBurnBookRequest) returns (google.longrunning.Operation) {
    option (google.api.http) = {
      delete: "/v1/{name=publishers/*/books/*}:slowBurn"
    };
    option (google.longrunning.operation_info) = {
      response_type: "google.protobuf.Empty"
      metadata_type: "SlowBurnBookMetadata"
    };
  }

(This admittedly weird toy example is from an extension of the AIP library example; in this case the goal is to test custom methods that use different HTTP methods in combination with long-running operations.)

[rofrankel]

Anyway, thanks for the context. I think we'll probably make the fix locally -- we were just waiting to see if there was some good reason not to that you might know about -- but happy to upstream it if you like.

[noahdietz]

Thanks for the example. Maybe this is nit picking the example, but something like that doesn't conform to the statement in AIP-136 The name of the method should be a verb followed by a noun, so I am less inclined to make a change. I think your still stands though, so...

I would like to take your suggested code change and figure out how to experiment with it across a broader set of APIs to compare the effectiveness. So, I will leave this open for now, but please feel free to move forward with your fork change and I will let you know if we want to accept it upstream so that you can open PR and get the contribution ;)

Does that sound OK?

[toumorokoshi]

drive-by thought: we could do some other more flexible matching, like see if the custom method name after the : is matched by a lowercase-ing of the RPC. so "slowBurn" would be handled there.

Without touching the conversation around the verb side of the AIPs, this would at least ensure consistency.