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

Complete refactoring per API WG discussion on end-point redesign #202

Closed
tloubrieu-jpl opened this issue Jul 6, 2022 · 23 comments · Fixed by NASA-PDS/registry-api#177
Closed
Assignees

Comments

@tloubrieu-jpl
Copy link
Member

tloubrieu-jpl commented Jul 6, 2022

Follow on to NASA-PDS/registry-api#131

See slides on https://docs.google.com/presentation/d/1CyrstNQ04Vm3eQvUNSSezxYzdyoyv3pSQXNCahMM2cg/edit#slide=id.gf8baa2253f_0_1

@al-niessner
Copy link
Contributor

I see the proposal, but what was the conclusion?

Do you really want /classes/bundle/{id}/members instead of /products/{id}/members? What is the error if /classes/bundles/{id} is not a bundle?

@tloubrieu-jpl
Copy link
Member Author

Table this ticket for this sprint, so to focus on bug fixes

@jordanpadams jordanpadams changed the title Implement the conclusion of the API WG on end-point redesign Complete refactoring per API WG discussion on end-point redesign Aug 19, 2022
@jordanpadams
Copy link
Member

@al-niessner we are going forward with Proposal 2 from the slides. For the error message, something like Class does not exist works for now.

@al-niessner
Copy link
Contributor

al-niessner commented Aug 22, 2022

@jordanpadams @jimmie @tloubrieu-jpl

To be clear, these are all of the endpoints:

/products/{id}/members
/products/{id}/members/members
/products/{id}/member-of

/classes/bundles/{id}/members 
/classes/collections/{id}/members
/classes/documents/{id}/member-of
/classes/observational/{id}/member-of
/classes/observational/{id}/member-of/member-of

This seems like a short list but it is all of the endpoints listed for proposal 2. Seems like you may also want endpoints in proposal 1:

/products/{id}
/products/{id}/all
/products?q=... searches products (any class) for
/classes
/classes/bundles?q=... searches bundles for

Note: change searches for x to searches x for

@jordanpadams
Copy link
Member

@al-niessner sorry for the confusion.

we must support ALL existing endpoints AND the new endpoints from proposal 2

@al-niessner
Copy link
Contributor

al-niessner commented Aug 22, 2022

@jordanpadams

Existing as in endpoints in version 1.0.2 or as in endpoints in new approach? Proposal 1 seems to be the base ones where /gid changed to /classes and /uid changed to /products in the newer style and proposal 2 a rework of referencing. So, when you say ALL do you mean 1.0.2 endpoints plus proposal 1 and proposal 2?

@jordanpadams
Copy link
Member

@al-niessner sorry for the lack of clarity here.

By existing endpoints, I am referring to the currently released version 1.0.2, e.g. what is here: https://app.swaggerhub.com/apis-docs/PDS_APIs/registry/1.0.0

beyond that, we want the endpoints proposed with Proposal 2:

/products/{id}/members returns products contained directly in the current product
/products/{id}/members/members returns products indirectly contained in the current product
/products/{id}/member-of returns products directly containing the current product (e.g. collections, bundles)

/classes/bundles/{id}/members get collections of a bundle
/classes/collections/{id}/members get observational products or documents of a collection
/classes/documents/{id}/member-of
/classes/observational/{id}/member-of
/classes/observational/{id}/member-of/member-of

@jordanpadams
Copy link
Member

jordanpadams commented Aug 22, 2022

actually, you are right. implement everything:

  • current v1.0.2 API endpoints
  • Proposal 1 (search/queries)
  • Proposal 2 (hierarchy)

@al-niessner
Copy link
Contributor

@jordanpadams

If I can, do you want me to mark the older not represented by newer (no overlap) as deprecated? Obviously, /products/{identifier} is the same in both. Several overlap.

@al-niessner
Copy link
Contributor

al-niessner commented Aug 22, 2022

@jordanpadams @jimmie @tloubrieu-jpl

In the case of these:

/classes/bundles/{id}/members get collections of a bundle
/classes/collections/{id}/members get observational products or documents of a collection
/classes/documents/{id}/member-of
/classes/observational/{id}/member-of
/classes/observational/{id}/member-of/member-of

Do you want just those or do you want:

/classes/{class id}/{identifier}/members
/classes/{class id}/{identifier}/members/members
/classes/{class id}/{identifier}/members-of
/classes/{class id}/{identifier}/members-of/members-of

Obviously bundles and collections would be strange with bundles and members-of like an error message but keeps from having to maintain a bigger list that can expand wildly given the large number of PDS product classes.


Actually, never mind. Have the same error message with /products/{identifier}/member-of when the {identifier} is a bundle. So, just do it with latter and worry about messages later.

@al-niessner
Copy link
Contributor

al-niessner commented Aug 22, 2022

@jordanpadams @jimmie @tloubrieu-jpl

Have not added the old 1.0 version stuff, but have the proposal stuff in place at swagger pds-api-202.5

for unknown reasons swagger made me update versions again

@jordanpadams
Copy link
Member

@al-niessner just added a couple comments. but looks good.

@al-niessner
Copy link
Contributor

al-niessner commented Aug 22, 2022

@jordanpadams @jimmie @tloubrieu-jpl

These will be put into a deprecated group unless you want named something else.

  /bundles/{identifier}
  /bundles/{identifier}/all
  /bundles/{identifier}/latest
  /bundles/{identifier}/collections
  /bundles/{identifier}/collections/all
  /bundles/{identifier}/collections/latest
  /bundles/{identifier}/products
  /collections/{identifier}
  /collections/{identifier}/all
  /collections/{identifier}/latest
  /collections/{identifier}/bundles
  /collections/{identifier}/products
  /collections/{identifier}/products/all
  /collections/{identifier}/products/latest
  /products/{identifier}/bundles
  /products/{identifier}/bundles/all
  /products/{identifier}/bundles/latest
  /products/{identifier}/collections
  /products/{identifier}/collections/all
  /products/{identifier}/collections/latest

@al-niessner
Copy link
Contributor

@jordanpadams @jimmie @tloubrieu-jpl

All have been added. Check it out on swagger using link 3 comments above.

@al-niessner
Copy link
Contributor

@jordanpadams @jimmie @tloubrieu-jpl

Snag - as in stuck until clarified. The old API and the interim API that I ad-hoc did as a demonstration contained /latest and /all for references. The new API on the slides does not. Was this an omission on the slide but they should exist in the new API or did we get rid of them for good and only the latest are ever returned?

If we got rid of them, then what should be done with the /all in the old API that has been deprecated? Should those endpoints return an error or just return the latest ignoring the all request?

If they require /all and /latest then that is fine but it needs to be made clear.

@al-niessner
Copy link
Contributor

al-niessner commented Aug 31, 2022

@jordanpadams @jimmie @tloubrieu-jpl

New API is still broken. Trying to implement and not working out well as in not possible.

/products/{identifier}/member-of is supposed to mean what? If it depends on the type of the identifier then that really is going to slow down the API. It means have to look up the identifier to determine is product type then decide what member_of means. Can it mean multiple things like collection or mission or something else? If you change it to /products/{identifier}/member-of/{class} then that appropriately constrains the request and not defining class can mean any/all so that you get all members not just bundle->collection->product tree. Same goes for member and member/member and member-of/member-of.


Oh, I think I am beginning to understand the disconnect. The documentation is written as if /products/{identifier}/member-of means the identifier is neither collection nor bundle. However, there is nothing to prevent the user from using a bundle or collection identifier which then changes the meaning of member in all forms. It also limits the membership to just hierarchy (bundle->collection->product) removing references in general.

Am I reading the proposal correctly?

@jordanpadams
Copy link
Member

/products/{identifier}/member-of
this specifically applies to traversing the bundle->collection->product hierarchy. so for example:

/products/urn:nasa:pds:some_collection_identifier/member-of/

should return the latest version of the parent bundle(s)

/products/urn:nasa:pds:some_product_identifier/member-of/

should return the latest version of the parent collection(s)

/products/urn:nasa:pds:some_product_identifier/member-of/member-of

should return the latest version of the grandparent bundle(s)

@jordanpadams
Copy link
Member

looks like i should have finished reading your comment

It also limits the membership to just hierarchy (bundle->collection->product) removing references in general.

Correct. "Membership" is specifically related to organizational tree structure of the data (bundle->collection->product == trunk->branch->leaf).

I would considered "references" a much broader relationship type than "membership", including trunk<->trunk, branch<->branch, and leaf<->leaf relationships, which would not necessarily be considered "membership".

@jordanpadams
Copy link
Member

@jordanpadams @jimmie @tloubrieu-jpl

Snag - as in stuck until clarified. The old API and the interim API that I ad-hoc did as a demonstration contained /latest and /all for references. The new API on the slides does not. Was this an omission on the slide but they should exist in the new API or did we get rid of them for good and only the latest are ever returned?

If we got rid of them, then what should be done with the /all in the old API that has been deprecated? Should those endpoints return an error or just return the latest ignoring the all request?

If they require /all and /latest then that is fine but it needs to be made clear.

@al-niessner if we can, I would like to keep all and latest endpoints, with the default always going to latest when it is not specified. The rationale being /all provides the ability to display the provenance of a product.

@al-niessner
Copy link
Contributor

@jordanpadams @jimmie @tloubrieu-jpl

I will put the all and latest choices back in then.

As a user reading a document for a camera, I cannot use member-of to find all the instruments that use that camera -- as in the document is a member of the instrument. Are going to add something other than membership for this example?

@jordanpadams
Copy link
Member

@al-niessner possibly. TBD on whether we handle that explicitly via the API endpoints or just provide an example query. That specific example can be a bit tricky with PDS metadata.

@jordanpadams
Copy link
Member

But that is a valid example we will want to be able to support somehow

al-niessner pushed a commit to NASA-PDS/registry-api that referenced this issue Sep 1, 2022
Update swagger.yml to match the proposal plus any additions and clarification on the original ticket NASA-PDS/pds-api#202.

Re-arranged the code to fit the new API. Required some new exceptions and tweaks to the basic interfaces to support the membership concept outside of just referencing in general and that a lidvid necessarily match a particular product type.

Added new swagger transmutation layers to support the easy removal of deprecated code in the future and to support easier maintenance in the future. Left the old referencing suff only because there is a chance it may come back in the newish future to support some other use cases.

The new swagger transmutation layers still all funnel through a single processor once all of the inputs have been collected. The deprecated code mostly works itself through the new API showing that what could be done still can using the inew API. Did not want to update the standard handler to also check that desired class matches the lid(vid) so did not route those but they could also be done with a touch more code change just for that demonstration whcih is why it was not done.
@jimmie
Copy link
Contributor

jimmie commented Sep 1, 2022

Mostly done, needs a looking at by @jordanpadams, @jimmie.

al-niessner added a commit to NASA-PDS/registry-api that referenced this issue Sep 3, 2022
* update API

Added all of the new endpoints in groups classes, references, and products.

Added all of the old endpoints in the group deprecrated.

* changed to fit proposal

Update swagger.yml to match the proposal plus any additions and clarification on the original ticket NASA-PDS/pds-api#202.

Re-arranged the code to fit the new API. Required some new exceptions and tweaks to the basic interfaces to support the membership concept outside of just referencing in general and that a lidvid necessarily match a particular product type.

Added new swagger transmutation layers to support the easy removal of deprecated code in the future and to support easier maintenance in the future. Left the old referencing suff only because there is a chance it may come back in the newish future to support some other use cases.

The new swagger transmutation layers still all funnel through a single processor once all of the inputs have been collected. The deprecated code mostly works itself through the new API showing that what could be done still can using the inew API. Did not want to update the standard handler to also check that desired class matches the lid(vid) so did not route those but they could also be done with a touch more code change just for that demonstration whcih is why it was not done.

* fill out verify()

Given the new endpoints of /classes/{class}/{identifier} have to verify the {identifier} is actually of the type {class}. Previous commit had a blank for this function but need it filled out to test it.

* methods need to be in controller

Seems there is a limitation in spring (maybe swagger) that the methods to be used in a @controller must be present in the class and not inherited. This makes the code a bit more repetitive, but it works.

* adjusted regress

Had to update the endpoints to match the new API. Simply tested what already existed and did not add any new tests for new corner cases.

Had to update the number of collections and products too. Seems my latest install of data from harvest is smaller than before. However it all fits the current data in my opensearch now.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants