Conversation
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (96.96%) is below the target coverage (100.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #177 +/- ##
===========================================
- Coverage 100.00% 99.92% -0.08%
===========================================
Files 115 115
Lines 2706 2765 +59
Branches 330 346 +16
===========================================
+ Hits 2706 2763 +57
- Misses 0 2 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
src/cmr/concepts/concept.js
Outdated
| * @param {String} revisionId The specific revision ID to extract | ||
| * @param {Array} ummKeys Keys that were requested | ||
| */ | ||
| async parseAndMergeMetaFields(allRevisionsResponse, revisionId, ummKeys) { |
There was a problem hiding this comment.
We need to move most of the logic in this class over to the src/utils/parseRequestedFields.js
There was a problem hiding this comment.
I think I was wrong that was from the PR notes I'm not really seeing how we'd do that here and this certainly isn't creating new scope in this class behavior like this is already there
There was a problem hiding this comment.
Now moved processing of keys to parseRequestedKeys
macrouch
left a comment
There was a problem hiding this comment.
Ed is right, most of what you have in concept.js needs to be moved to parseRequestedFields.js.
The concept class should just be given lists of keys in the requestInfo, and the concept class should only have the knowledge that it needs to fetch the provided keys. It shouldn't be responsible for parsing parameters, or parsing ummKeyMappings to determine if different requests should be made. The concept class logic should be "if ummKeys exist, fetchUmm. if jsonKeys exist, fetchJson. if concept endpoint should be fetched, fetch the concept endpoint"
src/resolvers/collection.js
Outdated
| context.singleRevisionRequest = !!args.params?.revisionId | ||
|
|
||
| // Add the conceptId to the context | ||
| context.conceptId = args.params?.conceptId |
There was a problem hiding this comment.
This should not be done. source is where the nested resolvers should be getting the conceptId
There was a problem hiding this comment.
Done, now reverted unnecessary change: no need to store conceptId in context.
There was a problem hiding this comment.
This file is still missing the documentation for any fields that can throw an error when a revision id is provided
There was a problem hiding this comment.
I actually don't know either where those get added. I'd assume its either in magidoc/pages/02.Query Fields/04.Items.md or its something that we just add it on the collection.graphql file and its kind of like @deprecated(reason: "Use params.includeTags") apparently you can actually use custom schema directives https://www.apollographql.com/docs/apollo-server/v3/schema/creating-directives
There was a problem hiding this comment.
It's just the string that is above every field in this file
|
Once you've made those suggested changes, double check that you can query revisions on a collection when you don't pass revisionId as an argument. Right now, that feature isn't working. |
src/cmr/concepts/concept.js
Outdated
| ) | ||
|
|
||
| // Second: If meta-only fields are needed, fetch all revisions from search endpoint | ||
| const needsMetaFields = ummKeys.some((key) => this.requestInfo.ummKeyMappings[key]?.startsWith('meta.') |
There was a problem hiding this comment.
We have the list of meta keys or at least it seems like it, on const { jsonKeys, metaKeys, ummKeys } = this.requestInfo
can we use that field for the conditional?
There was a problem hiding this comment.
Now moved to parseRequestFields
src/cmr/concepts/concept.js
Outdated
| * @param {String} revisionId The specific revision ID to extract | ||
| * @param {Array} ummKeys Keys that were requested | ||
| */ | ||
| async parseAndMergeMetaFields(allRevisionsResponse, revisionId, ummKeys) { |
There was a problem hiding this comment.
I think I was wrong that was from the PR notes I'm not really seeing how we'd do that here and this certainly isn't creating new scope in this class behavior like this is already there
src/cmr/concepts/concept.js
Outdated
| } | ||
|
|
||
| // Get the existing item key (should be only one item from concepts endpoint) | ||
| const existingItemKeys = Object.keys(this.items) |
There was a problem hiding this comment.
might just be able to use the existing this.getItem() in this case but, not certain
There was a problem hiding this comment.
Now using this.getItems()
I tried main, that is also broken |
|
|
||
| describe('Collection', () => { | ||
| describe('fetch', () => { | ||
| test('fetches a specific revision of a collection', async () => { |
There was a problem hiding this comment.
Remove the 'fetch' field from under the describe block with capital letter 'Collection' (line 50 in your resolvers) and place all these tests you've added under describe block for lower case 'collection' (line 17). Reason being that these are all done when retrieving a collection, not a field on collection.
"fetches a specific revision of a collection when revisionId is provided in arguments"
There was a problem hiding this comment.
I don't see any new changes?
There was a problem hiding this comment.
Sorry, missed a 'git push'
| }) | ||
| }) | ||
|
|
||
| test('fetches all revisions when meta-only fields are needed', async () => { |
There was a problem hiding this comment.
What is this test supposed to be doing? It wants to fetch all revisions but you passed it a revisions id and did not request revisions in your query.
There was a problem hiding this comment.
Done, updated the description to make it more clear what been tested.
There was a problem hiding this comment.
I've found a couple of queries that are not working correctly
- This query is sending too many requests to CMR.
query Collection($params: CollectionInput) {
collection(params: $params) {
shortName
}
}
By only requesting the shortName, which is a UMM field, cmr-graphql should only make one request to CMR for the UMM (from the concept endpoint). Currently it is making a request to both https://cmr.sit.earthdata.nasa.gov/search/collections.json and https://cmr.sit.earthdata.nasa.gov/search/concepts/C1200237617-MMT_1/23.umm_json. There is no reason to send the request to collections.json
- This query is potentially returning incorrect data.
query Collection($params: CollectionInput) {
collection(params: $params) {
boxes
}
}
boxes is only in the json endpoint, so you have to be sure to return the boxes from the correct revision. Currently it is only sending the conceptId to the json endpoint, so that will be returning the boxes from the latest revision, which could be different from the revision the user provided. The only way I can think to solve this is if revisionId is provided and jsonKeys are populated, you would have to include the all_revisions parameter and filter the returned data to the correct revision.
Both of these use cases should have a test that shows the correct requests are being made and the correct data being returned.
Suggested Changes:
-
concept.js:
revisionIdandconceptIdare still being used to determine logic in this file. You are making the logic in this file more complicated by trying to do it this way.parseRequestedFieldsshould be returning everything necessary to send these requests to CMR fromconcept.jsFollowing the standing conventions of
parseRequestedFields, I'd return aconceptEndpointKeys, which contains every requested field that can be returned from the concept endpoint. The field should only be present in one of the 3 list of keys returned (json, umm, conceptEndpoint).metaKeysis useful for determining which fields should be inconceptEndpointKeysorummKeys, but it should not be necessary inconcept.jsThen in
concept.jsyou don't have to look at the request at all, ifconceptEndpointKeysexist, call the new fetch function, ifummKeysexist, callfetchUmmand ifjsonKeysexist callfetchJson -
I feel like using
revisionsin the names of things is slightly limiting. The real thing you are trying to indicate is the concepts endpoint needs to be used instead of the search endpoint. If at some point in the future we have another need to use the concepts endpoint, what you are adding now should be able to be used to accomplish that. I'd renamefetchRevisionUmmtofetchConceptEndpoint, and make therevisionIdparameter optional. Then update thepathas necessary -
parseAndMergeMetaFields, I don't understand what this function is doing, but I don't think any of it is necessary. If you make the above changes to
parseRequestedFieldsI think you end up with aparseConceptEndpointthat looks very similar toparseJsonandparseUmm
I'm sure there will be other issues in concept.js, like you'll have to filter a list of json revisions down to the correct revision from my second example above, but I think if you start with parseRequestedFields returning the right data, all the logic in concept.js should be simplified
src/types/collection.graphql
Outdated
| Error: The 'revisions' field cannot be requested when querying a specific revision. | ||
| Remove the 'revisionId' parameter from the collection query to fetch all revisions. |
There was a problem hiding this comment.
This reads like something has gone wrong, when it is just supposed to be informing the user on a condition on this field.
Note: This field is not supported if you are performing a `collection` query and have provided the `revisionId` parameter. `null` will be returned with an error
src/resolvers/collection.js
Outdated
| const { revisionId } = info.variableValues?.params ?? {} | ||
| if (revisionId) { | ||
| throw new Error( | ||
| 'The "revisions" field cannot be requested when querying a specific revision. ' | ||
| + 'Remove the "revisionId" parameter from the collection query to fetch all revisions.' | ||
| ) | ||
| } |
There was a problem hiding this comment.
When I use the example error query in your PR description, this revisionId is undefined and this error is not what is thrown. I get a CMR error: "The mime type [application/json] is not supported when all_revisions = true."
I'm not sure why this is on two lines
There was a problem hiding this comment.
solved issue with revisionId undefined. Now just store it in the context, that cover all ways how the parameter revisionId is placed in the query.
Overview
What is the feature?
This feature adds support for fetching specific revisions of collections in the GraphQL API. It allows querying a particular revision of a collection by its concept ID and revision ID.
What is the Solution?
The solution involves modifying the Collection concept class and related resolvers to handle revision-specific queries. Key changes include:
when revision ID is given, fetch data from the 'concepts' endpoint and also from the revisions endpoint if needed (meta keys given).
For merging, changes are made because response from the 'concepts' endpoint is different from existing concept type end point.
What areas of the application does this impact?
This change impacts several areas of the application:
src/cmr/concepts/concept.js: Major updates to the Concept class to support revision-specific queries and merging.src/resolvers/collection.js: Updates to collection resolvers to handle revision-specific queries and add appropriate error checks.src/types/collection.graphql: Addition of revisionId to the CollectionInput type.src/resolvers/__tests__/collection.test.js: New tests added to cover the new functionality and error cases.Testing
Reproduction steps
Checklist