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

Document interaction with http4s Router and relative paths #152

Open
keynmol opened this issue Mar 26, 2022 · 2 comments
Open

Document interaction with http4s Router and relative paths #152

keynmol opened this issue Mar 26, 2022 · 2 comments

Comments

@keynmol
Copy link
Contributor

keynmol commented Mar 26, 2022

Let's say you define your smithy ops as mounted at root:

@http(method: "POST", uri: "/{name}", code: 200)

If you then build the routes using val routes = SimpleRestJsonBuilder.routes(...).resource, you can't just shift the API to a custom prefix:

val shifted = Router("/api" -> routes)

The request to /api/bla will fail with 404.

This approach will work with manually defined routes, e.g.

val other = HttpRoutes.of[IO] {
  case r if r.pathInfo.segments.headOption.exists(_.decoded() == "hello") =>
    import org.http4s.dsl.io.*
    Ok(r.toString)
}

val shifted = Router("/api" -> (routes <+> other))

The request to /api/hello/bla will succeed.

I don't know exactly whether the recommendation should be writing absolute paths in the smithy files, or whether this should be considered undesired behaviour. Note that it will obviously affect the generated Swagger docs as well.

@Baccata
Copy link
Contributor

Baccata commented Mar 26, 2022

I don't know exactly whether the recommendation should be writing absolute paths in the smithy files

That is a protocol-specific concern, and for the "simpleRestJson" protocol, the implicit rule is that the API spec should contain absolute paths. The rationale is as follows :

  1. It reinforces the source of truth aspect of the smithy specs
  2. It facilitates interop with other tooling (such as swagger/openapi)

Facilitating shifting would break that paradigm. That being said, the fact that it doesn't work is somewhat accidental, and I'd like to understand why. But I'm not sure I want to fix it, since the bug is, in essence, a happy accident 😅.

You are correct in the fact that it ought to be documented

@kubukoz
Copy link
Member

kubukoz commented Nov 14, 2022

https://smithy.io/2.0/spec/http-bindings.html?highlight=http#uri

The resolved absolute URI of an operation is formed by combining the URI of the operation with the endpoint of the service. (that is, the host and any base URL of where the service is deployed). For example, given a service endpoint of https://example.com/v1 and an operation pattern of /myresource, the resolved absolute URI of the operation is https://example.com/v1/myresource.

It's fine that @simpleRestJson doesn't support this, but I think SmithyHttp4sRouter could. For other protocols which might want to reuse that, it'd save some trouble and still be in line with the Smithy spec (I think).

As for why it doesn't work, I believe Router doesn't modify the Uri that's passed to the underlying services, but rather updates the Request.Keys.PathInfoCaret attribute, which affects r.pathInfo.

Smithy4s doesn't look at r.pathInfo, it looks at the entire r.uri.path.

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

No branches or pull requests

3 participants