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

Document the use of get_queryset for RelatedField #3605

Merged
merged 5 commits into from Jan 20, 2016
Merged

Document the use of get_queryset for RelatedField #3605

merged 5 commits into from Jan 20, 2016

Conversation

ryanhiebert
Copy link
Contributor

@ryanhiebert ryanhiebert commented Nov 5, 2015

Currently get_queryset is not a part of the documented API of RelatedField. This pull request makes it a documented part of the API, and adds a test using that feature with a subclass of SlugRelatedField.

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 5, 2015

My use-case for this feature is overriding SlugRelatedField to take into account the context so that I can use it to have a writable field with nested routes (using django-nested-routers).

py33: python3.3
py34: python3.4
py35: python3.5

Copy link
Collaborator

@xordoquy xordoquy Nov 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this change is in the PR ?

Copy link
Contributor Author

@ryanhiebert ryanhiebert Nov 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seemed uncontroversial. If it is, I'm happy to remove it.

Copy link
Contributor Author

@ryanhiebert ryanhiebert Nov 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have it in a separate commit, and have some commentary on why I did it in the commit message.

Copy link
Member

@jpadilla jpadilla Nov 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ryanhiebert that does seem valid, but we suggest perhaps submitting that as a separate PR. That way we can discuss and progress both separately.

Copy link
Contributor Author

@ryanhiebert ryanhiebert Nov 5, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. #3606. I've removed that commit from this PR.

