[WIP] Add generator option to ignore case in pattern matching #2095

serbrech · 2021-04-19T00:00:48Z

References to other Issues or PRs

Fixes #2091

Have you read the Contributing Guidelines?

yes

Brief description of what is fixed or changed

This adds an option to the gateway generation to ignore the url case when matching the request path.
In this first iteration, it only allows to set the option at generation time for all routes.
default to false.

Other comments

google-cla · 2021-04-19T00:00:52Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.

What to do if you already signed the CLA

Individual signers

It's possible we don't have your GitHub username or you're using a different email address on your commit. Check your existing CLA data and verify that your email is set on your git commits.

Corporate signers

Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the Google project maintainer to go/cla#troubleshoot (Public version).
The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
The email used to register you as an authorized contributor must also be attached to your GitHub account.

ℹ️ Googlers: Go here for more info.

serbrech · 2021-04-19T00:02:00Z

@googlebot I signed it!

serbrech · 2021-04-19T00:04:38Z

examples/internal/clients/abe/BUILD.bazel

@@ -5,36 +5,19 @@ package(default_visibility = ["//visibility:public"])
 go_library(


I have no idea why the bazel file changed so much...

serbrech · 2021-04-19T00:05:15Z

examples/internal/proto/examplepb/echo_service.pb.gw.go

@@ -1032,21 +1032,21 @@ func RegisterEchoServiceHandlerClient(ctx context.Context, mux *runtime.ServeMux
 }

 var (
-	pattern_EchoService_Echo_0 = runtime.MustPattern(runtime.NewPattern(1, []int{2, 0, 2, 1, 2, 2, 1, 0, 4, 1, 5, 3}, []string{"v1", "example", "echo", "id"}, ""))
+	pattern_EchoService_Echo_0 = runtime.MustPattern(runtime.NewPattern(1, []int{2, 0, 2, 1, 2, 2, 1, 0, 4, 1, 5, 3}, []string{"v1", "example", "echo", "id"}, "", false))


regenerated the example gateway with the updated generator

serbrech · 2021-04-19T00:05:44Z

protoc-gen-grpc-gateway/internal/gengateway/generator.go

 }

 // New returns a new generator which generates grpc gateway files.
 func New(reg *descriptor.Registry, useRequestContext bool, registerFuncSuffix string,
-	allowPatchFeature, standalone bool) gen.Generator {
+	allowPatchFeature, standalone bool, ignoreCase bool) gen.Generator {


add ignoreCase to the constructor

serbrech · 2021-04-19T00:06:30Z

protoc-gen-grpc-gateway/internal/gengateway/template.go

@@ -727,7 +727,7 @@ func (m response_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}}) XXX_ResponseBody(
 var (
 	{{range $m := $svc.Methods}}
 	{{range $b := $m.Bindings}}
-	pattern_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}} = runtime.MustPattern(runtime.NewPattern({{$b.PathTmpl.Version}}, {{$b.PathTmpl.OpCodes | printf "%#v"}}, {{$b.PathTmpl.Pool | printf "%#v"}}, {{$b.PathTmpl.Verb | printf "%q"}}))
+	pattern_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}} = runtime.MustPattern(runtime.NewPattern({{$b.PathTmpl.Version}}, {{$b.PathTmpl.OpCodes | printf "%#v"}}, {{$b.PathTmpl.Pool | printf "%#v"}}, {{$b.PathTmpl.Verb | printf "%q"}}, false))


for now, default to false. this is where we'll grab the option and use it to set it on each pattern

serbrech · 2021-04-19T00:07:18Z

runtime/mux.go

@@ -185,7 +185,7 @@ func (s *ServeMux) HandlePath(meth string, pathPattern string, h HandlerFunc) er
 		return fmt.Errorf("parsing path pattern: %w", err)
 	}
 	tp := compiler.Compile()
-	pattern, err := NewPattern(tp.Version, tp.OpCodes, tp.Pool, tp.Verb)
+	pattern, err := NewPattern(tp.Version, tp.OpCodes, tp.Pool, tp.Verb, false)


default to false in the HandlePath implementation for now. deferring to later commits or PR for this

serbrech · 2021-04-19T00:08:03Z

