fatihbm commented on PR #13810: URL: https://github.com/apache/apisix/pull/13810#issuecomment-5310045139
> Thanks for the contribution — the overall engineering here is solid: strict schema with `encrypt_fields`, correct reuse of `apisix/utils/redis.lua`, a thoughtful cache key (consumer identity, route scope, body SHA-256, negotiation headers, cookie allowlist), the circuit-breaker fallback, and the trace-correlated integration profile are all appreciated. > > That said, a few things need attention before this can move forward. The inline comments cover the correctness issues (one of them is a real cache-safety bug). In addition: > > * **`ui/.gitkeep` is unrelated to this PR — please remove it.** > * **Relationship with `proxy-cache`**: APISIX already ships `proxy-cache` and `graphql-proxy-cache`. Please add a short section to the doc explaining why body-aware QUERY caching is a separate plugin rather than an extension of `proxy-cache`'s cache key — this question will come up in review anyway. > * **CI cost**: the dedicated workflow builds the full APISIX docker image and pulls an Elasticsearch image on every PR touching these paths. Please consider folding it into the existing CI, and pin `actions/upload-artifact` by commit SHA like the checkout step. > * Minor: no zh doc yet (fine to follow up); new plugins conventionally start at `version = 0.1`; `parse_cookie` captures trailing spaces in cookie names, so `a = 1` is treated as an unallowlisted cookie and bypasses the cache. Thank you for the thorough review. I have pushed follow-up commits addressing the unrelated file, proxy-cache documentation, CI cost, version, and cookie parsing points. The Elasticsearch-specific workflow has been removed in favor of mock-upstream coverage in the existing test suite. I will follow up on the inline comments separately -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
