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

feat(AIP-123): lint for pattern segments matching the singular. #1146

Closed
toumorokoshi opened this issue May 8, 2023 · 6 comments
Closed

Comments

@toumorokoshi
Copy link
Contributor

From @noahdietz (#1143 (comment))

I think another idea for these annotations, singular and plural is to ensure that they appear in the pattern(s) as the resource_id segment and collectionId segment respectively.

@noahdietz
Copy link
Collaborator

@andrei-scripniciuc this would be a good starter rule to work on if you are interested. I'll detail the requirements a bit more in this issue in a bit.

@noahdietz
Copy link
Collaborator

Guidance from AIP-123 is as follows:

  • Singular must be the lower camel case of the type.
    • Pattern variables must be the singular form of the resource type e.g. a pattern variable representing a Topic resource ID is named {topic}.
  • Plural must be the lower camel case plural of the singular.
    • Pattern collection identifier segments must match the plural of the resources...

Given a message with a google.api.resource annotation, ensure the following:

  • the plural form is present in all pattern entries as the Collection Identifier
  • the singular is present in snake_case form in all pattern entries as the Resource ID variable segment

Example:

message FooBar {
  option (google.api.resource) = {
    type: "foo.googleapis.com/FooBar"
    singular: "fooBar"
    plural: "fooBars"

    // Valid because it contains `fooBars` as the collection ID
    // and {foo_bar} (singular: fooBar -> foo_bar) as the resource ID variable segment
    pattern: "projects/{project}/fooBars/{foo_bar}"

    // Invalid because it doesn't contain `fooBars` and doesn't contain `{foo_bar}`
    pattern: "organizations/{organization}/foobars/{foo_bar_id}"
  }
}

These should be separate rules (this issue I guess was opened for singular), one for each field, and should only be executed if:

  • the message is a resource (i.e. has google.api.resource)
  • the resource has singular OR plural, respectively

@noahdietz
Copy link
Collaborator

@andrei-scripniciuc just remove yourself if you don't have time :)

@noahdietz
Copy link
Collaborator

Oh, here is a "demo" PR for adding a new rule: https://github.com/googleapis/api-linter/pull/968/files

Notice that there are markdown files required for documentation.

@SanjayVas
Copy link

Note that this goes against the guidance in AIP-122 for shortening names of nested collections:

In this situation, the message and resource type are still called UserEvent; only the collection and resource identifiers in the pattern(s) are shortened. Since the resource type is not shortened, the singular and plural are similarly not shortened.

message UserEvent {
  option (google.api.resource) = {
    type: "example.googleapis.com/UserEvent"
    // Only the collection & resource identfiers in the `pattern` are shortened.    
    pattern: "projects/{project}/users/{user}/events/{event}"
    singular: "userEvent"
    plural: "userEvents"
  };

  string name = 1;
}

@noahdietz
Copy link
Collaborator

Thanks for pointing that out @SanjayVas ! I actually implemented the rule with that exact edge case in mind - just forgot this issue existed - but we are on the same page!

#1403

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

No branches or pull requests

4 participants