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

Resolve #106 per additional API changes #114

Merged
merged 5 commits into from
Jul 3, 2021
Merged

Conversation

nutjob4life
Copy link
Member

@nutjob4life nutjob4life commented Jun 15, 2021

📜 Summary

Merge this to resolve #106 but also work around additional changes to the server's delivery of data to the client.

See, previously, using pds.api_client (versions 0.4.0 and 0.5.0), you'd get the properties attribute of the pds.api_client.models.Product class with key-value pairs like this:

{
…
    'ops:Label_File_Info.ops:file_ref': ['https://pds-gamma.jpl.nasa.gov/data/pds4/test-data/registry/urn-nasa-pds-insight_documents/bundle_insight_documents.xml'],
    'ops:Label_File_Info.ops:md5_checksum': ['a366a14158f5a7f0dc7a1b4c06c003ae'],
…
}

Something changed, and rather than pds.api_client shielding its users from that change, it passes the change onto them 😮

The new properties attribute is no longer a list of values, but a dict with a single values element with a list of values:

{
…
    'ops:Label_File_Info.ops:file_ref': {'values': ['https://pds-gamma.jpl.nasa.gov/data/pds4/test-data/registry/urn-nasa-pds-insight_documents/bundle_insight_documents.xml']},
    'ops:Label_File_Info.ops:md5_checksum': {'values': ['a366a14158f5a7f0dc7a1b4c06c003ae']},
…
}

So to make sure the fix to #106 actually worked, this PR also absorbs and deals with the change in data coming from the API server.

Update: now the server always gives sequences of values for properties but without the single-item dictionary with a values key—even for those which are semantically singly-valued, like label file MD5. Also, this works around PDS API Client stealing the pds package name and preventing dependent code from being found.

🩺 Test Data and/or Report

$ venv/bin/python setup.py test
…
running test
…
----------------------------------------------------------------------
Ran 20 tests in 0.678s

OK

🧩 Related Issues

Copy link

@collinss-jpl collinss-jpl left a comment

Choose a reason for hiding this comment

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

👍

@jordanpadams
Copy link
Member

@nutjob4life would this work with the previous implementation of the API as well? That is the way the response format should be, so as long as it works for both, we should be ok.

@nutjob4life
Copy link
Member Author

would this work with the previous implementation of the API as well? That is the way the response format should be, so as long as it works for both, we should be ok.

@jordanpadams no, it won't.

Some would say it's the job pds-api-client to provide a uniform/stable interface to application programmers regardless of what comes across the wire, though 🤔

@jordanpadams
Copy link
Member

@nutjob4life copy. then please revert the changes to support the new "values" dict. that should not be there and is a regression to the API. one of the fun parts of working from a beta version of an API without a robust regression suite :-)

nutjob4life added a commit that referenced this pull request Jun 17, 2021
@nutjob4life
Copy link
Member Author

@jordanpadams okay, reverted 😉

@nutjob4life nutjob4life changed the title Resolve #106 and work around sudden server-side changes Resolve #106 AGAIN and work around MORE sudden server-side changes Jul 2, 2021
also kicks CICD
@jordanpadams jordanpadams changed the title Resolve #106 AGAIN and work around MORE sudden server-side changes Resolve #106 per additional API changes Jul 2, 2021
@jordanpadams
Copy link
Member

@nutjob4life do we still need --disable-pagination-workaround?

@nutjob4life
Copy link
Member Author

@nutjob4life do we still need --disable-pagination-workaround?

@jordanpadams that's covered by #107

@jordanpadams jordanpadams merged commit a88ce10 into main Jul 3, 2021
@jordanpadams jordanpadams deleted the i106-workaround-excision branch July 3, 2021 13:48
@nutjob4life
Copy link
Member Author

@jordanpadams good catch on the Versioneer stuff in setup.cfg 🍻

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.

Improvements per API updates to remove workarounds from code
3 participants