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

/bundles/{identifier}/collections endpoint is broken #222

Closed
jordanpadams opened this issue Dec 22, 2022 · 5 comments
Closed

/bundles/{identifier}/collections endpoint is broken #222

jordanpadams opened this issue Dec 22, 2022 · 5 comments
Assignees
Labels
B13.1 bug Something isn't working invalid This doesn't seem right s.high High severity

Comments

@jordanpadams
Copy link
Member

jordanpadams commented Dec 22, 2022

🐛 Describe the bug

Cannot query for collections of a bundle.

📜 To Reproduce

$ curl --get 'https://pds.nasa.gov/api/search/1.0/bundles/urn%3Anasa%3Apds%3Amess_rs_derived%3A%3A3.0/collections' \
     --header 'Accept: application/json' | json_pp
{
   "error" : "Internal Server Error",
   "path" : "/bundles/urn%3Anasa%3Apds%3Amess_rs_derived%3A%3A3.0/collections",
   "status" : 500,
   "timestamp" : 1671738912305
}

New API endpoints don't work at all:

$ curl --get 'https://pds.nasa.gov/api/search/1.0/classes/bundles/urn%3Anasa%3Apds%3Amess_rs_derived%3A%3A3.0' \
     --header 'Accept: application/json' | json_pp

And we are missing an endpoint

$ curl --get 'https://pds.nasa.gov/api/search/1.0/products/urn%3Anasa%3Apds%3Amess_rs_derived%3A%3A3.0/members' \
     --header 'Accept: application/json' | json_pp

{
   "message" : "The lidvid 'urn:nasa:pds:mess_rs_derived::3.0' of type product does not support members membership",
   "request" : "/products/urn%3Anasa%3Apds%3Amess_rs_derived%3A%3A3.0/members"
}

🕵️ Expected behavior

📚 Version of Software Used

🩺 Test Data / Additional context

🏞Screenshots

🖥 System Info

  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Version [e.g. 22]

🦄 Related requirements

⚙️ Engineering Details

This is blocking NASA-PDS/deep-archive#134

@jordanpadams
Copy link
Member Author

@alexdunnjpl @tloubrieu-jpl this bug and the missing endpoints per #223 are top of list for fixes.

@alexdunnjpl
Copy link
Contributor

Cannot replicate against minimal dataset loaded into registry profile pds-registry-core for the following calls - they all work (in browser or using the equivalent curl calls to those given in ticket:

http://localhost:8080/bundles/urn:nasa:pds:insight_rad::2.1/collections
http://localhost:8080/classes/bundles

Replicated the following errors:
http://localhost:8080/products/urn:nasa:pds:insight_rad::2.1/members

makes sense as a bug given the whole products vs aggregate-products/basic-products thing

the functionality is implemented for the new explicitly-aggregate routes (/classes/bundles and /classes/collections) but not the old ones (/bundles and /collections):

@jordanpadams is it an acceptable fix to forward requests to the old routes directly to the new routes? I have a vague memory of asking a similar question but can't find it. Seems like the correct approach to supporting deprecated, 1:1 equivalent(?) routes.

BUT there's also an additional wrinkle - any aggregate product accessed through the /products route is at some point considered by the API to be of type "product", so even though equivalent (REST) resources are returned by

you can only call /members on the latter.

Additionally, the route for 'http://localhost:8080/products/urn:nasa:pds:insight_rad:data_calibrated::14.0' doesn't even work an returns HTTP404.

So it look like there's a handful of tickets' here (for clarity/organisation's sake) - if you agree I'll break this out into one ticket per route - but there's also a need for tests for all of these routes (still need to tease out if we have existing tests in registry-api, a mock for OpenSearch, etc).

There's also the question of which routes should support the members/member-of subroutes - i.e. should /products/{someAggregateProductId}/members work? Given that /products/{someAggregateProductId} returns an equivalent resource to /classes/collections I'd think so, but it seems questionable that /products/{someNonAggregateProductId}/members is a valid route, but would presumably need to return some kind of error?

@tloubrieu-jpl
Copy link
Member

Hi @alexdunnjpl ,

i believe the error Jordan found might be related to release 1.1.11 which is deployed in production. This might be fixed with version 1.1.12 or 1.2.0-snapshot.

regarding the old URL endpoints being implemented as proxies to new endpoints I 100% support this idea. Al told me this was already done this way, to be confirmed with him.

at last the /members or member-of URL only applies to the new endpoints /classes/{class}, we don’t need to implement that on old endpoints eg /collections/{Id}/members is not a valid endpoint and should return 404. The old endpoint is /collections/{Id}/products . That should not work either on /products/{id}/members which should return 404. But /products/{id}/collections or /products/{id}/bundles should work as for the deprecated endpoints scheme.

Sorry this is all confusing, we are hoping that will be clearer when the deprecated endpoints will go away or at least will be implemented in a non intrusive way in the code thanks to proxies.

feel free to create specific bugs (eg unhandled 404 errors) for what you think is not right, Jordan/Al or I can validate them if we confirm the intention behind the development.

@jordanpadams
Copy link
Member Author

jordanpadams commented Dec 30, 2022

@alexdunnjpl 👍 to all that @tloubrieu-jpl just mentioned.

one difference being /products/{someAggregateProductId}/members does not work. I think I am a bit torn as to whether this should be implemented or not. not entirely necessary because we have the /classes/(collections|bundles) endpoints for the "browse" experience from aggregate to individual products. but may be good to create new tickets to track these new /(members|member-of) requirements and we can see how they fit into priority in the future.

@jordanpadams
Copy link
Member Author

jordanpadams commented Dec 30, 2022

closing this as invalid. this is not a bug in a current main branch, the fix in ops will be done per:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B13.1 bug Something isn't working invalid This doesn't seem right s.high High severity
Projects
None yet
Development

No branches or pull requests

3 participants