'&' in milestone name breaks custom report

[pyrhockz+trac@…]

If a milestone is named 'Beta Foo&Bar', and a custom query is created (via the point-and-click custom query interface) with a constrain on the milestone being 'milestone = 'Beta Foo&Bar', then a query is broken and produces something like the following:

query:?owner=fooser
&
status=accepted
&
status=assigned
&
status=new
&
status=reopened
&
milestone=Beta+Foo
&
Bar
&
groupdesc=1

[Remy Blank]

Confirmed. Some escaping is in order.

[Remy Blank]

This is due to [8465], fixing #8561. So that change wasn't so innocent after all. I'll revert the change, and still try to make the resulting query more readable (e.g. by not escaping "~").

[anonymous]

Thank you.

[Remy Blank]

Don't quote safe characters in query URL when saving as a report.

[Remy Blank]

Don't quote safe characters in Href.

[Remy Blank]

9965-query-urlencode-r10416.patch​ reverts [8465], and makes the generated query URLs slightly more readable by not quoting safe characters ("unreserved characters" as per ​RFC 2396, i.e. "-_.!~*'()"). A simpler solution could have been to un-quote those characters in Query.to_string() after retrieving the URL with get_href(), but read on.

According to ​RFC 2396:

Unreserved characters can be escaped without changing the semantics of the URI, but this should not be done unless the URI is being used in a context that does not allow the unescaped character to appear.

So it seems that we are quoting more than strictly necessary ("-_." aren't quoted by default, but the others are). 9965-href-safe-r10416.patch​ changes that default and avoids quoting all unreserved characters in Href. This only broke a handful of unit tests, which were easily fixed.

I would prefer the second solution, to follow the RFC. In that case, we could even make path_safe and query_safe module-level variables instead of attributes, as they will probably never change.

Opinions?

[Christian Boos]

I verified the fix and I think the changes are safe. We could indeed hardcode the default, and nevertheless make it possible to override them by doing something like:

trac/web/href.py

@@ -132 +132 @@
     '/milestone/<look,here%3E?param=%3Chere,too>'
     """
 
-    def __init__(self, base, path_safe="/!~*'()", query_safe="!~*'()"):
+    path_safe = "/!~*'()"
+    query_safe = "!~*'()"
+
+    def __init__(self, base, path_safe=None, query_safe=None):
         self.base = base.rstrip('/')
-        self.path_safe = path_safe
-        self.query_safe = query_safe
+        if path_safe is not None:
+            self.path_safe = path_safe
+        if query_safe is not None:
+            self.query_safe = query_safe
         self._derived = {}
 
     def __call__(self, *args, **kw):

[Remy Blank]

Replying to cboos:

We could indeed hardcode the default, and nevertheless make it possible to override them by doing something like:

Isn't that exactly the same as default arguments, only more verbose? What I meant with "hardcode" was to make path_safe and query_safe module-level constants, with no provision to change them. This would make sense if we don't expect to ever change these values on a per-instance basis.

[Christian Boos]

Replying to rblank:

Replying to cboos:

We could indeed hardcode the default, and nevertheless make it possible to override them by doing something like:

Isn't that exactly the same as default arguments, only more verbose?

Well, that's slightly more light-weight as the attributes won't be created unless needed. But you're probably right about the lack of need to change them.

This would make sense if we don't expect to ever change these values on a per-instance basis.

And if we ever need to, we would then still be able to set those attributes externally on a given Href instance.

[Remy Blank]

I have kept the more readable (IMO) version of using default arguments in [10420].