feat: support google custome verb #413
Conversation
| {"/{userId}:init", "/{userId}"}, | ||
| {"/{userId:init}", "/{userId:init}"}, | ||
| {"/users/{id:init}:init", "/users/{id:init}"}, | ||
| {"/users/{id}", "/users/{id}"}, |
There was a problem hiding this comment.
this will fail: "/init/users/{id:init}:init" , "/init/users/{id:init}"
There was a problem hiding this comment.
i added it to case, and run success, is something wrong?
testCase := []struct{
path string
expectedPath string
}{
{"/{userId}:init", "/{userId}"},
{"/{userId:init}", "/{userId:init}"},
{"/users/{id:init}:init", "/users/{id:init}"},
{"/users/{id}", "/users/{id}"},
{"/init/users/{id:init}:init" , "/init/users/{id:init}"},
}
There was a problem hiding this comment.
tbh, i did not execute this test. i should have said "will this fail?". I came up with this test because I saw "ReplaceAllString" in removeCustomVerb. And this case "/:init/users:init/" is probably not a valid path, right?
There was a problem hiding this comment.
emmm, it likes "/users/{id:init}:init", so i think the case is not necessary.
and replace regex is ":customVerb$", it will just replace last word.
curly.go
Outdated
| @@ -72,6 +72,15 @@ func (c CurlyRouter) matchesRouteByPathTokens(routeTokens, requestTokens []strin | |||
| return false, 0, 0 | |||
| } | |||
| requestToken := requestTokens[i] | |||
| if hasCustomVerb(routeToken){ | |||
There was a problem hiding this comment.
the hasCustomVerb is doing a regexp operation. this will affect performance. for this reason, I would like to introduce a field in the Router called CustomVerbEnabled with an default value of false.
There was a problem hiding this comment.
@emicklei, now it will calculate if a route has custom verb when build route
There was a problem hiding this comment.
so, we don't need the CustomVerbEnabled !
path_processor.go
Outdated
| @@ -29,6 +29,11 @@ func (d defaultPathProcessor) ExtractParameters(r *Route, _ *WebService, urlPath | |||
| } else { | |||
| value = urlParts[i] | |||
| } | |||
| if hasCustomVerb(key) { | |||
There was a problem hiding this comment.
is here need to check CustomVerbEnabled? @emicklei
There was a problem hiding this comment.
yes, it should be checked
| } | ||
|
|
||
| // Initialize for Route | ||
| func (r *Route) postBuild() { | ||
| r.pathParts = tokenizePath(r.Path) | ||
| r.hasCustomVerb = hasCustomVerb(r.Path) |
|
to complete the PR, we should have an entry in the features list on the README refering to https://cloud.google.com/apis/design/custom_methods and a minimal example in the /examples folder. |
|
@emicklei , i have added a example (examples/restful-google-custom-method.go) and updated readme.md, |
| // This example shows how to create a Route with google custom method | ||
| // Requires the use of a CurlyRouter and path should end with the custom method | ||
| // | ||
| // GET http://localhost:8080/resource/some-resource-id:init |
| func main() { | ||
| DefaultContainer.Router(CurlyRouter{}) | ||
| ws := new(WebService) | ||
| ws.Route(ws.GET("/basepath/{resourceId}:init").To(fromPathParam)) |
There was a problem hiding this comment.
i think it is good to add another Route to demonstrate that the verb is causing another function to be called.
so next to :init have :reset or some other verb you can think of.
There was a problem hiding this comment.
@emicklei , well, i have already fixed example
|
i think we are ready to merge |
|
ok, lets do it, and we can resolve #394 |
|
well module support was quite a challenge with subpackages not importing a version. I will have a look again. Would be better to separate that from this PR. |
|
@emicklei ok, i removed the |
|
yes, i understand. It will be one of the first changes next to do. |
|
good! and now we can merge it, right? |
support google custome verb
resolve this issue. #394