Code

Opened 4 years ago

Closed 2 years ago

#13646 closed Bug (wontfix)

Wrong RE in comma_separated_int_list_re

Reported by: aledr Owned by: nobody
Component: Database layer (models, ORM) Version: 1.2
Severity: Normal Keywords: CommaSeparatedIntegerField
Cc: alexandre@… Triage Stage: Design decision needed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The actual RE validator in comma_separated_int_list_re for CommaSeparatedIntegerField will allow some kind of following values:

',,,' '1,,2,3,'

It is

comma_separated_int_list_re = re.compile('^[\d,]+$')

and should be:

comma_separated_int_list_re = re.compile('^[\d,]*\d$')

Attachments (0)

Change History (9)

comment:1 Changed 4 years ago by erikr

  • Keywords CommaSeparatedIntegerField added; comma separated removed
  • milestone set to 1.3
  • Needs documentation unset
  • Needs tests set
  • Owner changed from nobody to erikr
  • Patch needs improvement unset
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 4 years ago by erikr

  • Component changed from Uncategorized to Database layer (models, ORM)
  • Needs tests unset
  • Triage Stage changed from Accepted to Design decision needed

Oh, just noticed this actually seems to be by design:

From tests/modeltests/model_forms/models.py:

>>> f = CommaSeparatedIntegerForm({'field': '1,,2'})
>>> f.is_valid()
True

So, this seems to be an intentional feature, and not a bug.
I'll mark this as design decision needed, for someone more experienced to have a look at it.

comment:3 Changed 4 years ago by erikr

  • Owner changed from erikr to nobody
  • Status changed from assigned to new

comment:4 Changed 4 years ago by erikr

  • milestone 1.3 deleted

comment:5 follow-up: Changed 4 years ago by aledr

  • Cc alexandre@… added

Just figured out that the correct should be:

re.compile('^(\d,)*\d$')

Is it possible to change in ticket description?

Thanks.

comment:6 in reply to: ↑ 5 ; follow-up: Changed 4 years ago by aledr

Replying to aledr:

Just figured out that the correct should be:

re.compile('!^(\d,)*\d$')

Is it possible to change in ticket description?

Thanks.

Wrong again, sorry... It's not possible to use '^' in this RE, It will always fail.

After tests done, correct is:

re.compile('(\d,)*\d$')

It will only allow integers, not nulls or commas or anything else comma separated.

comment:7 in reply to: ↑ 6 Changed 4 years ago by JohnDoe

Replying to aledr:

Replying to aledr:

Just figured out that the correct should be:

re.compile('!^(\d,)*\d$')

Is it possible to change in ticket description?

Thanks.

Wrong again, sorry... It's not possible to use '^' in this RE, It will always fail.

After tests done, correct is:

re.compile('(\d,)*\d$')

It will only allow integers, not nulls or commas or anything else comma separated.

What you're doing here doesn't make much sense.

IF this is how it should be designed, the answer is

re.compile('^(\d+,)*\d+$')

Still, there's no final design decision.

comment:8 Changed 3 years ago by julien

  • Severity set to Normal
  • Type set to Bug

comment:9 Changed 2 years ago by aaugustin

  • Easy pickings unset
  • Resolution set to wontfix
  • Status changed from new to closed
  • UI/UX unset

There are tests for the current behavior, so it appears to be intentional.

I don't think it's worth breaking backwards compatibility.

It's easy to write your own subclass with a different regex.

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.