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

Path parameter parsing should allow dots in segments and ignore query parameters #30

Closed
mtritschler opened this issue Jan 27, 2017 · 1 comment
Labels
type/bug A general bug
Milestone

Comments

@mtritschler
Copy link
Contributor

There seems to be a problem with pattern matching for paths in HTTP requests in reactor-netty 0.6.0.RELEASE. Please consider the four examples below. They outline two different problems that are all related to (at least for me) unexpected behaviour of the pattern matcher. In summary they are

  1. Dots in parameterized segments break pattern matching (/{x} won't match /a.b)
  2. Query parameters break pattern matching, event without path params (/search won't match /search?q=reactor)

The last example with /test/{order} illustrates nicely the brokenness of the implementation here as soon as query parameters come into play (given a path /test/foo?q=bar, the value for order will be foo?q=bar).

package reactor.ipc.netty.http.server;
import org.junit.Test;
import reactor.ipc.netty.http.server.HttpPredicate.UriPathTemplate;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.MatcherAssert.assertThat;

public class UriPathTemplateTest {

    @Test
    public void patternShouldMatchPathWithOnlyLetters() {
        UriPathTemplate uriPathTemplate = new UriPathTemplate("/test/{order}");
        // works as expected
        assertThat(uriPathTemplate.match("/test/1").get("order"), is("1"));
    }

    @Test
    public void patternShouldMatchPathWithDots() {
        UriPathTemplate uriPathTemplate = new UriPathTemplate("/test/{order}");
        // does not match, the dot in the segment parameter breaks matching
        // expected: a map containing {"order": "2.0"}, found: empty map
        assertThat(uriPathTemplate.match("/test/2.0").get("order"), is("2.0"));
    }

    @Test
    public void staticPatternShouldMatchPathWithQueryParams() {
        UriPathTemplate uriPathTemplate = new UriPathTemplate("/test/3");
        // does not match, the query parameter breaks matching
        // expected: true, found: false
        assertThat(uriPathTemplate.matches("/test/3?q=reactor"), is(true));
    }

    @Test
    public void parameterizedPatternShouldMatchPathWithQueryParams() {
        UriPathTemplate uriPathTemplate = new UriPathTemplate("/test/{order}");
        // does not match, the query parameter breaks matching
        // expected: a map containing {"order": "3"}, found: a map containing {"order": "3?q=reactor"}
        assertThat(uriPathTemplate.match("/test/3?q=reactor").get("order"), is("3"));
    }

}
@mtritschler
Copy link
Contributor Author

Maybe the issue is not caused by the UriPathTemplate but rather by the fact that the server uses the full result of HttpServerRequest#uri() (including query params) for matching the path instead of just the path

HttpServer.create(port).newRouter(routes ->
    routes.get("/echo/{message}", (req, res) -> 
        res.sendString(Mono.just(req.param("message"))));
// GET /echo/hello => 200 OK "hello"
// GET /echo/hello?a=b => 200 OK "hello?a=b"
// GET /echo/hello.world => 404 Not Found

This makes it impossible to use this API for defining routes for endpoints that accept query params or parameterized path segments may contain dots.

mtritschler pushed a commit to mtritschler/reactor-netty that referenced this issue Jan 31, 2017
@smaldini smaldini added this to the 0.6.2.RELEASE milestone Feb 13, 2017
@smaldini smaldini added the type/bug A general bug label Feb 13, 2017
@smaldini smaldini modified the milestones: 0.6.2.RELEASE, 0.6.3.RELEASE Mar 7, 2017
@smaldini smaldini modified the milestones: 0.6.3.RELEASE, 0.7.0.RELEASE May 3, 2017
@simonbasle simonbasle modified the milestones: 0.7.0.RELEASE, 0.7.0.M1 Jun 19, 2017
@violetagg violetagg modified the milestones: 0.7.0.M1, 0.7.0.M2 Jul 17, 2017
@smaldini smaldini modified the milestones: 0.7.0.M2, 0.8.0.RELEASE Aug 31, 2017
@violetagg violetagg modified the milestones: 0.8.0.RELEASE, 0.8.x Backlog Jun 8, 2018
@violetagg violetagg removed this from the 0.8.x Backlog milestone Sep 17, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants