fix: Django REST Framework integration

[j0nm1]

Hi,

this PR fixes the integration with Django REST Framework (especially the filtering and the exception handling). So it should probably fix #50. I used #6 as a starting point to implement the functionality but needed to extend it a little bit.

The filtering is fixed by returning a valid model (abstract or not doesn't really matter). DRF then allows filtering against all defined properties.

The exception handling is a little bit more complicated as Django (and DRF) decided to only handle 404 errors for the model.DoesNotExist exception (https://github.com/django/django/blob/f9ec777a826816e20e68021c0e73b5a76be650af/django/shortcuts.py#L86). So if there is a normal model, we can raise the provided DoesNotExist exception. But for abstract models, there is no DoesNotExist exception. In this case it seems to be the easiest way to monkey patch the model and provide the normal ObjectDoesNotExist.

[clokep]

Looks like #50 has some info about when this might have broken (in #38).

Looks like #6 didn't add tests that explicitly tested against DjangoFilterBackend. 😢

I'm not a huge fan of supporting the model field though since it could vary between the different QuerySets. (I believe this is why I removed it in #38). If you're using the same model everywhere I think most things can now be done without QuerySetSequence?

[j0nm1]

Yes, I understand why QuerySetSequence is not supporting the model property at the moment. The problem is, that without it, I don't really see a way to make the DRF 404 and especially the filtering work correctly.

We currently use the library where we have multiple different models that share a property like name that needs to be filtered. The model provided to QuerySetSequence is an abstract one with just the properties that DRF needs to filter against. I hope that makes sense.

[clokep]

We currently use the library where we have multiple different models that share a property like name that needs to be filtered. The model provided to QuerySetSequence is an abstract one with just the properties that DRF needs to filter against. I hope that makes sense.

This makes sense and should work most of the time. Any idea how badly this would break if a user put an unrelated model in? I assume somewhere Django or DRF would start throwing unknown field errors, but maybe that's clear enough it would be fine.

[j0nm1]

Hard to say, as long as the provided (abstract) model only contains fields that are defined in the models as well, everything is fine. I will try this with some incorrect models and report back what DRF does.

[clokep]

Thanks! I skimmed the changes and overall they look OK. Will try to take a closer look soon!

Thanks for including tests! 👍

[j0nm1]

You're welcome.

I've locally modified the test to check what happens. If you add an invalid property to the search_fields like invalid_field, like so:

    class AbstractModelTestListView(generics.ListAPIView):
        queryset = QuerySetSequence(Author.objects.all(), model=AbtractModel)
        serializer_class = TestSerializer
        permission_classes = []
        filter_backends = [filters.SearchFilter]
        search_fields = ["^name", "invalid_field"]

Django raises:

django.core.exceptions.FieldError: Cannot resolve keyword 'invalid_field' into field. Choices are: article, blogpost, book, id, name

Which looks good in my opinion. Furthermore, if you modify the AbtractModel to include a property that the actual models do not have, like:

class AbtractModel(models.Model):
    name = models.CharField(max_length=50)
    invalid_field = models.CharField(max_length=50)

    class Meta:
        abstract = True

It still raises

django.core.exceptions.FieldError: Cannot resolve keyword 'invalid_field' into field. Choices are: article, blogpost, book, id, name

So I think this is fine. Should I include some test cases for this behavior as well?

[clokep]

So I think this is fine. Should I include some test cases for this behavior as well?

I agree, just wanted to make sure it would point reasonably to what might be wrong!

I don't think we need tests for that behavior since it is internal to Django.

Thank you for checking!

[clokep]

Thanks for working through this and adding the tests! 🎉

[clokep]

@j0nm1 I've released version 0.16 with this fix (and a couple of maintenance tasks). Sorry for the delay! Thanks again!