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

Fix pekko route naming #13491

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

samwright
Copy link
Contributor

There are a few problems with the http.route values generated in pekko's instrumentation.

  1. Path matching does not need to end with a PathEnd matcher, as the current solution assumes, e.g.:
pathPrefix("foo" ~ not("bar")) { complete("ok") }

will match /fooabc/123, but no http.route is generated. Given the glob-style approach of using * to represent a captured variable, I suggest the correct http.route here should be /foo**.

  1. pathPrefix doesn't work as expected, e.g.
pathPrefix("a") {
  pathPrefix("b") {
    path("c") {
      complete("ok")
    }
  }
}

will match /a/b/c, but the http.route generated is abc.

This solves those issues, and adds a variety of PathMatchers to the tests.

@samwright samwright requested a review from a team as a code owner March 11, 2025 08:59
@laurit
Copy link
Contributor

laurit commented Mar 11, 2025

will match /fooabc/123, but no http.route is generated. Given the glob-style approach of using * to represent a captured variable, I suggest the correct http.route here should be /foo**

I personally would prefer /foo* to /foo**

Copy link
Contributor Author

@samwright samwright left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I went back to using PekkoRouteHandler from the Tapir instrumentation, since this lets people combine pekko path-matching with tapir path-matching. I doubt there's a good reason anyone would do that, but it was working before this PR so I shouldn't break it. I added a test to confirm.

@@ -19,6 +23,7 @@
import sttp.tapir.server.ServerEndpoint;

public class RouteWrapper implements Function1<RequestContext, Future<RouteResult>> {
private static final Uri.Path EMPTY = Uri.Path$.MODULE$.apply("", Charset.defaultCharset());
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be the only way to get the Uri.Path.Empty object from java - apparently once it's 3 or more levels deep, accessing nested scala classes from java gets tricky.

@samwright
Copy link
Contributor Author

@laurit @masonedmison any other comments?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants