new field and getter in Request for matchedRoute

[robax]

PR for #1048 ping @perwendel

[robax]

@perwendel @tipsy @jakaarl @joatmon @M-Razavi Have any of you had a chance to look at this? I would greatly appreciate it. :)

[tipsy]

Looks good to me.

[perwendel]

@robax, @tipsy
I've had a quick look. There are a few things I'm unsure of.

First of all the method has not been implemented in RequestWrapper which makes this always return null in any route or filter.

The matchedRouteUri field in Request is not updated in changeMatch which is invoked from a couple of classes AfterFilters, BeforeFilters, AfterAfterFilters. There seems to be a discrepancy in functionality here. E.g. take this code:

        get("/hello", (request, response) -> "hello: " + request.matchedRouteUri());
        get("/users/:u", (q, a) -> "user: " + q.matchedRouteUri());

        after((q, a) -> {
            System.out.println("after: " + q.matchedRouteUri());
        });

the sysout in the after filter will say:

localhost:4567/hello -> /hello
localhost:4567/users/per -> /users/:u
localhost:4567/notmatched -> +/*paths

which is unambiguous. In the first two cases it's value is based on matching of prior routes whereas in the latter case it's the matching of the filter itself.

[tipsy]

Seems like I didn't read this as well as I should have. I think the functionality is good, but it looks like the implementation needs some work.

@perwendel I don't see the problem with the output in your example though. In each case it prints the path that was used to match the route/filter. You could return early in a before, in which case there won't be any matching route.

Maybe matcherPath() would be a better name than matchedRouteUri() ?

[robax]

Thanks for the thorough responses guys!

I added a method matchedRouteUri() in the RequestWrapper class and I'm now updating the matched route in the changeMatch() method. I'm also happy to change the method name as per @tipsy.

I don't see the problem with your example @perwendel. Does updating the route in changeMatch() fix your concern? Or can you further illustrate the issue?

[robax]

Would you guys be able to take another peek at this please? @perwendel @tipsy

[tipsy]

@robax Per is away for some time. It looks fine to me, but it would be good if you could include an integration test where you call it in a filter too.

[robax]

Good recommendation @tipsy ! I added the integration tests and I'll wait until @perwendel gets back.

[jon-ruckwood]

This feature would be really useful for us. I was wondering if it is any closer to being accepted?

[robax]

I pinged @perwendel on twitter (https://twitter.com/perwendel/status/1073127244988600321) a little over a month ago and he said he was close to reviewing PR's. I hope we can merge this in soon as it is a tiny change and one that would help us quite a bit aswell!

[robax]

@tipsy Sorry to keep pinging you on this, but you seem to be the only active maintainer. Would you be able to merge this PR? I pinged @perwendel again on Twitter a month ago and haven't received a response.

[tipsy]

Sure, this shouldn't be too controversial to merge.

[jon-ruckwood]

Is there any timeframe for when this change will appear in a released version?

[perwendel]

@robax The functionality of this is good. However, I'm not sure about the name matchedRouteUri, as @tipsy commented previously. Maybe matchedRoutePath or just matchedPath is better. I'm unsure.

@jon-ruckwood the plan is to release 2.9.0 this week.

[perwendel]

If no other input I will change to matchedPath before releasing 2.9.0. That goes best with the current nomenclature that a route is made up from a verb and a path.