Postpone param/splat decoding until after matching.

[eholmer]

Adresses #528 #343 #267 #266
Instead of taking the already decoded URI from jetty httpRequest.getPathInfo() we go back to the way it was done before b621367, using httpRequest.getRequestURI() and decode the parameters just before they are added to the params map instead.

This solves the problem of matching routes using encoded slashes.

[kiana]

What is the process on getting this commit merged into master?

[N0odlez]

@kiana Currently @perwendel is on holiday and hasn't got access to a computer nor internet to review PRs. He is due to return early September.

[kiana]

@AzureusNation we are using a work-around, so priority has downgraded. Thank you for updating us.

[perwendel]

👍

[niklasha]

After having been running with a forked spark implementation before the Java 8 switch, we yesterday dropped the java 7 requirement on our codebase, so I went ahead and updated spark-core to 2.5.5. I fairly soon discovered this change, which breaks our code, since it decodes URL-encoded slashes before splatting, thus the route lookup fails.

I.e. we have /route/:timezone, expecting a URL-encode ISO timezone name, and feed in e.g. Europe%2FStockholm and suddenly Spark tries to match with another route, in our case, /route/:filter/:timezone instead, breaking horribly. I don't understand the statement that @bearythebear makes "This solves the problem of matching routes using encoded slashes."
What was the problem? To me it seems the problem instead was added.

Locally I will try to reencode slashes after the URL-decoding in the FilterTools.

[eholmer]

@niklasha It sounds like you're having the problem that I fixed with this PR. I don't know if I understand you correctly but before my commit Europe%2FStockholm would be decoded to Europe/Stockholm and then matched. With this commit, the matching is supposed to be done on the entire URL-encoded string. Which is what you want, no?

[niklasha]

Hmm, maybe I am confused, but I see this in stock 2.5.5. Yes, what you describe is what I see, but apparently in my case your fix is not enough. I get to FilterTools.getRelativePath, and that method destroys the URI (an action I find very dubious, given the name of the method, why should it decode?). I have a slightly contorted setup with extra delegations which may make our use cases different. I need to look closer at what happens I guess. Anyway for now I do a real ugly thing, which works for me:

diff --git a/src/main/java/spark/servlet/FilterTools.java b/src/main/java/spark/servlet/FilterTools.java
index f06efbe..92f43ba 100644
--- a/src/main/java/spark/servlet/FilterTools.java
+++ b/src/main/java/spark/servlet/FilterTools.java
@@ -52,7 +52,7 @@
         }
 
         try {
-            path = URLDecoder.decode(path, "UTF-8");
+            path = URLDecoder.decode(path.replace("%2F", "%252F"), "UTF-8");
         } catch (UnsupportedEncodingException ex) {
             // this can't really ever happen
  
}

[niklasha]

OK, I now know the source of confusion. We probably have different use cases. I assume you use Jetty as integrated in spark, just like the integration test does? That use case does not pass through the critical point of FilterTools.getRelativePath, which is called from SparkFilter.doFilter, the entry point if spark is made a servlet and installed via web.xml. I don't know if there are integration tests for my use case, I don't know spark's internals very well. I still believe that the decoding in getRelativePath is bogus, but I am not sure about what impact it might have to completely remove it. #266 which added it is not completely clear to me.

[niklasha]

OK I redid my patch to something I believe is more correct, as per the specs of getRequestURI: https://docs.oracle.com/javaee/7/api/javax/servlet/http/HttpServletRequest.html#getRequestURI--
This is the diff:
master...niklasha:didi2
comments?