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

Types embedded in url paths #34

Closed
jayvdb opened this issue Apr 24, 2020 · 6 comments
Closed

Types embedded in url paths #34

jayvdb opened this issue Apr 24, 2020 · 6 comments
Labels
enhancement New feature or request low priority issues that would be nice to fix but have low impact

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Apr 24, 2020

https://github.com/django-oscar/django-oscar-api/blob/e57b34b/oscarapi/urls.py says basket_pk has type int

However I get warnings

WARNING #130: could not derive type of path parameter "basket_pk" because model "<class 'oscar.apps.basket.models.Line'>" did contain no such field. consider annotating parameter with @extend_schema. defaulting to "string".
...
WARNING #144: could not derive type of path parameter "basket_pk" because model "<class 'oscar.apps.basket.models.LineAttribute'>" did contain no such field. consider annotating parameter with @extend_schema. defaulting to "string".

OscarHyperlinkedModelSerializer from https://github.com/django-oscar/django-oscar-api/blob/master/oscarapi/serializers/utils.py uses rest_framework.serializers.HyperlinkedModelSerializer, but the problem seems to be a customisation
extra_url_kwargs

@tfranzel
Copy link
Owner

url path and query parameters are a tricky business. spectacular already tries reasonable hard to find those, but everything that is manually handled in method bodies is more or less impossible to extract. there are only heuristics for possibly finding the right thing in the model.

there might be cases that can be improved but given the other issues i will assign this low prio and pick it up once the other issues are resolved

@tfranzel tfranzel added the low priority issues that would be nice to fix but have low impact label Apr 24, 2020
@tfranzel
Copy link
Owner

@jayvdb at least the format suffix is DRF native, so i added something reasonable for that (although not 100% perfect): 60625c4

also, i was curious yesterday and set up the example oscar-api app. i got 22 unique warnings after that. not too bad i suppose. however, some of those warnings are not resolvable without an additional override mechanism.

@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 28, 2020

<int:x> is DRF native also.

Much appreciated for the format suffix fix - that makes a bit difference.

@tfranzel
Copy link
Owner

with native i meant a more or less static DRF feature virtually always to be a string.

you make a good point with <int:x>. that could be leveraged, but the DRF path traversal strips this i think. worth looking into though.

@tfranzel tfranzel added the enhancement New feature or request label Apr 28, 2020
@tfranzel
Copy link
Owner

@jayvdb implemented said feature. turned out quite nice imho. this is pretty much the limit of what can extracted from the path.

@jayvdb
Copy link
Contributor Author

jayvdb commented Apr 28, 2020

Awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request low priority issues that would be nice to fix but have low impact
Projects
None yet
Development

No branches or pull requests

2 participants