Repository: go-kratos/kratos
Pull request #1704 (2021-12-21 22:34:26)

fix(protoc-gen-go-http): Update http rule when path mappings
<!--
🎉 Thanks for sending a pull request to Kratos! Here are some tips for you:

1. If this is your first time contributing to Kratos, please read our contribution guide: https://go-kratos.dev/en/docs/community/contribution/
2. Ensure you have added or ran the appropriate tests and lint for your PR, please use `make lint` and `make test` before filing your PR, use `make clean` to tidy your go mod.
3. If the PR is unfinished, you may need mark it as a WIP(Work In Progress) PR or draft PR
4. Please use a conventional commits format title: `<type>[optional scope]: <description>`
    
    Some suggestion on <type>:

    fix: A bug fix
    feat: A new feature
    test: Adding missing tests or correcting existing tests
    refactor: A code change that neither fixes a bug nor adds a feature
    break: Changes has break change

    docs: Documentation only changes
    deps: Changes external dependencies
    style: Changes that do not affect the meaning of the code (white-space, formatting, etc)
    chore Daily work, examples, etc.
    ci: Changes to our CI configuration files and scripts
-->

#### Description (what this PR does / why we need it):
When generating the HTTP code including path mappings, like 

```proto
   service Messaging {
       rpc GetMessage(GetMessageRequest) returns (Message) {
         option (google.api.http) = {
             get: "/v1/{name=messages/*}"
         };
       }
  }
```

It was not detecting `name` as a variable, so this PR switches to use regex instead, it also creates an additional rule for these cases to support what's described in the HTTP rule https://github.com/googleapis/googleapis/blob/master/google/api/http.proto#L85

<!--
* The description should include the motivation for this PR or contrast this with previous behavior
-->


#### Which issue(s) this PR fixes (resolves / be part of):
<!--
* Automatically closes linked issue when PR is merged.
* If you PR is not fully resolved issue, please use `part of #<issue number>` instead.

Usage: `fixes/resolves #<issue number>`, or `fixes/resolves (paste link of issue)`.
-->
fixes #

#### Other special notes for reviewer:
<!--
* Somethings that need extra attention for reviewer
* Some additional notes, TODO list, etc.
-->


