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

Booleanfield broken in 2.2 #460

Closed
hyperknot opened this issue Jan 10, 2019 · 16 comments
Closed

Booleanfield broken in 2.2 #460

hyperknot opened this issue Jan 10, 2019 · 16 comments

Comments

@hyperknot
Copy link

hyperknot commented Jan 10, 2019

Booleanfield's checked state got broken in 2.2

If an object's member variable is True, the generated field's data will be False.

# user.verified is True

class UserEditForm(Form):
    verified = BooleanField('verified')

form = UserEditForm(obj=user)
print(user.verified) # True
print(form.verified.data) # False
@ftm
Copy link
Contributor

ftm commented Feb 11, 2019

I can't seem to verify your example using WTForms version 2.2.1. Are you still having this error?

>>> from wtforms import Form, BooleanField
>>> class User:
...     verified = True
... 
>>> user = User()
>>> user.verified
True
>>> class UserForm(Form):
...     verified = BooleanField('verified')
... 
>>> form = UserForm(obj=user)
>>> form.verified.data
True

@ftm
Copy link
Contributor

ftm commented May 5, 2019

It's been nearly three months since I asked if the error is still occurring and there has been no response so I'm going to assume this is no longer an issue.

@ftm ftm closed this as completed May 5, 2019
@hyperknot
Copy link
Author

@ftm yes it's still present!

I've downgraded that production site to < 2.2 since I've opened this ticket, for a workaround. Just now I've got your message and I tried it locally, and the bug is still present in 2.2.1!

@hyperknot
Copy link
Author

hyperknot commented May 5, 2019

I made a minimal reproducible case. The key is request.POST and the reason it worked in your script. request.POST triggers the bug in 2.2 and 2.2.1.

from wsgiref.simple_server import make_server

from pyramid.config import Configurator
from wtforms import BooleanField, Form


class User:
    verified = None


class UserForm(Form):
    verified = BooleanField('verified')


def user_edit(request):
    user = User()
    user.verified = True

    form = UserForm(request.POST, obj=user)

    print(user.verified)
    print(form.verified.data)

    return {}


if __name__ == '__main__':
    with Configurator() as config:
        config.add_route('user_edit', '/')
        config.add_view(user_edit, route_name='user_edit', renderer='json')
        app = config.make_wsgi_app()
    server = make_server('0.0.0.0', 7000, app)
    server.serve_forever()

You only need to install pyramid for this and just open or curl http://localhost:7000.

@ftm
Copy link
Contributor

ftm commented May 5, 2019

Ah, okay, thanks for providing that example. I can now verify this issue, I'll take a look.

@ftm ftm reopened this May 5, 2019
@ftm
Copy link
Contributor

ftm commented May 5, 2019

I believe I have found the issue but it's not actually to do with BooleanField, it's to do with how the lack of POST data is being interpreted incorrectly.

When a field is processed, the form data (request.POST) is processed after the data passed in (user) but only if the form data is not None (see here). The issue is that even on a GET request, request.POST is not None, but is a WebOb NoVars object which WTForms essentially interprets as valid form data. Therefore WTForms tries to find the verified field but obviously can't and so tries to process an empty list.

So when BooleanField comes to process this "form data" it sees the empty list and sets its data to False, overriding the data passed into the form (the User). This is the expected behaviour for BooleanField as in a regular HTML form submission, an unchecked checkbox is represented by just not including it in the form data.

The problem code is below, specifically lines 40-42:
https://github.com/wtforms/wtforms/blob/49e0a6b914132f05234324f7f3a861615f5f4045/src/wtforms/meta.py#L29-L48

formdata is the NoVars object so it tries to interpret it and returns the WebobInputWrapper as it can't tell the difference between NoVars and a WebOb object containing submitted data.

My proposed fix for this issue is to add an additional check in that function to try and detect the NoVars object by checking its length and returning None as follows:

if formdata is not None and not hasattr(formdata, 'getlist'):
            if hasattr(formdata, 'getall'):
                # Presence of getall attribute implies Webob-style multidict
                if len(formdata) == 0:
                    # If length is 0, formdata is NoVars: it doesn't contain any
                    # information so None should be returned to prevent errors
                    # occuring due to formdata not being None
                    return None
                
                return WebobInputWrapper(formdata)
            else:
                raise TypeError("formdata should be a multidict-type wrapper that supports the 'getlist' method")
        return formdata

I've tested this and it makes @hyperknot's example work correctly, I'll write up and submit a PR.

@hyperknot
Copy link
Author

Ah I get it, reading WebobInputWrapper in utils.py. not hasattr(formdata, 'getlist') is actually a check for non-MultiDict, which means using WebOb specific API.

The big question though is what changed between 2.1 and 2.2 which is causing this.

@hyperknot
Copy link
Author

I have asked WebOb developers about what is their recommended approach to detect the NoVars case. My personal recommendation would be to use type(formdata).__name__ == 'NoVars', but I'm waiting for their reply. Checking for len = 0 as None might not be a good idea, for example if the user submits a valid, empty form.

@ftm
Copy link
Contributor

ftm commented May 5, 2019

Ah yeah that's a good point, this would break a valid but empty form submission. Could you put the comments regarding what they say on the pull request instead of this issue?

It looks like what changed was #280. This changed the formdata check from just checking if formdata was truthy to checking if formdata was None. It seems NoVars and the WTForms wrapper are both not truthy so it never tried processing the data.

@hyperknot
Copy link
Author

Yes, I see, it's exactly #280 what introduced this. I'll comment on the PR.

@ftm
Copy link
Contributor

ftm commented May 5, 2019

Until this gets fixed I guess a temporary workaround could be to just check if the request method was POST and only pass request.POST in if it was. I'm not familiar with Pyramid but I assume you could do something like the following:

def user_edit(request):
    user = User()

    if request.method == "POST":
        form = UserForm(request.POST, obj=user)
    else:
        form = UserForm(obj=user)

    return {}

@hyperknot
Copy link
Author

Yes, it's a good workaround!

@ftm
Copy link
Contributor

ftm commented Jun 12, 2019

@hyperknot Did the WebOb developers ever get back to you?

@azmeuk
Copy link
Member

azmeuk commented Aug 20, 2019

I handle it this way:

def user_edit(request):
    user = User()
    form = UserForm(request.POST or None, obj=user)
    return {}

@davidism
Copy link
Member

This is a specific case of #402.

@hyperknot
Copy link
Author

I just did some new testing with Pyramid 2.0 and WTForms 3.0 latest. The issue is still present and the best workaround is to follow @azmeuk 's solution of

    form = UserForm(request.POST or None, obj=user)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

4 participants