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

Missing form field falls back to json by default, which throws 400 error #444

Closed
Deimos opened this issue Dec 7, 2019 · 2 comments
Closed

Comments

@Deimos
Copy link

Deimos commented Dec 7, 2019

I'm working on trying to update my Pyramid application to webargs > 5.0, which is requiring a lot of changes. I've just run into an unexpected error with one of my forms, where I don't think webargs' default behavior is correct. It's somewhat similar to closed issue #409, but I wasn't specifying locations like they were, and just using the default behavior.

Specifically, my view is for a POST endpoint, and has a decorator like this on it (along with a second one that uses a schema for the other data):

@use_kwargs({"confirm": Boolean(missing=False)})

The form doesn't always contain the confirm input (it's for a special case), so my expectation is that when they submit the form without any data for confirm, it should get set to False.

However, because of this part of webargs' default behavior (from here):

By default, webargs will search for arguments from the URL query string (e.g. "/?name=foo"), form data, and JSON data (in that order).

What actually happens is that it fails to find confirm in the form data, so it tries to look for it in JSON, and then throws a 400 error (pyramid.httpexceptions.HTTPBadRequest: {'json': ['Invalid JSON body.']}) because there is no JSON on the request.

It works as expected if I specify locations=("form",) in @use_kwargs, but having the default behavior cause a crash by falling back to looking for nonexistent JSON data doesn't seem right. Should the lack of JSON data be treated as "no data" instead of an error?

Edit: wanted to also note that I'm upgrading from 4.4.1, and this worked fine in that version without crashing.

@lafrech
Copy link
Member

lafrech commented Dec 8, 2019

This should be fixed (more precisely obsoleted) by #420.

Deimos added a commit to spectria/tildes that referenced this issue Dec 9, 2019
As expected, these updates ended up requiring quite a few changes. I was
initially going to update only Marshmallow, but the older version of
webargs couldn't work with an updated Marshmallow, so I ended up needing
to do both at the same time.

The main changes required were:

* Schemas don't need to be specified as "strict" any more, so that could
  be removed from constructors and Meta classes.
* .validate() now returns a dict of errors (if any) instead of raising a
  ValidationError if anything goes wrong. This meant that I either need
  to check the returned dict, or switch to .load() to still get raised
  errors.
* Marshmallow doesn't support loading from two different field names as
  easily (and changed the name of that to data_key), so all the routes
  using "group_path" needed to be changed to just "path".
* Some of the Field methods and some decorated schema ones like
  @pre_load receive new arguments and needed to be updated to handle
  them and/or pass them on.
* webargs will no longer send a keyword argument for any fields that
  aren't specified and don't have a default value when using
  @use_kwargs. Because of this, I added missing= values for most
  optional fields, but still required some special treatment for the
  order query variable in a couple of topic listing views.

And finally, there is some strange behavior in webargs by default when a
form doesn't send any data for a field (due to the input not being
included or similar). When it doesn't find the field in form data, it
tries to fall back to checking for JSON data, but then crashes because
the request doesn't have any JSON data attached. I had to specify only
to look in the form data in a few places to fix this, but I've also
registered an issue against webargs related to it:
marshmallow-code/webargs#444
@lafrech
Copy link
Member

lafrech commented Feb 14, 2020

Closing this now that #420 was merged.

You can try the latest 6.0.0 beta releases to confirm that the issue is fixed.

If not, please comment here and we'll reopen.

@lafrech lafrech closed this as completed Feb 14, 2020
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

No branches or pull requests

2 participants