Skip to content
This repository has been archived by the owner on Oct 23, 2023. It is now read-only.

Resolver returns a bad url #940

Closed
koleror opened this issue Jan 16, 2017 · 21 comments
Closed

Resolver returns a bad url #940

koleror opened this issue Jan 16, 2017 · 21 comments

Comments

@koleror
Copy link

koleror commented Jan 16, 2017

I'm using Django 1.7 (I know) and I just upgraded from raven 5.24.3 to 5.32.0, which causes me some new issues with the new UrlResolver. In some case, the culprit get translated from path like v1/cart/mine/items/ to /{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{api_name}/{resource_name}/{pk}/items which then goes to the transaction tag in sentry.

I investigated a bit and found out the culprit is raven.contrib.django.resolver.RouteResolver._resolve which calls itself recursively and increase the prefix size for whatever reason...
Do you have any clue how I can fix this please?

@dcramer
Copy link
Member

dcramer commented Jan 16, 2017

@koleror could you provide some details on how the route is formed? that would help us write a test case to ensure we can resolve the behavior

@koleror
Copy link
Author

koleror commented Jan 17, 2017

I'm not sure what you need exactly?

@EdgyEdgemond
Copy link

EdgyEdgemond commented Jan 17, 2017

I too have this issue.

I believe it is to do with the base, maybe also child, urls.py file(s) having multiple entries...

14 url(r'^api/(?Pv[\d.]+)/', include('apps.api.urls.v1.one')),
15 url(r'^api/(?Pv[\d.]+)/', include('apps.api.urls.v1.two')),
16 url(r'^api/(?Pv[\d.]+)/', include('apps.api.urls.v1.three')),
17 url(r'^api/(?Pv[\d.]+)/', include('apps.api.urls.v1.four')),

This means if it matches v1.one you get 'api/{version}/one'

If it matches v1.two you get 'api/{version}/api/{version}/two'

etc etc.

@koleror
Copy link
Author

koleror commented Jan 17, 2017

Hmm could be, as I'm using multiple includes as well
Nice to know I'm not the only one out there :D

@koleror
Copy link
Author

koleror commented Jan 17, 2017

In case it may help, the buggy routes are generated through the Django Tastypie plugin

Thanks for your help

@dvndrsn
Copy link

dvndrsn commented Feb 13, 2017

We're also facing a similar issue on Tastypie 0.12.1 with Django 1.7.9. I did some analysis on this (below).

I believe that rolling back to raven==5.26.0 will resolve the problem as a short term fix, but a long term fix will be needed in raven/contrib/django/resolvers.py. I wasn't able to find any documentation on how to running the raven-python Django tests to contribute myself, but looking at tastypie docs on API shows how to setup a API route which can be used for building a failing test.

When inspecting the output of a resolver generated in a Django project using tasty pie…

try:
    from django.urls import get_resolver
except ImportError:
    from django.core.urlresolvers import get_resolver

resolver = get_resolver(None)
resolver.reverse_dict

…we see that in patterns generated by tastypie, URL Parameters are used to capture api_name (such v1) as and resource_name (such as locations):

u'v1/(?P<api_name>api)/(?P<resource_name>locations)/(?P<pk>.*?)/$’

There was a PR merged in Sept 2016 to the raven-python repo to Replace culprits with transaction names #768. If we check the release tags, this was merged in 5.27.0.

$ cd raven-python
$ git tag --contains 3e8d8f82
5.27.0
5.27.1
5.28.0
5.29.0
5.30.0
5.31.0
5.32.0

In these changes, a new Django resolver was introduced which replaces captured parameter values with the parameter name. With tastypie generated patterns instead of using /v1/locations/123/, we will see /{api_name}/{resource_name}/{pk}/ after calls to RouteResolver._simplify replaces parameter values with their names.

Additionally, there is another problem where recursive calls to the raven-python RouteResolver._resolve function will cause /{api_name}/ to be added to the resolved URL multiple times. This is likely happening when generating the prefix due to parents being recursively added multiple times.

@koleror
Copy link
Author

koleror commented Feb 13, 2017

Hmm so it looks like its an issue with either django 1.7 or tastypie

@dcramer
Copy link
Member

dcramer commented Feb 15, 2017

I'm working on reproducing this right now, though at first glance it doesnt seem to be happening. Is it a specific style of API url pattern that you're applying (thats not default) ?

@teeberg
Copy link
Contributor

teeberg commented Feb 17, 2017

Here's a minimal app that reproduces the bug: https://github.com/teeberg/raven-bug

Steps to reproduce in the README.md

@dcramer
Copy link
Member

dcramer commented Feb 17, 2017

@teeberg thanks! I'll try to get this into the main repo and resolve the issue over the weekend. Once done we'll ship an updated version of the SDK.

@koleror
Copy link
Author

koleror commented Feb 17, 2017

Ohh very nice thank you guys!
Can't wait to see the fix :D

@dcramer
Copy link
Member

dcramer commented Feb 17, 2017

@teeberg im a bit confused by the example repo -- tastypie doesnt support 1.10+ right? (Im still struglging to reproduce this within our test suite)

@dcramer
Copy link
Member

dcramer commented Feb 17, 2017

nevermind -- the issue requires multiple resolvers and then it reproduces immediately

@koleror
Copy link
Author

koleror commented Feb 17, 2017

Dont know but i have the issue with django 1.7.11.
I'll try this code with the versions i'm using this weekend.

@dcramer
Copy link
Member

dcramer commented Feb 17, 2017

tl;dr recursion requires thought -- we were capturing the parent in a unified list vs generating a new list of parents for each recursive call

@koleror
Copy link
Author

koleror commented Feb 17, 2017

Yay thanks! I'll test the fix asap

@teeberg
Copy link
Contributor

teeberg commented Feb 17, 2017

Thanks for reporting @koleror! :-) We've seen this for a while but I didn't know if it was raven's fault and never bothered to verify and report. Also can't wait to update!

@koleror
Copy link
Author

koleror commented Feb 17, 2017

it works! Thanks! When will the fix be available on pip?

@koleror
Copy link
Author

koleror commented Feb 17, 2017

Hmm it looks like it's in the version 6.0.0? Am I correct?

@dcramer
Copy link
Member

dcramer commented Feb 17, 2017

@koleror yep

@koleror
Copy link
Author

koleror commented Feb 17, 2017

👍

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants