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-159 – Reading across collections #36

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

Conversation

lukesneeringer
Copy link
Contributor

No description provided.

@lukesneeringer lukesneeringer requested a review from a team as a code owner June 14, 2021 21:03
@google-cla google-cla bot added the cla: yes label Jun 14, 2021
GET /v1/publishers/-/books?filter=...
```

- The URI pattern **must** still be specified with `*` and permit the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is protobuf specific.

to be unique.
- However, services **must not** support reading across collections on `Get`
requests if the child resources might have a collision.
- Cross-parent requests **should not** support `order_by`. If they do, the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems very Google-specific because Google uses this most often for locations. For situations where there is a single backend / database, this guidance is unimportant.

collection to be specified; a URI pattern **must not** hard-code the `-`
character.
- The operation **must** explicitly document that this behavior is supported.
- The resources provided in the response **must** use the canonical name of the
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This has some Google-specific assumptions about what a name is, and why the name would contain the parent ID at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That said, if a parent ID is provided, it should be filled out (publisher_id should never come back as -).

in a standard [`List`][aip-132] operation:

```
GET /v1/publishers/-/books?filter=...
Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • @mkistler Maybe suggest both the Google and IBM approach, with guidance to pick one.
    • Maybe suggest the rationale behind both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I forgot what the IBM approach was. :-(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The IBM approach is creating a new top-level collection.

One approach is to evade the problem entirely by promoting resources above
their erstwhile parents in the event that reading across collections is likely
to be a requirement.

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 Under this approach, a service may represent the book in multiple places; e.g. top-level collection and a subcollection under publishers.
    • This is usually done when it is many-to-many though and this is one-to-many, so we would not do it here.
    • @lukesneeringer Is this a fundamentally different pattern if it is many-to-many?
    • @hudlow The other pattern is we would say that there must be a canonical path (you can not have it return a different href in the body no matter what).
    • @lukesneeringer Probably we should say that you may have multiple paths but must have a single canonical path, and move the many-to-many case @hudlow mentions to AIP-144.
    • @mkistler The spirit of the AIP is "how do I get to all members of a subcollection regardless of parent"; I feel uncomfortable saying "just promote it". I believe they should not be removed as subcollections.
      • @lukesneeringer Does this violate the "only one way to do it" principle?
      • @mkistler Not for reading across collection.
      • @lukesneeringer I just realized an answer to my own objection: What if you can only GET the virtual collection, but not the individual resources?
        • @hudlow 404 seems a bit rude, maybe 308?
        • @lukesneeringer If you 404, you can allow IDs that are unique to the scope of the collection rather than globally unique. Maybe say that you may do this and 404, or may have gloablly-unique IDs and 308?
        • @hudlow This still does the thing @Alfus hates though.
  • @lukesneeringer Note: The book IDs must be unique across the collections if this approach is used.
  • @hudlow It is not obvious to me how y'all (Google) decide how to make things into subcollections. Surely sometimes you come across a situation where something has a strong parent-child relationship with multiple parents.
    • @lukesneeringer We do, but every resource remains a child of a single parent. A book that is a child of an author is not a child of a publisher.

Consensus:

  • The top-level collection is called a "virtual collection".
    • For individual resources, you may 308 the non-canonical single-resource GET if IDs are globally unique, or 404 the non-canonical single-resource GET if they are unique only in the scope of the collection.
    • The resource must provide the parent ID in the body of the resource somewhere.

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. 👍

I left a few minor suggestions but these aren't essential.

I think we should add proto and oas example for this AIP in the future.

@@ -0,0 +1,98 @@
# Reading across collections

Sometimes, it is useful for a user to be able to retrieve resources across

Choose a reason for hiding this comment

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

Suggested change
Sometimes, it is useful for a user to be able to retrieve resources across
Sometimes it is useful for a user to be able to retrieve resources across

Comment on lines +64 to +67
- The URI pattern **must** still be specified with `*` and permit the
collection to be specified; a URI pattern **must not** hard-code the `-`
character.
- The operation **must** explicitly document that this behavior is supported.

Choose a reason for hiding this comment

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

I think these lines can be deleted as they seem to simply restate lines 23-25

Suggested change
- The URI pattern **must** still be specified with `*` and permit the
collection to be specified; a URI pattern **must not** hard-code the `-`
character.
- The operation **must** explicitly document that this behavior is supported.

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.

2 participants