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

Fix list_route, detail_route with kwargs contains curly bracket in url_path #5187

Merged
merged 3 commits into from May 29, 2017

Conversation

no-dap
Copy link
Contributor

@no-dap no-dap commented May 28, 2017

refs : #5172

before: #5177

when list_route and detail_route's url_path contains curly brackets, it raise IndexError
as we can see at #5172.

added test code ensure that when I escape curly bracket it works fine.

I think list_route and detail_route need url_path with regex lookup for making restful url
unless we can use nested route on list_route and detail_route.

@no-dap no-dap changed the title List route regex Fix list_route, detail_route with kwargs contains curly bracket in url_path May 28, 2017
def replace_curly_brackets(url_path):
if ('{' and '}') in url_path:
url_path = url_path.replace('{', '{{').replace('}', '}}')
return url_path
Copy link
Member

@tomchristie tomchristie May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Won't this also end up breaking the regex?

Copy link
Contributor Author

@no-dap no-dap May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Error raised at here in routers.py, 258L

regex = route.url.format( 
    prefix=prefix, 
    lookup=lookup, 
    trailing_slash=self.trailing_slash 
) 

Actually, this escaping is not for regex, for formatting string.
I also check that this replace makes well-routed url with inserted kwargs in url_path in my testcase.

Copy link
Member

@tomchristie tomchristie May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, I think this is valid enough, yup, tho let's make a couple of minor tweaks:

  1. escape_curly_brackets would be slightly more clear.
  2. Let's include a docstring noting that we escape any brackets in the regex for the purposes of string formatting.

Copy link
Contributor Author

@no-dap no-dap May 29, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gladly!

@tomchristie tomchristie merged commit 9c9525b into encode:master May 29, 2017
1 check passed
@tomchristie tomchristie added this to the 3.6.4 Release milestone May 29, 2017
@tomchristie
Copy link
Member

tomchristie commented May 29, 2017

Thanks!

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

Successfully merging this pull request may close these issues.

None yet

2 participants