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

Fix route parsing when concrete and wildcard paths overlap #81

Merged
merged 1 commit into from
Oct 2, 2017

Conversation

tgr
Copy link
Contributor

@tgr tgr commented Sep 27, 2017

When building a node tree from a set of paths, unrelated paths
were incorrectly coalesced (or not, depengin on path order). E.g.

path 1: /{var}/bar
path 2: /foo/baz
Expected: { foo: { baz: {} }, '*': { bar: {} } }
Actual: { '*': { bar: {}, baz: {} } }

Use exact the new match exactness parameter in Node.getChild
to prevent this.

The swagger-router part of the patch is wikimedia/swagger-router#56.

When building a node tree from a set of paths, unrelated paths
were incorrectly coalesced (or not, depengin on path order). E.g.

    path 1: /{var}/bar
    path 2: /foo/baz
    Expected: { foo: { baz: {} }, '*': { bar: {} } }
    Actual: { '*': { bar: {}, baz: {} } }

Use exact the new match exactness parameter in Node.getChild
to prevent this.
@coveralls
Copy link

coveralls commented Sep 27, 2017

Coverage Status

Coverage remained the same at 92.417% when pulling 44c7326 on tgr:wildcard-route-parsing into 3e4754b on wikimedia:master.

@gwicke
Copy link
Member

gwicke commented Sep 27, 2017

LGTM. We also need to bump the swagger-router dependency to ^0.7.0 (already published, v0.7.1 currently a PR), and should probably release a new minor hyperswitch version.

Copy link
Contributor

@Pchelolo Pchelolo left a comment

Choose a reason for hiding this comment

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

I second @gwicke review.

@d00rman d00rman merged commit 2a1ad08 into wikimedia:master Oct 2, 2017
@d00rman
Copy link
Contributor

d00rman commented Oct 2, 2017

Dep bumped and new version released in #82

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants