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

Bad ambiguous URI resolution #1567

Open
Isammoc opened this issue Jul 15, 2024 · 7 comments
Open

Bad ambiguous URI resolution #1567

Isammoc opened this issue Jul 15, 2024 · 7 comments
Labels
good first issue Good for newcomers

Comments

@Isammoc
Copy link

Isammoc commented Jul 15, 2024

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?

@kubukoz
Copy link
Member

kubukoz commented Jul 15, 2024

We must have missed the fact that smithy-lang/smithy#1029 has been resolved. We'll have to tackle this for sure...

@kubukoz
Copy link
Member

kubukoz commented Jul 15, 2024

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).

@Baccata
Copy link
Contributor

Baccata commented Jul 16, 2024

Yup sorting seems like it'll be good enough.

@Isammoc
Copy link
Author

Isammoc commented Jul 22, 2024

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?

@kubukoz
Copy link
Member

kubukoz commented Jul 22, 2024

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
Copy link
Member

kubukoz commented Oct 3, 2024

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

@kubukoz kubukoz added the good first issue Good for newcomers label Oct 3, 2024
@Isammoc
Copy link
Author

Isammoc commented Oct 6, 2024

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

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

No branches or pull requests

3 participants