Handle 'lookup_field' containing relationships for path parameters

[spookylukey]

This PR fixes introspection cases like:

class MyViewSet(GenericViewSet):
    queryset = MyModel.objects.all()
    lookup_field = 'parent__id'

and similar, which previously would just default to string type with a warning.

It also fixes cases where a lookup type like contains or iexact was used. These are valid in DRF as can be seen from the tests - https://github.com/encode/django-rest-framework/blob/9ce541e90990307e06da1b7f5a2576406366a5e5/tests/test_routers.py#L41

Strategy

The strategy I used was to trace how these are handled by DRF, which is ultimately just a call to QuerySet.filter - see https://github.com/encode/django-rest-framework/blob/9ce541e90990307e06da1b7f5a2576406366a5e5/rest_framework/generics.py#L95

Eventually these get parsed by Query.names_to_path. So I have just wrapped up that logic with a helper function. That Query class is an undocumented internal in Django. However, I think this approach is going to be more robust than attempting to duplicate that logic ourselves. Any breakage in this internal API should be easy to spot with good tests (that I have added).

In terms of output, I've found that both lookup_field = 'fkrelation' and lookup_field = 'fkrelation__id' will produce correct introspection of types, but the latter will also give you a good description, due to a difference in how the lookup terminates. It would be nice if we automatically got the good description for both cases, but after looking at the output from names_to_path, I'm not sure it is worth the additional complexity and fragility it might produce.

Tests are added for the new cases handled, and a regression test for the warning in case of a missing field. There are still some cases possibly not handled, especially if you are using QuerySet.annotate and do lookup_field on the added field.

Limitations

• There are bugs (as there were before) for any cases where lookup_url_kwarg != lookup_field, noted in the comment below.
• If you use a Django query lookup that changes the type of the parameter, then you will get incorrect introspection with no warning. So:
  • if you have a recorded_at timestamp field, and use lookup_field = 'recorded_at__year' (which is probably kind of unlikely for a API lookup since it won't be unique), then with this patch it will act as if you specified just recorded_at and assume a date-formatted string, instead of an integer.
  • for many other cases, like iexact, which don't change the type, there will be no problem.

[codecov]

Codecov Report

Merging #509 (88f923f) into master (1407059) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #509      +/-   ##
==========================================
+ Coverage   98.58%   98.60%   +0.02%     
==========================================
  Files          57       57              
  Lines        5710     5801      +91     
==========================================
+ Hits         5629     5720      +91     
  Misses         81       81              

Impacted Files	Coverage Δ
drf_spectacular/openapi.py	96.92% <100.00%> (+<0.01%)	⬆️
drf_spectacular/plumbing.py	97.82% <100.00%> (+0.02%)	⬆️
tests/test_regressions.py	100.00% <100.00%> (ø)
tests/test_warnings.py	100.00% <100.00%> (ø)
tests/test_fields.py	100.00% <0.00%> (ø)
drf_spectacular/generators.py	97.98% <0.00%> (+0.05%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1407059...88f923f. Read the comment docs.

[spookylukey]

I think this PR needs more work - it doesn't correctly handle cases where you have lookup_url_kwargs and lookup_field

[spookylukey]

OK, I've got to the bottom of it.

• DRF creates paths for ViewSets like: /foo/{my_param}/ where my_param is from lookup_url_kwarg, which defaults to being the same as lookup_field, but doesn't have to be.
• drf-spectacular looks at paths like /foo/{bar_id}/baz/{id}/ and tries to deduce types on the assumption that these things are model fields.
  • In the case of lookup_url_kwarg being some custom value, different to lookup_field, there is no reason this should be true
  • Fixing this is not easy, because lookup_field is usually only relevant for the interpretation of the last path parameter, and drf-spectacular currently treats them all the same.

So, I've updated my patch to drop tests where lookup_url_kwarg is different to lookup_field, because these cases don't actually work. This patch still provides a lot of value in terms of being able to traverse relationships, hopefully it can be accepted with these caveats.

I've also added a note above about other limitations.

[tfranzel]

Excellent PR! the introspection rabbit hole goes deeper and deeper 😄

I was a bit worried about using the "internal" names_to_path , but model._meta.get_field isn't exactly a public interface either. so works for me. from my current understanding it should cover a superset of cases without any regressions.

regarding the __date: i can live with that. if somebody is annoyed by it, they can still use @extend_schema to fix it up. in any case, this is an improvement.

regarding lookup_url_kwarg: maybe we can iterate on this afterwards. fine by me for now.

[tfranzel]

this is very neat! sadly we couldn't use this for follow_field_source. the whole thing is a lot easier if you don't leave the SQL domain.

[tfranzel]

would have liked a comment to note that this is actually not 100% correct but expected for the time being.

[tfranzel]

i fixed up the comment and one formatting gripe my OCD complained about. I would ask you to review this additional change. does it make sense for you and does it work for that removed test? i personally do not use this as much and my confidence there is not that strong.

Fixing this is not easy, because lookup_field is usually only relevant for the interpretation of the last path parameter, and drf-spectacular currently treats them all the same.

i would argue that this is not really a problem. The viewset only provides the last one and everything else is "injected" (e.g. drf-nested-routers). if you set the lookup_url_kwarg django will use it no matter what. sure, there may be problems left but i think it's a straight improvement.

+                    if getattr(self.view, 'lookup_url_kwarg', None) == variable:
+                        model_field_name = getattr(self.view, 'lookup_field', variable)
+                    else:
+                        model_field_name = variable
                     model = get_view_model(self.view)
-                    model_field = follow_model_field_lookup(model, variable)
+                    model_field = follow_model_field_lookup(model, model_field_name)

[spookylukey]

Thanks for fixing up the patch and merging it, sorry for the slow reply.

Regarding the last question - I re-added a version of the earlier test, it does work with your code with a small adjustment - see #524