First ticket: adding querystring and URL fragment support to reverse

Django Internals
1

Hey everybody,

I’ve been around the Django Discord for a while and I was looking around for a ticket to pick up as an entrypoint into contributing to Django in other ways. I found this one from quite a few years ago, with little movement, and thought it might be a good place to start:

#25582 Add a way to build URLs with query strings

It’s something I’ve relatively frequently wondered why it isn’t supported myself.

As I see it, there’s three places this could be supported:

  • django.urls.reverse
  • django.shortcuts.redirect
  • the url template tag

As a starting point, I wrote a basic implementation of the feature for django.urls.reverse and started a draft pull request with it, along with three very simple tests.

The other two places are more challenging from a design perspective, because they both assume any kwargs passed to them are URL kwargs, and therefore “stealing” two kwargs for another purpose could break some existing codebases.

As this is my first go at working on the Django codebase, I’d appreciate the following:

  • any guidance or suggestions for improving the existing PR, including the tests
  • any design suggestions on how to implement the feature for the remaining to places it could be of use, if indeed it’s wanted
  • tips for contributing in general, I guess!

I look forward to any feedback :slight_smile:

Thanks,
Ben

3 Likes
2

Hey Ben, thanks for picking up a 9 year old ticket :pray:

Just took at look at what $job did… I can’t share the snippet but the person who wrote it for some reason felt the need to filter out None values from the supplied params :man_shrugging:

Also worth considering whether urlunparse() is an option to construct the result, though not sure what else it would bring other than a slight readability (debatable) gain?

>>> urlunparse(ParseResult(path='/path/to/something/', query='foo=bär&fizz=büz', fragment='frägment', scheme='', netloc='', params=''))
'/path/to/something/?foo=bär&fizz=büz#frägment'

(declaring all elements of ParseResult is req’d)

3

Looks very straight forward and reasonable to me. I had a minor thing about it seeming reasonable to support a plain dict, but that’s like two lines of code.

4

Thanks for the reply both of you!

Interesting, I hadn’t come across that. It changes from this:

    if query:
        if isinstance(query, QueryDict):
            query_string = query.urlencode()
        else:
            query_string = urlencode(query, doseq=True)
        resolved_url += "?" + query_string
    if fragment:
        resolved_url += "#" + fragment
    return resolved_url

To this:

    if query:
        if isinstance(query, QueryDict):
            query_string = query.urlencode()
        else:
            query_string = urlencode(query, doseq=True)
    else:
        query_string = None
    return urlunparse(
        ParseResult(
            path=resolved_url,
            query=query_string,
            fragment=fragment,
            scheme="",
            netloc="",
            params="",
        )
    )

I’m not sure it makes much of a difference, though concatenating strings doesn’t feel as nice as a proper parser.

Unless I misunderstand you, it does support a plain dict (the else clause to the isinstance check).

5

Ah, my bad. I misremembered how urlencode works. Then I have no notes. Solid change.

1 Like
6

Thinking about this some more, I don’t know that it’s necessary to support the option in these two places.

With redirect, you can easily construct the URL you want using reverse and pass it directly:

return redirect(reverse("my-view", query={"foo": "bar"}))
return redirect(reverse("my-view", query=request.GET))

And with the introduction of the querystring template tag, then it feels suitable in a template to put the querystring directly:

{% url "my-view" %}?foo=bar
{% url "my-view" %}{% querystring %}
5 Likes