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

http4s backend doesn't fail when there is additional path to consume after match #23

Closed
hejfelix opened this issue Jan 1, 2019 · 11 comments
Assignees
Labels
bug Something isn't working http4s server

Comments

@hejfelix
Copy link
Contributor

hejfelix commented Jan 1, 2019

e.g. /my/path/is/good is matched by endpoint.get.in("my" / "path") even though there is more to consume.

@hejfelix
Copy link
Contributor Author

Hmm, looks like this causes other problems. Please don't merge yet!

@hejfelix
Copy link
Contributor Author

hejfelix commented Jan 20, 2019

I added this test, and it causes the two backends to disagree:

  testServer(in_empty_path_out_string, () => pureResult(Either.right[Unit, String]("")), "empty path should match empty path") { baseUri =>
    sttp.get(uri"$baseUri").send().map(_.code shouldBe com.softwaremill.sttp.StatusCodes.Ok)
  }

Akka doesn't like this, http4s does (note: I'm not actually using http4s' matching mechanism in that interpeter)

@ghostbuster91
Copy link
Contributor

ghostbuster91 commented Jan 20, 2019

Shouldn't emptyPath be defined as

  val in_empty_path_out_string: Endpoint[Unit, Unit, String] =
    endpoint.get.out(stringBody)

?

This change makes both http4s and akka servers to fail on empty path should not match nonempty path test case.

@hejfelix
Copy link
Contributor Author

Honestly it's not really clear what the expected behaviour is here :/

@adamw
Copy link
Member

adamw commented Jan 20, 2019

I'd say that if there are no path constraints specified, then the path is unconstrained. Same as with the method .get matches only the GET method, while without it matches anything So:

endpoint // matches any path
endpoint.get.out(stringBody) // matches any path
endpoint.path("x" / "y") // matches *only* /x/y and /x/y/
endpoint.path("x") // matches *only* /x and /x/

What about "" in path then? Following the same logic:

endpoint.path("" / "y") // matches *only* //y and //y/
endpoint.path("") // matches *only* / and //

So I think it might be logical that specifying "" as the only path component would mean "the root". That would need special-casing, so that GET http://example.com matches such a path specification, as does GET http://example.com/. We could probably add a / to the path if it is empty before any matching.

@ghostbuster91
Copy link
Contributor

Isn't more obvious for the user to pass / as a root? I think we should not allow empty string

@hejfelix
Copy link
Contributor Author

We are hurting a bit by stringly types here. In the http4s dsl there's a singleton root path called Root IIRC

@adamw
Copy link
Member

adamw commented Jan 21, 2019

@ghostbuster91 well we'll always only be able to detect empty strings at runtime (not compile-time). But as I understand, we have two possibilities:

  1. / as an "empty path" object & / as a method to combine paths:
endpoint.path(/) // matches only root (no path and /)
endpoint.path(/ "x" / "y" / "z") // matches only /x/y/z and /x/y/z/
endpoint.path(/ "x").path(/ "y" / "z")
  1. "" as an empty path:
endpoint.path("") // matches only root (no path and /)
endpoint.path("x" / "y" / "z") // matches only /x/y/z and /x/y/z/
endpoint.path("x").path("y" / "z")

Which one is better, is of course the question? :)

@hejfelix
Copy link
Contributor Author

I vote for 1

@adamw
Copy link
Member

adamw commented Jan 24, 2019

@hejfelix yeah, I'm for 1 as well. Created #41

@ghostbuster91
Copy link
Contributor

I have a third proposition which is just a slightly modification of the 2nd.
Let's use endpoint without any path specification as a root. Path of endpoints which are built upon another will be treated as relative. And we should fail in runtime if the path is an empty string.

Cons:

  • still stringly typed

Pros:

  • concise syntax

Pro & Con:
There is no way to build an endpoint using another endpoint as a base and specify different absolute path.

@adamw adamw self-assigned this Feb 26, 2019
adamw added a commit that referenced this issue Feb 27, 2019
@adamw adamw closed this as completed Feb 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working http4s server
Projects
None yet
Development

No branches or pull requests

3 participants