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

Investigate potential bug in environment.get_url() #141

Open
3 tasks
bittner opened this issue Apr 5, 2022 · 0 comments
Open
3 tasks

Investigate potential bug in environment.get_url() #141

bittner opened this issue Apr 5, 2022 · 0 comments

Comments

@bittner
Copy link
Member

bittner commented Apr 5, 2022

As mentioned in #140 (review), odds are that we have a problem with the implementation of environment.get_url().

  • The to argument should correspond to the first positional parameter of the Django shortcut's resolve_url function. Our implementation may break when to is something else than a string.
  • Specifically, when get_absolute_url() returns a URL containing a protocol and a hostname or IP address then there will be two base_urls in the resulting URL string.

Pylint (correctly) complains about to being a keyword-argument used as a positional argument, and to being a bad name. In Django's resolve_url implementation, to is a positional argument, but we want it to be optional (not necessarily a keyword-argument, strictly speaking), e.g.

# returns the value of `base_url`
context.get_url()
# returns base_url + '/admin'
context.get_url('/admin')

When to Is a Model or a View Name

The problem may occur when the argument passed to get_url is not a simple URL path string.

In this case, Django's resolve_url shortcut might return a URL string that has a URL protocol and hostname already included!

Other Issues

Also, it's unclear whether **kwargs also pass the to keyword argument to resolve_url in our implementation, or just the remainder.

What to Do

  • Add tests that demonstrate that the described bug exists.
  • Add a test that demonstrates that to is not included in **kwargs.
  • Fix the implementation of environment.get_url().
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant