Check prepare had been called when project is called

[pmg-certn]

If a user writes DRF endpoint where they override get_queryset() but forget to call self.prepare(qs) as highlighted in the documentation https://www.django-readers.org/reference/rest-framework/, then all they will see is a QueriesDisabledException and may assume there is something wrong with their spec.

class MyListView(SpecMixin, ListAPIView):
    spec = [
        # ...
    ]

    def get_queryset(self):
        queryset = # Whatever
        # oops we forgot to `return self.prepare(queryset)`
        return queryset

This change tracks whether prepare has been called for a spec before project ia called, and if not, then we throw an exception with a specific message for this particular case.

[pmg-certn]

Question -- I wonder if this enforcement should move deeper, into specs.process() for instance https://github.com/pmg-certn/django-readers/blob/877ad5d9feae1263c518b92a030a0f9a1b0843ad/django_readers/specs.py#L19.

That function already enforces queries disabled. Perhaps it should also enforce "project only after prepare"

[pmg-certn]

Another question is -- if we detect that prepare was not called, should we then call it ourselves rather than bork?

[george-waller-certn]

@pmg-certn I would say this should be added at a lower level as there is no reason as to why you would want to call project without preparing the queryset so a helpful error would help the developer experience.

I would also prefer the error rather than it magically doing it for you so you are more in control with what the code is doing, this is probably more of a point of preference rather than one being objectively better than the other though.

[pmg-certn]

Of course I'm not checking that the queryset they called prepare on is actually the same one they're calling project on...

[j4mie]

This looks like a good start, and would certainly solve the specific issue which has come up a few times.

I have one concrete concern, particulary with the suggestion that this should be done at a lower level than the view mixin, which is that I often test project functions (really produce) in isolation, even sometimes by passing a mock object rather than a real instance. I don't think this is necessarily a problem at the view level, because it's more likely that views will be integration tested than unit tested, but I could definitely see it being a problem at lower levels.

More generally, I can also imagine advanced use cases where the spec-generated prepare function is replaced with another (perhaps to select_related a queryset rather than prefetching), so binding the two together feels like it could restrict what can be done with the library. The implementation as-is doesn't provide an escape hatch from this behaviour, because the is_prepared state is inside a closure, so it's not even something a user could override.

I'm more comfortable with this checking being performed at a higher level (in the DRF view mixin), but in that case is there any benefit to the approach you've taken vs keeping the is_prepared state on the view itself? By doing it that way, it's also easier to override: a user could manually set the _is_prepared instance variable in a test (say) if they really needed to, or we could even provide a bit of public API to override the behaviour entirely, like an SHOULD_ENSURE_PREPARE_IS_CALLED property on the view, that defaults to True.