runtime/pattern.go

 }

 // NewPattern returns a new Pattern from the given definition values.
 // "ops" is a sequence of op codes. "pool" is a constant pool.
 // "verb" is the verb part of the pattern. It is empty if the pattern does not have the part.
 // "version" must be 1 for now.
 // It returns an error if the given definition is invalid.
-func NewPattern(version int, ops []int, pool []string, verb string) (Pattern, error) {
+func NewPattern(version int, ops []int, pool []string, verb string, caseSensitive bool) (Pattern, error) {


set the caseSensitive option in the Pattern struct, so we can change the Match behavior

serbrech · 2021-04-19T03:18:02Z

seems that the golangci-lint github action is not reporting where the failures occur?

When I run golangci-lint --enable goimports locally, I get a bunch of weird unrelated failures :

/...
// Package generator provides an abstract interface to code generators.
runtime/internal/examplepb/example.pb.go:1: /Users/stephane/go/go1.15.7/src/runtime/panic.go:830:20: invalid operation: shift count i (variable of type int) must be unsigned integer (typecheck)
...

and if I run goimports on the whole repo, many of the changes are in the generated files, and unrelated to my modification.
so is that github action broken for this project?

serbrech · 2021-04-19T04:48:32Z

So, this works as it is now.
passing ignore_case=true on generation will use EqualFold() instead of string equality to match literals.
The implementation only allows to define this on the generation, not per binding.

If this is ok in terms of functionality I think that would be enough.

I had a quick look at implementing it as a Binding parameter, and it seems a little bit more involved, but still relatively easy.

Let me know what you think.

adambabik · 2021-04-19T18:21:26Z

protoc-gen-grpc-gateway/internal/gengateway/template.go

@@ -727,7 +731,7 @@ func (m response_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}}) XXX_ResponseBody(
 var (
 	{{range $m := $svc.Methods}}
 	{{range $b := $m.Bindings}}
-	pattern_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}} = runtime.MustPattern(runtime.NewPattern({{$b.PathTmpl.Version}}, {{$b.PathTmpl.OpCodes | printf "%#v"}}, {{$b.PathTmpl.Pool | printf "%#v"}}, {{$b.PathTmpl.Verb | printf "%q"}}))
+	pattern_{{$svc.GetName}}_{{$m.GetName}}_{{$b.Index}} = runtime.MustPattern(runtime.NewPattern({{$b.PathTmpl.Version}}, {{$b.PathTmpl.OpCodes | printf "%#v"}}, {{$b.PathTmpl.Pool | printf "%#v"}}, {{$b.PathTmpl.Verb | printf "%q"}}, {{$IgnoreCase}}))


As mentioned in the comment, this breaks backward compatibility.

it defaults to false false, so the behavior is the same.
it does break runtime.NewPattern(...) signature.
I can create a separate one if that's what you mean.

runtime.NewCaseInsensitivePattern(...)?
it's a mouthful, but works for me if it works for you :)

ideally, the parameters would be collapsed into an option struct, or use functional options, so that they can be modified without breaking the api in the future...

hm, I see what you mean with backward compatibility.
so, for minor releases, the requirement is that we can update the package without regenerating the gw?
that's very restrictive, and I would say that any func used in the generated code should be redesigned to have a functional option, or struct base parameter to reduce breaking changes...

what about :

type PatternOption func(*Pattern) func WithIgnoreCase() PatternOption { return func(h *Pattern) { h.ignoreCase = true } } NewPattern(version int, ops []int, pool []string, verb string, ...PatternOption) ) (Pattern, error) { }

that would allow the old generated code to work with the new version of the package

I think it's a valid feature so we want it. But it's an open source project on which we work after-hours and it requires different amount of time needed to spot a breaking change versus digging into a problem and finding a viable solution. I can only ask for a bit more patience :)

Let's consider a solution adding a new method:

Code generated with version 1 and library in version 2 (with this new public method); it will work fine as the method simply won't be called.

Code generated with version 2 and library in version 1; it will break as the generated code will contain the unknown method. But I don't think we promise such guarantees. One should always use the same or never version with regards to which was used to generate the code. @johanbrandhorst can you confirm, please?

