yezyilomo / django-restql

Turn your API made with Django REST Framework(DRF) into a GraphQL like API.
https://yezyilomo.github.io/django-restql
MIT License
620 stars 43 forks source link

Extra context from EagerLoadingMixin #156

Closed jheld closed 4 years ago

jheld commented 4 years ago

Although we can access the view & request objects in the serializer via standard DRF practices, it would be great if the mixin would also add, for example: "eager_loading": self.should_auto_apply_eager_loading to the serializer context.

Some projects have view or other dynamic settings for this and having the mixin make an easier way to check would be helpful when trying to check the value from the serializer (less code but still correct).

Additionally sometimes projects also call into serializers without coming from a view, and thus a convention of using a specific key for the setting, would make it easier for all use cases. They can rely on it (they'll just set their context manually).

yezyilomo commented 4 years ago

The point am seeing here is to make both DynamicFieldsMixin & EagerLoadingMixin independent of a view, i.e they could be applied directly to a serializer as long as a user provides a query and queryset.

jheld commented 4 years ago

I suppose that's true, but that's not quite the ask here.

My preference would be to give the eager loading mixin a new power (e.g. context), separate from any other enhancements to it. My use case relates specifically to the view at this time (and DRF serializers although they may take an object/queryset, I believe we'd have to add the eager loading function application in there somewhere to ensure it wouldn't require the view to run it first).

Am I misunderstanding?

yezyilomo commented 4 years ago

What do you mean by

to ensure it wouldn't require the view to run it first ?

Because AFAIK the view has to run first i.e generate a queryset(which is where EagerLoadingMixin is applied) then pass it to a serializer for it to serialize and return what's required(this is where DynamicFieldsMixin is applied).

jheld commented 4 years ago

Correct.

But I'm sure not all projects 100% let eager loading take over on get_queryset.

However, that wasn't quite the ask.

I'm looking for the serializer context injection specifically for projects that are looking to move their views & serializers over to RestQL and eager loading but wish to test it (e.g. regular and eager loading branching) before removing their old code paths. Having the context injection will make the logic easier so they don't have to even bother checking the context's views's setting of eager loading, just the context directly.

yezyilomo commented 4 years ago

Yeah if you don't want to let eager loading take over get_queryset you can disable it with auto_apply_eager_loading=False property and apply it after checking your conditions with eager_queryset = apply_eager_loading(queryset).

So what you are looking for is to avoid this on your serializer

view = self.context.get('view', None)
method_name = "should_auto_apply_eager_loading"
if view is not None and hasattr(view, method_name):
    should_auto_apply = getattr(view, method_name)
    # Do what you want to do here

Instead you want to be able to do it like this

should_auto_apply = self.context.get('should_auto_apply_eager_loading', False)
# Do what you want to do here

Is this what you mean?..

jheld commented 4 years ago

Yes, quite similar! That's the idea.

We have done versions of all of that.

We have started to use "eager_loading" as the key (brevity but still clear). But yes, that's exactly it.

yezyilomo commented 4 years ago

Okay but this is like breaking down view into small components and send them to a serializer through context, the problem is we don't know what all users want or will want, you might want to use should_auto_apply_eager_loading but someone might want to use get_eager_queryset or get_dict_parsed_restql_query etc, they would also want to access it through context, so in time we might find ourselves filling the context will things which could be accessed through view which is not good. Am sure this is the reason why DRF is not passing queryset through context despite the fact that it's used a lot in serializers, cuz it would be absurd to pass both view and qeryset knowing we can access queryset from view. My solution to your case would be to create a function on a serializer which does just that and reuse it to all serializers which require that value.

yezyilomo commented 4 years ago

If you really want it to be the way you said another good way would be to inherit EagerLoadingMixin and override get_serializer_context. eg

class MyEagerLoadingMixin(EagerLoadingMixin):
    def get_serializer_context(self):
        context = super().get_serializer_context()

        # Add all values which you want to be available in yourcontext
        context.update(
            {"should_auto_apply_eager_loading": self.should_auto_apply_eager_loading}
        )
        return context

This will do the trick and you could add any thing which you want to be available in your context.

jheld commented 4 years ago

Closing because it's not a major issue. No plans to add support.