litestar-org / litestar

Production-ready, Light, Flexible and Extensible ASGI API framework | Effortlessly Build Performant APIs
https://litestar.dev/
MIT License
5.5k stars 375 forks source link

Enhancement: WTForms support #956

Closed Goldziher closed 1 year ago

Goldziher commented 1 year ago

We should add support for WTForms under starlite.contrib. As an example see: https://github.com/muicss/starlette-wtf and https://github.com/wtforms/flask-wtf

jenish2014 commented 1 year ago

Another example is from Quart-framework. Both starlette and quart implement ImmutableMultiDict data structure with getlist() method to retrive form data. Starlette version has a release but it doesn't implement file handling. Quart version does implement file handling but it has some issues to resolve and apparently devs at Quart are not interested in it.

Goldziher commented 1 year ago

Another example is from Quart-framework. Both starlette and quart implement ImmutableMultiDict data structure with getlist() method to retrive form data. Starlette version has a release but it doesn't implement file handling. Quart version does implement file handling but it has some issues to resolve and apparently devs at Quart are not interested in it.

We can undeprecate getlist, or we can simply implement an adapter.

jenish2014 commented 1 year ago

Current Starlite situation

if you do :

    form_data = await request.form()
    form = RegistrationForm(form_data) <- WTForm,   raises keyerror

however:

    form_data = await request.form()
    usename = form_data['username']
    password = form_data['password']
    form = RegistrationForm(username=username, password=password) <- WTForm,   works fine
jenish2014 commented 1 year ago

So, I looked into WTForms code and found out it tries hasattrs for getlist method and if it doesn't find that it tries hasattrs for getall. If getall is found it wraps getlist over getall.

In case of Starlite it finds getlist which is deprecated.

Goldziher commented 1 year ago

@jenish2014 - we can "undeprecate" it - if this is a blocker.

jenish2014 commented 1 year ago

Yes, either undeprecate getlist or remove it all-together.

WTForms provide CSRF feature. If developers want to use WTFforms' CSRF over starlite's then either it needs to be implemented as per its documents or be left to dev to implement it.

Edit:

getlist calls super.getall() instead change it to self.getall() and that would work too.

Goldziher commented 1 year ago

Yes, either undeprecate getlist or remove it all-together.

WTForms provide CSRF feature. If developers want to use WTFforms' CSRF over starlite's then either it needs to be implemented as per its documents or be left to dev to implement it.

Edit:

getlist calls super.getall() instead change it to self.getall() and that would work too.

Do you want to add a PR perhaps?

jenish2014 commented 1 year ago

It is safe to assume its 25th Dec where ever you are and I wish you Merry Christmas / Happy Holidays and Happy New Year.

I've got the form working - it creates HTML tags, prints it in browser and validates data on the server. But, CSRF and File Upload features are not working. I'll try to find a solution for it in next two days and add a PR then.

provinzkraut commented 1 year ago

I believe a proper WTForms integration would be the better way to go here. We can create a StarliteForm and integrate that e.g. as a validation target type in route handlers.

jenish2014 commented 1 year ago

Hi, I looked over @provinzkraut 's comment and I agree with him there should be proper integration of WTForms so, instead of making changes to starlite source-code, I went ahead and created a separate repo starlite-wtf. It patches FormMultiDict returned from request.form().

Check it out and let me know if that is okay. Consider it as early draft and maybe, if it fulfills all the requirements, then it could be merge with starlite as contrib.

@provinzkraut can you provide pseudo code for or expand on what you mean by - validation target type in route handlers.

Unfortunately, I couldn't workout how to implement WTForms' SessionCSRF and is left out. I've tested disabling WTForms' csrf and using Starlite's built-in and works fine.

jenish2014 commented 1 year ago

Hi, This is awkward and embarrassing to admit at this point that there is not need for starlette-wtf like integration. Just use Starlite's best practices and WTForms works.

Mistakes I made:

Following code snippet is how one can pass form data to data parameter in Starlite:

class BasicForm(Form):
    firstname = StringField(name='firstname', validators=[Length(min=5, max=8)])
    lastname = StringField(name='lastname')
    active = BooleanField(name='active')

@get()
async def basic_form() -> Template:
    form = BasicForm()
    return Template(name='form.html', context={'form': form})

@post()
async def register(
    data: Any = Body(media_type=RequestEncodingType.URL_ENCODED)
) -> Response:
    form = BasicForm(data=data)
    if form.validate():
        # Do some stuff
        return Response('Success')
    else:
        return Response(f'Validation Failed {form.errors}')

Conclusion: Both WTForms and Starlite are excellent python libraries. WTForms provides multiple ways to pass data and Starlite makes it easy to fetch data using Body(). WTForm's SessionCSRF still won't work but as you know Starlite's CSRF works just fine just disable WTForms csrf.

In the end I would say just follow Starlite's best practices to get request-data and pass that to data parameter of WTForms' Form. I don't see any need for starlette-wtf like approach to WTForm's integration with Starlite. I will leave my repo starlite-wtf as it is if anyone wants alternate interface and async validators.

hasansezertasan commented 6 months ago

WTForms provide CSRF feature. If developers want to use WTFforms' CSRF over starlite's then either it needs to be implemented as per its documents or be left to dev to implement it.

I think it should be left to dev to implement it.

I also think offering async validators could be helpful.