Code

Opened 6 years ago

Closed 6 years ago

#7113 closed (fixed)

The `ChangeList` does not support custom queryset subclasses

Reported by: jbronn Owned by: mtredinnick
Component: contrib.admin Version: master
Severity: Keywords: admin changelist queryset custom
Cc: andrew@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Custom QuerySet classes (like GeoDjango's GeoQuerySet) can't be merged with regular QuerySet objects (using |, & operators). This problem manifests itself when trying to use search fields with a model that uses a QuerySet subclass:

TypeError ...
Cannot merge querysets of different types ('GeoQuerySet' and 'QuerySet'.

The root cause is that ChangeList hard-codes using QuerySet for the search fields.

In my patch ChangeList takes advantage of existing information to determine the type of queryset being used and store it in a root_query_set_class attribute that is used when performing searches.

Attachments (1)

changelist_custom_qs.diff (1.1 KB) - added by jbronn 6 years ago.
Allows for custom querysets in the admin changelist.

Download all attachments as: .zip

Change History (7)

Changed 6 years ago by jbronn

Allows for custom querysets in the admin changelist.

comment:1 Changed 6 years ago by jbronn

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Summary changed from The `ChangeList` does not support custom queryset classes to The `ChangeList` does not support custom queryset subclasses

comment:2 Changed 6 years ago by brosner

  • Triage Stage changed from Unreviewed to Accepted
  • Version changed from newforms-admin to SVN

This looks right to me, but I think this should be applied to trunk first and then merged over to newforms-admin since this code isn't being directly modified in newforms-admin. Otherwise I'd apply this.

comment:3 Changed 6 years ago by mtredinnick

  • Owner changed from nobody to mtredinnick

comment:4 Changed 6 years ago by mtredinnick

I think you're fixing the symptom, rather than the root problem, with this patch. The problem with the fix here is that it's only the tip of the iceberg. Every single reusable app that uses a QuerySet will have to write similarly awkward code.

The real issue is the tighter restrictions on QuerySet combinations, which has gone a little overboard. The idea behind that change was to prevent people doing silly things like trying to merge a QuerySet and a ValuesQuerySet (which I saw attempted in a post to django-users), not to prevent sensible merges. I didn't think of all the consequences at the time.The problem with the fix here is that it's only the tip of the iceberg. Every single reusable app that uses a QuerySet will have to write similarly awkward code.

I'll rethink the sanity check and possibly remove it entirely (leading to more "GIGO" behaviour) if there's nothing better.

comment:5 Changed 6 years ago by anonymous

  • Cc andrew@… added

comment:6 Changed 6 years ago by mtredinnick

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

Fixed in [7742] (by removing the overly-strict error checking).

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.