Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#17320 closed Cleanup/optimization (fixed)

Sites.domain should not contain whitespace

Reported by: jnovinger Owned by: zerok
Component: contrib.sites Version: 1.3
Severity: Normal Keywords: sprint2013
Cc: fcurella, 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 jnovinger 3 years ago.
17320.diff (1.6 KB) - added by krzysiumed 3 years ago.

Download all attachments as: .zip

Change History (15)

Changed 3 years ago by jnovinger

comment:1 Changed 3 years ago by fcurella

  • Cc fcurella added
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 3 years ago by aaugustin

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Bug to Cleanup/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 3 years ago by krzysiumed

comment:3 Changed 3 years ago by krzysiumed

  • Owner changed from nobody to krzysiumed
  • Patch needs improvement unset
  • Status changed from new to assigned

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 3 years ago by krzysiumed

  • Cc krzysiumed@… added

comment:5 Changed 2 years ago by zerok

  • Keywords sprint2013 added
  • Owner changed from krzysiumed to zerok
  • 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 2 years ago by dokterbob

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 2 years ago by zerok

  • 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 2 years ago by zerok (previous) (diff)

comment:8 Changed 2 years ago by benkonrath

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 2 years ago by zerok

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

comment:10 Changed 2 years ago by dokterbob

  • Triage Stage changed from Accepted to Ready for checkin

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

  • Resolution set to fixed
  • Status changed from assigned to closed

In c7294614795a3c0b3a872707bbea93733ef2077d:

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

comment:12 Changed 2 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 2 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