Repository navigation
fix(grpc-transcode): normalize integer proto references before caching - #14076
Closed
hanzhenfang wants to merge 1 commit into
Closed
hanzhenfang wants to merge 1 commit into
hanzhenfang wants to merge 1 commit into
Conversation
Continue the integer proto ID fix proposed in apache#13811 with shared cache keys and regression coverage for ID types and schema updates. Co-authored-by: alirezashamsabad <khajeshamsabad@outlook.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
A route with
grpc-transcode.proto_id: 1cannot find a proto stored by the Admin API as"1", although the schema and documentation allow integer IDs. Normalize the requested ID before the proto cache lookup and compare stored IDs as strings, so integer and string references resolve the same proto and share one cache entry.This continues @alirezashamsabad's fix in #13811, which remains open, unmerged and marked stale. The original diagnosis and string-normalization approach are credited to that PR. This replacement is based on current master (
cea64fbf) to take over maintenance and add broader regression coverage; it does not imply the original implementation was rejected or that its author endorsed this replacement.Compared with #13811:
fetch, before the LRU lookup, rather than only insidecreate_proto_obj.1and"1"now share the compiled proto cache; configuration-version invalidation is retained."01"; do not coerce IDs to numbers.grpc-transcode.t, following the contribution guide.Which issue(s) this PR fixes:
Fixes #8952 (previously closed as stale without this fix).
Related implementation: #13811.
Validation
Using current checkout sources with the Linux
apache/apisix:3.19.0-debianruntime, an isolated etcd, and the repository's Go gRPC test server:prove -I/tmp/test-nginx/lib -I. t/plugin/grpc-transcode-proto-id.t: 7 cases / 21 assertions pass. Before the fix, integer requests fail withfailed to find proto by id: 1; string requests succeed.grpc-transcode-reload-bugfix.t,grpc-transcode2.t,grpc-transcode3.t, andgrpc-transcode4.talso pass.grpc-transcode.treports worker-eventsfailed to receive the header bytes: closedduring TEST 19. Re-running that existing test with the unmodified masterproto.luareproduces the same failure. This environment does not provide a clean full grpc-transcode suite result; the failure is not suppressed.luacheck -q apisix t/lib: 0 warnings/errors in 406 files.lj-releng, repository test-style checks, andgit diff --check: pass.make lintis blocked by existing Unicode separator line-length errors in unchanged discovery files (Consul, Kubernetes, Nacos, among others). Those unrelated files are not changed here.The full APISIX service matrix was not run locally. Draft pending repository CI.
Checklist