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

Fix: REST API "download=False" error #5576

Merged
merged 6 commits into from
Jul 5, 2022

Conversation

eimrek
Copy link
Member

@eimrek eimrek commented Jun 18, 2022

My proposal for #5574

for context, see the discussion in the issue.

Copy link
Member

@ltalirz ltalirz left a comment

Choose a reason for hiding this comment

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

Thanks for the fix @eimrek !

Would you mind adding a test that failed before your fix and now passes?

aiida/restapi/resources.py Outdated Show resolved Hide resolved
@eimrek
Copy link
Member Author

eimrek commented Jun 28, 2022

Ok, I figured out the source of this problem:

The JSONEncoder imported here

from flask.json import JSONEncoder

in flask 1.x was based on simplejson, that inherently converts bytes objects into strings. In flask 2.0.0, it was redesigned based on the default python json. See here: https://flask.palletsprojects.com/en/2.0.x/changes/ and here pallets/flask#3555

As a result, in Aiida 2.0 REST (where flask 1 was updated to flask 2), bytes objects are not supported any more.

I implemented a conversion of a bytes array into str in the CustomJSONEncoder. In principle there are other way of doing this as well (e.g. by checking isinstance(o, bytes)). I can change it, if some other implementation is preferable.

Similar issue is probably also the source of #5575 but this PR doesn't fix that. i will investigate that in a separate PR.

I'll try to add a test as well.

@eimrek eimrek requested a review from ltalirz June 28, 2022 13:17
@unkcpz unkcpz self-assigned this Jun 29, 2022
Copy link
Member

@giovannipizzi giovannipizzi left a comment

Choose a reason for hiding this comment

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

Thanks!

@unkcpz
Copy link
Member

unkcpz commented Jul 5, 2022

This is related to #5573
If you look at

if query_type == 'download' and download not in ['false', 'False', False] and results:
, my suggestion change there can also address this issue.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

I think it makes sense to combine #5573 to this PR.

@eimrek
Copy link
Member Author

eimrek commented Jul 5, 2022

Thanks @unkcpz. I disagree, as the source issues are different. #5573 fixes a bug that has been there forever, and affects how boolean query parameters are parsed. The issue of this PR is in the encoding of the binary string to JSON, that was introduced by the aiida 2.0 upgrade. Although the issue comes out in case of download=False, that might seem similar, it probably also affects other parts of the restapi.

I think it makes sense to isolate the code changes that fix specific issues, instead of merging everything together. the current PR fixes the linked issue (and the PR of #5573 does not affect this).

PR #5573 fixes a separate issue, namely, that lowercase "false" in the query parameters is interpreted as True, but could be bypassed by specifying an uppercase "False" instead. I suspect this is the reason is it was not fixed for over 3 years.

@unkcpz
Copy link
Member

unkcpz commented Jul 5, 2022

@eimrek thanks, I see. I misunderstood it, my bad. here you explicitly use download=False which does not have the issue of #5573.

Copy link
Member

@unkcpz unkcpz left a comment

Choose a reason for hiding this comment

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

@eimrek sorry for the back and forth. One more minor request, others are all good. Could you then rebase and make a detailed commit message? Thanks.

try:
return o.decode('utf-8')
except (UnicodeDecodeError, AttributeError):
pass
Copy link
Member

Choose a reason for hiding this comment

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

I think an elif for bytes type makes more sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

@unkcpz unkcpz enabled auto-merge (squash) July 5, 2022 15:30
@unkcpz unkcpz merged commit ef9c544 into aiidateam:main Jul 5, 2022
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