-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
fix path with : in the middle #3517 issue #3532
base: main
Are you sure you want to change the base?
Conversation
@@ -3918,6 +3918,7 @@ func TestTemplateToOpenAPIPath(t *testing.T) { | |||
{"/{user.name=prefix/*}:customMethod", "/{user.name}:customMethod"}, | |||
{"/{user.name=prefix1/*/prefix2/*}:customMethod", "/{user.name}:customMethod"}, | |||
{"/{parent=prefix/*}/children:customMethod", "/{parent}/children:customMethod"}, | |||
{"/item/search:items/{item_no_query}", "/item/search:items/{item_no_query}"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it should be camel case here.. "/item/search:items/{itemNoQuery}"
Maybe you need to add a test to TestTemplateWithJsonCamelCase
https://github.com/grpc-ecosystem/grpc-gateway/blob/main/protoc-gen-openapiv2/internal/genopenapi/template_test.go#L3843C6-L3843C35
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, thanks. This is the proper place for the test. Also corrected the fix in the actual function, deleted the wrong if depth == 0 branch...
This should also reproduce the issue and at least fixes the case mentioned in the issue.
parts = append(parts, buffer) | ||
buffer = path[i:] | ||
break pathLoop | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried about just removing this, are all the tests still passing?
References to other Issues or PRs
Fixes #3517
Brief description of what is fixed or changed
The assumption that a colon can only appear on the end of a path seems to wrong. Removing the special case code that exploits this assumption seems to fix the issue. However I'm not sure about the full impact on other parts of the code.
Other comments