If what I described above is correct, a solution with a new constructor like NewPatternWithOptions() should also work. It could result in shorted generated code because options can be calculated at the top and usage would look like this: runtime.MustPattern(runtime.NewPatternWithOptions(..., patternOptions)).

Adam is correct, option #2 should be fine.

Thanks for clarifying @adambabik is there any doc about backward/forward compatibility guarantees that should be observed when contributing, and to be expected when using the library?
Good that there is a path forward. I'll update the PR today.

Is there any other concern about the implementation?
documentation needed as part of the PR?
further testing I should write?

I saw that the example have some integration testing. I can change or add one to use ignore_case, and add some integration tests if needed.

You're welcome! Unfortunately I don't think there is any doc explaining that. We are trying to follow industry standards.

Is there any other concern about the implementation?

No.

documentation needed as part of the PR?

There is no separate documentation for flags so no need. The flag description should be enough.

further testing I should write?

There will be a few broken unit tests which will need to be fixed. The most important is "template_test.go". I think it can be also extended to cover this feature.

I saw that the example have some integration testing. I can change or add one to use ignore_case, and add some integration tests if needed.

An integration test would be nice. If you want to add a new protobuf file which will cover ignore_case=true, take a look at Makefile's proto target to see how that can be done. Some flags have their own protobuf and *.buf.gen.yaml files.

Thank you Adam, I had to fix a few template tests for the previous iteration, so that should be fine.

I'll look into adding a separate protobuf file to illustrate and test ignore_case.

stale · 2021-06-20T12:08:57Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

add option

b2a2fe1

google-cla bot added the cla: no label Apr 19, 2021

google-cla bot added cla: yes and removed cla: no labels Apr 19, 2021

serbrech commented Apr 19, 2021

View reviewed changes

serbrech added 2 commits April 19, 2021 13:53

implement ignoreCase in pattern matching

b891dbe

pass the ignore_case parameter to the generator

d678e4f

serbrech added 3 commits April 19, 2021 14:53

goimports

1a0a189

revert bazel file changes

79f4c1a

fix equality check that was inverted

2897556

adambabik requested changes Apr 19, 2021

View reviewed changes

serbrech requested a review from johanbrandhorst April 20, 2021 04:17

stale bot added the wontfix label Jun 20, 2021

stale bot closed this Jun 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add generator option to ignore case in pattern matching #2095

[WIP] Add generator option to ignore case in pattern matching #2095

serbrech commented Apr 19, 2021 •

edited

Loading

google-cla bot commented Apr 19, 2021

serbrech commented Apr 19, 2021

serbrech Apr 19, 2021

serbrech Apr 19, 2021

serbrech Apr 19, 2021

serbrech Apr 19, 2021 •

edited

Loading

serbrech Apr 19, 2021

serbrech Apr 19, 2021

serbrech commented Apr 19, 2021 •

edited

Loading

serbrech commented Apr 19, 2021

adambabik Apr 19, 2021

serbrech Apr 19, 2021

serbrech Apr 19, 2021

serbrech Apr 19, 2021

serbrech Apr 19, 2021

adambabik Apr 20, 2021

johanbrandhorst Apr 20, 2021

serbrech Apr 20, 2021

adambabik Apr 20, 2021 •

edited

Loading

serbrech Apr 20, 2021

stale bot commented Jun 20, 2021

		@@ -5,36 +5,19 @@ package(default_visibility = ["//visibility:public"])
		go_library(

[WIP] Add generator option to ignore case in pattern matching #2095

[WIP] Add generator option to ignore case in pattern matching #2095

Conversation

serbrech commented Apr 19, 2021 • edited Loading

References to other Issues or PRs

Have you read the Contributing Guidelines?

Brief description of what is fixed or changed

Other comments

google-cla bot commented Apr 19, 2021

What to do if you already signed the CLA

Individual signers

Corporate signers

serbrech commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serbrech Apr 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serbrech commented Apr 19, 2021 • edited Loading

serbrech commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adambabik Apr 20, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stale bot commented Jun 20, 2021

serbrech commented Apr 19, 2021 •

edited

Loading

serbrech Apr 19, 2021 •

edited

Loading

serbrech commented Apr 19, 2021 •

edited

Loading

adambabik Apr 20, 2021 •

edited

Loading