Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds a server-side validation flow: a new ChangesServer-Side Validation Feature
Sequence Diagram(s)sequenceDiagram
participant Client
participant ADCServer
participant Differ
participant Backend
participant Validator
Client->>ADCServer: PUT /validate (ValidateInput)
ADCServer->>ADCServer: validate input schema, lint, fill labels
ADCServer->>Differ: DifferV3.diff(local, {} , backend.defaultValue())
Differ-->>ADCServer: events[]
ADCServer->>Backend: backend.validate(events)
Backend-->>Validator: translate & send validate request (body + eventIndex)
Validator-->>Backend: validation response (errors or success)
Backend-->>ADCServer: BackendValidateResult (errors augmented with event)
ADCServer-->>Client: 200 { success, errors }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
apps/cli/e2e/server/basic.e2e-spec.ts (1)
267-353: ⚡ Quick winAdd a failing backend-validation case.
The new tests cover parse/lint failures, but they never exercise the core branch where
backend.validate()returns{ success: false, errorMessage, errors }. Without that, the response contract described in this PR is still unpinned.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/e2e/server/basic.e2e-spec.ts` around lines 267 - 353, Add a new e2e test "test validate with backend failure" that exercises the code path where backend.validate() returns { success: false, errorMessage, errors }: use server.TEST_ONLY_getExpress() to PUT '/validate' with task.opts pointing to a test backend that is stubbed to return that failing response (or mock the backend factory/create to return an object whose validate method returns the failing payload), send a valid config, then assert the response status and body indicate failure (expect body.success toEqual false and that body.errorMessage and body.errors match the values returned by the mocked backend.validate).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/e2e/server/sync-standalone.e2e-spec.ts`:
- Around line 6-17: The test suite only allocates fixtures in beforeAll and
never runs a test; add at least one it(...) case that exercises the standalone
sync flow (e.g., start server, call a basic endpoint or verify server.listen
state) and add an afterAll teardown to stop/cleanup the fixtures. Locate the
beforeAll block that constructs BackendAPISIXStandalone and ADCServer and
implement an it('runs standalone sync flow', async () => { ... }) that uses
those instances, then implement afterAll to call the appropriate
shutdown/close/stop methods on BackendAPISIXStandalone and ADCServer to release
resources.
- Around line 10-12: The test instantiates BackendAPISIXStandalone without the
required ADCSDK.BackendOptions causing TS2554 and runtime crashes; update the
beforeAll instantiation of BackendAPISIXStandalone to pass a proper opts object
containing the mandatory fields (server, token, cacheKey, httpAgent, httpsAgent)
— e.g., construct an options object (or reuse existing helpers) with a test
server URL for server, a valid token string, a cacheKey string, and valid
http/https agent instances, then call new BackendAPISIXStandalone(opts) so the
constructor has the required data and accessing opts.server no longer throws.
In `@apps/cli/src/server/schema.ts`:
- Around line 24-33: The opts schema uses z.looseObject so unknown keys like
tlsSkipVerify can slip through as strings; update the opts definition in
schema.ts (the z.looseObject({ ... }) block for opts) to include a typed
tlsSkipVerify field (e.g., tlsSkipVerify: z.boolean().optional().default(false))
so that task.opts.tlsSkipVerify in validate.ts is guaranteed to be a boolean
before branching on it. Ensure you reference the existing opts entry and add the
tlsSkipVerify key alongside backend, server, token, etc., rather than relying on
looseObject to accept it implicitly.
In `@apps/cli/src/server/validate.ts`:
- Around line 77-94: The AXIOS_DEBUG event handler is currently logging raw
request/response headers and bodies (see the backend.on('AXIOS_DEBUG'...)
handler and similar logging around the other backend debug handler), which can
leak Authorization, Cookie, tokens (Bearer, JWT, access_token, refresh_token),
client_secret, and plugin config; update these handlers to redact sensitive
fields before logging or returning them by stripping or masking sensitive header
keys (e.g., Authorization, Cookie, Set-Cookie), scanning and masking token-like
fields in request/response bodies and config (access_token, refresh_token,
client_secret, secret, password), and replace raw error objects returned to
clients with a safe generic message; ensure you perform masking in the
AXIOS_DEBUG callback where response.config.* and response.* are read and in the
other debug handler referenced in the comment so logs and any 500 responses
never include unredacted secrets.
---
Nitpick comments:
In `@apps/cli/e2e/server/basic.e2e-spec.ts`:
- Around line 267-353: Add a new e2e test "test validate with backend failure"
that exercises the code path where backend.validate() returns { success: false,
errorMessage, errors }: use server.TEST_ONLY_getExpress() to PUT '/validate'
with task.opts pointing to a test backend that is stubbed to return that failing
response (or mock the backend factory/create to return an object whose validate
method returns the failing payload), send a valid config, then assert the
response status and body indicate failure (expect body.success toEqual false and
that body.errorMessage and body.errors match the values returned by the mocked
backend.validate).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c67ae1fe-1550-4bff-8552-e3d30070ea12
📒 Files selected for processing (6)
apps/cli/e2e/server/basic.e2e-spec.tsapps/cli/e2e/server/sync-standalone.e2e-spec.tsapps/cli/e2e/support/utils.tsapps/cli/src/server/index.tsapps/cli/src/server/schema.tsapps/cli/src/server/validate.ts
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/cli/e2e/server/validate.e2e-spec.ts (1)
18-109: ⚡ Quick winAdd one e2e for backend validation failure contract.
Current tests miss the key
/validatefailure shape from backend (success: false,source: 'validate',message, mappederrors). Adding that case would lock the new API behavior and prevent regressions.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/cli/e2e/server/validate.e2e-spec.ts` around lines 18 - 109, Add an E2E that asserts the server returns the backend validation-failure contract (success: false, source: 'validate', a top-level message, and mapped errors) for the /validate endpoint; copy the style of existing tests in validate.e2e-spec.ts and add a new it(...) that PUTs to '/validate' with task.opts including backend: 'mock', server/token/cacheKey and a config payload designed to trigger the mock backend validation failure, then assert status (likely 200 or the expected code), body.success === false, body.source === 'validate', body.message is present, and body.errors contains the mapped errors.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/cli/src/server/validate.ts`:
- Around line 124-137: The code is directly logging and returning result.errors
(from validate handler) which may contain sensitive payloads via
BackendValidationError.event; update the logging and response to sanitize errors
before exposing them: introduce/use a sanitizer that maps each error in
result.errors to a safe shape (e.g., { message, code, field } only) and replace
references to result.errors in logger.log (function logger.log) and in
res.status(200).json (response returned from the validate handler) with the
sanitizedErrors array; ensure BackendValidationError instances are handled
specially by extracting non-sensitive fields only and never including the raw
event/payload.
---
Nitpick comments:
In `@apps/cli/e2e/server/validate.e2e-spec.ts`:
- Around line 18-109: Add an E2E that asserts the server returns the backend
validation-failure contract (success: false, source: 'validate', a top-level
message, and mapped errors) for the /validate endpoint; copy the style of
existing tests in validate.e2e-spec.ts and add a new it(...) that PUTs to
'/validate' with task.opts including backend: 'mock', server/token/cacheKey and
a config payload designed to trigger the mock backend validation failure, then
assert status (likely 200 or the expected code), body.success === false,
body.source === 'validate', body.message is present, and body.errors contains
the mapped errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5bda7bd-8d30-43af-b227-ff9d7a8dbc0d
📒 Files selected for processing (2)
apps/cli/e2e/server/validate.e2e-spec.tsapps/cli/src/server/validate.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
libs/backend-apisix/test/validator.spec.ts (1)
33-235: ⚡ Quick winAdd explicit test for the 404 “validate unsupported” branch
The validator has a dedicated 404 path with a specific upgrade message; this file currently doesn’t assert that behavior. Adding it will lock down a key compatibility UX path.
Proposed test addition
describe('Validator', () => { + it('should throw upgrade hint when validate endpoint is unsupported (404)', async () => { + const events = [createEvent(ADCSDK.ResourceType.SERVICE, 'httpbin.org')]; + const client = axios.create(); + const err = new AxiosError( + 'Request failed with status code 404', + AxiosError.ERR_BAD_REQUEST, + ); + err.response = { + status: 404, + statusText: 'Not Found', + headers: {}, + data: {}, + config: {} as any, + }; + vi.spyOn(client, 'post').mockRejectedValue(err); + + const validator = new Validator({ + client, + eventSubject: new Subject<ADCSDK.BackendEvent>(), + }); + + await expect(validator.validate(events)).rejects.toThrow( + 'Validate is not supported by the current APISIX version. Please upgrade to a newer version.', + ); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@libs/backend-apisix/test/validator.spec.ts` around lines 33 - 235, Add a new unit test in validator.spec.ts that exercises the Validator.validate 404 branch: mock the axios client.post used by the Validator instance to reject with an Axios error whose response.status === 404 and response.data contains the validator’s "upgrade/unsupported" payload, call new Validator({ client, eventSubject: new Subject() }).validate(events), and assert the returned result indicates failure and that result.errorMessage matches the validator’s 404 upgrade message (use the same string/constant defined in Validator or compare using includes if the constant isn’t exported); ensure the test uses createEvent to build events and follows the pattern of other tests for setup and expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@libs/backend-apisix/test/validator.spec.ts`:
- Around line 33-235: Add a new unit test in validator.spec.ts that exercises
the Validator.validate 404 branch: mock the axios client.post used by the
Validator instance to reject with an Axios error whose response.status === 404
and response.data contains the validator’s "upgrade/unsupported" payload, call
new Validator({ client, eventSubject: new Subject() }).validate(events), and
assert the returned result indicates failure and that result.errorMessage
matches the validator’s 404 upgrade message (use the same string/constant
defined in Validator or compare using includes if the constant isn’t exported);
ensure the test uses createEvent to build events and follows the pattern of
other tests for setup and expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3e8bd189-6de6-4c0b-a474-e554b6a91399
📒 Files selected for processing (2)
apps/cli/src/server/sync.tslibs/backend-apisix/test/validator.spec.ts
💤 Files with no reviewable changes (1)
- apps/cli/src/server/sync.ts
guoqqqi
approved these changes
May 6, 2026
AlinsRan
added a commit
to AlinsRan/apisix-ingress-controller
that referenced
this pull request
May 7, 2026
Previously the apisix-standalone backend bypassed the ADC server and called APISIX's /apisix/admin/configs/validate directly. With api7/adc#440, the ADC server now exposes a /validate endpoint (same input format as /sync) that handles both apisix-standalone and apisix backends uniformly. Changes: - Remove apisix-standalone special-case in runHTTPValidateForSingleServer; all backends now call ADC server POST /validate - Fix ADCValidateResult.ErrorMessage JSON tag: errorMessage -> message to match the ADC server response format from api7/adc#440 - Remove buildAPISIXValidateRequest, apisixValidateRequest, newBackendHTTPClient, buildAPISIXValidatePayload and helpers - Update unit tests accordingly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AlinsRan
added a commit
to api7/api7-ingress-controller
that referenced
this pull request
May 7, 2026
Previously the apisix-standalone backend bypassed the ADC server and called APISIX's /apisix/admin/configs/validate directly. With api7/adc#440, the ADC server now exposes a /validate endpoint (same input format as /sync) that handles both apisix-standalone and apisix backends uniformly. Changes: - Remove apisix-standalone special-case in runHTTPValidateForSingleServer; all backends now call ADC server POST /validate - Fix ADCValidateResult.ErrorMessage JSON tag: errorMessage -> message to match the ADC server response format from api7/adc#440 - Remove buildAPISIXValidateRequest, apisixValidateRequest, newBackendHTTPClient, buildAPISIXValidatePayload and helpers - Update unit tests accordingly Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Add the validate API to help improve the pre-check warning experience in AIC.
The input format for this new API is the same as that of
/sync.The output structure is:
Checklist
Summary by CodeRabbit
New Features
Tests
Chores