ForeignKey/OneToOneField attribute names should be valid kwargs in queries
|Reported by:||dstora||Owned by:||nobody|
|Component:||Database layer (models, ORM)||Version:||1.0|
|Severity:||Keywords:||foreign key ForeignKey OneToOneField query filter|
|Cc:||Triage Stage:||Design decision needed|
|Has patch:||yes||Needs documentation:||no|
|Needs tests:||no||Patch needs improvement:||no|
Currently, only the field names are available as kwargs for queries.
However, it makes (even more) sense that attribute names are supported too for ForeignKey/OneToOneField fields.
Let's take the following example:
- I initially have a table "Payment" with a "ccy_id" field designating a 3-letter currency code.
My model definition looks like:
from django.db import model class Payment(models.Model): # ... ccy_id = models.CharField(max_length=3)
In my python code I can retrieve payments by currency id as follows:
payment_list = list(Payment.objects.filter(ccy_id="USD"))
- Later, I decide to actually create a "Currency" table to hold some information about currencies.
And I also decide to define a foreign key constraint from "Payment" to "Currency".
My model now looks like:
from django.db import model class Currency(models.Model): ccy_id = models.CharField(max_length=3, primary_key=True) #... class Payment(models.Model): # ... ccy = models.ForeignKey(Currency, to_field="ccy_id", db_column="ccy_id")
The problem here is that my existing Python code is broken, because "ccy_id" is not accepted anymore as kwarg for "Payment" queries.
Django now only accepts "ccy" instead.
Based on the principle that defining things like foreign keys should only add functionality (and not remove any) it seems quite important that on top of the field names queries also support attribute name kwargs.
Looking at the code, I can see two ways of achieving this.
In both cases, the implementation of this feature is pretty small and fully backward-compatible:
- The first way is to register fields in options.Option against both their name and attname if these differ (SVN diff against trunk below)
Index: django/db/models/options.py =================================================================== --- django/db/models/options.py (revision 10924) +++ django/db/models/options.py (working copy) @@ -329,6 +329,8 @@ cache[f.name] = (f, model, True, True) for f, model in self.get_fields_with_model(): cache[f.name] = (f, model, True, False) + if f.attname != f.name: + cache[f.attname] = (f, model, True, False) if self.order_with_respect_to: cache['_order'] = OrderWrt(), None, True, False if app_cache_ready():
- The second way is at query level to always enable a logic currently marked as a hack that looks at attname if no field is found with the requested name (SVN diff against trunk below)
Index: django/db/models/sql/query.py =================================================================== --- django/db/models/sql/query.py (revision 10924) +++ django/db/models/sql/query.py (working copy) @@ -1678,8 +1678,7 @@ self.used_aliases = used_aliases def setup_joins(self, names, opts, alias, dupe_multis, allow_many=True, - allow_explicit_fk=False, can_reuse=None, negate=False, - process_extras=True): + can_reuse=None, negate=False, process_extras=True): """ Compute the necessary table joins for the passage through the fields given in 'names'. 'opts' is the Options class for the current model @@ -1715,10 +1714,7 @@ field, model, direct, m2m = opts.get_field_by_name(name) except FieldDoesNotExist: for f in opts.fields: - if allow_explicit_fk and name == f.attname: - # XXX: A hack to allow foo_id to work in values() for - # backwards compatibility purposes. If we dropped that - # feature, this could be removed. + if name == f.attname: field, model, direct, m2m = opts.get_field_by_name(f.name) break else: @@ -2026,8 +2022,7 @@ try: for name in field_names: field, target, u2, joins, u3, u4 = self.setup_joins( - name.split(LOOKUP_SEP), opts, alias, False, allow_m2m, - True) + name.split(LOOKUP_SEP), opts, alias, False, allow_m2m) final_alias = joins[-1] col = target.column if len(joins) > 1:
Change History (7)
comment:1 Changed 7 years ago by
|Patch needs improvement:||unset|
|Triage Stage:||Unreviewed → Design decision needed|