Opened 12 years ago

Closed 11 years ago

#18816 closed Cleanup/optimization (fixed)

Remove "trim" argument from add_filter()

Reported by: Anssi Kääriäinen Owned by: nobody
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The sql.query.Query.split_exclude() needs to trim one final join when it adds filters to the original query. It does this by passing "trim" to add_filter(), and add_filter() then knows to trim the last join.

We can likely directly trim the last part of the join path in split_exclude() directly. This will remove the need to burden the add_filter() with the trim argument, and thus makes the ORM code easier to follow.

The patch in https://github.com/akaariai/django/tree/remove_trim is not commit ready. There are two problems in need of an answer:

  • is the unconditional use of "pk__in" as the lookup in case there were just one lookup part in the path correct? It seems the pk would need to be changed to something else in case there is a to_field argument for the relation used.
  • removing the trim argument from add_filter() will result in allow_m2m value being different when add_filter() is called from split_exclude().

For the above reasons: while tests pass with the above patch, I don't think it is correct.

The removal of trim arg would also maybe make it possible to remove the current trim_joins() altogether when combined with the patch from #10790, and this in turn would allow to change the return value from setup_joins() to something nicer. So, this is one more part in trying to clean the ORM.

Change History (3)

comment:1 by Anssi Kääriäinen, 12 years ago

I have updated the patch, and both of the above issues are resolved. Resolving the first issue got somewhat dirty (because the related field API is somewhat dirty), the second issue isn't as the allow_many change for add_filter(trim=True) is there to prevent indefinite recursion. This doesn't happen when the prefix is pre-trimmed.

I do think this is still a net gain in code clarity. Currently, split_exclude() passes a flag to add_filter which passes it to trim_joins. However, neither add_filter or trim_joins should know anything about this special prefix trimming need of split_exclude(). In addition, it is hard to see what exactly split_exclude() is doing, as it just passes a flag around instead of just trimming the prefix.

Patch is at the same branch as before, tests in one commit, fix in another.

comment:2 by Anssi Kääriäinen, 11 years ago

Triage Stage: UnreviewedAccepted

Accepting this as I am going to do this...

comment:3 by Anssi Kääriäinen <akaariai@…>, 11 years ago

Resolution: fixed
Status: newclosed

In f811649710fb51e48217e9a78991735977decfd8:

Fixed #18816 -- Removed "trim" argument from add_filter()

The trim argument was used by split_exclude() only to trim the last
join from the given lookup. It is cleaner to just trim the last part
from the lookup in split_exclude() directly so that there is no need
to burden add_filter() with the logic needed for only split_exclude().

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