Skip to content

Change should to must for Resources #926

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.

Sign up for GitHub

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

Open
shwoodard wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Change should to must for Resources #926

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions aip/general/0131.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,10 @@ resource.

## Guidance

APIs **should** generally provide a get method for resources unless it is not
valuable for users to do so. The purpose of the get method is to return data
from a single resource.
APIs **must** provide a `Get` method *for resources* and **should** do so otherwise,
Copy link
Copy Markdown
Contributor

@GregFurmanek GregFurmanek Aug 3, 2022

Choose a reason for hiding this comment

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

I am not sure if this is clear.

I am guessing the intent here is to enforce existence of get unless justified otherwise.
As in: "do not leave this field blank" Either implement or justify why not. Right?

Copy link
Copy Markdown
Contributor Author

@shwoodard shwoodard Aug 3, 2022

Choose a reason for hiding this comment

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

Is it a field or an rpc?

Copy link
Copy Markdown
Contributor

@GregFurmanek GregFurmanek Aug 3, 2022

Choose a reason for hiding this comment

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

metafor for RPC

Copy link
Copy Markdown
Contributor

@bgrant0607 bgrant0607 Aug 3, 2022

Choose a reason for hiding this comment

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

Reasons to not provide a Get method cannot not be open-ended. There must be a specified set of permitted exceptions and specific guidance regarding what to do for each exceptional case instead.

Copy link
Copy Markdown
Contributor

@jgeewax jgeewax Aug 4, 2022

Choose a reason for hiding this comment

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

I'm confused by this wording also.... It sounds like we're saying "You must unless ", which is not what "must" means... We copy RFC-2119 (as stated in AIP-8), so if we're going to make this must, there can be no exceptions. If we want exceptions, this must remain as "should". I also agree it should be should.

Assuming that it remains "should", I agree with @bgrant0607 that we should be explicit about guidance for not providing a Get method. Do we have a list of those cases? If not, we might have to leave it ambiguous and circle back with a list of specific exceptions based on our experience.

Copy link
Copy Markdown
Contributor

@bgrant0607 bgrant0607 Aug 4, 2022

Choose a reason for hiding this comment

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

The omission of Get from https://google.aip.dev/128 was an oversight. We could constrain "must" to declarative-friendly APIs.

Are there legitimate cases for not providing Get of a resource, at least for new services? Good question. I'm not aware of any offhand.

One case that comes up is resources containing sensitive data. However, sensitive user data really should not be represented in resource attributes. Resources ideally should be considered non-sensitive "metadata" rather than user data. Sensitive user data belongs in an appropriate storage system like a secret manager.

Other cases I'm aware of require performing a List, then extracting the specific resource in the client. We should definitely recommend against that.

Copy link
Copy Markdown
Contributor

@jgeewax jgeewax Aug 4, 2022

Choose a reason for hiding this comment

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

Are there legitimate cases for not providing Get of a resource, at least for new services? Good question. I'm not aware of any offhand.

I also don't have one on the top of my head, but I can say for sure there have been scenarios where this came up, and I recall thinking "OK yea, that makes sense. Go ahead."

Copy link
Copy Markdown
Contributor

@bgrant0607 bgrant0607 Aug 4, 2022

Choose a reason for hiding this comment

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

The 2 cases I mentioned were the ones I've seen. I would include both and indicate what the recommended alternatives are.

@loudej has thoughts also

unless there is a strong reason not to do so. An example of such, would be if there
is sensitive data that would be included in the response. The purpose of the `Get` method
Copy link
Copy Markdown
Contributor

@GregFurmanek GregFurmanek Aug 3, 2022

Choose a reason for hiding this comment

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

Are we capitalizing get?

Copy link
Copy Markdown
Contributor Author

@shwoodard shwoodard Aug 3, 2022

Choose a reason for hiding this comment

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

I prefer it as long as it has backticks. It is a proper thing, imho.

Copy link
Copy Markdown
Contributor

@jgeewax jgeewax Aug 4, 2022

Choose a reason for hiding this comment

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

Precedent AFAICT is "Get" and "get". I think we should try to always use Get in AIPs to make it explicit that we're talking about a method. In aipdesignbook I try to use "Get" throughout to clarify.

is to return data from a single resource.

Get methods are specified using the following pattern:

Expand Down