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

Router ordering of paths is not respected #92

Closed
jayvdb opened this issue Jun 11, 2020 · 10 comments
Closed

Router ordering of paths is not respected #92

jayvdb opened this issue Jun 11, 2020 · 10 comments
Labels
bug Something isn't working

Comments

@jayvdb
Copy link
Contributor

jayvdb commented Jun 11, 2020

This appears in the now familiar case of oscarapi and oscarapicheckout.

The order in urls.py must be (c.f. README of oscarapicheckout):

    url(r'^api/', include(apps.get_app_config("oscarapicheckout").urls[0])), # Must be before oscar_api.urls
    url(r'^api/', include(oscar_api.urls)),

However drf-spectacular processes the second one, including the identical path, and internally replaces the first view/serializer/etc with the unused second view/serializer/etc.

If I invert the order, the desired oscarapicheckout endpoint appears in the schema, but then it is not the one used by the url router.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 11, 2020

A hacky workaround would be a 'fix' extension which could force drf-spectacular to ignore the second view it encounters.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 11, 2020

Complicating things slightly is an extra /oscarapi/checkout{format} which is the oscar-api view, which drags in the extra unwanted 'identically named' CheckoutSerializer even if the /oscarapi/checkout route is fixed, but that is a different problem.
(Further complicating this example is that I have a WIP for drf-spectacular needed to get oscarapicheckout operational, which I havent submitted as a PR because all similar PRs have been rejected and closed)

Even if I rename one of the serializers, this problem still exists. The schema endpoints are not the same as the real endpoints.

I have worked-around this by filtering include(oscar_api.urls)), so the undesirable/unused endpoints do not end up in the URLConf, so drf-spectacular doesnt get confused.

@tfranzel
Copy link
Owner

very interesting. spectacular should process the routes as they come in. the endpoint sorting might be influencing it though.

i believe this is an dict override (in spectacular) versus. "take the first matching route" in Django.

i'll look into it.

@tfranzel tfranzel added the bug Something isn't working label Jun 11, 2020
@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 11, 2020

Not a huge priority for me, as I need my hacking of the urlconf to also get rid of /oscarapi/admin. I'm not sure how frequent this over-riding of endpoints happens in Django projects - Oscar is the first time I have seen it done so commonly by plugins.

@tfranzel
Copy link
Owner

@jayvdb please review the change and close. urlpattern overrides should now be properly handled in the schema.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jun 30, 2020

Yes this is working, but I cant use it, because I need to also disable the oscar-api /oscarapi/checkout{format} which isnt hidden by the oscar-api-checkout /oscar/checkout/. That could actually be another sanity "check" which drf-spectacular performs, along with consistency of trailing /. Anyway, another day for such crazy ideas.

Sorry I didnt reply earlier. It is a bit hard for me to easily verify anything as I have diverged so far. Project almost over. Will get to tidy/catch up later this week.

@tfranzel
Copy link
Owner

np. you probably also have to wrap your oscar-api-checkout overrides with https://www.django-rest-framework.org/api-guide/format-suffixes/#format_suffix_patterns in the urlpatterns. that way the override should be "complete".

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 1, 2020

Ya, or more likely forcibly filter oscar-api urls to remove the suffix {format}, as it needlessly duplicates stuff in the api until #110 is fixed.

@jayvdb
Copy link
Contributor Author

jayvdb commented Jul 11, 2020

@tfranzel , I just noticed that this code was duplicated from DRF core, which means the bug probably exists there. The code there hasnt changed. There isnt any open issue related to "get_api_endpoints". encode/django-rest-framework#6527 looks really relevant - but appears closed without a fix.

@Lucidiot , do you know whether your problem was eventually solved in DRF core?

@Lucidiot
Copy link

My issue occurred while support for OpenAPI was barely starting in DRF 3.9; at that time, the default --format was still coreapi in manage.py generateschema. The gist of the issue was that if you had more than one namespace (if you had at least one include() and another include or a path in your urlpatterns), all namespaces would be ignored and the OpenAPI schema would have 0 endpoints.

With Django 2.2 and DRF 3.9 I managed to recreate my issue:

ns1 = [
    path('foo', FooView.as_view(), name='foo')
]

ns2 = [
    path('bar', BarView.as_view(), name='bar')
]

urlpatterns = [
    path('ns1/', include((ns1, 'ns1'), namespace='ns1'))
    path('ns2/', include((ns2, 'ns2'), namespace='ns2'))
]

With CoreAPI, I would get the two /ns1/foo and /ns2/bar as expected, but not with OpenAPI. This bug was indeed fixed with #6532, when actual support for OpenAPI was added. With DRF >=3.10, I do have two endpoints in all schema formats.

But I just tried out something else instead: If I still have my two views with different patterns and different namespaces and different everything, but I use the same serializer in both views, then both views get the same operationId: RetrieveFoo, which would make one endpoint override the other anyway with APIStar or other libraries that rely on the operationId. Is this what is meant by the /oscarapi/checkout{format} overriding the other?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants