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

Match path maps in order of longest matching key prefix (#394) #586

Merged
merged 1 commit into from
Jan 5, 2017

Conversation

jonas
Copy link
Member

@jonas jonas commented Nov 25, 2016

Fixes #394 by ensuring order of the provided mappings instead of leaving it up to the undefined order provided by the map implementation.

It might be a bit overkill to include all of the test cases created by @gosubpl but I included them for completeness and to test with ListMap.

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) labels Nov 25, 2016
@akka-ci
Copy link

akka-ci commented Nov 25, 2016

Test FAILed.

@akka-ci akka-ci removed the validating PR that is currently being validated by Jenkins label Nov 25, 2016
@jonas
Copy link
Member Author

jonas commented Nov 25, 2016

Failed test is #493

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins needs-attention Indicates a PR validation failure (set by CI infrastructure) and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Nov 30, 2016
@akka-ci
Copy link

akka-ci commented Nov 30, 2016

Test FAILed.

@jonas
Copy link
Member Author

jonas commented Nov 30, 2016

Hmm looks like #443 and #492

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed needs-attention Indicates a PR validation failure (set by CI infrastructure) validating PR that is currently being validated by Jenkins labels Dec 1, 2016
@akka-ci
Copy link

akka-ci commented Dec 1, 2016

Test PASSed.

Copy link
Member

@ktoso ktoso left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, great research @gosubpl and @jonas !
Would like a second LGTM for sanity here though. cc @akka/akka-http-team

@jypma
Copy link
Member

jypma commented Jan 5, 2017

LGTM

@ktoso
Copy link
Member

ktoso commented Jan 5, 2017

PLS BUILD

@akka-ci akka-ci added validating PR that is currently being validated by Jenkins tested PR that was successfully built and tested by Jenkins and removed tested PR that was successfully built and tested by Jenkins validating PR that is currently being validated by Jenkins labels Jan 5, 2017
@akka-ci
Copy link

akka-ci commented Jan 5, 2017

Test PASSed.

*
* @group pathmatcherimpl
*/
implicit def _valueMap2PathMatcher[T](valueMap: Map[String, T]): PathMatcher1[T] =
if (valueMap.isEmpty) PathMatchers.nothingMatcher
else valueMap.map { case (prefix, value) ⇒ _stringExtractionPair2PathMatcher((prefix, value)) }.reduceLeft(_ | _)
else valueMap.toSeq.sortWith(_._1 > _._1).map(_stringExtractionPair2PathMatcher).reduceLeft(_ | _)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sortyBy(_._1)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

checked that, actually would fail hm

@ktoso ktoso merged commit 91d1dee into akka:master Jan 5, 2017
@ktoso
Copy link
Member

ktoso commented Jan 5, 2017

Thanks a lot @jonas @gosubpl !

@jonas jonas deleted the fix-394 branch January 5, 2017 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tested PR that was successfully built and tested by Jenkins
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants