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

Make UriTranslation check prefix contents before matching #1964

Merged
merged 5 commits into from Aug 14, 2018

Conversation

@aeons
Copy link
Member

@aeons aeons commented Jul 26, 2018

This PR changes the behaviour of UriTranslation to take the contents of the prefix into account as well.

It inlines the old implementation in Router, to not do the check twice.

Fixes #1962

@aeons
Copy link
Member Author

@aeons aeons commented Jul 26, 2018

I'm not completely clear on the semantics of UriTranslation.

Should it just ignore whether there's a slash at the front of the prefix and request?

case x if x.startsWith("/") => x.length
case x => x.length + 1
}
def translateRoot[F[_]: Applicative](prefix: String)(service: HttpService[F]): HttpService[F] =

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Jul 26, 2018
Member

This changes the function signature, so isn't binary compatible

This comment has been minimized.

@aeons

aeons Jul 26, 2018
Author Member

Hmm, yes, that's true.
I'm not sure if this can be solved without being to signal a not-matched via OptionT.none.

This comment has been minimized.

@aeons

aeons Aug 10, 2018
Author Member

@rossabaker could you take a look at this? Should I retarget master and just let this be a known bug in 0.18?

This comment has been minimized.

@rossabaker

rossabaker Aug 11, 2018
Member

Our type is called Uri, but this object is called URI. That's inconsistent. We could deprecate thid old buggy one, and use this new implementation as UriTranslation in release-0.18.x. Or TranslateUri.

This comment has been minimized.

@aeons

aeons Aug 13, 2018
Author Member

👍

@rossabaker rossabaker added this to the 0.18.16 milestone Aug 11, 2018
@aeons aeons force-pushed the aeons:uritranslation branch from 628d2dd to df6dbf7 Aug 14, 2018
@aeons
Copy link
Member Author

@aeons aeons commented Aug 14, 2018

I rebased and changed the following:

  • Readd deprecated URITranslation
  • Add the new implementation as TranslateUri.apply (I got a warning about only differing in case if it was called UriTranslation)
@@ -4,6 +4,7 @@ package middleware

import cats.Functor

@deprecated("Use org.http4s.server.middleware.UriTranslation instead", since = "0.18.16")

This comment has been minimized.

@ChristopherDavenport

ChristopherDavenport Aug 14, 2018
Member

Do you mean TranslateUri?

This comment has been minimized.

@rossabaker

rossabaker Aug 14, 2018
Member

Good catch. If only we could compile our deprecation messages.

This comment has been minimized.

@aeons

aeons Aug 14, 2018
Author Member

Yeah, I tried just changing the case, but got a compiler warning.

@aeons aeons merged commit 4e6ba2f into http4s:release-0.18.x Aug 14, 2018
0 of 2 checks passed
0 of 2 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@aeons aeons deleted the aeons:uritranslation branch Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants