Opened 12 years ago

Closed 11 years ago

Last modified 11 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 12 years ago.
17320.diff (1.6 KB ) - added by Christopher Medrela 12 years ago.

Download all attachments as: .zip

Change History (15)

by Jason Novinger, 12 years ago

comment:1 by Flavio Curella, 12 years ago

Cc: Flavio Curella added

comment:2 by Aymeric Augustin, 12 years ago

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.

by Christopher Medrela, 12 years ago

Attachment: 17320.diff added

comment:3 by Christopher Medrela, 12 years ago

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 by Christopher Medrela, 12 years ago

Cc: krzysiumed@… added

comment:5 by Horst Gutmann, 11 years ago

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 by Mathijs de Bruin, 11 years ago

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 by Horst Gutmann, 11 years ago

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 11 years ago by Horst Gutmann (previous) (diff)

comment:8 by Ben Kornrath, 11 years ago

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 by Horst Gutmann, 11 years ago

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

comment:10 by Mathijs de Bruin, 11 years ago

Triage Stage: AcceptedReady for checkin

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

Resolution: fixed
Status: assignedclosed

In c7294614795a3c0b3a872707bbea93733ef2077d:

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

comment:12 by Florian Apolloner <florian@…>, 11 years ago

In 5e52dc2ade8b040f2cee743a108267562baf117e:

Merge pull request #828 from zerok/tickets/17320

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

comment:13 by Florian Apolloner <florian@…>, 11 years ago

In b88bf9f1257080d5dcc08792c8e57118fb183987:

Fixed python 3 support. Refs #17320

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