Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#17320 closed Cleanup/optimization (fixed)

Sites.domain should not contain whitespace

Reported by: Jason Novinger Owned by: Horst Gutmann
Component: contrib.sites Version: 1.3
Severity: Normal Keywords: sprint2013
Cc: Flavio Curella, krzysiumed@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Having space characters in the domain field of the Site model causes problems when using an instance to generate links (e.g. Share on Facebook).

Attachments (2)

remove_whitespace_from_domain.diff (1.7 KB) - added by Jason Novinger 5 years ago.
17320.diff (1.6 KB) - added by Christopher Medrela 5 years ago.

Download all attachments as: .zip

Change History (15)

Changed 5 years ago by Jason Novinger

comment:1 Changed 5 years ago by Flavio Curella

Cc: Flavio Curella added
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 5 years ago by Aymeric Augustin

Patch needs improvement: set
Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

I agree that spaces in domain names are invalid.

However, Django avoids "magic auto-fix", because it quickly becomes auto-break for some expectations.

If would be better to implement a clean_fields method in the Site model.

Changed 5 years ago by Christopher Medrela

Attachment: 17320.diff added

comment:3 Changed 5 years ago by Christopher Medrela

Owner: changed from nobody to Christopher Medrela
Patch needs improvement: unset
Status: newassigned

I'd implemented clean method instead of clean_fields - it's much more simpler (you've not to call parent).

Another issue is that my implementation check if domain doesn't contain spaces, but it doesn't check other whitespaces (for example '\r'). Is it necessary?

If it is, we can do it in a few ways:

if self.domain != (self.domain.translate(None, string.whitespace)):

or

if all(map(lambda whitespace: whitespace not in self.domain, string.whitespace)):

or

if all(map(lambda whitespace: self.domain.find(whitespace) == -1, string.whitespaces)):

or

if re.search('\s', self.domain):

The first three are ugly. The last one is the best, but it's slow (the pattern '\s' must be compiled each time - the solution is to cache compiled pattern).

comment:4 Changed 5 years ago by Christopher Medrela

Cc: krzysiumed@… added

comment:5 Changed 4 years ago by Horst Gutmann

Keywords: sprint2013 added
Owner: changed from Christopher Medrela to Horst Gutmann
Patch needs improvement: set

Personally, I believe that validating regarding spaces alone should be sufficient (unless we want to go for an all-out domain-name validation) but the current patch marks the whole model as invalid and not just the domain field. Also, since this is a user-facing validation message, it should be localized.

So the question raised above remains: Do we only need space-validation or should we go for an all-out domain name validation for this field?

comment:6 Changed 4 years ago by Mathijs de Bruin

Doing domain-name validation 'the right way' leads to a lot of added complexity in the code and, what's worse: the risk of false negatives. People not being able to add a reference to their own site is *not* what we want.

As for the validation: the error can and should happen at the field level by adding the validator to validators for the database field. For this an instance of Django's RegexValidator can be used.

comment:7 Changed 4 years ago by Horst Gutmann

Patch needs improvement: unset

Added a pull request: https://github.com/django/django/pull/828

I opted with a separate function to make space for documenting the simple nature of the validation.

Last edited 4 years ago by Horst Gutmann (previous) (diff)

comment:8 Changed 4 years ago by Ben Kornrath

I wonder if the error message should be geared more towards non-technical users and (to me) 'whitespace' feels like a technical term. What do you think about this for an error message?: "The domain name cannot contain any spaces or tabs."

comment:9 Changed 4 years ago by Horst Gutmann

@benkonrath sounds good to me. Amended the pull-request :)

comment:10 Changed 4 years ago by Mathijs de Bruin

Triage Stage: AcceptedReady for checkin

comment:11 Changed 4 years ago by Horst Gutmann <hgutmann@…>

Resolution: fixed
Status: assignedclosed

In c7294614795a3c0b3a872707bbea93733ef2077d:

Fixed #17320 -- Added whitespace validation to the Site.domain field

comment:12 Changed 4 years ago by Florian Apolloner <florian@…>

In 5e52dc2ade8b040f2cee743a108267562baf117e:

Merge pull request #828 from zerok/tickets/17320

Fixed #17320 -- Added whitespace validation to Site.domain field

comment:13 Changed 4 years ago by Florian Apolloner <florian@…>

In b88bf9f1257080d5dcc08792c8e57118fb183987:

Fixed python 3 support. Refs #17320

Note: See TracTickets for help on using tickets.
Back to Top