bzp2010 commented on code in PR #12718:
URL: https://github.com/apache/apisix/pull/12718#discussion_r2548436344
##########
t/admin/standalone.spec.ts:
##########
@@ -194,6 +194,86 @@ describe('Admin - Standalone', () => {
});
describe('Normal', () => {
+ it('validate config (success case with json)', async () => {
+ const resp = await client.post(`${ENDPOINT}/validate`, config1);
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
+ valid: true,
+ });
+ });
+
+ it('validate config (success case with yaml)', async () => {
+ const resp = await client.post(`${ENDPOINT}/validate`,
YAML.stringify(config1), {
+ headers: { 'Content-Type': 'application/yaml' },
+ });
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
+ valid: true,
+ });
+ });
+
+ it('validate config (success case with multiple resources)', async () => {
+ const multiResourceConfig = {
+ routes: [
+ {
+ id: 'r1',
+ uri: '/r1',
+ upstream: {
+ nodes: { '127.0.0.1:1980': 1 },
+ type: 'roundrobin',
+ },
+ },
+ {
+ id: 'r2',
+ uri: '/r2',
+ upstream: {
+ nodes: { '127.0.0.1:1980': 1 },
+ type: 'roundrobin',
+ },
+ },
+ ],
+ services: [
+ {
+ id: 's1',
+ upstream: {
+ nodes: { '127.0.0.1:1980': 1 },
+ type: 'roundrobin',
+ },
+ },
+ ],
+ routes_conf_version: 1,
+ services_conf_version: 1,
+ };
+
+ const resp = await client.post(`${ENDPOINT}/validate`,
multiResourceConfig);
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
+ valid: true,
+ });
+ });
+
+ it('validate config with consumer credentials', async () => {
+ const resp = await client.post(`${ENDPOINT}/validate`, credential1);
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
+ valid: true,
+ });
+ });
+
+ it('validate config does not persist changes', async () => {
+ // First validate a configuration
+ const validateResp = await client.post(`${ENDPOINT}/validate`, config1);
+ expect(validateResp.status).toEqual(200);
+
+ // Then check that the configuration was not persisted
+ const getResp = await client.get(ENDPOINT);
+ expect(getResp.data.routes).toBeUndefined();
+ });
Review Comment:
I recommend moving these validate API tests to a separate describe block,
which will help achieve a clearer separation.
##########
t/admin/standalone.spec.ts:
##########
@@ -641,5 +721,228 @@ describe('Admin - Standalone', () => {
'invalid routes at index 0, err: invalid configuration: failed to
match pattern
"^((uri|server_name|server_addr|request_uri|remote_port|remote_addr|query_string|host|hostname|mqtt_client_id)|arg_[0-9a-zA-z_-]+)$"
with "args_invalid"',
});
});
+ it('validate config (duplicate route id)', async () => {
Review Comment:
What is the purpose of these newly added tests? Did we previously have
duplicate ID checking functionality without test coverage?
##########
apisix/admin/standalone.lua:
##########
@@ -158,6 +157,120 @@ local function check_conf(checker, schema, item, typ)
end
+local function validate_configuration(req_body, collect_all_errors)
+ local validation_results = {
+ valid = true,
+ errors = {}
+ }
+
+ for key, conf_version_key in pairs(ALL_RESOURCE_KEYS) do
+ local items = req_body[key]
+ local resource = resources[key] or {}
+
+ -- Validate conf_version_key if present
+ local new_conf_version = req_body[conf_version_key]
+ if new_conf_version and type(new_conf_version) ~= "number" then
+ if not collect_all_errors then
+ return false, conf_version_key .. " must be a number"
+ end
+ validation_results.valid = false
+ table_insert(validation_results.errors, {
+ resource_type = key,
+ error = conf_version_key .. " must be a number, got " ..
type(new_conf_version)
+ })
+ end
+
+ if items and #items > 0 then
+ local item_schema = resource.schema
+ local item_checker = resource.checker
+ local id_set = {}
+
+ for index, item in ipairs(items) do
+ local item_temp = tbl_deepcopy(item)
+ local valid, err = check_conf(item_checker, item_schema,
item_temp, key)
+ if not valid then
+ local err_prefix = "invalid " .. key .. " at index " ..
(index - 1) .. ", err: "
+ local err_msg = type(err) == "table" and err.error_msg or
err
+ local error_msg = err_prefix .. err_msg
+
+ if not collect_all_errors then
+ return false, error_msg
+ end
+ validation_results.valid = false
+ table_insert(validation_results.errors, {
+ resource_type = key,
+ index = index - 1,
+ error = error_msg
+ })
+ end
+
+ -- check for duplicate IDs
+ local duplicated, dup_err = check_duplicate(item, key, id_set)
+ if duplicated then
+ if not collect_all_errors then
+ return false, dup_err
+ end
+ validation_results.valid = false
+ table_insert(validation_results.errors, {
+ resource_type = key,
+ index = index - 1,
+ error = dup_err
+ })
+ end
+ end
+ end
+ end
+
+ if collect_all_errors then
+ return validation_results.valid, validation_results
+ else
+ return validation_results.valid, nil
+ end
+end
+
+local function validate(ctx)
+ local content_type = core.request.header(nil, "content-type") or
"application/json"
+ local req_body, err = core.request.get_body()
+ if err then
+ return core.response.exit(400, {error_msg = "invalid request body: "
.. err})
+ end
+
+ if not req_body or #req_body <= 0 then
+ return core.response.exit(400, {error_msg = "invalid request body:
empty request body"})
+ end
+
+ local data
+ if core.string.has_prefix(content_type, "application/yaml") then
+ local ok, result = pcall(yaml.load, req_body, { all = false })
+ if not ok or type(result) ~= "table" then
+ err = "invalid yaml request body"
+ else
+ data = result
+ end
+ else
+ data, err = core.json.decode(req_body)
+ end
+
+ if err then
+ core.log.error("invalid request body: ", req_body, " err: ", err)
+ return core.response.exit(400, {error_msg = "invalid request body: "
.. err})
+ end
+
+ local valid, validation_results = validate_configuration(data, true)
+ if not valid then
+ return core.response.exit(400, {
+ error_msg = "Configuration validation failed",
+ valid = false,
+ errors = validation_results.errors
+ })
+ end
+
+ return core.response.exit(200, {
+ message = "Configuration is valid",
+ valid = true
+ })
Review Comment:
When the check passes, neither the `message` nor the `valid` field carries
any meaning; an HTTP 200 response is sufficient to imply this outcome. In
practice, neither of these return fields will be utilised anywhere. **Remove
them.**
When the check fails, the `valid` field similarly holds no significance. An
HTTP 400 response can indicate this error. **Remove it.** The `error_msg`
should maintain a consistent style, **using lowercase initial letters.**
##########
t/admin/standalone.spec.ts:
##########
@@ -194,6 +194,86 @@ describe('Admin - Standalone', () => {
});
describe('Normal', () => {
+ it('validate config (success case with json)', async () => {
+ const resp = await client.post(`${ENDPOINT}/validate`, config1);
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
+ valid: true,
+ });
+ });
+
+ it('validate config (success case with yaml)', async () => {
+ const resp = await client.post(`${ENDPOINT}/validate`,
YAML.stringify(config1), {
+ headers: { 'Content-Type': 'application/yaml' },
+ });
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
+ valid: true,
+ });
+ });
+
+ it('validate config (success case with multiple resources)', async () => {
+ const multiResourceConfig = {
+ routes: [
+ {
+ id: 'r1',
+ uri: '/r1',
+ upstream: {
+ nodes: { '127.0.0.1:1980': 1 },
+ type: 'roundrobin',
+ },
+ },
+ {
+ id: 'r2',
+ uri: '/r2',
+ upstream: {
+ nodes: { '127.0.0.1:1980': 1 },
+ type: 'roundrobin',
+ },
+ },
+ ],
+ services: [
+ {
+ id: 's1',
+ upstream: {
+ nodes: { '127.0.0.1:1980': 1 },
+ type: 'roundrobin',
+ },
+ },
+ ],
+ routes_conf_version: 1,
+ services_conf_version: 1,
+ };
+
+ const resp = await client.post(`${ENDPOINT}/validate`,
multiResourceConfig);
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
+ valid: true,
+ });
+ });
+
+ it('validate config with consumer credentials', async () => {
+ const resp = await client.post(`${ENDPOINT}/validate`, credential1);
+ expect(resp.status).toEqual(200);
+ expect(resp.data).toEqual({
+ message: 'Configuration is valid',
Review Comment:
This `message` does not need assertion, as checking such constant strings is
meaningless. You may switch to the `toMatchObject` API to assert fields other
than `message`. Only fields expected to exist in the configuration will be
checked.
If no further assertion of the response is required, then only assert the
response status code.
--
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]
