[Django] #31426: Can't use uuid for order_by
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: nobody
Type: New | Status: new
feature |
Component: Database | Version: 2.2
layer (models, ORM) | Keywords: ORDER_PATTERN,
Severity: Normal | order_by
Triage Stage: | Has patch: 1
Unreviewed |
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 1
UI/UX: 0 |
-------------------------------------+-------------------------------------
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 "-"
\\
from
{{{
ORDER_PATTERN = re.compile(r'\?|[-+]?[.\w]+$')
}}}
to
{{{
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?
--
Ticket URL: <https://code.djangoproject.com/ticket/31426>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORDER_PATTERN, | Triage Stage: Accepted
order_by |
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* has_patch: 1 => 0
* version: 2.2 => master
* type: New feature => Bug
* easy: 1 => 0
* stage: Unreviewed => Accepted
Comment:
`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 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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:1>
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Keywords: ORDER_PATTERN, | Triage Stage: Accepted
order_by |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* status: new => closed
* resolution: => needsinfo
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:2>
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: nobody
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution: needsinfo
Keywords: ORDER_PATTERN, | Triage Stage: Accepted
order_by |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Comment (by Maxim):
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
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: nobody
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
order_by |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* status: closed => new
* type: Bug => Cleanup/optimization
* resolution: needsinfo =>
Comment:
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.
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORDER_PATTERN, | Triage Stage: Accepted
order_by |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* owner: nobody => Simon Charette
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORDER_PATTERN, | Triage Stage: Accepted
order_by |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
Changes (by Simon Charette):
* has_patch: 0 => 1
Comment:
https://github.com/django/django/pull/12669
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: Simon
Type: | Charette
Cleanup/optimization | Status: assigned
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: ORDER_PATTERN, | Triage Stage: Accepted
order_by |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* cc: Maxim (added)
Comment:
Sounds great, thank you
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Maxim | Owner: Simon
Type: | Charette
Component: Database layer | Version: master
(models, ORM) |
Keywords: ORDER_PATTERN, | Triage Stage: Accepted
order_by |
Needs tests: 0 | Patch needs improvement: 0
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"513948735b799239f3ef8c89397592445e1a0cd5" 51394873]:
{{{
#!CommitTicketReference repository=""
revision="513948735b799239f3ef8c89397592445e1a0cd5"
Fixed #31426 -- Added proper field validation to QuerySet.order_by().
Resolve the field reference instead of using fragile regex based string
reference validation.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/31426#comment:8>