codecov-commenter: # [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1704?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) Report
> Merging [#1704](https://codecov.io/gh/go-kratos/kratos/pull/1704?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (c9f1ff4) into [main](https://codecov.io/gh/go-kratos/kratos/commit/94d674fd4c2f5ff4c7230e2420a0f54438e35db5?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) (94d674f) will **decrease** coverage by `0.08%`.
> The diff coverage is `n/a`.

> :exclamation: Current head c9f1ff4 differs from pull request most recent head 32285ed. Consider uploading reports for the commit 32285ed to get more accurate results
[![Impacted file tree graph](https://codecov.io/gh/go-kratos/kratos/pull/1704/graphs/tree.svg?width=650&height=150&src=pr&token=XwnmBTz78x&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)](https://codecov.io/gh/go-kratos/kratos/pull/1704?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)

```diff
@@            Coverage Diff             @@
##             main    #1704      +/-   ##
==========================================
- Coverage   81.17%   81.08%   -0.09%     
==========================================
  Files          78       78              
  Lines        3431     3431              
==========================================
- Hits         2785     2782       -3     
- Misses        443      445       +2     
- Partials      203      204       +1     
```


| [Impacted Files](https://codecov.io/gh/go-kratos/kratos/pull/1704?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos) | Coverage Δ | |
|---|---|---|
| [selector/node/ewma/node.go](https://codecov.io/gh/go-kratos/kratos/pull/1704/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos#diff-c2VsZWN0b3Ivbm9kZS9ld21hL25vZGUuZ28=) | `83.87% <0.00%> (-3.23%)` | :arrow_down: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1704?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/go-kratos/kratos/pull/1704?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Last update [94d674f...32285ed](https://codecov.io/gh/go-kratos/kratos/pull/1704?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=go-kratos).

longXboy: in this pr, if the request uri is "/v1/messages/123456",  name's value still is "123456" not "messages/123456".
is that expected?
bakjos: @longXboy Thanks for the quick response, it is expected in the way I made the PR, but not sure if it should work in that way, I think it should match what you get on the GRPC method so it should be message/12346 instead of 123456, I will make the change, although it implies modifying how the variables are capture in the http module 
bakjos: @longXboy I think I found a way to match the route without the need of having 2 routes and following the guidelines, let me know what you think
longXboy: Sorry for the late reply, now the implementation is more elegant and standard, LGTM!
@bakjos 

bakjos: @longXboy wasn't sure where to add tests, can you point me out to where I can add them?
longXboy: > @longXboy wasn't sure where to add tests, can you point me out to where I can add them?. I wanted to add one, but couldn't find any for protocol-gen-go-http

just add some test case for Function `replacePath ` and `buildPathVars`
bakjos: > > @longXboy wasn't sure where to add tests, can you point me out to where I can add them?. I wanted to add one, but couldn't find any for protocol-gen-go-http
> 
> @bakjos just add some test cases for Function `replacePath ` and `buildPathVars`

@longXboy  Done, thanks for the guidence
longXboy: @bakjos  github ci did not pass,Maybe you forgot to execute
 `go get github.com/stretchr/testify/assert@v1.7.0` or `go mod tidy`
bakjos: > @bakjos github ci did not pass,maybe you forgot to execute `go get github.com/stretchr/testify/assert@v1.7.0` or `go mod tidy`

@longXboy  Yup, forgot to include the go.mod file. Thanks

Patch:
diff --git a/cmd/protoc-gen-go-http/go.mod b/cmd/protoc-gen-go-http/go.mod
index fe412640..d38285e8 100644
--- a/cmd/protoc-gen-go-http/go.mod
+++ b/cmd/protoc-gen-go-http/go.mod
@@ -4,6 +4,7 @@ go 1.16
 
 require (
 	github.com/go-kratos/kratos/v2 v2.1.3
+	github.com/stretchr/testify v1.7.0
 	google.golang.org/genproto v0.0.0-20210805201207-89edb61ffb67
 	google.golang.org/protobuf v1.27.1
 )
diff --git a/cmd/protoc-gen-go-http/http.go b/cmd/protoc-gen-go-http/http.go
index 4d6c80d9..fef91d7a 100644
--- a/cmd/protoc-gen-go-http/http.go
+++ b/cmd/protoc-gen-go-http/http.go
@@ -3,6 +3,7 @@ package main
 import (
 	"fmt"
 	"os"
+	"regexp"
 	"strings"
 
 	"google.golang.org/protobuf/reflect/protoreflect"
@@ -157,9 +158,14 @@ func buildHTTPRule(g *protogen.GeneratedFile, m *protogen.Method, rule *annotati
 
 func buildMethodDesc(g *protogen.GeneratedFile, m *protogen.Method, method, path string) *methodDesc {
 	defer func() { methodSets[m.GoName]++ }()
-	vars := buildPathVars(m, path)
+
+	vars := buildPathVars(path)
 	fields := m.Input.Desc.Fields()
-	for _, v := range vars {
+
+	for v, s := range vars {
+		if s != nil {
+			path = replacePath(v, *s, path)
+		}
 		for _, field := range strings.Split(v, ".") {
 			if strings.TrimSpace(field) == "" {
 				continue
@@ -192,16 +198,33 @@ func buildMethodDesc(g *protogen.GeneratedFile, m *protogen.Method, method, path
 	}
 }
 
-func buildPathVars(method *protogen.Method, path string) (res []string) {
-	for _, v := range strings.Split(path, "/") {
-		if strings.HasPrefix(v, "{") && strings.HasSuffix(v, "}") {
-			name := strings.TrimRight(strings.TrimLeft(v, "{"), "}")
-			res = append(res, name)
+func buildPathVars(path string) (res map[string]*string) {
+	res = make(map[string]*string)
+	pattern := regexp.MustCompile(`(?i){([a-z\.0-9_\s]*)=?([^{}]*)}`)
+	matches := pattern.FindAllStringSubmatch(path, -1)
+	for _, m := range matches {
+		name := strings.TrimSpace(m[1])
+		if len(name) > 1 && len(m[2]) > 0 {
+			res[name] = &m[2]
+		} else {
+			res[name] = nil
 		}
 	}
 	return
 }
 
+func replacePath(name string, value string, path string) string {
+	pattern := regexp.MustCompile(fmt.Sprintf(`(?i){([\s]*%s[\s]*)=`, name))
+	idx := pattern.FindStringIndex(path)
+	if len(idx) > 0 {
+		path = fmt.Sprintf("%s{%s:%s}",
+			path[:idx[0]], // The start of the match
+			name,
+			strings.ReplaceAll(value, "*", ".*"))
+	}
+	return path
+}
+
 func camelCaseVars(s string) string {
 	vars := make([]string, 0)
 	subs := strings.Split(s, ".")


Test patch:
diff --git a/cmd/protoc-gen-go-http/http_test.go b/cmd/protoc-gen-go-http/http_test.go
new file mode 100644
index 00000000..f28a7f41
--- /dev/null
+++ b/cmd/protoc-gen-go-http/http_test.go
@@ -0,0 +1,54 @@
+package main
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestNoParameters(t *testing.T) {
+	path := "/test/noparams"
+	m := buildPathVars(path)
+	assert.Emptyf(t, m, "Map should be empty")
+}
+
+func TestSingleParam(t *testing.T) {
+	path := "/test/{message.id}"
+	m := buildPathVars(path)
+	assert.Len(t, m, 1)
+	assert.Empty(t, m["message.id"])
+}
+
+func TestTwoParametersReplacement(t *testing.T) {
+	path := "/test/{message.id}/{message.name=messages/*}"
+	m := buildPathVars(path)
+	assert.Len(t, m, 2)
+	assert.Empty(t, m["message.id"])
+	assert.NotEmpty(t, m["message.name"])
+	assert.Equal(t, *m["message.name"], "messages/*")
+}
+
+func TestNoReplacePath(t *testing.T) {
+	path := "/test/{message.id}"
+	assert.Equal(t, path, replacePath("message.id", "", path))
+
+	path = "/test/{message.id=test}"
+	assert.Equal(t, "/test/{message.id:test}", replacePath("message.id", "test", path))
+}
+
+func TestReplacePath(t *testing.T) {
+	path := "/test/{message.id}/{message.name=messages/*}"
+	newPath := replacePath("message.name", "messages/*", path)
+	assert.Equal(t, "/test/{message.id}/{message.name:messages/.*}", newPath)
+}
+
+func TestIteration(t *testing.T) {
+	path := "/test/{message.id}/{message.name=messages/*}"
+	vars := buildPathVars(path)
+	for v, s := range vars {
+		if s != nil {
+			path = replacePath(v, *s, path)
+		}
+	}
+	assert.Equal(t, "/test/{message.id}/{message.name:messages/.*}", path)
+}