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
Factorize/unify uris validation #1586
Factorize/unify uris validation #1586
Conversation
@@ -130,8 +129,8 @@ class URLField(EmptyNone, Field): | |||
def pre_validate(self, form): | |||
if self.data: | |||
try: | |||
db.URLField().validate(self.data) | |||
except db.ValidationError: | |||
uris.validate(self.data) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the logic from udata/models/url_field.py
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the same logic, udata/models/url_field.py
also use uris.validate()
but to me this is more explicit to show the comme piece uris.validate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the idea of decoupling a little bit more here: rely on URLField
that relies on uris
in turn. But not a big problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you prefer I can, but:
- I though it was easier to find the common dependency when being explicitly written
db.URLField.validate()
is an instance method relying on custom field configuration. I though about giving the same parameters here too, but this is not currently in use- model and form validation are slightly different, and I wanted to be able highlight the differences while keeping a neutral behavior in
uris.validate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
udata/tests/test_model.py
Outdated
|
||
|
||
class DatetimedTest(DBTestMixin, TestCase): | ||
class Darivate(DBTestMixin, TestCase): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Darivate
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Weird, I do known from where that comes from
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably a multi-select gone wrong ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it must be
'http://[::]', | ||
'http://[::]:8080', | ||
'http://[::]:8080/index.html', | ||
] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I see what you mean when you said bulletproof ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😜
udata/uris.py
Outdated
from netaddr import IPAddress, AddrFormatError | ||
|
||
from udata.settings import Defaults | ||
# from tlds import tld_set as TLDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
udata/uris.py
Outdated
) | ||
|
||
# SCHEMES = Defaults.URLS_ALLOWED_SCHEMES | ||
# TLDS = Defaults.URLS_ALLOWED_TLDS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Remove?
This PR factorize URls handling ensuring every part of the app use the same rules (models, forms, harvesters...).
The common helpers is fully tested, support utf8, rely on official IANA TLDs list by default and is configurable to work localy or on an intranet.
The following parameters are added:
URLS_ALLOW_PRIVATE
default:
False
Whether or not to allow private URLs (private IPs...) submission
URLS_ALLOW_LOCAL
default:
False
Whether or not to allow local URLs (localhost...) submission.
When developping you might need to set this to
True
.URLS_ALLOW_CREDENTIALS
default:
True
Whether or not to allow credentials in URLs submission.
URLS_ALLOWED_SCHEMES
default:
('http', 'https', 'ftp', 'ftps')
List of allowed URL schemes.
URLS_ALLOWED_TLDS
default: All IANA registered TLDS
List of allowed TLDs.
When using udata on an intranet, you might want to add your own custom TLDs: