Code

Opened 4 years ago

Closed 3 years ago

#13511 closed Cleanup/optimization (fixed)

RegexValidator pattern is optional and should be required

Reported by: davidfischer Owned by: davidfischer
Component: Core (Other) Version: master
Severity: Normal Keywords: validators,regexvalidator
Cc: djfische@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: no UI/UX:

Description

While working on ticket #13341, I noticed that the code for RegexValidator could use a slight cleanup. Specifically, the first parameter of RegexValidator -- the regex pattern -- is optional. However, if it is not provided, a TypeError is thrown because the default value of None is passed to re.compile. The regex pattern should be required.

Attachments (2)

13511-regexvalidator.diff (1.6 KB) - added by davidfischer 4 years ago.
switched regex parameter to be required
13511-regexvalidator2.diff (2.0 KB) - added by davidfischer 3 years ago.
Patch leaves "regex" as optional, but documents and tests functionality

Download all attachments as: .zip

Change History (11)

comment:1 Changed 4 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by davidfischer

  • Cc djfische@… added
  • Owner changed from nobody to davidfischer

Changed 4 years ago by davidfischer

switched regex parameter to be required

comment:3 Changed 4 years ago by davidfischer

  • Has patch set
  • Status changed from new to assigned

I switched the regex parameter to required to match the documentation. While running the unit tests (forms & validation), I noticed that URLValidator relied on the fact that the regex parameter was optional by having a regex instance variable. I corrected that. However, this brings up a potential backwards compatibility issue.

The issue with this patch is that anybody who subclassed RegexValidator and relied on regex to be optional will have to update their code. However, it should be pretty obvious what went wrong and they probably should not have done that in the first place. I'm not sure how strict to be on maintaining backwards compatibility, but I think that breaking it in this small way will result in the cleanest code.

comment:4 Changed 4 years ago by darkrho

I just face this bug when subclassing RegexValidator. I think using self.regex instead regex as re.compile's parameter works and keeps backward compatilibity. Though, using RegexValidator without regex parameter will work in the same way that re.compile("") works.

  • django/core/validators.py

    diff --git a/django/core/validators.py b/django/core/validators.py
    a b  
    2929            self.code = code 
    3030 
    3131        if isinstance(self.regex, basestring): 
    32             self.regex = re.compile(regex) 
     32            self.regex = re.compile(self.regex) 
    3333 
    3434    def __call__(self, value): 
    3535        """ 

comment:5 Changed 4 years ago by davidfischer

@darkrho

That looks like it will work and it is better than what I had attached. It passes the unit test suite (validators) and it allows you to subclass RegexValidator. As you mentioned, creating a RegexValidator without specifying the "regex" will function the same as if the empty string was passed (a RegexValidator that always matches). I think that is fine, but maybe it needs to be communicated in the docs. The docs currently specify that "regex" is not optional. It can either be left like it is because a RegexValidator that matches everything is pretty useless, but perhaps the docs should just say that by default it matches everything.

comment:6 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Cleanup/optimization

comment:7 Changed 3 years ago by julien

  • Needs tests set

Changed 3 years ago by davidfischer

Patch leaves "regex" as optional, but documents and tests functionality

comment:8 Changed 3 years ago by davidfischer

  • Easy pickings unset

I added a patch based on darkrho's suggestion. It will not break backward compatibility with subclasses of RegexValidator. I added a note in the docs that if the user does not specify any regex, then RegexValidator will match any passed string.

comment:9 Changed 3 years ago by SmileyChris

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

In [16351]:

Fixes #13511 -- make regex parameter to RegexValidator to be optional. Also tidies up related docs (parameters aren't attributes). Thanks for the patch work, davidfischer.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.