-
Notifications
You must be signed in to change notification settings - Fork 9
Fix BLOB store/get, Update API.py GET #137
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
Conversation
…/cwms-python into bug/get-blob-response
…ure of HTTP response. This will prevent resource (connection) leaks in the event the request fails.
…brary and redirects still return valid data. Simplify by accepting anything under 400 with response.ok
…nsure VALUE stored to CDA is a base64 encoded string.
…w if no content type is set fall back to json
…he JSON response is happy for mypy
|
I'm uncertain if it makes more sense for get_clob to return an object or if the string should be passed through. I tried setting a Union type for the In the future as more types are added it could be possible to do things like add Also, sorry for the spam. I was actively remembering (contributing doc) as I went and how much I could run/test locally before committing. |
Enovotny
left a comment
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 like the changes. just have a couple comments. Thank for making the improvements to the api calls.
Update blob to correct version; Change types to Any that can return various mimetypes (not just json); Change from 102 to xml/v2
…t for versions higher than two, allow CDA to say version is not supported.
|
Your requests
Few more things
I also ran my python file above to make sure these worked against the national CDA instance, not just the mocks |
|
I like some of the changes, but we should keep the calls get_....xml. Something that I learned from Jordan when first setting this up was the each function should do one thing or provide a single data format. This is for testing purposes. That is why we created the cwms data type to provide the json/dataframe which is the backbone of this package. Anything that provides a different format of data in the get calls should be a separate function. get_ratings_xml ect... I think I am fine with the change in the api.py calls. I think that makes things a little more readable and maintainable long term. but we should not have format as a parameter in any of the get... functions. I originally had that parameters as well to get json or dataframe and Jordan steered me into the current implementation. |
|
I tested these changes against the below import os
import cwms
cwms.init_session(api_key="apikey " + os.getenv("CDA_API_KEY", ""), api_root=os.getenv("CDA_HOST", "") + "/")
cwms.store_blobs(
data={
"office-id": "SWT",
"id": "TEST.TXT",
"description": "Your description here",
"media-type-id": "text/plain",
"value": "A test of cwms-python blob store",
},
fail_if_exists=False
)
print(cwms.get_blob(blob_id="DATACHECK.HYDROPOWER.JSON", office_id="SWT"))
print(cwms.get_blob(blob_id="EUFA.PLOT.PNG", office_id="SWT"))
print(cwms.get_blob(blob_id="TEST.TXT", office_id="SWT"))
print(cwms.get_blobs(office_id="SWT", blob_id_like="TEST").json)
print("Stored!")Saved to: Output here: I reverted the format and Let me know if I missed any changes you would like to see. Side note, the Could also create a |
505c192 to
73bdcad
Compare
|
Realized we could handle the images mimetype too. Those are stored in blob as base64 strings. Lines 232 to 233 in 505c192
Depending on the mime-type it might not play nice with an attempt at python doing a decode on it. But will leave any extras I missed (XML/XLSX/etc) for the next PR... Line 235 in 505c192
|
|



ISSUE
When testing the blobs for storing and retrieving with cwms-python version 0.6.0 the API was unable to retrieve the BLOB with the following error:
ERROR:root:Error decoding CDA response as JSON: Expecting value: line 1 column 3 (char 2) on line 1This was because the mime-type returned by the blobs endpoint for a GET request is
application/octet-streamSeen here:
If you run what was present before in the get method
cwms-python/cwms/api.py
Line 238 in 2684dcf
It attempts to force the response into json even if it is not json.
FIX
To fix this I made sure to check the content type in the
getmethod ofapi.py. Dynamically deciding when to apply the various methods to the response data.MISSING CDA TYPES
Currently CDA only lets you store/retrieve
octet-stream- I posted an issue for this on the CDA repo here:SUMMARY OF CHANGES
I also took the opportunity to :
store_blobcasted the value to a base64 encoded string if it was not already done. This is required in order for the API to store the value. Otherwise you will get a serialization error in the logs and a 500 status..okas 3## redirects are handled byrequestsand imo anything less than 400 should be reasonable - I'm not dead set on this but we could make it < 400 if you prefer the verbosity. (not the < 300 we had).response.close()to a context manager which properly ensures no resource/connection leaks.response.textandresponse.contentalong withresponse.json()based on the content-type returned, updated theget_xmlto use the newgetmethod for backwards compatibility.TESTS
I went about writing a quick get script with some mock tests for
store_bloband various other endpoints to make sure it was properly building the payload (base64 encoded value) and I did not introduce any other breaking changes.Here is that script for reference: