You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
api: follow up the admin sandbox list's node filters - #1405
This follows up the review of #1385, which added the node_id and assigned filters to the admin sandbox list (GET /admin/api/v1/sandboxes). It addresses the five low-severity points left after that pull request merged. Only one changes what SQL runs, and the list's results don't change.
One definition of "reported on a node". The unpaged list, which is used when there is no cursor, now filters assigned with Direct's reported_on_a_node predicate, as the paged statement already did. Before, it spelled the same rule a second way, as COALESCE(direct_route.current_node_id, s.active_node_id) IS NOT NULL over the joined route (crates/l40_api/api/src/controllers/sandbox/list_page/admin.rs). The two are equivalent because a route's current_node_id is NOT NULL and sandbox_id is its primary key. The shared predicate looks a filtered row's route up again by primary key. Measured, it plans no worse than the old spelling in any case.
A planner claim corrected.reported_on_node's doc (crates/l40_api/api/src/controllers/substrate/direct/reported_node.rs) said the planner always reaches a node's sandboxes through idx_sandboxes_active_node and the primary key. Measured, that holds only for a custom plan, and that plan uses the unique (id, org_id) index rather than the primary key. The admin statements wrap the predicate as $10::UUID IS NULL OR …. Under a generic plan the planner cannot drop that IS NULL arm, so it tests every row in list order. The comment now claims only that.
The 400 for a malformed filter, documented. The admin OpenAPI description now says a malformed parameter answers 400 with or without cursor, rather than being ignored. For example, a node_id that is a node name, or assigned=1, is refused. See Rollout.
Tests that could not fail.an_empty_filter_parameter_is_unset seeded only sandboxes with no node, so an assigned= read as false listed the same rows as no filter. It now seeds one sandbox on a node. The combined on-a-node case of each_filter_returns_exactly_its_rows sets created_after again. Its node filter and created_after each decide its result, so a statement that binds one placeholder in another's place lists other rows. In the_node_filters_match_the_node_the_response_reports, only the node_id cases compared the listed rows with the response's own active_node_id; the assigned and unassigned cases checked fixed seed indexes. Now every case does, for the paged and the unpaged statements: each listed row's response has (or lacks) a node, and no unlisted seeded row's does.
Validation
At this head: cargo fmt --all -- --check and cargo clippy -p pplx-space-api --all-targets --all-features -- -D warnings pass. cargo nextest run -p pplx-space-api --all-features runs 2188 tests: all pass, and one #[ignore] benchmark is skipped. The database tests ran against the repository's nextest Postgres and Redis fixtures. cargo arch, against main's baseline and architecture, reports the same 113 advisory findings as main, and none is required.
an_empty_filter_parameter_is_unset fails against a temporary change that reads assigned= as false, and the version on main passes against it.
With the response's active_node_id temporarily reduced to the row's column, ignoring the route, and the node_id cases removed, the assigned case of the_node_filters_match_the_node_the_response_reports fails. The previous version of the test passes against the same change.
The plan claim was measured with EXPLAIN (ANALYZE) of both admin statements under force_custom_plan and force_generic_plan. The fixture had 28,005 seeded sandboxes over 300 nodes, plus 2,003 Direct routes. Filtered by a node with five sandboxes, the custom plan read them through a BitmapOr of idx_sandboxes_active_node and sandboxes_id_org_id_key, in about 0.7 ms. The generic plan filtered all 28,005 rows: about 67 ms paged and 55 ms unpaged.
The same fixture compared the unpaged assigned filter with the old spelling. For assigned=false the new predicate takes 4.0 ms against 13.6 ms under a custom plan, and 0.7 s against 1.8 s under a generic plan. For assigned=true it takes 1.5 ms with either spelling, and 1.3 s against 1.8 s.
No new plan test was added, because the claim does not hold under a generic plan. No Kind or deployed run was made.
Severity and risk
Severity: low. These are test gaps, an inaccurate comment and a second spelling of one SQL rule. No result was wrong.
Risk: the unpaged list's assigned filter runs a different but equivalent predicate. Existing tests check both statements' assigned and unassigned results, including sandboxes whose route and row disagree. To roll back, revert the commit.
Rollout
No migration or ordering. The behavior change described here shipped in #1385, and this pull request only documents it. On the unpaged list (no cursor), node_id and assigned used to be unknown names, so the list ignored them. Since #1385, a non-empty but malformed value answers 400 instead, such as a node name for node_id or assigned=1. A caller that sent one used to get the whole list and now gets an error. That is intended: a filter that silently matched every sandbox would be worse. An empty value is still unset.
Not covered
Under a generic plan, the unpaged statement with an assigned filter is slow with either spelling: about 0.7–1.3 s on the fixture above. Nested loops over the response's joins run before the filter and LIMIT. This predates this change, and the change makes it smaller.
A node_id request also tests every row under a generic plan: on the fixture above, both statements filtered all 28,005 rows, in about 67 ms paged and 55 ms unpaged.
Both are tracked separately.
AI assistance: Claude Code drafted the code, the tests and this description.
No actionable comments were generated in the recent review.
Walkthrough
The admin list query now uses reported_on_a_node(s) for assigned and unassigned filtering. Tests check node, assigned, and unassigned filters across paged and unpaged results, including reported active_node_id states. Route tests seed assigned and unassigned sandboxes for empty-filter checks. Endpoint documentation describes malformed-filter responses with or without a cursor.
The admin list query uses reported_on_a_node(s) for assigned and unassigned filtering. Predicate documentation describes conditional index use and notes that a generic plan may test every row.
Paged and unpaged filter validation crates/l40_api/api/src/controllers/sandbox/list_page/admin/tests.rs, crates/l40_api/api/src/routes/admin/sandbox_list/tests.rs, crates/l40_api/api/src/routes/admin/docs.rs
Tests compare paged and unpaged filter results and reported node states. Route tests seed assigned and unassigned sandboxes. Endpoint documentation describes malformed-filter responses and gives invalid node_id and assigned examples.
Merge Risk: Minimal · up to a13f2
The filter rewrite preserves existing results, while the added tests and documentation improve coverage and clarity.
Pre-merge checks: 5
Check name
Status
Explanation
Title check
The title starts with the lowercase area "api:", uses a lowercase summary, and accurately describes the admin sandbox list node-filter changes. It has no trailing period, ticket ID, or internal link.
Description Follows The Pr Template
The PR description contains all required sections with real content: Summary explains the change, Validation lists commands and results, Severity and risk gives low severity and risk details, and a li…
Bug Fixes Name A Regression Test
PASS. The PR does not fix an application behavior defect: its only production SQL change replaces an equivalent assigned predicate with reported_on_a_node, while the other changes update documenta…
Weakened Safeguards Are Justified
No weakened safeguard is introduced. The diff changes no CI, lint, allowlist, baseline, or test configuration. The existing test functions remain. Test changes add an assert!, cover paged and unpage…
Public-Safe Content
PASS. The title, description, and reviewed diff contain no credentials, private URLs or trackers, internal hostnames, cloud IDs, real resource ARNs, cluster names, workstation paths, incident details,…
Full details: Description Follows The Pr Template
Explanation
The PR description contains all required sections with real content: Summary explains the change, Validation lists commands and results, Severity and risk gives low severity and risk details, and a line starting with "AI assistance:" identifies the drafted parts.
Full details: Bug Fixes Name A Regression Test
Explanation
PASS. The PR does not fix an application behavior defect: its only production SQL change replaces an equivalent assigned predicate with reported_on_a_node, while the other changes update documentation and tests. The Validation section also names an_empty_filter_parameter_is_unset and the_node_filters_match_the_node_the_response_reports as tests that fail under temporary regressions and pass with this change.
Full details: Weakened Safeguards Are Justified
Explanation
No weakened safeguard is introduced. The diff changes no CI, lint, allowlist, baseline, or test configuration. The existing test functions remain. Test changes add an assert!, cover paged and unpaged results, and seed distinct assigned rows. Updated expected values and fixtures have specific reasons in the pull request description.
Full details: Public-Safe Content
Explanation
PASS. The title, description, and reviewed diff contain no credentials, private URLs or trackers, internal hostnames, cloud IDs, real resource ARNs, cluster names, workstation paths, incident details, or undisclosed vulnerabilities. The description reports measurements from an explicitly seeded test fixture, not production logs, query output, or production measurements. The diff contains only repository-relative Rust changes and test fixtures.
Recent review info
Automatic approval policy
Commit: a13f2877d333.
Setting
Effective value
Source
Automatic approvals
Enabled
Global configuration
Merge risk
Low
Override 2
Security risk
Not set
Global configuration
Matching overrides (numbered as in the resolved configuration):
Any matching disabled override withholds approval, regardless of risk. Otherwise, the strictest explicitly set limit wins for each score. Limits omitted by all matching overrides inherit the global configuration.
Not set means no limit was configured after inheritance. Use @coderabbitai configuration to inspect the resolved settings and their sources.
The unpaged admin list now tests assigned with Direct's
reported_on_a_node predicate, as the paged statement does, so one
definition answers whether a response reports a node. Before, it spelled
the same rule as the joined route's COALESCE. Measured with forced plan
modes, the shared predicate plans no worse in any case.
reported_on_node's doc no longer says the planner always narrows through
idx_sandboxes_active_node and the primary key. A custom plan does, through
idx_sandboxes_active_node and the unique (id, org_id) index. A generic plan
of a statement that makes the filter optional cannot drop the IS NULL arm
and tests every row in list order.
The admin OpenAPI description now says a malformed parameter answers 400
with or without cursor, so a node_id that is not a UUID or an assigned
that is not true or false is refused rather than matching every sandbox.
Tests: an_empty_filter_parameter_is_unset seeds a sandbox on a node, so
assigned= read as false (or true) lists other rows than unset; before,
every seeded sandbox had no node and the assigned case could not fail.
The combined on-a-node case of each_filter_returns_exactly_its_rows sets
created_after again, and both its node filter and created_after decide
its result.
the_node_filters_match_the_node_the_response_reports checked its
assigned and unassigned cases only against fixed seed indexes. Only the
node_id cases compared the listed rows with the response's own
active_node_id. Now every case does, for both the paged and the unpaged
statements: every listed row's response has (or, for unassigned, lacks)
the node the filter asks for, and no unlisted seeded row's response does.
With the response's active_node_id reduced to the row's column, ignoring
the route, the assigned case now fails. Before, it passed.
Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 24 minutes.
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
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
This follows up the review of #1385, which added the
node_idandassignedfilters to the admin sandbox list (GET /admin/api/v1/sandboxes). It addresses the five low-severity points left after that pull request merged. Only one changes what SQL runs, and the list's results don't change.cursor, now filtersassignedwith Direct'sreported_on_a_nodepredicate, as the paged statement already did. Before, it spelled the same rule a second way, asCOALESCE(direct_route.current_node_id, s.active_node_id) IS NOT NULLover the joined route (crates/l40_api/api/src/controllers/sandbox/list_page/admin.rs). The two are equivalent because a route'scurrent_node_idisNOT NULLandsandbox_idis its primary key. The shared predicate looks a filtered row's route up again by primary key. Measured, it plans no worse than the old spelling in any case.reported_on_node's doc (crates/l40_api/api/src/controllers/substrate/direct/reported_node.rs) said the planner always reaches a node's sandboxes throughidx_sandboxes_active_nodeand the primary key. Measured, that holds only for a custom plan, and that plan uses the unique(id, org_id)index rather than the primary key. The admin statements wrap the predicate as$10::UUID IS NULL OR …. Under a generic plan the planner cannot drop thatIS NULLarm, so it tests every row in list order. The comment now claims only that.cursor, rather than being ignored. For example, anode_idthat is a node name, orassigned=1, is refused. See Rollout.an_empty_filter_parameter_is_unsetseeded only sandboxes with no node, so anassigned=read asfalselisted the same rows as no filter. It now seeds one sandbox on a node. The combined on-a-node case ofeach_filter_returns_exactly_its_rowssetscreated_afteragain. Its node filter andcreated_aftereach decide its result, so a statement that binds one placeholder in another's place lists other rows. Inthe_node_filters_match_the_node_the_response_reports, only thenode_idcases compared the listed rows with the response's ownactive_node_id; theassignedandunassignedcases checked fixed seed indexes. Now every case does, for the paged and the unpaged statements: each listed row's response has (or lacks) a node, and no unlisted seeded row's does.Validation
At this head:
cargo fmt --all -- --checkandcargo clippy -p pplx-space-api --all-targets --all-features -- -D warningspass.cargo nextest run -p pplx-space-api --all-featuresruns 2188 tests: all pass, and one#[ignore]benchmark is skipped. The database tests ran against the repository's nextest Postgres and Redis fixtures.cargo arch, againstmain's baseline and architecture, reports the same 113 advisory findings asmain, and none is required.an_empty_filter_parameter_is_unsetfails against a temporary change that readsassigned=asfalse, and the version onmainpasses against it.active_node_idtemporarily reduced to the row's column, ignoring the route, and thenode_idcases removed, theassignedcase ofthe_node_filters_match_the_node_the_response_reportsfails. The previous version of the test passes against the same change.EXPLAIN (ANALYZE)of both admin statements underforce_custom_planandforce_generic_plan. The fixture had 28,005 seeded sandboxes over 300 nodes, plus 2,003 Direct routes. Filtered by a node with five sandboxes, the custom plan read them through aBitmapOrofidx_sandboxes_active_nodeandsandboxes_id_org_id_key, in about 0.7 ms. The generic plan filtered all 28,005 rows: about 67 ms paged and 55 ms unpaged.assignedfilter with the old spelling. Forassigned=falsethe new predicate takes 4.0 ms against 13.6 ms under a custom plan, and 0.7 s against 1.8 s under a generic plan. Forassigned=trueit takes 1.5 ms with either spelling, and 1.3 s against 1.8 s.No new plan test was added, because the claim does not hold under a generic plan. No Kind or deployed run was made.
Severity and risk
Severity: low. These are test gaps, an inaccurate comment and a second spelling of one SQL rule. No result was wrong.
Risk: the unpaged list's
assignedfilter runs a different but equivalent predicate. Existing tests check both statements'assignedandunassignedresults, including sandboxes whose route and row disagree. To roll back, revert the commit.Rollout
No migration or ordering. The behavior change described here shipped in #1385, and this pull request only documents it. On the unpaged list (no
cursor),node_idandassignedused to be unknown names, so the list ignored them. Since #1385, a non-empty but malformed value answers 400 instead, such as a node name fornode_idorassigned=1. A caller that sent one used to get the whole list and now gets an error. That is intended: a filter that silently matched every sandbox would be worse. An empty value is still unset.Not covered
assignedfilter is slow with either spelling: about 0.7–1.3 s on the fixture above. Nested loops over the response's joins run before the filter andLIMIT. This predates this change, and the change makes it smaller.node_idrequest also tests every row under a generic plan: on the fixture above, both statements filtered all 28,005 rows, in about 67 ms paged and 55 ms unpaged.Both are tracked separately.
AI assistance: Claude Code drafted the code, the tests and this description.