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

GQL-3: Appending variableKeyMap response to add new JSON fields #82

Closed
wants to merge 4 commits into from

Conversation

eudoroolivares2016
Copy link
Contributor

@eudoroolivares2016 eudoroolivares2016 commented Nov 20, 2023

Overview

What is the feature?

Adding JSON fields which were recently indexed into CMR to the JSON key mapping for variables. The purpose of this is to leverage CMR's JSON response's superior speed to ensure that collections which contain a large number of variables do not timeout on API gateway

What is the Solution?

Added the variables to its keymap. Overrode the parent concept class's parseJson function to include logic to support consistently returning the metadata in camelCase with the exception of tags

List impacted areas.

All JSON responses from any of the concepts which are nested (except tags)

Testing

Reproduction steps

Using this query on localhost ensure that even after the changes the response remains the same as the current response from the umm_json endpoint

https://studio.apollographql.com/sandbox/explorer?endpoint=http%3A%2F%2Flocalhost%3A3013%2Fdev%2Fapi&explorerURLState=N4IgJg9gxgrgtgUwHYBcQC4QEcYIE4CeABABQAkADgIZ5VwDO6RAajQJZUBGANggJJIKMFAEoiwADpIiRAG7suvEtVoMmlGnXpjJ0mUTZJ6KKkij8kAMwh44VFGwhIp%2BovShtk5gNIICAdxswehd9JHs2WX4wUJkoJ3MKFD4YvRkwBEtDNgcnUIBfKXyQABoQeTwOHgR6DBBdGQkQFS0mpgb9JvizBCSUtqIm5gBGACYABnGAFgB2KYBOKYAOAFoAEQBRAFkAeQB9AAUAJR3mJoKikHygA

Attachments

Please include relevant screenshots or files that would be helpful in reviewing and verifying this change.

Checklist

  • I have added automated tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings

Copy link

codecov bot commented Nov 20, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (06b7ba2) 100.00% compared to head (a0ba4f4) 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #82   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           69        69           
  Lines         1544      1547    +3     
  Branches       209       210    +1     
=========================================
+ Hits          1544      1547    +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

src/utils/umm/variableKeyMap.json Show resolved Hide resolved

// Once accessed, ensure that the child key/value pairs are consistently returning in `camelCase`
// Note applying camelcase logic to all keys in `concepts` will not work because of `tags` business logic
if (jsonKey === 'scienceKeywords' || jsonKey === 'instanceInformation') {
Copy link
Contributor

Choose a reason for hiding this comment

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

If the only field we know of that we don't want camelCased is 'tags', then use !== 'tags', that will ensure future fields that need to follow the naming scheme will match without needing more conditionals.

That logic also seems like it should be uniform across all concepts. Is this line the only thing different between concept.js and variable.js? If both those things are true this logic should be in concept.js

Copy link
Contributor Author

@eudoroolivares2016 eudoroolivares2016 Nov 21, 2023

Choose a reason for hiding this comment

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

Yea I did it that way initially I'll revert it back to doing it that way. That is the only difference. I do think it is just tags actually. For the data payloads on associations it looks like we are already setting the child fields to be camelCased not certain if that is "intended" or a future issue but, we should have that to ensure client apps don't error out

})

describe('the nested key is tag', () => {
test('the child-keys of the tag return snake_cased', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this test name to be more clear about what is happening in the code. The child keys aren't being returned snake_cased, they are being returned in their original casing

Copy link
Member

@dpesall dpesall left a comment

Choose a reason for hiding this comment

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

We should investigate that CMR’s bulk_indexing for variables is working as expected before merging. I’ve found some new info that suggests simply the bulk_index for all providers may not be executing for ALL providers I asked drew to add a requested changes block to ensure this isn't merged -Ed

@eudoroolivares2016
Copy link
Contributor Author

eudoroolivares2016 commented Nov 30, 2023

I closed this pull request because until we resolve issues around bulk-reindexing we cannot reliably ensure that we retrieve these fields from the .json endpoint and that they match the .umm_json endpoint for existing variables as both systems propagate through the envs. A ticket was written to address the bulk_reindexing issue on CMR.

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.

4 participants