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-162 – Resource revisions #35

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

feat: AIP-162 – Resource revisions #35

wants to merge 5 commits into from

Conversation

lukesneeringer
Copy link
Contributor

No description provided.

@lukesneeringer lukesneeringer requested a review from a team as a code owner May 19, 2021 18:56
@google-cla google-cla bot added the cla: yes label May 19, 2021
particular resource, and intentionally avoid the term _version_, which refers
to the version of an API as a whole.

## Guidance
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • @hudlow: We should rewrite this to merge etags and revision IDs semantically, and say that you can preserve etags.
    • During discussion, this held up quite well.
  • @hudlow: Also need discussion about whether every field constitutes a new revision or whether the service can decide specific fields.

publishers/123/books/les-miserables@c7cfa2a8

**Note:** The `@` character is selected because it is the only character
permitted by [RFC 1738 §2.2][] for special meaning within a URI scheme that is
Copy link
Contributor

@gibson042 gibson042 May 25, 2021

Choose a reason for hiding this comment

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

What is the significance of RFC 1738 (which is obsolete)? https://datatracker.ietf.org/doc/html/rfc3986 is the IETF document most relevant to URLs (and https://datatracker.ietf.org/doc/html/rfc3987 for not-necessarily-ASCII analogs).

Choose a reason for hiding this comment

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

I won't claim any competence at ABNF, but my reading of RFC 3987 section 3.3, particularly this part:

pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"

is that "@" is allowed in path segments, so using it to designate a revision is fine as long as we are clear that we've taken it for that and it should not be used otherwise.


**Note:** The `@` character is selected because it is the only character
permitted by [RFC 1738 §2.2][] for special meaning within a URI scheme that is
not already used elsewhere.
Copy link
Contributor

Choose a reason for hiding this comment

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

It still seems to carry a collision risk, though—using the example above, what if a book's title itself includes "@"?

application/json:
schema:
$ref: '#/components/schemas/Book'
/publishers/{publisherId}/books/{bookId}:listRevisions:
Copy link
Contributor

@gibson042 gibson042 May 25, 2021

Choose a reason for hiding this comment

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

This deviates too far from RESTful best practices; the path looks like it should return a Book but it actually returns a collection of books. I would feel much more comfortable with modeling revisions as either subresources (e.g., /publishers/{publisherId}/books/{bookId}/revisions) or a parallel collection to the revision-capable resources (e.g., /publishers/{publisherId}/bookRevisions).

the response.

If necessary, APIs **may** explicitly support listing child resources across
parent revisions by accepting the `@-` syntax. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

This also is quite ugly, I think it's arguing against the @ patterns.

Copy link
Contributor

Choose a reason for hiding this comment

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

Note also that it prohibits use of - as a revision identifier.

Copy link

@mkistler mkistler left a comment

Choose a reason for hiding this comment

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

Looks good. 👍

@gibson042 and @hudlow make some good points that we should address, but I think that can be done in a follow-on PR if necessary.

I fixed up the oas example -- pls make sure I didn't inadvertently break something.

aip/general/0162/aip.md.j2 Outdated Show resolved Hide resolved
aip/general/0162/aip.md.j2 Outdated Show resolved Hide resolved
aip/general/0162/aip.md.j2 Outdated Show resolved Hide resolved

- The resource **must** contain a `revision_id` field, which **should** be a
string and contain a short, automatically-generated random string. A good
rule of thumb is the last eight characters of a UUID4.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rule of thumb is the last eight characters of a UUID4.
rule of thumb is the last eight characters of a version 4 UUID.

the response.

If necessary, APIs **may** explicitly support listing child resources across
parent revisions by accepting the `@-` syntax. For example:
Copy link
Contributor

Choose a reason for hiding this comment

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

Note also that it prohibits use of - as a revision identifier.

aip/general/0162/aip.yaml Outdated Show resolved Hide resolved
Changes as agreed in working meeting on 11/15/2022.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants