[Django] #24585: Remove repetitive guarding against double quoting database identifiers
Django
----------------------------------------------+--------------------
Reporter: jdufresne | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Database layer (models, ORM) | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
----------------------------------------------+--------------------
All built-in database backends define a `quote_name()` function. In all
cases, the function first checks if the string argument is already quoted.
IMO, this looks a bit like "just in case" style programming and a bit of a
code smell. Ideally, the calling code should not be passing quoted names
in the first place. The Django logic should be such that names are quoted
exactly once and not multiple time. I believe if names are quoted twice,
it should be treated as a bug in the calling code, not a bug in the
`quote_name()` function.
After testing, these guards are unnecessary and all tests pass without it
across multiple database backends.
Removing the guard means it won't need to be checked every time SQL names
are quoted.
PR to follow.
--
Ticket URL: <https://code.djangoproject.com/ticket/24585>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: jdufresne | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by jdufresne):
* needs_better_patch: => 0
* has_patch: 0 => 1
* needs_tests: => 0
* needs_docs: => 0
Comment:
https://github.com/django/django/pull/4454
--
Ticket URL: <https://code.djangoproject.com/ticket/24585#comment:1>
Django
Reporter: jdufresne | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by claudep):
I know about one use case (undocumented/untested) where this check might
be useful. It's about unmanaged PostgreSQL tables in non-default (i.e. not
public) schema (the target of #6148). Currently, you can define the
`db_table` meta attribute to `'"schema"."table_name"'`, and it works, at
least with standard ORM methods. I'm afraid that your patch will break
this.
--
Ticket URL: <https://code.djangoproject.com/ticket/24585#comment:2>
Django
Reporter: jdufresne | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Severity: Normal | Resolution:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by aaugustin):
Does the possibility of backwards-incompatibility exist if user code
relies on the current behavior?
If I have to choose between "make things look a bit more clean" and "avoid
breaking user code" and there are no other advantages, the latter wins.
--
Ticket URL: <https://code.djangoproject.com/ticket/24585#comment:3>
Django
Reporter: jdufresne | Owner: nobody
Type: | Status: closed
Cleanup/optimization |
Component: Database layer | Version: master
(models, ORM) |
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by shaib):
* status: new => closed
* resolution: => wontfix
Comment:
This is also used by Oracle users to reference out-of-schema tables and to
preserve case (normally, the Oracle backend turns all names to uppercase).
We can think up cleaner alternatives, but the current behavior cannot be
simply removed without replacement; and this replacement would need to go
through a deprecation cycle as well.
The right place for discussing such alternatives is the
DevelopersMailingList.
--
Ticket URL: <https://code.djangoproject.com/ticket/24585#comment:4>