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

Modify urls for nested routers #251

Merged
merged 1 commit into from
Apr 30, 2021
Merged

Conversation

merll
Copy link
Contributor

@merll merll commented Jan 6, 2021

drf-spectacular uses the coerce_path method of the Django Rest Framework, replacing pk path parameters with id (or the appropriate primary key). When using the drf-nested-routers library for extending URL actions to related objects, auto-generated URL paths are created with {object_pk} patterns.

Since these are not valid fields on the queryet model, they require a manual definition of URL path parameters using @extend_schema. This PR replaces such _pk suffixes with _id (or the appropriate primary key), so that further introspection is also able to guess the parameter type.

The included test is based on a manually-created URL path, since it seemed unnecessary to add nested-routers as a dependency just for this purpose.

@codecov
Copy link

codecov bot commented Jan 6, 2021

Codecov Report

Merging #251 (56d9b93) into master (882ade7) will increase coverage by 0.02%.
The diff coverage is 97.43%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #251      +/-   ##
==========================================
+ Coverage   98.32%   98.34%   +0.02%     
==========================================
  Files          53       53              
  Lines        4233     4539     +306     
==========================================
+ Hits         4162     4464     +302     
- Misses         71       75       +4     
Impacted Files Coverage Δ
tests/test_extend_schema_view.py 98.57% <92.85%> (-1.43%) ⬇️
drf_spectacular/generators.py 97.28% <100.00%> (+0.73%) ⬆️
tests/test_regressions.py 99.86% <0.00%> (-0.14%) ⬇️
drf_spectacular/types.py 100.00% <0.00%> (ø)
tests/test_polymorphic.py 100.00% <0.00%> (ø)
drf_spectacular/serializers.py 100.00% <0.00%> (ø)
tests/contrib/test_django_filters.py 100.00% <0.00%> (ø)
drf_spectacular/openapi.py 96.29% <0.00%> (+0.40%) ⬆️
drf_spectacular/utils.py 99.53% <0.00%> (+0.82%) ⬆️
... and 2 more

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 882ade7...56d9b93. Read the comment docs.

@kingy04
Copy link

kingy04 commented Apr 22, 2021

@tfranzel Can we look at having this merged?, i am also hitting this issue. Many thanks

@tfranzel
Copy link
Owner

i'm sorry that this fell off the waggon. i had an issue with some aspects and wasn't able to follow up on it. will give it another go

@tfranzel
Copy link
Owner

sry this took so long, but as you probably figured yourself this is more complicated than it looks. i had to rebuild the change myself to get a decent understanding on whats going on here.

  • this only works for one extra variable not multiple, which looks common if you use that lib. readme example
  • rel_model turns out to not be the related model but the viewset model, thus ref_pk_name is also not correct.
  • the distinction that is made in the orig. coerce_path with getting the pk field_name does not apply to relations, thus usage of get_pk_name there makes no sense.
  • on that note, the _id suffix for relations is baked into django and can only be changed by changing the column name in the field. your proposal would do this: relation_xyz with xyz as the PK for the related model. afaik that is not what django is doing internally. you could say the PR is doing the right thing by accident due to the above mentioned bug.
  • i prefer dedicated test for these things - not complicating the already complicated tests - unless it is covering fundamental functionality.

i prepared a separate PR #369 on top of this PR. @merll @kingy04 please review if those changes work for you. it is basically a simplification of this PR. waiting for feedback from you guys as i'm not a drf-nested-router user

@kingy04
Copy link

kingy04 commented Apr 26, 2021

Thanks for spending some time on this @tfranzel, i have tested #369 and it is working well for my use case, the correct field types are showing in the schema and also any django filters are correctly showing now.

tfranzel added a commit that referenced this pull request Apr 30, 2021
@tfranzel tfranzel merged commit 93f1c63 into tfranzel:master Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants