Open pmg-certn opened 9 months ago
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"
Another question is -- if we detect that prepare was not called, should we then call it ourselves rather than bork?
@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.
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.
If a user writes DRF endpoint where they override
get_queryset()
but forget to callself.prepare(qs)
as highlighted in the documentation https://www.django-readers.org/reference/rest-framework/, then all they will see is aQueriesDisabledException
and may assume there is something wrong with theirspec
.This change tracks whether
prepare
has been called for a spec beforeproject
ia called, and if not, then we throw an exception with a specific message for this particular case.