Opened 12 years ago
Closed 12 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 , 12 years ago
comment:2 by , 12 years ago
Triage Stage: | Unreviewed → Accepted |
---|
Accepting this as I am going to do this...
comment:3 by , 12 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.