feat: add validate API to standalone mode #12718
Conversation
dosubot
bot
added
size:L
Revolyssup
requested review from
bzp2010,
membphis and
nic-6443
and removed request for
bzp2010
apisix/admin/standalone.lua
Outdated
| }) | ||
| end | ||
|
|
||
| local function validate(ctx) |
There was a problem hiding this comment.
can we reuse this new validate function in exist update function to reduce code duplicate between those two functions?
dosubot
bot
added
size:XL
AlinsRan
force-pushed
the
revolyssup/standalone-validate
branch
from
8f48941 to
b0be53b
Compare
|
The CI failure is not related to the current PR modification. |
membphis
left a comment
membphis
left a comment
There was a problem hiding this comment.
LGTM, only some minor code style issues
| end | ||
| end | ||
|
|
||
| if collect_all_errors then |
There was a problem hiding this comment.
code style
if collect_all_errors then
return validation_results.valid, validation_results
end
return validation_results.valid
apisix/admin/standalone.lua
Outdated
|
|
||
| local valid, validation_results = validate_configuration(data, true) | ||
|
|
||
| if valid then |
There was a problem hiding this comment.
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
})
AlinsRan
force-pushed
the
revolyssup/standalone-validate
branch
from
1849cb3 to
7355892
Compare
| error_msg = "Configuration validation failed", | ||
| valid = false, | ||
| errors = validation_results.errors | ||
| }) | ||
| end | ||
|
|
||
| return core.response.exit(200, { | ||
| message = "Configuration is valid", | ||
| valid = true | ||
| }) |
There was a problem hiding this 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.
| 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(); | ||
| }); |
There was a problem hiding this comment.
I recommend moving these validate API tests to a separate describe block, which will help achieve a clearer separation.
| const resp = await client.post(`${ENDPOINT}/validate`, credential1); | ||
| expect(resp.status).toEqual(200); | ||
| expect(resp.data).toEqual({ | ||
| message: 'Configuration is valid', |
There was a problem hiding this 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.
| '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 () => { |
There was a problem hiding this comment.
What is the purpose of these newly added tests? Did we previously have duplicate ID checking functionality without test coverage?
5 participants
Provide a validate API for standalone mode that only validate input like admin api but does not save input to memory.
Required for ingress controller to validate plugin configurations that convert from CRD.
Checklist