Handle change to Werkzeug 2.1.0 change to Request.get_json().

[stacywsmith]

Fixes #422

pallets/werkzeug#2339 changed the behavior of Request.get_json() and the Request.json property to raise a BadRequest if Request.json is accessed and the content type is not application/json.

Argument parsing allows parsing from multiple locations, but if the locations include json and the content type is not application/json, a BadRequest is now raised with Werkzeug >= 2.1.0.

Handle this case and continue argument parsing.

[davidism]

This fix is ok, if you want to keep the rest of the code calling getattr with a list of keys. But when I made the change I was expecting code that optionally accepts JSON to do request.get_json(silent=True), or if request.is_json.

Perhaps the following?

for l in self.location:
    if l in {"json", "get_json"}:
        value = request.get_json(silent=True)
    else:
        ...

[stacywsmith]

@davidsm Yes, I considered a fix along the lines you're suggesting. I don't have a strong preference, I'm fine with either approach and happy to change my PR if your approach is preferred.

[ghost]

I just was about to open a similar PR, dependabot rallies the community. I prefer what David's suggested, as the mantra of "explicit is better then implicit" seems applicable: if in the future some entry made it's way into self.location that would raise a BadRequest that shouldn't be masked to None that bug would be annoying to track down.

The way that an upstream change in werkzeug led to an easily-traced-down exception was good, and would have been much harder to track down if that exception was being swallowed.

[j5awry]

As a maintainer, i prefer @davidism 's answer as well. A few other points:

I actually like that Werkzeug is raising an error. I see why some people, given how disparate clients are, wouldn't like it (the example of JQuery is interesting). Given we do not know the client, keeping the old behaviour, utilizing the method provided by Werkzeug makes sense

Finally, please check unit tests around this. there seems to be an issue with running black check, which is annoying. But we need to make sure all tests are passing, and there's adequate coverage for the change.

[davidism]

You'll need to update Black as well, it had a bug with how it was handling compatibility with Click, but it was fixed in Black 22.3.0. https://github.com/psf/black/issues/2964

[stacywsmith]

PR has been updated. There is still one failing test which appears to be unrelated.

[delroth]

~~Note that this breaks compatibility with Werkzeug < 2.x which does not have Request.get_json. Maybe not a problem, but should still be considered.~~

[davidism]

Huh? get_json is not new.

[delroth]

Oh, you're right, I got confused by files being reorganized in Werkzeug between 1.x and 2.x, sorry. I'm debugging some issue which at first glance seems to be linked to this patch + older Flask/Werkzeug version, and I misdiagnosed.

[delroth]

Actually I think we're both correct: get_json exists, but by default wrappers.request.Request in Werkzeug 1.0.1 doesn't include JSONMixin which is what provides get_json. This gets reflected in failures like this in the unit tests:

FAILED tests/test_reqparse.py::ReqParseTestCase::test_parse_lte_gte_mock - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_parse_none - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_parse_store_missing - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_parse_unicode - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_passing_arguments_object - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_request_parser_copy - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_request_parser_replace_argument - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_strict_parsing_off_partial_hit - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_strict_parsing_on - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_strict_parsing_on_partial_hit - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_trim_argument - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_trim_request_parser - AttributeError: 'Request' object has no attribute 'get_json'
FAILED tests/test_reqparse.py::ReqParseTestCase::test_type_callable - AttributeError: 'Request' object has no attribute 'get_json'

See https://werkzeug.palletsprojects.com/en/1.0.x/request_data/?highlight=jsonmixin#how-to-extend-parsing from the 1.0.x documentation.

It's still unclear to me whether the impact goes past the unit tests being broken, but that should be addressed anyway (unless Werkzeug < 2.x is dropped explicitly).

[delroth]

FWIW, the following should be ~ equivalent and I confirmed it passes tests on both < 2 and >= 2.1. I don't know whether there was a good reason to not write the patch in this way (which seems like a smaller incremental diff), and I'm definitely not suggesting it's a good solution (let alone the best solution). I'm just providing this as a baseline for something that keeps backwards compat.

    def source(self, request):
        """Pulls values off the request in the provided location
        :param request: The flask request object to parse arguments from
        """
        if isinstance(self.location, six.string_types):
            value = getattr(request, self.location, MultiDict())
            if callable(value):
                if self.location in {"json", "get_json"}:
                    value = value(silent=True)
                else:
                    value = value()
            if value is not None:
                return value
        else:
            values = MultiDict()
            for l in self.location:
                value = getattr(request, l, None)
                if callable(value):
                    if self.location in {"json", "get_json"}:
                        value = value(silent=True)
                    else:
                        value = value()
                if value is not None:
                    values.update(value)
            return values

        return MultiDict()

[sreerajkksd]

Can we move this forward ? Do we think there is anything blocking us merging this ?

[pype-leila]

Are you guys planning on creating a new release with this at any point?