membphis commented on code in PR #13942: URL: https://github.com/apache/apisix/pull/13942#discussion_r4011470012
########## apisix/plugins/openapi-to-mcp/cache.lua: ########## @@ -0,0 +1,69 @@ +-- +-- 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 loader = require("apisix.plugins.openapi-to-mcp.openapi.loader") +local ref = require("apisix.plugins.openapi-to-mcp.openapi.ref") +local generator = require("apisix.plugins.openapi-to-mcp.tools.generator") +local tostring = tostring + +local _M = {} + +-- A generated tool list is kept for an hour, for up to 100 documents. +local SPEC_TTL = 3600 +local SPEC_COUNT = 100 + +-- A failed fetch is cached only briefly. Without neg_ttl core.lrucache caches +-- nothing on failure, which would let an unreachable spec host be re-dialed on +-- every single request; a long negative TTL would instead keep the route broken +-- long after the host recovers. +local NEG_TTL = 5 +local NEG_COUNT = 32 + +local CACHE_VERSION = "1" + +local lru = core.lrucache.new({ Review Comment: **Non-blocking: cache expiry behavior** With a constant `CACHE_VERSION` and `invalid_stale` unset, `core.lrucache` revives expired entries whose version still matches instead of invoking `build_tools` again. In a targeted probe using the existing cache wrapper with a mocked clock and loader, calls after 3,601 and 7,202 seconds still returned the original tools, with only one fetch. A document updated at the same URL can therefore stay stale beyond the advertised one-hour TTL, until eviction or worker restart. This does not block merging the PR. Please double-check the intended refresh policy and decide whether to fix this behavior, for example by invalidating expired entries and adding a same-URL refresh regression test. ########## apisix/plugins/openapi-to-mcp/openapi/endpoints.lua: ########## @@ -0,0 +1,82 @@ +-- +-- 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 pairs = pairs +local ipairs = ipairs +local type = type +local math_huge = math.huge +local table_sort = table.sort + +local _M = {} + +-- Order of openapi-types' OpenAPIV3.HttpMethods enum. extractToolsFromApi +-- iterates Object.values(OpenAPIV3.HttpMethods), so tools come out in this +-- order per path. Do not "fix" this to CRUD order. +local METHOD_ORDER = { + "get", "put", "post", "delete", "options", "head", "patch", "trace", +} + +local METHOD_RANK = {} +for rank, method in ipairs(METHOD_ORDER) do + METHOD_RANK[method] = rank +end + + +function _M.extract(spec, path_order) + local out = {} + if type(spec) ~= "table" or type(spec.paths) ~= "table" then + return out + end + path_order = path_order or {} + + for path, path_item in pairs(spec.paths) do + if type(path_item) == "table" then + for _, method in ipairs(METHOD_ORDER) do + local operation = path_item[method] + if type(operation) == "table" then + out[#out + 1] = { + method = method, + path = path, + operation = operation, Review Comment: **Non-blocking: Path Item parameter inheritance** Only `operation` is forwarded here, and the generator reads `operation.parameters`; parameters declared on the enclosing Path Item are not merged in. For example, a required `id` declared under `paths["/pets/{id}"].parameters` is omitted from the tool input schema, and a generated call can retain `/pets/{id}` in the request URL. OpenAPI defines these parameters as applying to all operations on that path, with operation-level overrides matched by name and location: [Path Item Object](https://spec.openapis.org/oas/v3.0.3#path-item-object). This does not block merging the PR. Please double-check whether Path Item parameters should be supported and decide whether to add inheritance, override handling, and corresponding tests here or in a follow-up. ########## apisix/plugins/openapi-to-mcp/tools/handler.lua: ########## @@ -0,0 +1,281 @@ +-- +-- 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 http = require("resty.http") +local json_pretty = require("apisix.plugins.openapi-to-mcp.json_pretty") +local pairs = pairs +local ipairs = ipairs +local type = type +local tostring = tostring +local str_lower = string.lower +local str_upper = string.upper +local str_gsub = string.gsub +local str_gmatch = string.gmatch +local str_find = string.find +local table_concat = table.concat +local table_sort = table.sort +local escape_uri = ngx.escape_uri + +local _M = {} + +local DEFAULT_TIMEOUT = 30000 +local NESTED_KEYS = { "pathParameters", "queryParameters", "headerParameters" } + + +local function path_param_names(template) + local names = {} + for name in str_gmatch(template, "{([^}]+)}") do + names[#names + 1] = name + end + return names +end + + +local function names_by_location(tool, location) + local names = {} + for _, param in ipairs(tool.parameters or {}) do + if type(param) == "table" and param["in"] == location then + names[#names + 1] = param.name + end + end + return names +end + + +local function pick(arguments, names) + local out = {} + for _, name in ipairs(names) do + if arguments[name] ~= nil then + out[name] = arguments[name] + end + end + return out +end + + +-- Whether the caller used the flat or the nested argument shape is decided by +-- the arguments themselves, not by the plugin's flatten_parameters setting. +-- A client that sends flat arguments to a nested tool therefore still works. +local function split_arguments(tool, arguments) + local nested = false + for _, key in ipairs(NESTED_KEYS) do + if arguments[key] ~= nil then + nested = true + break + end + end + + if nested then + return type(arguments.pathParameters) == "table" and arguments.pathParameters or {}, + type(arguments.queryParameters) == "table" and arguments.queryParameters or {}, + type(arguments.headerParameters) == "table" and arguments.headerParameters or {} + end + + return pick(arguments, path_param_names(tool.path_template)), + pick(arguments, names_by_location(tool, "query")), + pick(arguments, names_by_location(tool, "header")) +end + + +local function apply_query_defaults(tool, query) + for _, param in ipairs(tool.parameters or {}) do + if type(param) == "table" and param["in"] == "query" + and type(param.schema) == "table" + and param.schema.default ~= nil + and query[param.name] == nil + then + query[param.name] = param.schema.default + end + end +end + + +local function build_path(template, path_params) + local path = template + for name, value in pairs(path_params) do + -- the name is a literal, so escape any pattern magic in it + local pattern = "{" .. str_gsub(name, "([%^%$%(%)%%%.%[%]%*%+%-%?])", "%%%1") .. "}" + local escaped = escape_uri(tostring(value)) + -- The replacement has to be a function: percent-encoding produces "%2F" + -- and friends, and gsub would read those as capture references in a + -- string replacement and raise "invalid capture index". + path = str_gsub(path, pattern, function() + return escaped + end) + end + return path +end + + +-- Bracket notation, as the common JavaScript HTTP clients serialize it: an +-- array becomes "tags[]=a&tags[]=b" and an object "filter[a]=x", nested to any +-- depth. Dropping the brackets, or dropping objects entirely, would lose the +-- structure the schema advertised. +local function encode_param(name, value, parts) + if type(value) ~= "table" then + parts[#parts + 1] = escape_uri(name) .. "=" .. escape_uri(tostring(value)) + return + end + + if #value > 0 then + for _, item in ipairs(value) do + encode_param(name .. "[]", item, parts) Review Comment: **Non-blocking: default array query encoding** Appending `[]` changes the query parameter name: `tags = {"a", "b"}` becomes `tags%5B%5D=a&tags%5B%5D=b`. For an OpenAPI query parameter with the default `style: form` and `explode: true`, the expected encoding is `tags=a&tags=b`. A backend expecting `tags` may therefore treat the parameter as missing. This encoder also does not consult the declared `style` or `explode`: [Parameter Object](https://spec.openapis.org/oas/v3.0.3#parameter-object). This does not block merging the PR. Please double-check whether bracket notation is intentional and decide whether to align the supported query serialization with OpenAPI, with coverage for defaults and explicit options. ########## apisix/plugins/openapi-to-mcp/tools/handler.lua: ########## @@ -0,0 +1,281 @@ +-- +-- 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 http = require("resty.http") +local json_pretty = require("apisix.plugins.openapi-to-mcp.json_pretty") +local pairs = pairs +local ipairs = ipairs +local type = type +local tostring = tostring +local str_lower = string.lower +local str_upper = string.upper +local str_gsub = string.gsub +local str_gmatch = string.gmatch +local str_find = string.find +local table_concat = table.concat +local table_sort = table.sort +local escape_uri = ngx.escape_uri + +local _M = {} + +local DEFAULT_TIMEOUT = 30000 +local NESTED_KEYS = { "pathParameters", "queryParameters", "headerParameters" } + + +local function path_param_names(template) + local names = {} + for name in str_gmatch(template, "{([^}]+)}") do + names[#names + 1] = name + end + return names +end + + +local function names_by_location(tool, location) + local names = {} + for _, param in ipairs(tool.parameters or {}) do + if type(param) == "table" and param["in"] == location then + names[#names + 1] = param.name + end + end + return names +end + + +local function pick(arguments, names) + local out = {} + for _, name in ipairs(names) do + if arguments[name] ~= nil then + out[name] = arguments[name] + end + end + return out +end + + +-- Whether the caller used the flat or the nested argument shape is decided by +-- the arguments themselves, not by the plugin's flatten_parameters setting. +-- A client that sends flat arguments to a nested tool therefore still works. +local function split_arguments(tool, arguments) + local nested = false + for _, key in ipairs(NESTED_KEYS) do + if arguments[key] ~= nil then + nested = true + break + end + end + + if nested then + return type(arguments.pathParameters) == "table" and arguments.pathParameters or {}, + type(arguments.queryParameters) == "table" and arguments.queryParameters or {}, + type(arguments.headerParameters) == "table" and arguments.headerParameters or {} + end + + return pick(arguments, path_param_names(tool.path_template)), + pick(arguments, names_by_location(tool, "query")), + pick(arguments, names_by_location(tool, "header")) +end + + +local function apply_query_defaults(tool, query) + for _, param in ipairs(tool.parameters or {}) do + if type(param) == "table" and param["in"] == "query" + and type(param.schema) == "table" + and param.schema.default ~= nil + and query[param.name] == nil + then + query[param.name] = param.schema.default + end + end +end + + +local function build_path(template, path_params) + local path = template + for name, value in pairs(path_params) do + -- the name is a literal, so escape any pattern magic in it + local pattern = "{" .. str_gsub(name, "([%^%$%(%)%%%.%[%]%*%+%-%?])", "%%%1") .. "}" + local escaped = escape_uri(tostring(value)) + -- The replacement has to be a function: percent-encoding produces "%2F" + -- and friends, and gsub would read those as capture references in a + -- string replacement and raise "invalid capture index". + path = str_gsub(path, pattern, function() + return escaped + end) + end + return path +end + + +-- Bracket notation, as the common JavaScript HTTP clients serialize it: an +-- array becomes "tags[]=a&tags[]=b" and an object "filter[a]=x", nested to any +-- depth. Dropping the brackets, or dropping objects entirely, would lose the +-- structure the schema advertised. +local function encode_param(name, value, parts) + if type(value) ~= "table" then + parts[#parts + 1] = escape_uri(name) .. "=" .. escape_uri(tostring(value)) + return + end + + if #value > 0 then + for _, item in ipairs(value) do + encode_param(name .. "[]", item, parts) + end + return + end + + -- Lua tables are unordered, so emit object members in sorted order to keep + -- the query string deterministic across workers. + local keys = {} + for key in pairs(value) do + keys[#keys + 1] = key + end + table_sort(keys) + for _, key in ipairs(keys) do + encode_param(name .. "[" .. tostring(key) .. "]", value[key], parts) + end +end + + +local function build_query(query) + local keys = {} + for key in pairs(query) do + keys[#keys + 1] = key + end + if #keys == 0 then + return nil + end + table_sort(keys) + + local parts = {} + for _, key in ipairs(keys) do + encode_param(key, query[key], parts) + end + if #parts == 0 then + return nil + end + return table_concat(parts, "&") +end + + +local function lower_headers(headers) + local out = {} + for key, value in pairs(headers or {}) do + out[str_lower(key)] = value + end + return out +end + + +-- Every response body goes through a JSON decode and falls back to the raw +-- string, without looking at the content type. +local function decode_body(body) + if body == nil or body == "" then + return body + end + local decoded = core.json.decode(body) + if decoded == nil then + return body + end + return decoded +end + + +local function text_result(payload, is_error) + local text, err = json_pretty.encode(payload) + if not text then + text = "failed to encode response: " .. tostring(err) + end + return { + content = { { type = "text", text = text } }, + isError = is_error or nil, + } +end + + +function _M.call(tool, arguments, opts) + arguments = type(arguments) == "table" and arguments or {} + + local path_params, query_params, header_params = split_arguments(tool, arguments) + apply_query_defaults(tool, query_params) + + local path = build_path(tool.path_template, path_params) + local query = build_query(query_params) + + local headers = {} + for key, value in pairs(opts.headers or {}) do + headers[key] = value + end + for key, value in pairs(header_params) do + headers[key] = tostring(value) + end + + local body = arguments.requestBody + if body ~= nil and type(body) ~= "string" then + body = core.json.encode(body) + headers["Content-Type"] = headers["Content-Type"] or "application/json" Review Comment: **Non-blocking: request media type** The generator records `tool.request_body_content_type`, but the handler does not use it when building the request. For a tool declaring `text/plain`, passing `requestBody = "hello"` skips this branch and sends no `Content-Type` unless it was separately configured. A backend requiring the declared media type may reject that request with 415. This does not block merging the PR. Please double-check the intended behavior and decide whether to default `Content-Type` from the tool's declared media type while respecting explicitly configured headers, with a non-JSON request-body regression test. -- 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]
