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-203): add resource name IDENTIFIER enforcement #1241

Merged
merged 5 commits into from
Aug 30, 2023

Conversation

noahdietz
Copy link
Collaborator

@noahdietz noahdietz commented Aug 24, 2023

Enforces AIP-203 guidance added in aip-dev/google.aip.dev#1201 that the resource's name field must have google.api.field_behavior = IDENTIFIER.

@noahdietz noahdietz marked this pull request as ready for review August 28, 2023 22:38
@noahdietz noahdietz requested a review from a team as a code owner August 28, 2023 22:38
@noahdietz noahdietz merged commit 7d9daab into googleapis:main Aug 30, 2023
6 checks passed
@noahdietz noahdietz deleted the field-behavior-identifier branch August 30, 2023 22:49
@sam-utila
Copy link

sam-utila commented Aug 31, 2023

This breaks the lint for me.
I have an identifier, but it still fails on linting.

edit: This code explicitly check for "IDENTIFIER" and fails on anything else (i.e. OUTPUT_ONLY, REQUIRED, etc...)

I wonder how it did not fail any other test

@noahdietz
Copy link
Collaborator Author

@sam-utila can you open an issue instead of commenting on the PR? Also please include the proto that is failing.

worth noting this hasn't been released yet so you are using main which is probably not advisable.

@sam-utila
Copy link

Pardon me - I wasn't aware a new IDENTIFIER value is now allowed, should probably update my googleapis deps.

However - I wonder if this will continue to work for names that were marked as OUTPUT_ONLY and are filtered automatically from requests by tooling such as grpc-gateway/v2/protoc-gen-openapiv2.

Anyway I had to use main instead of latest since latest was broken for me and was fixed with the recent https://github.com/googleapis/api-linter/pull/1239 (specifically this commit: jhump/protoreflect@f770609).

@noahdietz
Copy link
Collaborator Author

I wonder if this will continue to work for names that were marked as OUTPUT_ONLY and are filtered automatically from requests by tooling such as grpc-gateway/v2/protoc-gen-openapiv2.

I can't speak to grpc-gateway's implementation of filtering based on field_behavior, that is their own implementation derived from presence of field_behavior annotations. I'd recommend filing an issue on their repo.

Please read the AIP-203 changes for the use of and reasoning for adding IDENTIFIER.

was broken for me and was fixed with the recent

I see makes sense! Consider using a specific version instead of latest so your API doesn't suddenly have issues if you are blocking development based on linter findings.

@sam-utila
Copy link

No problem, considering I can always put new linting rules on disabled.

Just wondering that sometimes IDENTIFIERs can also be OUTPUT_ONLY and this is something that is now not encouraged by this AIP - haven't sinked in yet :-) .

@flyfire2002
Copy link

I too am interested in the implication for name fields currently marked as OUTPUT_ONLY. I guess semantically IDENTIFIER doesn't preclude a behavior that is OUTPUT_ONLY ("Server ignores it if set as input"). However now we don't have a language for https://google.aip.dev/203#output-only scenario 3 where the name is the immutable field.

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.

4 participants