membphis commented on PR #13435: URL: https://github.com/apache/apisix/pull/13435#issuecomment-4561230280
Review notes from merge-risk check: ### [P1] `graphql-proxy-cache` can be enabled without the required NGINX proxy-cache variables/directives - Problem: the new plugin writes/uses `upstream_cache_key`, `upstream_cache_zone`, `upstream_cache_bypass`, `upstream_no_cache`, and disk purge relies on `upstream_cache_zone_info`, but the OSS NGINX template only defines these variables and `proxy_cache*` directives when `enabled_plugins["proxy-cache"]` is true. - Why this blocks merge: `graphql-proxy-cache` is registered as an independent plugin and the tests/docs show it being enabled directly. In custom deployments that include `graphql-proxy-cache` but not `proxy-cache`, the plugin can hit undefined NGINX variables or run without the required disk cache configuration. - Impact: runtime failures or non-functional disk cache/purge for users with a minimal custom plugin list. - Trigger: configure `plugins:` with `graphql-proxy-cache` but without `proxy-cache`, then enable the plugin on a route. - Evidence: `apisix/plugins/graphql-proxy-cache.lua` sets `ctx.var.upstream_cache_key` and purge sets `ngx_var.upstream_cache_key`; `apisix/cli/ngx_tpl.lua` wraps cache variable/directive generation in `enabled_plugins["proxy-cache"]`; `t/plugin/graphql-proxy-cache/graphql.t` enables only `graphql-proxy-cache` and `public-api` in its minimal plugin list. - Suggested fix: treat `graphql-proxy-cache` as a proxy-cache user in the template and startup validation, e.g. generate the cache variables/directives and require `apisix.proxy_cache` when either `proxy-cache` or `graphql-proxy-cache` is enabled. Add a startup/runtime test that enables only `graphql-proxy-cache`. ### [P2] Parse-error logging still writes the full GraphQL request body to error log - Problem: malformed GraphQL requests and empty query paths still log the raw `body` value. - Why this blocks merge: GraphQL bodies frequently contain variables or business data. A bad query should not cause request payloads to be written to `error.log`. - Impact: sensitive request data can leak into logs for any route using this plugin. - Trigger: send an invalid GraphQL query or an empty query body. - Evidence: `apisix/plugins/graphql-proxy-cache.lua` logs `failed to parse graphql: ..., body: ...` and `failed to parse graphql: empty query, body: ...`. - Suggested fix: log parser error plus `body_size`/request id only, not the raw body; add a regression test that malformed input is rejected without body content appearing in logs. -- 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]
