-
Notifications
You must be signed in to change notification settings - Fork 5
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
Implement API endpoint that returns name of collection containing document having a given id
#532
Conversation
@aclum, is this the type of HTTP response you had in mind for the endpoint that we discussed; the endpoint that, for a given {
"id": "nmdc:sty-10-123456abcdef",
"collection_name": "study_set",
"class_name": "Study",
}
CC: @sujaypatil96 |
Yes. I think this would also be useful for the data portal, I have a ticket that has been in for a while to be able to search by an nmdc identifier. microbiomedata/nmdc-server#964 cc @naglepuff @marySalvi @jeffbaumes |
What is expected for an I have updated the PR to reflect this and return a list of collections. |
The issue of being valid for multiple collections is fixed in berkeley schema. The original feature of this endpoint was to return which collection the ID is in so if it is ambiguous for now it should check which collection the document actually resides in. |
id
id
This endpoint only uses the schema, not the database. The In other words, it currently doesn't do this:
Instead, it returns a list of all collections that — according to the schema — a document having that CC: @aclum |
Sounds to me like one of the requirements for this endpoint is to also indicate which collection, if any, a document having the specified |
Update: I have added this feature to the endpoint. Here's the response shape: {
"id": "string", // `id` from the URL (the `hypothetical_doc_id` path parameter)
"compatible_class_name": "string", // name of the class of which an instance _could_ have that `id`
"compatible_collection_names": ["string"], // names of all collections that _could_ contain a document having that `id`
"containing_collection_name": "string" // name of the collection in which a document having that `id` _does_ exist, if any
} Example response: {
"id": "nmdc:sty-1-foobar",
"compatible_class_name": "Study",
"compatible_collection_names": ["study_set"],
"containing_collection_name": "study_set"
} |
I am ready for this PR branch to be reviewed/merged. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested the /nmdcschema/ids/{hypothetical_doc_id}/class-and-collection-names
endpoint with 3 ids.
- Case 1: An Extraction id which exists in the database (for testing, in my local Mongo), and can (theoretically) be part of multiple collections
- Case 2: A study id which can technically be part of only one collection
- Case 3: Non-conformant id
It worked like a charm in all three cases, so I'm happy with its behavior and am ready for this PR to be merged 🚀
Hi @aclum, are you OK with how this endpoint behaves? If so (and someone approves it via GitHub's Review mechanism), I'll merge it in. I accidentally invalidated @sujaypatil96's approval by making an additional commit. @sujaypatil96 added screenshots of some example request/response pairs above—thanks, @sujaypatil96! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm. thank you for writing tests!
@eecavanna You're good to merge. If/when there's time, I'd like to consolidate and replace these utils with your logic. (non-blocking):
|
Based on @aclum's feedback during today's infrastructure meeting, I will update the endpoint as follows:
|
id
id
Sorry about all the thrash here! Getting the automated tests running locally was not happening smoothly for me (I think one of the tests hung last night), so I've been relying on the GHA workflow to run them for me. A downside of that is that the test failures generate notifications—at least to me. |
Hi @aclum, I updated the API response based upon our conversation earlier today. The new behavior is:
I'm ready for this PR branch to be reviewed/merged in. |
Description
In this branch, I implemented a new API endpoint. Its behavior is described in its docstring, shown here:
Fixes #531
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration, if it is not simply
make up-test && make test-run
.Configuration Details: none
Checklist:
black nmdc_runtime/
?)docs/
and in https://github.com/microbiomedata/NMDC_documentation/?)make up-test && make test-run
)