Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect path parameter type for nested routes from rest_framework_nested #453

Closed
tiholic opened this issue Jul 7, 2021 · 14 comments
Closed
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@tiholic
Copy link

tiholic commented Jul 7, 2021

To create nested Viewsets, I'm using https://github.com/alanjds/drf-nested-routers

Route registration happens like this:

router = DefaultRouter()
connections_router = NestedSimpleRouter(router, r'connections', lookup='connection')
clients_router.register(r'members', MembersViewSet, basename='connections-members')

and methods in viewset are expected an additional keyword argument with <lookup>_pk:

class MembersViewSet:

  def list(request, connection_id):
    # ...

This will log a warning:

Warning #0: MembersViewSet: could not derive type of path parameter "connection_pk" because model "<class 'accounts.models.Member'>" did contain no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".
@tiholic
Copy link
Author

tiholic commented Jul 7, 2021

Solution I use:

Adding lookup_field = 'id' on Parent viewset to auto detect the type (from ParentViewset > Model > id field)

Hope this helps someone facing similar issue

@tfranzel
Copy link
Owner

tfranzel commented Jul 7, 2021

this might be related to #251.

are you using the most recent version of spectacular? i was under the impression that we fixed the pk/id issue. at least that is what that PR was about.

@tiholic
Copy link
Author

tiholic commented Jul 26, 2021

@tfranzel tried with 0.17.2

I still see warnings similar to this if I remove lookup_field = 'id' from parent ViewSet

Warning #0: XXXViewSet: could not derive type of path parameter "client_pk" because model "<class 'accounts.models.client.XXX'>" did contain no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".

@axieum
Copy link

axieum commented Aug 4, 2021

The following also does not work as it results in a parameter of {object_custom_field} which doesn't exist on the model as it's a property on the related model.

class ObjectViewSet(ModelViewSet):
    lookup_field = "custom_field"

class SubObjectViewSet(ModelViewSet):
    pass

I'm not sure if there are any other implications, but I removed the type warning via -

@extend_schema(parameters=[OpenApiParameter("object_custom_field", str, OpenApiParameter.PATH)])
class SubObjectViewSet(ModelViewSet):
    pass

@tfranzel
Copy link
Owner

hey @tiholic @axieum , finally got around working on this issue. could you try out #511? it contains several small fixes that revolve around this problem. also added tests for drf-nested-routers. feedback is very welcome.

@tiholic
Copy link
Author

tiholic commented Sep 16, 2021

Thanks for the update @tfranzel! Unfortunately I see the same behaviour as in this comment: #453 (comment)

>> import drf_spectacular
>> drf_spectacular.__version__
'0.18.2'

>> import rest_framework_nested
>> rest_framework_nested.__version__
'0.93.3'

>> import django
>> django.__version__
'3.2.7'

>> import rest_framework
>> rest_framework.__version__
'3.12.4'
[16/Sep/2021 06:41:07] "GET /api-docs/swagger-ui/ HTTP/1.1" 200 1213
Warning #0: Child1ViewSet: could not derive type of path parameter "domain_pk" because model "<class 'app.models.Child1Model'>" did contain no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".
Warning #1: Child2ViewSet: could not derive type of path parameter "domain_pk" because model "<class 'app.models.Child2Model'>" did contain no such field. Consider annotating parameter with @extend_schema. Defaulting to "string".

@tfranzel
Copy link
Owner

@tiholic this has not been released yet. have you installed the current master branch of drf-spectacular? the warning changed since since 0.18.2 release. if domain is available from Child1Model it should have worked on master

@tfranzel
Copy link
Owner

tfranzel commented Sep 21, 2021

this has been severely improved in ba2f799 (and prior commits). path variable detection should now work as expected without using lookup_field = 'id' or other patches.

related but independent from that, a new setting SCHEMA_COERCE_PATH_PK_SUFFIX was added to globally coerce _pk names for convenience.

@tfranzel tfranzel added enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending labels Sep 21, 2021
@tiholic
Copy link
Author

tiholic commented Sep 23, 2021

Looks like the fix was released in 0.19.0. Working just fine! 🎉

@tiholic tiholic closed this as completed Sep 23, 2021
@mounirmesselmeni
Copy link

mounirmesselmeni commented Mar 9, 2022

I hope it's fine to comment on this issue. But for me I'm getting params parsed but as an int.
Using nested rest_framework_nested==0.93.4 with drf-spectacular==0.21.1
Parent viewset has a custom lookup field (Not an int ID), the parameter name is extracted correctly but the type is wrong and this is only for the nested routes.

/v1/level-one/{custom_id}/level-two:
    get:
      operationId: levelone_leveltwo_list
      parameters:
      - in: path
        name: custom_id
        schema:
          type: integer
        required: true

I though this is a issue for drf-nested-routers but digging inside a bit I find out that it's the same regex generated for the non-nested routes, which is giving the correct type string.

The lookup regex is the following: (?P<custom_id>[^/.]+)

@tfranzel
Copy link
Owner

tfranzel commented Mar 9, 2022

(Not an int ID)

but what is it then? are there warning emitted to the console? spectacular should default to str if nothing can be resolved. so i'm assuming some field (of type integer) got found there. we would need a reproduction to further investigate.

The lookup regex is the following: (?P<custom_id>[^/.]+)

that is the default regex for all of DRF's path parameters. if spectacular sees that, it tries to be smarter and find a field type for that name custom_id.

for further discussion please open a new issue.

@lucadelu
Copy link

SCHEMA_COERCE_PATH_PK_SUFFIX

I'm using drf-spectacular==0.21.2 and rest_framework_nested==0.93.4 but I'm getting

could not derive type of path parameter "plant_pk" because it is untyped and obtaining queryset from the viewset failed. Consider adding a type to the path (e.g. <int:plant_pk>) or annotating the parameter type with @extend_schema. Defaulting to "string".

if I use SCHEMA_COERCE_PATH_PK_SUFFIX the message change from plant_pk to plant_id

I tried also to use extend_schema but it duplicate the parameters.

@tfranzel
Copy link
Owner

Hi @lucadelu,

this is fully tested here:

def test_drf_nested_routers_basic_example(no_warnings, coerce_suffix, transforms):

The error says it was unable to get the model/querset, and i think it is unrelated to rest_framework_nested. Getting the model out of the view fails and so it cannot lookup the model type. I see 2 options:

  1. either make the get_queryset safe to call (maybe with the help of if getattr(self, 'swagger_fake_view', False): return YourModel.obects.none())
  2. or provide a queryset to view with YourModel.objects.none()

I tried also to use extend_schema but it duplicate the parameters.

You can explicitly exclude detected parameters with

@extend_schema(parameters=[...,OpenApiParameter('plant_', exclude=True))

for further discussion please open a new issue.

@lucadelu
Copy link

Hi @lucadelu,

this is fully tested here:

def test_drf_nested_routers_basic_example(no_warnings, coerce_suffix, transforms):

The error says it was unable to get the model/querset, and i think it is unrelated to rest_framework_nested. Getting the model out of the view fails and so it cannot lookup the model type. I see 2 options:

1. either make the `get_queryset` safe to call (maybe with the help of `if getattr(self, 'swagger_fake_view', False): return YourModel.obects.none()`)

2. or provide a queryset to view with `YourModel.objects.none()`

I provided queryset and it works, thanks

I tried also to use extend_schema but it duplicate the parameters.

You can explicitly exclude detected parameters with

@extend_schema(parameters=[...,OpenApiParameter('plant_', exclude=True))

for further discussion please open a new issue.

ok thanks a lot

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

5 participants