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

Async validators + Recaptcha #14

Open
wants to merge 38 commits into
base: master
Choose a base branch
from
Open

Conversation

omarryhan
Copy link
Collaborator

#13

New features:

  • Support for async and sync validators
  • testing py37

The code I added isn't of best quality, especially the tests. This is only meant to be a prototype.

I think in order to merge this feature, we'd have to go 1 of the following paths:

  1. Break backward compatibility and make validate_on_submit a coroutine.
  2. Keep both validate_on_submit and validate_on_submit_async (maybe name it differently). However, if we do this, then the user shouldn't call validate_on_submit_async followed by validate_on_submit, because validate_on_submit_async will do lots of monkey patching (We can have some sort of flag though that explicitly enables this functionality).
  3. Make a new SanicForm class. Maybe call it SanicForm_ or sth
  4. Do nothing 😄

@omarryhan
Copy link
Collaborator Author

So, I did some cleanup. I think the only thing missing is patching FieldList. If left as is, then it will only be called synchronously by any given form. However, it will fail if any of it's methods are coroutines as they will not be awaited. Basically, as long as you don't use custom FieldList with coroutines in it, you're fine.

As for the tests coverage, I think I covered the main use cases. There might be some untested edge cases though, so if someone can help me spot some, it would be great.

@omarryhan omarryhan changed the title Don't merge me Async validators Dec 24, 2018
Updated readme
Bumped version to 0.7
100% backward compatible changes
More tests. Coverage at 86%
Docs still do not reflect the changes in the repo
@omarryhan omarryhan changed the title Async validators Async validators + Recaptcha Dec 26, 2018
@pyx
Copy link
Owner

pyx commented Jan 2, 2019

Thanks for the feature, I was very busy lately, I did not have time to go through the code right now, do you mind give me sometime. Thanks again.

@omarryhan
Copy link
Collaborator Author

Sure, np! :)

@pyx
Copy link
Owner

pyx commented Jun 7, 2019

Hi Omar, sorry that I was super busy lately, it seems you put a lot of effort into this, I will go through this in a few days.

Thank you so much.

@pyx
Copy link
Owner

pyx commented Dec 14, 2020

Hi Omar Ryhan
I am sorry I failed in manage time to update the project, are you still interested in using this? If you are interested, I will put you into collaborators list.

@pyx
Copy link
Owner

pyx commented Dec 16, 2020

@mikekeda How about this one, @omarryhan invested a lot of time into this, what's your thought?

@pyx
Copy link
Owner

pyx commented Dec 16, 2020

@mikekeda I at OP accidentally in last post.

@omarryhan
Copy link
Collaborator Author

Hey @pyx. No worries.

I only passively maintain the Sanic projects I was working on. The Sanic team recently introduced some breaking changes to the request context API and I'm not really sure whether my PR might break things or not. My Sanic projects are still running on their latest LTS version which differs from their latest version. I'm pip installing from my own sanic-wtf repo, and things are working fine.

I suggest we leave this PR open until someone shows some interest and maybe tests things out on the latest Sanic version, then maybe we can merge it?

@omarryhan
Copy link
Collaborator Author

I must also add that I monkey patched some of wtform's private methods to make this lib support async validations. The downside of this that wtforms cannot guarantee us that they won't break their private APIs. So I hard-coded the wtforms version to avoid things breaking. One downside of that is that we'll not be getting the latest security patches from wtforms should there be any.

@pyx
Copy link
Owner

pyx commented Dec 19, 2020

@omarryhan you are right, relying on private API is fragile to say the least and we should try to avoid. We will wait and see.

Please remember to add your name in AUTHOR with next commit, not necessarily this one but can be something like a separate one updating python versions tested upon, you have the commit rights.
Thank you.

install:
pip install tox-travis
script:
tox
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add a new line to the end


__version__ = '0.6.0.dev0'
from ._patch import patch
from .recaptcha import RecaptchaField
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's better to use absolute imports

@@ -110,7 +114,6 @@ def getlist(self, name, default=None):
"""
return super().get(name, default)


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

according to pep8 here should be 2 blank lines, no need to remove 1 blank line
https://www.python.org/dev/peps/pep-0008/#blank-lines

def validate_on_submit(self):
''' For async validators: use self.validate_on_submit_async.
This method is only still here for backward compatibility
'''
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

self._errors = None
success = True
for name, field in self._fields.items():
if extra_validators is not None and name in extra_validators:
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's more pythonic to use if extra_validators instead of extra_validators is not None
but be aware that for example if 0 returns False and if 0 is not None returns True

_base_config = {'WTF_CSRF_ENABLED': False}
test_app = App({**_base_config, **config})
return Request(test_app)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add 1 blank line before each top level function
(here should be 2 blank lines in total - https://www.python.org/dev/peps/pep-0008/#blank-lines)

def __init__(self, config):
self.config = config


Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove 1 blank line

widget = recaptcha_widget

def __init__(self, label='Recaptcha', extra_validators: Union[list, tuple, set]=None, **kwargs):
validators = set([recaptcha_validator])
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

validators = {recaptcha_validator}
will be more pythonic


widget = recaptcha_widget

def __init__(self, label='Recaptcha', extra_validators: Union[list, tuple, set]=None, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add spaces before and after = if you use typing
should be extra_validators: Union[list, tuple, set] = None instead of extra_validators: Union[list, tuple, set]=None

deps = -rrequirements-dev.txt
commands = py.test
commands =
./clear_pycache.sh
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why do we nee to clear pycache?

@mikekeda
Copy link
Collaborator

@mikekeda How about this one, @omarryhan invested a lot of time into this, what's your thought?

  1. I think it's better to split this into 2 RPs Async validators and Recaptcha
  2. monkey patching doesn't look good to me, can we create a separate class for this?
  3. What performance improvement we will get with Async validators?
  4. I don't think it's a good idea to make RECAPTCHA required, it's better to make it optional (in RECAPTCHA_PUBLIC_KEY and RECAPTCHA_PRIVATE_KEY are marked as required)
  5. Why clear and clear_pycache.sh are committed?

@omarryhan
Copy link
Collaborator Author

Thanks for taking the time to review @mikekeda

I added async not for the performance gain but to be able to call async code for validations, like querying a database to check if an email exists.

I'll address these issues once people show enough interest in the PR. Right now, it seems like I'm the only one using it.

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

Successfully merging this pull request may close these issues.

None yet

3 participants