Add matchedRoutePath to Request

[bradenschmidt]

We're in the processing of adding request tracing to all of our java systems, all of which are using Spark as their web framework. To do so we implemented global before and an afterAfter filter which records information about the request and the response and sends it off to our tracing implementation. After the first version was released we noticed an improvement could be made by sanitizing paths params and using the route parameter key as the cleaned version.

This technique was noted in https://github.com/perwendel/spark/issues/959 and https://github.com/perwendel/spark/issues/1048 with https://github.com/perwendel/spark/pull/1049 appearing to close both those issues.

The problem for our use case occurs because we implement automatic tracing of requests as ALL_PATHS filters. This becomes a problem because matchedUri() in a global after filter will return all paths and not that actual matched route path (https://github.com/perwendel/spark/pull/1049#issuecomment-421309271).

The proposal in this PR is to introduce a new field called matchedRoutePath with a getter and setter. The setter records the path during the route execution so it is available in the getter for after and afterAfter filters to use.

[bradenschmidt]

The naming of this field and thus the getter is up for debate. I had toyed with using an original prefix but decided it didn't make sense. Arguably this could be matchedRoute but I decided to leave that available to return the actual RouteMatch object should that be required in the future.

[perwendel]

Hmm, I've reviewed this quickly and I fail to see how this new method would return anything different the the already existing matchedPath() in Request?

[perwendel]

I've checked some more and concluded that this is just duplicate functionality of matchedPath(). I'll close for now.

[bradenschmidt]

Hey there, glad to get the review!

Initially I thought matchedPath() was what I was looking for but it didn't turn out to be the case which is why the PR ended up being raised with very similar yet subtly different semantics.

The problem with matchedPath() is that it returns the matched path of the filters instead of the path of the original matching route that the filter is operating on. This means that when you're looking to attach tracing information from an afterAfter(), for example, you'll get the filters matched path which will generally never be the actual route that was matched. In most cases it will indeed return ALL_PATHS==+/*paths.

The following snippet shows the difference adapted from one of the new tests added:

@Test
    public void shouldBeAbleToGetTheMatchedRoutePathInAfter() throws Exception {
        final AtomicReference<String> matchedRoutePath = new AtomicReference<>();
        final AtomicReference<String> matchedPath = new AtomicReference<>();
        after((q, p) -> {
            matchedRoutePath.set(q.matchedRoutePath());
            matchedPath.set(q.matchedPath());
        });

        http.get("/users/bob");

        assertNotNull(matchedRoutePath.get());
        assertThat(matchedRoutePath.get(), is(THE_MATCHED_ROUTE));
        assertThat(matchedPath.get(), is(THE_MATCHED_ROUTE)); // Assertion Failure
    }

This fails because the last assertion for the matchedPath.get() actually returns +/*paths which is the ALL_PATHS filter used as the default if none is specified.

This behaviour is further reinforced by the tests like this one. The test ensures the matchedPath() returns the filters path and not the original THE_MATCHED_ROUTE that was called. https://github.com/perwendel/spark/blob/10ddf62b1b66f52d1e8f7a0908af3aed28fb0a94/src/test/java/spark/RequestTest.java#L143

This situation was pointed out in the original implementation of matchedPath() here https://github.com/perwendel/spark/pull/1049#issuecomment-421309271 but really could be considered a feature and I do feel both are required to cover both cases.

Happy to discuss more cases and why I ended up requiring this feature. I've been using it for quite some time now.