-
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
mux: fix path components mutation #3001
mux: fix path components mutation #3001
Conversation
@cclien0725 @johanbrandhorst I believe this is the fix we need. If it looks good, you can just squash and merge it. |
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.
LGTM, though this code is so hairy I'm really pleased we have it well covered by the tests 😅
Thanks for your contribution! @cclien0725 could you confirm that this fixes your issue? |
@johanbrandhorst yeah it's all a bit hard to understand. Not claiming I have an obvious better way of writing it without looking into it more, but agree that it's hairy. Interestingly, by changing the code to NOT mutate data, we avoided a bug. Speaks to benefits of immutable data. |
@jonathaningram @johanbrandhorst thanks for both of your help, this fixes the issue! |
) [![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [github.com/grpc-ecosystem/grpc-gateway/v2](https://togithub.com/grpc-ecosystem/grpc-gateway) | require | minor | `v2.13.0` -> `v2.14.0` | --- ### Release Notes <details> <summary>grpc-ecosystem/grpc-gateway</summary> ### [`v2.14.0`](https://togithub.com/grpc-ecosystem/grpc-gateway/releases/tag/v2.14.0) [Compare Source](https://togithub.com/grpc-ecosystem/grpc-gateway/compare/v2.13.0...v2.14.0) #### New features This release contains two significant new OpenAPIv2 generator features, contributed by [@​krak3n](https://togithub.com/krak3n): 1. A new option to [disable rendering of 200 OK responses](https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_openapi_output/#disable-default-responses). This is useful if you define custom responses for your endpoints and you modify the return code a forward response writer. Note that this does not change the behavior of the gateway itself. 2. A new annotation for [defining header parameters](https://grpc-ecosystem.github.io/grpc-gateway/docs/mapping/customizing_openapi_output/#custom-http-header-request-parameters). This lets to define header parameters you want to be rendered in the swagger.json output in addition to those defined in your API messages. Note that this does not change the behavior of the gateway itself and must be coupled with custom header parsing in your application. #### What's Changed - release: Update release.yml with option to workaround SLSA generator failure by [@​asraa](https://togithub.com/asraa) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2987](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2987) - release: add a workflow_dispatch trigger for testing by [@​asraa](https://togithub.com/asraa) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2989](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2989) - Use io/os instread of ioutil and use suitable verb by [@​sashamelentyev](https://togithub.com/sashamelentyev) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2991](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2991) - runtime pkg cleanup by [@​sashamelentyev](https://togithub.com/sashamelentyev) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2993](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2993) - mux: fix path components mutation by [@​jonathaningram](https://togithub.com/jonathaningram) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3001](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3001) - fix: set consumes definition per operation by [@​stomy13](https://togithub.com/stomy13) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2995](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2995) - protoc gen oas v2 cleanup by [@​sashamelentyev](https://togithub.com/sashamelentyev) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2996](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2996) - Use ReplaceAll instead of Replace with -1 pos by [@​sashamelentyev](https://togithub.com/sashamelentyev) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3003](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3003) - Errors cleanup by [@​sashamelentyev](https://togithub.com/sashamelentyev) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3004](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3004) - Cleanup by [@​sashamelentyev](https://togithub.com/sashamelentyev) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3012](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3012) - Support disabling default response rendering by [@​krak3n](https://togithub.com/krak3n) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3006](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3006) - Support request header parameters by [@​krak3n](https://togithub.com/krak3n) in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3010](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3010) #### New Contributors - [@​asraa](https://togithub.com/asraa) made their first contribution in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2987](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2987) - [@​sashamelentyev](https://togithub.com/sashamelentyev) made their first contribution in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2991](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2991) - [@​stomy13](https://togithub.com/stomy13) made their first contribution in [https://github.com/grpc-ecosystem/grpc-gateway/pull/2995](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/2995) - [@​krak3n](https://togithub.com/krak3n) made their first contribution in [https://github.com/grpc-ecosystem/grpc-gateway/pull/3006](https://togithub.com/grpc-ecosystem/grpc-gateway/pull/3006) **Full Changelog**: grpc-ecosystem/grpc-gateway@v2.13.0...v2.14.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "before 6am on monday" in timezone Australia/Sydney, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/google/osv.dev). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNC4zNy4wIiwidXBkYXRlZEluVmVyIjoiMzQuMzcuMCJ9-->
References to other Issues or PRs
Fixes #2990
Have you read the Contributing Guidelines?
Yes
Brief description of what is fixed or changed
There's a for loop which looks at handlers that might match. However there's a case where the path components slice was mutated if idx > 0. Subsequent iterations of the for loop might end up with idx <= 0 and they'll be using a version of the path components that were mutated from the previous iteration. Instead, we keep a copy of the path components at the top, and inside each iteration we make a copy, then modify that.
Other comments