The user may wish to provide a dynamic queryset on a `RelatedField`
based on the `context`. The way to do that is to create a subclass of
`RelatedField` (or a child) and override the `get_queryset` method.
However, this is undocumented, and instantiating that field without a
`queryset` argument (because it's not needed) will raise an assertion
error.

Document `.get_queryset(self)` as an official part of the API of
`RelatedField`, and don't enforce the use of `queryset` when
`get_queryset` is overridden.
@xordoquy
Copy link
Collaborator

xordoquy commented Nov 13, 2015

It's not clear to me how this change impacts the discovery of the misconfiguration.
ie, by moving from __init__ to get_queryset it'll postpone to runtime this warning.

@xordoquy xordoquy added this to the 3.3.2 Release milestone Nov 13, 2015
@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 13, 2015

@xordoquy : it delays the assertion so that subclasses can override get_queryset instead of passing in the queryset in the constructor.

This is the actual code of my subclass:

class EndpointAdvisorSlugField(SlugRelatedField):
    def get_queryset(self):
        endpoint_name = self.context['view'].kwargs['endpoint_name']
        return EndpointUser.objects.filter(
            is_advisor=True,
            endpoint__name=endpoint_name,
        )

I didn't need any special behavior from the SlugRelatedField, except to be able to provide a queryset that was aware of the context. But even though my get_queryset doesn't use self.queryset, I have to instantiate my field like this to get around the check on queryset:

advisor_ids = EndpointAdvisorSlugField(
    # The queryset argument is needed for validation to work.
    source='advisors', many=True, slug_field='lms_id', queryset=True)

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 13, 2015

@xordoquy : re-reading your comment, I think I didn't properly address your question.

Yes, moving the assertion into the default get_queryset method will delay the error until runtime. This was the simple solution. How important is a solution that would keep the error at init?

What I might be able to do is check whether the get_queryset method is the one from RelatedField. If not, because it's been overridden, then don't bother checking it at init.

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 13, 2015

Perhaps this would work:

class RelatedField(Field):
    ...
    def __init__(self, *args, **kwargs):
        ...
        default_get_queryset = getattr(RelatedField.get_queryset, '__func__', 
                                       RelatedField.get_queryset)  # Py 3 compat
        if self.get_queryset.__func__ == default_get_queryset:
            assert self.queryset is not None or kwargs.get('read_only', None), (
                'Relational field must provide a `queryset` argument, '
                'or set read_only=`True`.'
            )

Assuming it works the way I intend, it checks to see if the method has been overridden before running the assertion.

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 13, 2015

I went ahead and added that solution. That should make it so that queryset isn't required, but only if get_queryset has been overridden in a subclass. Hopefully that alleviates the concern about the error being raised at runtime instead of initialization.

@tomchristie
Copy link
Member

tomchristie commented Nov 16, 2015

First impressions? Not convinced that I'm in favor of the complexity here. We might want to prefer better documentation and support for custom fields. Alternative to this - why not override .__init__ to set queryset dynamically?

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 16, 2015

@timchristie: Overriding at instantiation is not sufficient because I need to take into account the context, in my case the context['view'].kwargs, which cannot be available at field instantiation.

Overriding the already-documented methods isn't desirable, because there's no way for me to customize the queryset obtained without re-implementing all the logic of those methods. I'm in particular using SlugRelatedField, but I have my eye on using HyperlinkedRelatedField this way as well.

@tomchristie
Copy link
Member

tomchristie commented Nov 16, 2015

My meaning was rather that get_queryset is already available, and that you can instead override init so that the assertion doesn't fire. Having said that if there's an implementation that's visually a bit more obvious that I might be more likely to accept it. Right now it's rather unclear that it's testing if get_queryset has been overridden.

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 16, 2015

get_queryset is available, but it isn't documented, so I have some hesitation using it. Documentation means that it's supported, and that if it breaks it'll be considered a bug.

Overriding the __init__ method is a bit complicated, because there is already not only useful, but required logic in that method. I'd have to re-implement that and track those changes if I were to not call super() on it, and if I did call super() the assertion would fire, though I could get around it by putting a dummy value.

On making it more obvious:

I could see that functionality being useful elsewhere, so one option would be to make it a utility function:

def method_overridden(method_name, klass, instance):
    """
    Check if a method has been overridden.
    """
    method = getattr(klass, method_name)
    default_method = getattr(method, '__func__', method)  # Python 3 compat
    return default_method is not getattr(instance, method_name).__func__

which could be used simply in this case as:

if not method_overridden('get_queryset', RelatedField, self):
    # Check to make sure that self.queryset is given.

Another option that may be able to avoid a utility function could be to add a marker to the default method:

class RelatedField(Field):
    ...
    def __init__(self, *args, **kwargs):
        ...
        if not getattr(self.get_queryset, 'overridden', True):
            # Check to make sure that self.queryset is given

    def get_queryset(self, *args, **kwargs);
       ...
    get_queryset.overridden = False

I'm not sure it's sufficiently more readable than the other option. I'm probably inclined toward making the utility function to try and improve readability. It might be possible to make it a method on RelatedField as well if we don't think we'll use it elsewhere.

@tomchristie
Copy link
Member

tomchristie commented Nov 16, 2015

is self.get_queryset == RelatedField.get_queryset a valid comparison?

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Nov 16, 2015

@tomchristie : Not by itself, because it's wrapped in a bound method object. If you dig into the __func__, then yes, it's valid AFAICT.

"""
method = getattr(klass, method_name)
default_method = getattr(method, '__func__', method) # Python 3 compat
return default_method is not getattr(instance, method_name).__func__
Copy link
Member

@tomchristie tomchristie Nov 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's avoid introducing a module if poss.

Copy link
Contributor Author

@ryanhiebert ryanhiebert Nov 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure thing. It can go in another module, but I'm not sure where it fits. Perhaps just put it in relations.py for now?

if not method_overridden('get_queryset', RelatedField, self):
assert self.queryset is not None or kwargs.get('read_only', None), (
'Relational field must provide a `queryset` argument, '
'or set read_only=`True`.'
Copy link
Member

@tomchristie tomchristie Nov 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this now read "Relational field must provide a queryset argument, override get_queryset, 'or set read_only=True."?

Copy link
Contributor Author

@ryanhiebert ryanhiebert Nov 18, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea.

@ryanhiebert
Copy link
Contributor Author

ryanhiebert commented Dec 11, 2015

Is there anything else that I should do for this PR?

@xordoquy xordoquy modified the milestones: 3.3.2 Release, 3.3.3 Release Dec 14, 2015
@xordoquy
Copy link
Collaborator

xordoquy commented Jan 20, 2016

@ryanhiebert looks good, sorry for the delay.

xordoquy added a commit that referenced this pull request Jan 20, 2016
@xordoquy xordoquy merged commit f1b28b4 into encode:master Jan 20, 2016
3 checks passed
@@ -176,6 +176,14 @@ def test_representation(self):
representation = self.field.to_representation(self.instance)
assert representation == self.instance.name

def test_no_queryset_init(self):
class NoQuerySetSlugRelatedField(serializers.SlugRelatedField):
def get_queryset(this):
Copy link
Contributor

@ticosax ticosax Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't be self instead of this ?

Copy link
Collaborator

@xordoquy xordoquy Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ticosax it indeed should be.

Copy link
Collaborator

@xordoquy xordoquy Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe not, I need to dig this a bit.

Copy link
Contributor Author

@ryanhiebert ryanhiebert Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, because I needed to get to self from the test case inside this method.

Copy link
Contributor Author

@ryanhiebert ryanhiebert Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It perhaps would have been better to use staticmethod, or at least add a comment about why it was like that.

Copy link
Collaborator

@xordoquy xordoquy Jan 21, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Refactored slightly in #3861

@xordoquy xordoquy changed the title RelatedField get_queryset and context Document the use of get_queryset for RelatedField Feb 11, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants