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

Allow different param names in different methods with same path scheme #2209

Merged
merged 5 commits into from
Jul 11, 2022

Conversation

ortyomka
Copy link
Contributor

@ortyomka ortyomka commented Jun 29, 2022

Relates to issues #1726 and #2201

Parameters and paths are now separated by methods within the same node.
Add new method Context, which store path, parameters, and handler.
Add test from #1726.

CC @aldas

@ortyomka
Copy link
Contributor Author

@aldas could you run tests again? Added small fix

@ortyomka
Copy link
Contributor Author

I forgot about middleware tests. Fixed

@aldas

Signed-off-by: ortyomka <iurin.art@gmail.com>
@ortyomka
Copy link
Contributor Author

Found a quicker and easier way for the new structure. In methodHandler, I use Context instead of Handler and don't store map.

old - original
new - map with contexts
new2 - methodHandler with contexts

name \ time/op                       old.txt      new.txt      new2.txt
EchoStaticRoutes-4                   18.1µs ± 0%  18.4µs ± 0%  17.4µs ± 0%
EchoStaticRoutesMisses-4             17.7µs ± 0%  20.1µs ± 0%  17.5µs ± 0%
EchoGitHubAPI-4                      31.9µs ± 0%  34.0µs ± 0%  30.5µs ± 0%
EchoGitHubAPIMisses-4                40.7µs ± 0%  33.5µs ± 0%  31.1µs ± 0%
EchoParseAPI-4                       3.11µs ± 0%  2.30µs ± 0%  2.03µs ± 0%
RouterStaticRoutes-4                 14.0µs ± 0%  15.2µs ± 0%  13.5µs ± 0%
RouterStaticRoutesMisses-4            491ns ± 0%   492ns ± 0%   466ns ± 0%
RouterGitHubAPI-4                    25.3µs ± 0%  27.4µs ± 0%  23.7µs ± 0%
RouterGitHubAPIMisses-4               576ns ± 0%   594ns ± 0%   568ns ± 0%
RouterParseAPI-4                     1.28µs ± 0%  1.47µs ± 0%  1.23µs ± 0%
RouterParseAPIMisses-4                304ns ± 0%   363ns ± 0%   312ns ± 0%
RouterGooglePlusAPI-4                 854ns ± 0%  1137ns ± 0%   823ns ± 0%
RouterGooglePlusAPIMisses-4           466ns ± 0%   683ns ± 0%   477ns ± 0%
RouterParamsAndAnyAPI-4              1.96µs ± 0%  2.54µs ± 0%  2.03µs ± 0%

original vs current

name                                 old time/op    new time/op    delta
EchoStaticRoutes-4                     17.2µs ± 2%    17.3µs ± 2%    ~     (p=0.421 n=5+5)
EchoStaticRoutesMisses-4               17.0µs ± 2%    17.3µs ± 2%    ~     (p=0.151 n=5+5)
EchoGitHubAPI-4                        30.9µs ± 0%    31.3µs ± 3%    ~     (p=0.690 n=5+5)
EchoGitHubAPIMisses-4                  31.5µs ± 4%    31.5µs ± 5%    ~     (p=1.000 n=5+5)
EchoParseAPI-4                         2.04µs ± 3%    2.05µs ± 2%    ~     (p=0.841 n=5+5)
RouterStaticRoutes-4                   13.5µs ± 3%    13.6µs ± 2%    ~     (p=0.095 n=5+5)
RouterStaticRoutesMisses-4              474ns ± 1%     469ns ± 0%  -1.01%  (p=0.008 n=5+5)
RouterGitHubAPI-4                      23.9µs ± 1%    23.9µs ± 2%    ~     (p=0.841 n=5+5)
RouterGitHubAPIMisses-4                 577ns ± 0%     580ns ± 3%    ~     (p=0.730 n=4+5)
RouterParseAPI-4                       1.24µs ± 1%    1.24µs ± 1%    ~     (p=0.222 n=5+5)
RouterParseAPIMisses-4                  306ns ± 3%     300ns ± 0%  -1.84%  (p=0.008 n=5+5)
RouterGooglePlusAPI-4                   831ns ± 1%     806ns ± 1%  -2.93%  (p=0.008 n=5+5)
RouterGooglePlusAPIMisses-4             468ns ± 0%     462ns ± 0%  -1.29%  (p=0.008 n=5+5)
RouterParamsAndAnyAPI-4                1.94µs ± 0%    1.93µs ± 1%    ~     (p=0.071 n=5+5)

cc @aldas, ready for review

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

I do not understand why this new struct methodContext is necessary in this PR context? I do not see it adding any functional or performance value. Changing things from HandlerFunc to pointer seems unnecessary as function types are nillable.

@ortyomka
Copy link
Contributor Author

I do not understand why this new struct methodContext is necessary in this PR context? I do not see it adding any functional or performance value. Changing things from HandlerFunc to pointer seems unnecessary as function types are nillable.

Agree, I will add functionality

Signed-off-by: ortyomka <iurin.art@gmail.com>
@ortyomka ortyomka changed the title New Tree Methods structure Allow different param names in different methods with same path scheme Jun 29, 2022
@ortyomka
Copy link
Contributor Author

Added functionality, to store different paths and parameters in different methods

Done @aldas

@ortyomka ortyomka requested a review from aldas June 29, 2022 12:52
@ortyomka
Copy link
Contributor Author

ortyomka commented Jul 5, 2022

@aldas , if you have time, please review PR

router.go Outdated
@@ -13,14 +13,17 @@ type (
routes map[string]*Route
echo *Echo
}
methodContext struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not think that context describes this struct correctly. In world of http servers and Echo context is something more temporary. I have used routeMethod in v5 for similar struct but in v5 methodHandler struct is also renamed to routeMethods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Renamed to routeMethod

router.go Outdated Show resolved Hide resolved
Add paramsCount in each node for perfomance

Signed-off-by: ortyomka <iurin.art@gmail.com>
@ortyomka ortyomka requested a review from aldas July 6, 2022 08:44
@ortyomka
Copy link
Contributor Author

ortyomka commented Jul 6, 2022

@aldas I have fixed comments, could you take a look?

@codecov
Copy link

codecov bot commented Jul 6, 2022

Codecov Report

Merging #2209 (627588a) into master (0644cd6) will increase coverage by 0.02%.
The diff coverage is 97.50%.

@@            Coverage Diff             @@
##           master    #2209      +/-   ##
==========================================
+ Coverage   92.26%   92.29%   +0.02%     
==========================================
  Files          37       37              
  Lines        3090     3102      +12     
==========================================
+ Hits         2851     2863      +12     
  Misses        150      150              
  Partials       89       89              
Impacted Files Coverage Δ
router.go 96.58% <97.50%> (+0.07%) ⬆️
middleware/logger.go 87.71% <0.00%> (+0.44%) ⬆️

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 0644cd6...627588a. Read the comment docs.

router.go Outdated
} else {
// use previous match as basis. although we have no matching handler we have path match.
// so we can send http.StatusMethodNotAllowed (405) instead of http.StatusNotFound (404)
currentNode = previousBestMatchNode

ctx.path = path
Copy link
Contributor

Choose a reason for hiding this comment

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

See comment below. This is incorrect as path must be path of the route not path of the request.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

router.go Show resolved Hide resolved
Signed-off-by: ortyomka <iurin.art@gmail.com>
Signed-off-by: ortyomka <iurin.art@gmail.com>
@ortyomka
Copy link
Contributor Author

ortyomka commented Jul 8, 2022

@aldas
I corrected comments, but a question remains. Do we need to have parameters when we have 405?

I removed them now, but if we need them, what names should they have?

@ortyomka ortyomka requested a review from aldas July 8, 2022 07:22
@ortyomka
Copy link
Contributor Author

I would like to finish this PR by Tuesday(12.07) inclusive.

If your schedule allows it, I'm willing to fix comments ASAP.

CC @aldas

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

I think this PR is good enough for current state. Params for 405 etc are things we/I can address later when there is a decision what the behavior should be. These questions are not necessary to be solved with this change.

Thank you for PR.

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.

2 participants