feat: implement a check for AIP-193 (#911)

[maxim-tschumak]

Signed-off-by: Maxim Tschumak [email protected]

[noahdietz]

Thanks for this but I'm not sure which guidance this rule actually applies to. There is no guidance in AIP-193 that states fields named status, error, warning must be of type google.rpc.Status or that an API must just this in their response message. This AIP is directed at using google.rpc.Status as the error response format

[maxim-tschumak]

Hi Noah, this PR is more of a question whether the check can be useful here.

The AIP states that we need to use standardized types for error/warning fields in the response message (Services must return a google.rpc.Status message when an API error occurs). The idea of this check is to have a list of top-level field names for output type that suppose to be typed with the standardized message type. While the list is not explicitly specified in the AIP, I think this implementation is likely to catch such violations without producing false-positives.

If you think it's too much interpretation of the AIP, then let's drop this PR. We plan to use the check in our sub-org and I can simply move the implementation to our custom package. I've just thought it may be useful for a broader audience.

[noahdietz]

The idea of this check is to have a list of top-level field names for output type that suppose to be typed with the standardized message type.

To make sure I understand (and please tell me if I don't): This lints response messages and ensures that "if a field exists with one of the targeted names (e.g. status), it must be type google.rpc.Status"?

This ^ is an over-interpretation of the AIP. The AIP is not concerned with a field in the response message, but rather the proto/schema used to construct the error returned by server, which must be google.rpc.Status. For gRPC, the gRPC Status is based on this proto already. For HTTP 1.1./JSON APIs they must use this proto's JSON schema to construct the response payload (see https://cloud.google.com/apis/design/errors also).

There are many cases where a field of type google.rpc.Status is recommended (https://google.aip.dev/search?q=status), but those are generally for async jobs that wrap some other response (e.g. google.longrunning.Operation).

If I'm not tracking, lmk and maybe add an example :)

[maxim-tschumak]

To make sure I understand (and please tell me if I don't): This lints response messages and ensures that "if a field exists with one of the targeted names (e.g. status), it must be type google.rpc.Status"?

Yes, this is the intent of the check. "Use standard message type for errors". For example, this would generate a violation if API author specifies a top-level error field in the response message of type string (and not the standardized google.rpc.Status). I've seen a couple of such use-cases.

I still think it's a valid info-level check, but yes, it is maybe a over-interpretation. Feel free to drop this PR. I can move the implementation to our custom rule package.

[noahdietz]

@shwoodard WDYT about the above?

[noahdietz]

For example, this would generate a violation if API author specifies a top-level error field in the response message of type string (and not the standardized google.rpc.Status).

Yeah, I think I agree with this. There are a number of places in the AIPs that recommend having a google.rpc.Status field and refer to AIP-193, but there is no mention of what the field name should be. I think we should:

1. Update AIP-193 to put such guidance into words
2. Update the linter to enforce that a field with one of the allowed names must be of type google.rpc.Status (what this PR has)
3. Update the linter to enforce that a field of type google.rpc.Status is named with an allowed name (inverse check of this PR)
4. Update any AIP recommending use of google.rpc.Status as a field (151, 152, 153 afaik) to explicitly refer to the 193 guidance

[maxim-tschumak]

Please let me know once the decision has been made and the guideline is updated. I'm happy to implement 3 too.

[noahdietz]

I don't think this is currently lintable. Closing for the time being.