marshmallow-code / webargs

A friendly library for parsing HTTP request arguments, with built-in support for popular web frameworks, including Flask, Django, Bottle, Tornado, Pyramid, webapp2, Falcon, and aiohttp.
https://webargs.readthedocs.io/
MIT License
1.37k stars 158 forks source link

Inconsistent behavior on Python 2.7 vs. Python 3.7 on Flask #360

Closed mostafa closed 5 years ago

mostafa commented 5 years ago

Hey guys,

I am having an issue with webargs==5.x, in that, when I upgrade it from 4.x to 5.x, using Flask==1.0.2, it works and behaves normally on python 3.7, but it behaves differently on python 2.7 on the same code base. The issue is on this pull request mostafa/grest#84 and is evident on this build: https://travis-ci.org/mostafa/grest/builds/479199700. I've tried to trace my own code to see if there is a path I am not covering, but that seems not to be the case, because it passes all tests on python 3.x.

Thanks, Mostafa.

sloria commented 5 years ago

Thanks for reporting @mostafa . That definitely seems suspicious. I don't have time to look into this right now, but if you do some digging and end up finding a fix, I'd gladly review and merge a PR.

mostafa commented 5 years ago

I'll try to look into it to see if I can fix it! As far as I can trace the issue, it is related to the required property on different fields. It seems that Python 2.7 is required=True by default, but Python 3.7 is required=False. I know it is confusing, but that's one thing I just guessed by looking into multiple files from webargs and marshmallow.

sloria commented 5 years ago

Thanks for looking into it.

mostafa commented 5 years ago

As I investigated more, the issue rises due to some minor differences between Python 2.7 and Python 3.7 and those are:

  1. In Python 2.7, an empty string is an instance of bytes, hence isinstance("", bytes) returns True, but reverse proves to be the case for Python 3.7.
  2. In Python 2.7, when an empty string is passed to the json.loads method, it raises a ValueError: No JSON object could be decoded, but Python 3.7 raises a JSONDecodeError: Expecting value: line 1 column 1 (char 0), which in Python 3.7 version, the _get_value method silently ignores the error.

This all mean that if one wants to parse JSON body from the request object and it is empty, it will return missing in case of Python 3.7, and raises an exception in Python 2.7.

mostafa commented 5 years ago

One possible (very quick) solution is to pass {} as JSON request body instead of an empty body. This would eliminate the bug from happening altogether, obviously.

mostafa commented 5 years ago

It seems that the issue that was fixed in 05d8e1f1235f6df77279a02a76844ead4e4e7eea, has some relations to this bug. Because when I use the dev branch to test, it works fine.

sloria commented 5 years ago

Ah, interesting. I see that your https://github.com/mostafa/grest/pull/84 was merged, so it looks like 5.1.0 had the fix.

Closing this for now.

mostafa commented 5 years ago

I merged it, but it didn't fix the issue. So I downgraded to 4.3.1 to be able to have a working version till the issue resolves. This BUG is playing a hide and seek game with me, hence why I named it inconsistent!

sloria commented 5 years ago

I wonder if this is related to #363 . @mostafa Can you try explicitly installing simplejson in grest to see if that fixes it?

mostafa commented 5 years ago

It seems that the ujson and the simplejson APIs are consistent in exception handling, but the internal python implementation raises different exceptions in the same conditions, e.g. empty string. The simplejson library returns JSONDecodeError: Expecting value: line 1 column 1 (char 0) on Python 2 and 3, which is the standard Python 3 message and also the usjon one raises ValueError: Expected object or value, in which, the type of the exception is the same as Python 2's, but the message differs, and the exception and its messages are consistent in both Python versions. What I tried -- as a quick fix -- was to explicitly pass an empty string as json data, and all the tests passed, except the YAML one, which is something different and it raises the same 422 error code which is not what I want and which is the same exact error, but on PyYAML library.

mostafa commented 5 years ago

I monkey-patched the dumps and loads functions in flask with the ones from simplejson, and it worked perfectly. So, it seems safe to say that difference in the json APIs caused this!

mostafa commented 5 years ago

Hopefully the builds are passing now with the 5.1.0 version.