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

261: lookup product class when group not explicitly given #263

Merged
merged 4 commits into from
Mar 8, 2023

Conversation

al-niessner
Copy link
Contributor

@al-niessner al-niessner commented Feb 27, 2023

🗒️ Summary

When using products as general, also need to parse the product class from the DB because the expected group is no longer specified (this is a good thing). Modified the member transmuter to handle this condition.

⚙️ Test Data and/or Report

♻️ Related Issues

Closes #261
Closes #268

@al-niessner
Copy link
Contributor Author

@tloubrieu-jpl

Should be working now.

@al-niessner
Copy link
Contributor Author

@jordanpadams @nutjob4life @tloubrieu-jpl

Ready for review

Copy link
Member

@nutjob4life nutjob4life left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Member

@tloubrieu-jpl tloubrieu-jpl left a comment

Choose a reason for hiding this comment

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

Hi Al,

Now the request:

curl 'http://localhost:8081/products/urn:nasa:pds:insight_rad::2.1/members' --header 'Accept: application/json'

Returns an empty result:
{"summary":{"q":"","hits":0,"took":13,"start":0,"limit":100,"sort":[],"properties":[]},"data":[]}

But we should have a few collections returned with the reference test dataset.

@alexdunnjpl
Copy link
Contributor

alexdunnjpl commented Mar 7, 2023

@tloubrieu-jpl @al-niessner

If a LIDVID referenced by the aggregate product is missing from the registry, this will result in a LidVidNotFoundException. and prevent some/all of the found LIDVIDs from being included in the results returned to the user.

Such exceptions are caught and logged and do not bubble up to where exceptions are turned into non-200 HTTP responses (this behaviour also occurs in RefLogicCollection and RefLogicProduct - originally in LidVidUtils per 341e31b)

A decision needs to be made on whether it's still (ever) desirable to catch and log such errors without re-throwing. If not, re-throws should be added to each of the three catch blocks, or 341e31b should be reverted and a re-throw added to the original location in LidVidUtils. That commit was only ever made to allow future callers to access any resulting LidVidNotFoundExceptions rather than the utility function swallowing them.

I've confirmed that the absence of collection urn:nasa:pds:insight_rad:data_calibrated from the test dataset is the superficial cause of the erroneous behaviour.

@tloubrieu-jpl
Copy link
Member

@alexdunnjpl I am missing the context for the commit 341e31b but I would tell you that:
if a member of a collection or bundles referenced in its relationships, an exception should be thrown and appear in the log (WARNING) but the remaining members should be returned to the user. At least that is how I am thinking now.

Otherwise we should have a more explicit error message, saying some of the members are missing in the registry and the service was not able to retrieve the full response. I am not sure how it is best to provide a response to the user with warning, but let's not go this route now.

@alexdunnjpl
Copy link
Contributor

@tloubrieu-jpl isn't it sketchy to serve up known-incomplete data to the user? Currently, if the exception is allowed to bubble up, they're presented with a "$LIDVID does not exist in the registry", which at least tells them what they need to do to successfully make the call (harvest the missing product in question).

I can implement it as-described if you're sure, though.

@tloubrieu-jpl
Copy link
Member

tloubrieu-jpl commented Mar 8, 2023

I confirm that with the branch issue_261, I am getting this error message in logs:

2023-03-07 16:34:20.212 ERROR 43661 --- [nio-8081-exec-1] g.n.p.api.registry.model.RefLogicBundle  : Database referential integrity error -  LID is referenced but does not exist in db: 

gov.nasa.pds.api.registry.exceptions.LidVidNotFoundException: The lidvid Could not find any LIDVIDs for LID urn:nasa:pds:insight_rad:data_calibrated was not found

and the response gives an empty result, which might be what I asked for in my previous comment. I will confirm that by looking into the test database. I believe we need to understand better why these integrity errors appears and then what we should do about it.

wrapping a function around a for loop is not a value-add
@alexdunnjpl alexdunnjpl self-requested a review as a code owner March 8, 2023 00:54
@alexdunnjpl
Copy link
Contributor

alexdunnjpl commented Mar 8, 2023

@tloubrieu-jpl implemented as-requested (log warning, return retrievable elements of intended results set)

Manually tested members, members/members, member-of and member-of/member-of.

@tloubrieu-jpl
Copy link
Member

tloubrieu-jpl commented Mar 8, 2023

So @alexdunnjpl , in this case the collection is available in the registry (see request on your local test deployment https://elasticsearch:9200/registry/_doc/urn:nasa:pds:insight_rad:data_calibrated::7.0) but its status is staged which makes it invisible to the API users.

I believe we should definitely then return all the other responses. Another case I am thinking of is a collection where products are added incrementally over time. The latest products arrived might be in a stage status, but the user would need to access all the other products collected so far and in archived status.

So for ticket 261 I am thinking of 2 fixes:

  • in this case change the error message to warning in logs, saying one LID referenced in the product {LID} is not available in the registry, possibly because of its visibility status instead of Database referential integrity error - LID is referenced but does not exist in db
  • raise the warning so that it does not prevent the API from returning the rest of the members.

For later, we can create a ticket where we will inform the user that some of the members where not retrieved. See #270

@alexdunnjpl
Copy link
Contributor

alexdunnjpl commented Mar 8, 2023

@tloubrieu-jpl Done(ish - let me know if you still prefer your suggested log message to the updated one).

presence of emoji breaks branch integration testing
@tloubrieu-jpl
Copy link
Member

Thanks @alexdunnjpl I tested the request and it work as I expected. I still would like the log message to be update since it is not an integrity error.
The message should be:
The LID {LID not found} referenced in the product {LID parent} is not found in the registry, possibly because of its archive status

Thanks,

Thomas

@tloubrieu-jpl
Copy link
Member

Sorry I misread the error message provided by @alexdunnjpl's code update and what is proposed works for me.

@tloubrieu-jpl tloubrieu-jpl merged commit e2162a1 into main Mar 8, 2023
@tloubrieu-jpl tloubrieu-jpl deleted the issue_261 branch March 8, 2023 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

product's members does not work on a collection The members of a bundle can not be requested
4 participants