Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

InheritanceManager get_subclass may be clobbering previous select_subclasses #91

Closed

Conversation

kezabelle
Copy link
Collaborator

This is not merge ready, and is a sketch of something I think is an issue.

Basically, given something like:

qs = MyModel.objects.all().select_subclasses('only_one_type')
# lots of other code between the original qs and the call to get_subclass
if not request.user.is_superuser:
    qs = qs.filter(is_published=True)
qs = qs.exclude(some_thing=1)
# and so on ... imagine we've paged the original qs out of viewport.
# we don't know what qs contains, we just trust it to return the 
# correct thing.
obj = qs.get_subclass(pk=kwargs['pk'])

The expectation I'd have is that obj is of whatever only_one_type yields down to, even if, say, there's only_one_type__subclass further down the inheritance chain, because that's what was requested.

As it stands, the get_subclass method on the InheritanceQuerySet does a bare select_subclasses assuming that it needs to do so - this also means that given a parent model with a lot of subclasses, it does LEFT OUTER JOIN for each one, even if you've asked for only a subset to be joined.

The pull request is intended to change that so that the get_subclass method inspects self for a subclasses attr, in the same way the iterator itself does.

However, the tests as I've committed them are not usefully functional in demonstrating the problem at hand, with the exception of test_get_subclass_unrelated_instance - the very nature of the problem (a bare select_subclasses replacing any previous) means the others pass, and I can't currently think of a clear and non-convoluted way of inspecting the subclasses attr on the queryset, because it's not a queryset we're getting back, but an individual downcast object.

As such, I'm opening this PR by way of discussing whether we think it's a problem, and how it can be tested reasonably.

and selecting all possible related classes, even
when a previous queryset state may have
requested otherwise
@carljm
Copy link
Collaborator

carljm commented Nov 4, 2013

There may be a documentation clarification needed here, but I'm not convinced that there is a problem with the behavior. .get() already naturally respects a previous .select_subclasses(), so if you've previously done .select_subclasses() on the queryset, there is no reason to use .get_subclass() at all. .get_subclass() is just a convenience shorthand for .select_subclasses().get().

So I think the right "fix" here is just to document that .get_subclass() always casts the object to its actual leaf class, and does not respect a previous call to .select_subclasses() with a limited set of subclasses. If you want the equivalent of .get_subclass() but with a limited set of subclasses, you should just explicitly use .select_subclasses(...).get(...) instead of .get_subclass().

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants