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

airframe-http: Allow more flexible http path routing #418

Merged
merged 2 commits into from Mar 5, 2019

Conversation

takezoe
Copy link
Member

@takezoe takezoe commented Mar 5, 2019

The current implementation of airframe-http's router allows to define the parameter only at the last component of the path. This pull request allows more flexible http path routing like:

  • /:param/hoge
  • /:param1/hoge/:param2

See also #417

@takezoe takezoe changed the title Allow to more flexible http path routing Allow more flexible http path routing Mar 5, 2019
@takezoe takezoe changed the title Allow more flexible http path routing airframe-http: Allow more flexible http path routing Mar 5, 2019
@takezoe takezoe requested a review from xerial March 5, 2019 03:22
@codecov
Copy link

codecov bot commented Mar 5, 2019

Codecov Report

Merging #418 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #418      +/-   ##
==========================================
+ Coverage   82.41%   82.42%   +<.01%     
==========================================
  Files         165      165              
  Lines        6068     6071       +3     
  Branches      607      609       +2     
==========================================
+ Hits         5001     5004       +3     
  Misses       1067     1067
Impacted Files Coverage Δ
...tp/src/main/scala/wvlet/airframe/http/Router.scala 97.43% <100%> (+0.21%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 71cea45...ed1b9ab. Read the comment docs.

@xerial
Copy link
Member

xerial commented Mar 5, 2019

Thanks for the fix!

@xerial xerial merged commit 76b7995 into wvlet:master Mar 5, 2019
@takezoe
Copy link
Member Author

takezoe commented Mar 5, 2019

Thanks for merging.

Remaining concern is that it's unclear which endpoint is called if multiple endpoints are matched with the request path. We might have to improve the path matching logic to have the clear and stable behavior for conflicting path definitions.

For example, it might be better to give priority to the concrete path definitions (e.g. /user/info) than the parameterized path definitions (e.g. /:id/info). Currently, it depends on the order of definition of methods.

@xerial
Copy link
Member

xerial commented Mar 5, 2019

I think we should run some conflicting path check when creating Route instances.

@takezoe takezoe deleted the flexible-http-routing branch March 5, 2019 10:36
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

Successfully merging this pull request may close these issues.

None yet

2 participants