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 definitions on routes #131

Merged
merged 3 commits into from Aug 16, 2014

Conversation

2 participants
@grandbora
Contributor

grandbora commented Apr 14, 2014

  • Adds path definitions to routes
  • Refactors findRouteAndMatch method

Use case is; on controller error handler (definition, usage) we want to log which endpoint is failed. One easy way to do that is to log the request path, but most of the paths we use have dynamic values (id, tag etc.). Therefore when we log the request path, we are having trouble when aggregating the log output for monitoring purposes. So instead of request path what we need is the route that is matched.
The closest I could get was getting the PathPattern from router as shown below;

val pathPattern = finatraController.route.findRouteAndMatch(request, request.getMethod).get._2.toString

Unfortunately PathPattern is not user friendly and doesn’t look good on graphs(eg PathPattern(^/users/([^/?#]+)/xx/yy$,List(user))). Therefore I added the the string path definition(eg) to the routes of the controller. So with the same code above we can get the human readable definition of the route that is matched.

PS. this is a pain point for me, would be happy if you can either merge/release or suggest an alternative.
PS. also slightly refactored the method that does the matching.
PS. router does not have a spec.

@grandbora

This comment has been minimized.

Contributor

grandbora commented Apr 16, 2014

Shame that this did not go in with 1.5.3 release.

@capotej

This comment has been minimized.

Contributor

capotej commented Apr 16, 2014

I know! It came in right after we had submitted to maven central. It'll go
out in a week or two.

On Wed, Apr 16, 2014 at 10:14 AM, Bora Tunca notifications@github.comwrote:

Shame that this did not go in with 1.5.3 release.


Reply to this email directly or view it on GitHubhttps://github.com//pull/131?utm_campaign=website&utm_source=sendgrid.com&utm_medium=email#issuecomment-40625900
.

Julio Capote

@grandbora

This comment has been minimized.

Contributor

grandbora commented Apr 16, 2014

Ok, thanks :shipit:

@grandbora

This comment has been minimized.

Contributor

grandbora commented May 2, 2014

Is it possible to merge this, before I forgot about this pr and force push to branch again :)

@grandbora

This comment has been minimized.

Contributor

grandbora commented Aug 15, 2014

Feels like this won't be merged in, closing it.

@grandbora grandbora closed this Aug 15, 2014

@capotej

This comment has been minimized.

Contributor

capotej commented Aug 16, 2014

Hey, sorry about this, got lost in email. Please reopen and I'll merge. Thanks!

@capotej capotej reopened this Aug 16, 2014

capotej added a commit that referenced this pull request Aug 16, 2014

Merge pull request #131 from grandbora/master
path definitions on routes

@capotej capotej merged commit f7a402c into twitter:master Aug 16, 2014

1 check was pending

continuous-integration/travis-ci The Travis CI build is in progress
Details
@grandbora

This comment has been minimized.

Contributor

grandbora commented Aug 17, 2014

No worries, thanks for merging.

cacoco pushed a commit that referenced this pull request May 13, 2015

Merge pull request #131 from grandbora/master
path definitions on routes

cacoco pushed a commit that referenced this pull request May 14, 2015

Merge pull request #131 from grandbora/master
path definitions on routes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment