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

allow uuid object on uuid validations #609

Closed
wants to merge 1 commit into from
Closed

allow uuid object on uuid validations #609

wants to merge 1 commit into from

Conversation

owgreen
Copy link
Contributor

@owgreen owgreen commented May 24, 2020

Fixes #549

Allow uuid object on validations of UUID.

@whb07
Copy link
Contributor

whb07 commented Jul 30, 2020

what is this PR doing here again? Its directly pulling some UUID object and then you're passing it directly into a form for validation? Can you explain the workflow that gets accomplished by this please?

@thijs-obj
Copy link

Coincidentally, this PR happens to fix another problem: if the data is None then the UUID validator gives a TypeError instead of ValueError. and as a result form.validate() will raise an exception instead of returning False

@azmeuk
Copy link
Member

azmeuk commented Oct 4, 2020

@whb07 I think the workflow is explained in #549. Basically, in the flask-admin project, a uuid model attribute ends up generating a StringField with a UUID validator, and gives a UUID directly to the validator instead of a string.

HTML form data are always str but usually fields cast them into convenient types, so validators deals with casted data instead of raw str. For instance IntegerField casts the str value into a int, then NumberRange only expects to handle numeric types.

There is no UUID field so the UUID validator expects to handle strings. When the field data is not string, this causes the validator to crash:

>>> import wtforms
>>> import uuid
>>> class F(wtforms.Form):
...     foo = wtforms.StringField(validators=[wtforms.validators.UUID()])
>>> class MyObject:
...     foo = uuid.uuid4()
>>> f = F(obj=MyObject())
>>> f.foo.data
UUID('33e7a9f7-19f5-4eae-aa76-051a2c28815f')
>>> f.validate()
Traceback (most recent call last):
  ...
'UUID' object has no attribute 'replace'

However, I don't know if this example is good practice. Can we validate a Form without HTML form data? I suppose nothing prevents that, even if the usual way to validate a form is if request.POST and form.validate(): @davidism what do you think?

@azmeuk
Copy link
Member

azmeuk commented Oct 4, 2020

Another usecase : passing an UUID without casting to the default parameter.

>>> import wtforms
>>> import uuid
>>> class F(wtforms.Form):
...     foo = wtforms.StringField(
...         validators=[wtforms.validators.UUID()],
...         default=uuid.uuid4(),
...     )
>>> F().validate()
Traceback (most recent call last):
...
'UUID' object has no attribute 'replace'

@azmeuk
Copy link
Member

azmeuk commented Oct 4, 2020

Similar behavior with DateTimeFields and datetime.datetime as default value.

>>> import wtforms
>>> import datetime
>>> class F(wtforms.Form):
...     foo = wtforms.DateTimeField(
...         default=datetime.datetime.now(),
...     )
>>> F().validate()
Traceback (most recent call last):
...
TypeError: strptime() argument 1 must be str, not datetime.datetime

@davidism
Copy link
Member

davidism commented Oct 4, 2020

I thought there was a broader issue about this, where we discussed whether fields and validators should accept strings, or also accept their types. Maybe I'm getting confused with Click, which has a similar issue with command line strings and parameter types.

@davidism
Copy link
Member

davidism commented Oct 4, 2020

It also seems like we'd want UUID to be a field, not a validator, since I'd expect to work with UUID objects in Python, not strings that happen to be valid UUIDs.

@azmeuk
Copy link
Member

azmeuk commented Oct 6, 2020

I have been thinking about this. In my opinion StringField should cast any input into string, so the validator could safely assume that the field data will be a string. I suppose that would require implementing a StringField.process_data method to do the cast.

@azmeuk azmeuk mentioned this pull request Oct 6, 2020
6 tasks
@TurnrDev
Copy link

Is there any hope of these fix being merged? I'm having the issue with flask-admin mentioned above and this seems like such an easy fix.

Copy link

@TurnrDev TurnrDev left a comment

Choose a reason for hiding this comment

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

Let's do this properly, eh?

Comment on lines 523 to 526
try:
uuid.UUID(field.data)
uuid.UUID(str(field.data))
except ValueError:
raise ValidationError(message)
Copy link

Choose a reason for hiding this comment

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

It might be best to check if the type is uuid.UUID instead of blindly casting to str

Suggested change
try:
uuid.UUID(field.data)
uuid.UUID(str(field.data))
except ValueError:
raise ValidationError(message)
if isinstance(field.data, uuid.UUID):
return
try:
uuid.UUID(field.data)
except ValueError:
raise ValidationError(message)

[
"2bc1c94f-0deb-43e9-92a1-4775189ec9f8",
"2bc1c94f0deb43e992a14775189ec9f8",
uuid.uuid4(),
Copy link

Choose a reason for hiding this comment

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

Suggested change
uuid.uuid4(),
uuid.UUID("2bc1c94f-0deb-43e9-92a1-4775189ec9f8"),

RNG in tests is bad!

@azmeuk
Copy link
Member

azmeuk commented Oct 10, 2023

Closing as superseded by #769, and anyways there is a larger design question to be discussed first.

@azmeuk azmeuk closed this Oct 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

UUID validator fails if field.data is a UUID object instead of str
6 participants