smithy4s icon indicating copy to clipboard operation
smithy4s copied to clipboard

Bad ambiguous URI resolution

Open Isammoc opened this issue 1 year ago • 7 comments

From smithy 2.0 documentation, the server should route ambiguous URI in a specific order.

My current extract:

a path with a non-label segment is considered more specific than one with a label segment in the same position

Reproduction:

  1. Create a new project
    $ sbt new disneystreaming/smithy4s.g8
    # default for all
    
  2. Edit smithy definition
    -  operations: [Hello]
    +  operations: [Hello, SpecialHello]
    
    and add new operation
    @readonly
    @http(method: "GET", uri: "/hello/me", code: 200)
    operation SpecialHello {
      output: Greeting
    }
    
  3. Implement service (in Main.scala)
      override def specialHello(): IO[Greeting] = IO.pure(Greeting("Hello special me!"))
    
  4. Start sbt run

Current behavior

The method for broader endpoint is run:

$ curl http://localhost:9000/hello/me
{"message":"Hello me!"}

Expected behavior

More specific method is preferred:

$ curl http://localhost:9000/hello/me
{"message":"Hello special me!"}

Notes

If this is from the same service, the order of operations seems to affect the affect the order of matching.

For different services, the order will be directly from the scala code that calls.

How can I help?

Isammoc avatar Jul 15 '24 14:07 Isammoc

We must have missed the fact that https://github.com/smithy-lang/smithy/issues/1029 has been resolved. We'll have to tackle this for sure...

kubukoz avatar Jul 15 '24 16:07 kubukoz

Had a quick read of the rules, at first sight it looks like we'll just have to sort the routes before combining them.

This code in HttpUnaryServerRouter.scala:

    private val httpEndpointHandlers: List[HttpEndpointHandler] =
      service.endpoints.toList
        .map { makeHttpEndpointHandler(_) }
        .collect { case Right(endpointWrapper) => endpointWrapper }

    private val perMethodEndpoint: Map[HttpMethod, List[HttpEndpointHandler]] =
      httpEndpointHandlers.groupBy(_.httpEndpoint.method)

(there are two occurrences of it)

should be adjusted so that the endpoints are sorted according to the specificity algorithm. This can be done after grouping, so that we sort smaller lists. so, something like

    private val perMethodEndpoint: Map[HttpMethod, List[HttpEndpointHandler]] =
      httpEndpointHandlers.groupBy(_.httpEndpoint.method)
        .map { case (method, es) => (method, es.sortBy(specificity)) }

might do it. We'll need some tests for the whole thing, and I'd also like to see that code deduplicated (together with makeHttpEndpointHandler).

kubukoz avatar Jul 15 '24 17:07 kubukoz

Yup sorting seems like it'll be good enough.

Baccata avatar Jul 16 '24 15:07 Baccata

Note: I'm struggling to make the project compile. I'm currently unable to navigate correctly in the code. TBH, I'm not used to nix, flakes, or that kind of development environment. I will need some time to set up my environment for trying to handle this. To reflect my current state: ep.httpEndpoint is Any for me (instead of smithy4s.http.HttpEndpoint[I], I guess).

I'm using latest version of IntelliJ Idea (2024.1.4) with latest scala plugin (2024.1.25). What are you using?

Isammoc avatar Jul 22 '24 20:07 Isammoc

FYI you don't need Nix/flakes for local development, it's mostly useful here if you want to have a consistent experience with the website (which shouldn't be necessary here) :)

IntelliJ is known not to handle more complex types very well, we mostly use Metals in VS Code here. Maybe some of these hints will help? https://hmemcpy.com/2021/09/bsp-and-intellij/

Also, have you tried the nightlies?

kubukoz avatar Jul 22 '24 21:07 kubukoz

Forgot about this one for a bit. @Isammoc have you made any progress?

kubukoz avatar Oct 03 '24 13:10 kubukoz

Hi @kubukoz, Unfortunately, I still cannot make the project compile. I tried with IntelliJ, vscode, sbt only. But I don't know what's wrong

Also, have you tried the nightlies?

nightlies of which project?

But, yeah the holidays passed by, and I forgot too. Sorry about that

Isammoc avatar Oct 06 '24 13:10 Isammoc