Copilot commented on code in PR #13578: URL: https://github.com/apache/apisix/pull/13578#discussion_r3456732632
########## apisix/plugins/ai-cache.lua: ########## @@ -0,0 +1,189 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one or more +-- contributor license agreements. See the NOTICE file distributed with +-- this work for additional information regarding copyright ownership. +-- The ASF licenses this file to You under the Apache License, Version 2.0 +-- (the "License"); you may not use this file except in compliance with +-- the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. +-- + +local core = require("apisix.core") +local schema = require("apisix.plugins.ai-cache.schema") +local key_mod = require("apisix.plugins.ai-cache.key") +local redis_util = require("apisix.utils.redis") + +local ngx = ngx +local ngx_null = ngx.null +local ipairs = ipairs + +local CACHE_STATUS_HEADER = "X-AI-Cache-Status" +local CACHE_AGE_HEADER = "X-AI-Cache-Age" +local DEFAULT_TTL = 3600 +local DEFAULT_MAX_BODY = 1048576 + +local _M = { + version = 0.1, + priority = 1035, + name = "ai-cache", + schema = schema, +} + + +function _M.check_schema(conf) + return core.schema.check(schema, conf) +end + + +local function release(conf, red) + local ok, err = red:set_keepalive(conf.redis_keepalive_timeout or 10000, + conf.redis_keepalive_pool or 100) + if not ok then + core.log.warn("ai-cache: failed to set redis keepalive: ", err) + end +end + + +local function serve_hit(conf, ctx, cached) + ctx.ai_cache_status = "HIT" + if conf.cache_headers ~= false then + core.response.set_header(CACHE_STATUS_HEADER, "HIT") + local age = ngx.time() - (cached.created_at or ngx.time()) + core.response.set_header(CACHE_AGE_HEADER, age < 0 and 0 or age) + end + core.response.set_header("Content-Type", "application/json") + return core.response.exit(200, cached.body) +end + + +function _M.access(conf, ctx) + -- Streaming responses are not cached in PR-1 (SSE replay is a later + -- increment). ai-proxy (higher priority) has already classified the + -- request, so bypass before doing any work. + if ctx.var.request_type == "ai_stream" then + ctx.ai_cache_status = "BYPASS" + return + end + + if conf.bypass_on then + for _, rule in ipairs(conf.bypass_on) do + if core.request.header(ctx, rule.header) == rule.equals then + ctx.ai_cache_status = "BYPASS" + return + end + end + end + + local body, err = core.request.get_json_request_body_table() + if not body then + core.log.warn("ai-cache: cannot read request body, bypassing: ", err) + ctx.ai_cache_status = "BYPASS" + return + end + + ctx.ai_cache_key = "ai-cache:l1:" .. key_mod.scope(conf, ctx) + .. ":" .. key_mod.fingerprint(ctx, body) + + local red + red, err = redis_util.new(conf) + if not red then + -- fail-open: never let a cache-backend outage break the request. + core.log.warn("ai-cache: redis unavailable, fail-open as MISS: ", err) + ctx.ai_cache_status = "MISS" + return + end + + local res + res, err = red:get(ctx.ai_cache_key) + release(conf, red) + if err then + core.log.warn("ai-cache: redis get failed, fail-open as MISS: ", err) + ctx.ai_cache_status = "MISS" + return + end + + if res ~= nil and res ~= ngx_null then + local cached = core.json.decode(res) + if cached and cached.body then + return serve_hit(conf, ctx, cached) + end + core.log.warn("ai-cache: discarding malformed cache entry for ", ctx.ai_cache_key) + end + + ctx.ai_cache_status = "MISS" +end + + +function _M.header_filter(conf, ctx) + if ctx.ai_cache_status and conf.cache_headers ~= false then + core.response.set_header(CACHE_STATUS_HEADER, ctx.ai_cache_status) + end +end + + +function _M.body_filter(conf, ctx) + -- only a MISS gets written back; HIT exited in access, BYPASS opts out. + if ctx.ai_cache_status ~= "MISS" or ctx.ai_cache_oversized then + return + end + local chunk = ngx.arg[1] + if chunk and #chunk > 0 then + ctx.ai_cache_buf = (ctx.ai_cache_buf or "") .. chunk + if #ctx.ai_cache_buf > (conf.max_cache_body_size or DEFAULT_MAX_BODY) then + ctx.ai_cache_buf = nil + ctx.ai_cache_oversized = true + end + end +end Review Comment: `body_filter` currently appends response chunks via string concatenation (`buf = buf .. chunk`), which is O(n²) over many chunks and can become expensive even with a max size cap. Buffer chunks in a table and `table.concat` once at EOF to keep the work linear and reduce allocations. ########## apisix/plugins/ai-cache/schema.lua: ########## @@ -0,0 +1,95 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one or more +-- contributor license agreements. See the NOTICE file distributed with +-- this work for additional information regarding copyright ownership. +-- The ASF licenses this file to You under the Apache License, Version 2.0 +-- (the "License"); you may not use this file except in compliance with +-- the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. +-- + +local core = require("apisix.core") +local redis_schema = require("apisix.utils.redis-schema") + +local policy_to_additional_properties = core.table.deepcopy(redis_schema.schema) + +local _M = { + type = "object", + properties = { + layers = { + type = "array", + items = { + enum = { "exact" }, + }, + minItems = 1, + uniqueItems = true, + default = { "exact" }, + }, + + exact = { + type = "object", + properties = { + ttl = { type = "integer", minimum = 1, default = 3600 }, + }, + default = {}, + }, + + cache_key = { + type = "object", + properties = { + include_consumer = { type = "boolean", default = false }, + include_vars = { + type = "array", + items = { type = "string" }, + default = {}, + }, + }, + default = {}, + }, + + max_cache_body_size = { + type = "integer", minimum = 0, default = 1048576, + }, + + cache_headers = { + type = "boolean", default = true, + }, + + bypass_on = { + type = "array", + minItems = 1, + items = { + type = "object", + properties = { + header = { type = "string" }, + equals = { type = "string" }, + }, + required = { "header", "equals" }, Review Comment: `bypass_on` rules allow empty `header` / `equals` strings, which can lead to surprising behavior (e.g., matching an empty header name). Add `minLength` validation so invalid rules are rejected at schema-check time. ########## t/plugin/ai-cache.t: ########## @@ -0,0 +1,772 @@ +# +# Licensed to the Apache Software Foundation (ASF) under one or more +# contributor license agreements. See the NOTICE file distributed with +# this work for additional information regarding copyright ownership. +# The ASF licenses this file to You under the Apache License, Version 2.0 +# (the "License"); you may not use this file except in compliance with +# the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. +# + +BEGIN { + $ENV{TEST_ENABLE_CONTROL_API_V1} = "0"; +} + +use t::APISIX 'no_plan'; + +log_level("info"); +repeat_each(1); +no_long_string(); +no_root_location(); + +add_block_preprocessor(sub { + my ($block) = @_; + + if (!defined $block->request) { + $block->set_value("request", "GET /t"); + } + + my $user_yaml_config = <<_EOC_; +plugins: + - ai-proxy + - ai-cache +_EOC_ + if (!defined $block->extra_yaml_config) { + $block->set_value("extra_yaml_config", $user_yaml_config); + } +}); + +run_tests(); + +__DATA__ + +=== TEST 1: minimal valid exact-cache configuration +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.ai-cache") + local ok, err = plugin.check_schema({ + redis_host = "127.0.0.1", + redis_port = 6379, + }) + + if not ok then + ngx.say(err) + else + ngx.say("passed") + end + } + } +--- response_body +passed + + + +=== TEST 2: reject config missing required redis (policy=redis then-clause) +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.ai-cache") + local ok, err = plugin.check_schema({}) + + if not ok then + ngx.say(err) + else + ngx.say("passed") + end + } + } +--- response_body eval +qr/then clause did not match/ + + + +=== TEST 3: reject unknown layer value +--- config + location /t { + content_by_lua_block { + local plugin = require("apisix.plugins.ai-cache") + local ok, err = plugin.check_schema({ + redis_host = "127.0.0.1", + layers = { "nonsense" }, + }) + + if not ok then + ngx.say(err) + else + ngx.say("passed") + end + } + } +--- response_body eval +qr/layers/ + + + +=== TEST 4: flush redis, then set route with ai-proxy + ai-cache (mock upstream) +--- config + location /t { + content_by_lua_block { + local redis = require("resty.redis") + local red = redis:new() + red:set_timeout(1000) + local ok, rerr = red:connect("127.0.0.1", 6379) + if not ok then + ngx.say("redis connect failed: ", rerr) + return + end + red:flushall() Review Comment: `flushall()` is executed without checking its return value. If Redis is reachable but the FLUSHALL command fails, the test may proceed with stale state and become flaky. ########## apisix/plugins/ai-cache.lua: ########## @@ -0,0 +1,189 @@ +-- +-- Licensed to the Apache Software Foundation (ASF) under one or more +-- contributor license agreements. See the NOTICE file distributed with +-- this work for additional information regarding copyright ownership. +-- The ASF licenses this file to You under the Apache License, Version 2.0 +-- (the "License"); you may not use this file except in compliance with +-- the License. You may obtain a copy of the License at +-- +-- http://www.apache.org/licenses/LICENSE-2.0 +-- +-- Unless required by applicable law or agreed to in writing, software +-- distributed under the License is distributed on an "AS IS" BASIS, +-- WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +-- See the License for the specific language governing permissions and +-- limitations under the License. +-- + +local core = require("apisix.core") +local schema = require("apisix.plugins.ai-cache.schema") +local key_mod = require("apisix.plugins.ai-cache.key") +local redis_util = require("apisix.utils.redis") + +local ngx = ngx +local ngx_null = ngx.null +local ipairs = ipairs + +local CACHE_STATUS_HEADER = "X-AI-Cache-Status" +local CACHE_AGE_HEADER = "X-AI-Cache-Age" +local DEFAULT_TTL = 3600 +local DEFAULT_MAX_BODY = 1048576 + +local _M = { + version = 0.1, + priority = 1035, + name = "ai-cache", + schema = schema, +} + + +function _M.check_schema(conf) + return core.schema.check(schema, conf) +end + + +local function release(conf, red) + local ok, err = red:set_keepalive(conf.redis_keepalive_timeout or 10000, + conf.redis_keepalive_pool or 100) + if not ok then + core.log.warn("ai-cache: failed to set redis keepalive: ", err) + end +end + + +local function serve_hit(conf, ctx, cached) + ctx.ai_cache_status = "HIT" + if conf.cache_headers ~= false then + core.response.set_header(CACHE_STATUS_HEADER, "HIT") + local age = ngx.time() - (cached.created_at or ngx.time()) + core.response.set_header(CACHE_AGE_HEADER, age < 0 and 0 or age) + end + core.response.set_header("Content-Type", "application/json") + return core.response.exit(200, cached.body) +end + + +function _M.access(conf, ctx) + -- Streaming responses are not cached in PR-1 (SSE replay is a later + -- increment). ai-proxy (higher priority) has already classified the + -- request, so bypass before doing any work. + if ctx.var.request_type == "ai_stream" then + ctx.ai_cache_status = "BYPASS" + return + end + + if conf.bypass_on then + for _, rule in ipairs(conf.bypass_on) do + if core.request.header(ctx, rule.header) == rule.equals then + ctx.ai_cache_status = "BYPASS" + return + end + end + end + + local body, err = core.request.get_json_request_body_table() + if not body then + core.log.warn("ai-cache: cannot read request body, bypassing: ", err) + ctx.ai_cache_status = "BYPASS" + return + end + + ctx.ai_cache_key = "ai-cache:l1:" .. key_mod.scope(conf, ctx) + .. ":" .. key_mod.fingerprint(ctx, body) + + local red + red, err = redis_util.new(conf) + if not red then + -- fail-open: never let a cache-backend outage break the request. + core.log.warn("ai-cache: redis unavailable, fail-open as MISS: ", err) + ctx.ai_cache_status = "MISS" + return + end + + local res + res, err = red:get(ctx.ai_cache_key) + release(conf, red) + if err then + core.log.warn("ai-cache: redis get failed, fail-open as MISS: ", err) + ctx.ai_cache_status = "MISS" + return + end + + if res ~= nil and res ~= ngx_null then + local cached = core.json.decode(res) + if cached and cached.body then + return serve_hit(conf, ctx, cached) + end + core.log.warn("ai-cache: discarding malformed cache entry for ", ctx.ai_cache_key) + end + + ctx.ai_cache_status = "MISS" +end + + +function _M.header_filter(conf, ctx) + if ctx.ai_cache_status and conf.cache_headers ~= false then + core.response.set_header(CACHE_STATUS_HEADER, ctx.ai_cache_status) + end +end + + +function _M.body_filter(conf, ctx) + -- only a MISS gets written back; HIT exited in access, BYPASS opts out. + if ctx.ai_cache_status ~= "MISS" or ctx.ai_cache_oversized then + return + end + local chunk = ngx.arg[1] + if chunk and #chunk > 0 then + ctx.ai_cache_buf = (ctx.ai_cache_buf or "") .. chunk + if #ctx.ai_cache_buf > (conf.max_cache_body_size or DEFAULT_MAX_BODY) then + ctx.ai_cache_buf = nil + ctx.ai_cache_oversized = true + end + end +end + + +-- The response-capturing phases (body_filter / log) run in contexts where +-- cosockets are disabled, so the Redis write is deferred to a 0-delay timer +-- (timers run in a light thread where cosockets are allowed). +local function write_to_cache(premature, conf, cache_key, response_body) + if premature then + return + end + local red, err = redis_util.new(conf) + if not red then + core.log.warn("ai-cache: redis unavailable on write: ", err) + return + end + local envelope = core.json.encode({ body = response_body, created_at = ngx.time() }) + local ttl = (conf.exact and conf.exact.ttl) or DEFAULT_TTL + local ok + ok, err = red:set(cache_key, envelope, "EX", ttl) + if not ok then + core.log.warn("ai-cache: redis set failed: ", err) + end + release(conf, red) +end + + +function _M.log(conf, ctx) + if ctx.ai_cache_status ~= "MISS" or not ctx.ai_cache_key then + return + end + if ngx.status ~= 200 then + return + end Review Comment: The PR description says the cache writes on any 2xx response, but the implementation only writes when `ngx.status == 200` (and the tests assert that 201 is not cached). Please align either the description or the behavior/tests (e.g., cache all 2xx and persist the original status code in the cache envelope). -- 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]
