Opened 4 years ago

Closed 4 years ago

#31426 closed Cleanup/optimization (fixed)

Add proper field validation to QuerySet.order_by.

Reported by: Maxim Owned by: Simon Charette
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords: ORDER_PATTERN, order_by
Cc: Maxim Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no


When you annotate a QuerySet with a uuid key, the order_by functionality breaks for the uuid column because the uuid is "not a valid order_by argument".

Changing the constant django.db.models.sql.constants.ORDER_PATTERN by allowing a "-"


ORDER_PATTERN = re.compile(r'\?|[-+]?[.\w]+$')


ORDER_PATTERN = re.compile(r'\?|[-+]?[.\-\w]+$')

fixes this in PostgreSQL.

Is there a reason the former pattern was used, is it incompatible with other dbs?

Change History (8)

comment:1 by Simon Charette, 4 years ago

Easy pickings: unset
Has patch: unset
Triage Stage: UnreviewedAccepted
Type: New featureBug
Version: 2.2master

ORDER_PATTERN is a long standing hack that was added more than 12 years ago during the first iteration of the ORM and augmented later on to address #7098.

It should be replaced by proper field reference validation but could you go into more details about how you triggered this error? How did you manage to annotate a field containing an hyphen and pass it along to order_by.

Just to make it clear, order_by(str(uuid.uuid4()) is effectively not allowed and annotation using uuid as name should also not be supported

.annotate(**{str(uuid.uuid4()): expression})

is not supported and if you want to order by a programmatically added alias without risking field collision you should pass expression directly to .order_by().

Last edited 4 years ago by Simon Charette (previous) (diff)

comment:2 by Simon Charette, 4 years ago

Resolution: needsinfo
Status: newclosed

Closing as needsinfo for now until its determined whether or not it should be considered a cleanup of the regex based validation of order_by of if there's a legitimate issue with uuid annotation ordering.

comment:3 by Maxim, 4 years ago

The way I was able to annotate with an uuid as a key was by dict expansion like your example above. So while it might not be supported it did not throw any errors and from what I can tell works perfectly fine.

I'm currently running Django==2.2

comment:4 by Simon Charette, 4 years ago

Resolution: needsinfo
Status: closednew
Summary: Can't use uuid for order_byAdd proper field validation to QuerySet.order_by.
Type: BugCleanup/optimization

I'm not aware of any tested support for invalid Python identifiers as annotation name but I guess we could accept this ticket on the basis that order_by does weird things and should perform proper field validation instead of a naive regexp checks.

To summarize while tweaking the regex would address your immediate issue I think it's worth calling Query.names_to_path on str input to validate they are valid I'll submit a PR for that.

comment:5 by Simon Charette, 4 years ago

Owner: changed from nobody to Simon Charette
Status: newassigned

comment:7 by Maxim, 4 years ago

Cc: Maxim added

Sounds great, thank you

comment:8 by Mariusz Felisiak <felisiak.mariusz@…>, 4 years ago

Resolution: fixed
Status: assignedclosed

In 51394873:

Fixed #31426 -- Added proper field validation to QuerySet.order_by().

Resolve the field reference instead of using fragile regex based string
reference validation.

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