Code

Opened 3 months ago

Closed 6 weeks ago

#21863 closed New feature (fixed)

Consider adding get_transform() to supplement get_lookup()

Reported by: akaariai Owned by: nobody
Component: Database layer (models, ORM) Version: 1.7-alpha-1
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Currently the API for get_lookup() is get_lookup(lookup_name). However, there are situations where a field will need more information to decide if it needs to return a lookup or a transform. Consider HStoreField. A candidate API for HStoreField is that:

  • hstore_field__exact=val => exact lookup against the hstore_field
  • hstore_field=val => also exact lookup against the hstore_field
  • hstore_field__exact__exact=val => get key exact from hstore, do an exact lookup against that
  • hstore_field__nonlookup=val => get key nonlookup from hstore, do an exact lookup against that

This is similar to how fk__exact and fk__exact__exact work currently if the model pointed by fk has field named exact.

In addition, we want to support transforms in .values() etc calls in the future. So hstore_field__exact should always resolve to transform if used in values().

Currently this is impossible to do. The get_lookup() method doesn't know if there are still further lookups in the path. It knows just the current lookup. If we are resolving path exact__exact get_lookup() should return transform for the first exact, lookup for the second exact. If we are resolving just exact, then lookup should be returned for the first 'exact'. When resolving exact for .values('hstorefield__exact') call in later releases, the exact should resolve to transform.

If we add get_transform() method we could change the lookup resolution code in the ORM to do:

  • for non-final lookups: always use get_transform()
  • for final lookup in filter() calls: try get_lookup(lookup_name). If that doesn't return anything, use get_transform(lookup_name) + get_lookup('exact').
  • for any lookup in .values() etc calls: always use get_transform()

The above examples would be resolved as follows:

  • hstorefield__exact=val: get_lookup('exact') -> match, so use exact lookup against hstorefield
  • hstorefield=val: get_lookup('exact') -> same as above
  • hstorefield__exact__exact=val: get_transform('exact'), get_lookup('exact') -> match, so use exact lookup against key exact of hstorefield
  • hstorefield__nonlookup=val: get_lookup('nonlookup') -> no match, so try get_transform('nonlookup') + get_lookup('exact') -> exact lookup against key nonlookup

The values() call will use always get_transform() so .values('hstorefield__exact)` will work correctly.

There might also be need to add register_transform() method. If we have only register_lookup() but methods get_lookup() and get_transform() it might lead to some confusion.

Adding register_transform() will break the API, but that is hopefully OK during alpha.

Attachments (0)

Change History (4)

comment:1 Changed 2 months ago by mjtamlyn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 7 weeks ago by akaariai

  • Has patch set
  • Needs documentation set

I've created a patch for this, see https://github.com/akaariai/django/compare/ticket_21863. Docs missing, but it should be OK otherwise.

I think we can easily add this still before beta, the change isn't that big (only those who override get_lookup() will be affected), and this allows for better implementations of nested fields (JSONField for example). It is notable that even with this change using JSONField or HStoreField on existing database schemas might be hard. What if you have a key like 'foo-bar'? That is not allowed as a keyword lookup with Django. We might need ability to use strings instead of kwarg=value for lookups (say, Q('jsonfield__foo-bar__exact', value)). Then the only problematic case would be __ in keys, but that seems really minor corner case. Another way to tackle this problem is addition of .annotate(foo_bar=JSONExtract('jsonfield', 'foo-bar')) (that is, allow annotating non-aggregates to query). After doing the annotation one could just do qs.filter(foo_bar__exact=value).

comment:3 Changed 7 weeks ago by mjtamlyn

Looks good to me for a first glance. I've got an idea to cover the foo-bar problem - at least for hstore. I think we should also support hstorefield__contains={'foo-bar': 'value'} as well. This is a "more than one way to do it", but there are some reasons why that would be a cleaner API with unknown values (possibly also with nested JSON structures), and it does make logical sense. This means you've always got this to fall back on if the "shorthand" of hstorefield__foobar='value' does not work, as well as the more explicit hstorefield__foobar__exact='value'.

I will try to update the docs tomorrow, but it doesn't need too much mention IMO above the examples in the ticket description. Let's get it in before the beta.

comment:4 Changed 6 weeks ago by Marc Tamlyn <marc.tamlyn@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 219d928852c256a81d09dbaa29ed4cec42d2fdfa:

Fixed #21863 -- supplemented get_lookup() with get_transform()

Also fixed #22124 -- Expanded explanation of exactly what is going on in
as_sql() methods.

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.