Refactor remote policies to bind storage targets#380
Merged
Conversation
- Add `remote_storage_target_key` field to storage policy form, entity, and API DTOs (`CreatePolicyReq`, `PatchPolicyReq`, `TestPolicyParamsReq`, `ExecuteDraftStoragePolicyActionReq`) - Add DB migration to add `remote_storage_target_key` column and `idx_storage_policies_remote_target` index to `storage_policies` table - Validate target key on create/update: verify the target exists on the remote node, is applied, and has no errors; reject non-blank keys on non-remote policies - Auto-select default target when a remote node is chosen and no valid target key is already set; clear target key when the node changes - Add `resolve_target_by_key` service function; route internal/presigned requests through the policy-specified target instead of always falling back to the binding default - Propagate `target_key` query parameter through `RemoteStorageClient` to all object, capacity, compose, and list paths - Add `RemoteNodeField` secondary select for remote storage target in the policy dialog; show loading, empty, error, and hint states; reset selection on node change - Rename `createRemoteNodeError` to `createRemoteTargetError` throughout the wizard and edit form to cover both node and target validation - Add i18n keys for target field labels, placeholder, loading, empty, error, and hint in `en` and `zh` locales - Add integration test `test_remote_policy_uses_selected_remote_storage_target_key` verifying writes land on the selected target and not the binding default
Add inline remote storage target management to policy creation and edit flows: - Display read-only collapsible target list in policy dialogs when remote node is selected - Support quick target creation directly from policy forms without leaving the dialog - Remove max_file_size field from target creation form (moved to backend defaults) - Remove revision display from target cards (internal detail) - Add new i18n keys for policy-scoped target management UI - Thread remoteStorageTargetDriverDescriptors and onCreateRemoteStorageTarget through dialog component tree - Refactor RemoteNodeRemoteStorageTargetSection to support read-only mode with optional creation - Add AnimatedCollapsible wrapper for target list in read-only mode with viewport height constraint - Update test fixtures and mocks to cover new creation flow and driver descriptor loading - Extend AdminPoliciesPage state to load driver descriptors and handle target creation for selected node
… level - Drop `max_file_size` column from `remote_storage_targets` table via new migration `m20260705_000001_drop_remote_storage_target_max_file_size` with rollback support - Remove `max_file_size` field from `RemoteStorageTarget` entity, API models, and all create/update request structs - Read `max_file_size` from a signed query parameter (`max_file_size`) on incoming requests instead of the target record, sourcing the value from the storage policy - Propagate `policy_max_file_size` through `resolve_authorized_ingress` for internal, presigned PUT, and presigned GET request paths - Attach `max_file_size` query param to PUT, streaming PUT, presigned PUT, and compose request URLs via `append_policy_max_file_size` in `RemoteStorageClient` - Rename `with_storage_target_key` to `with_policy_context` and add `max_file_size` parameter; clamp negative values to zero - Remove `max_file_size` driver field descriptor from Local and S3 connectors and delete the `remote_storage_target_number_field` helper - Strip `max_file_size` from frontend form data, payload builders, and all related tests
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthrough本次改动把远程存储目标从目标级 Changes后端：迁移、服务、协议与测试
Estimated code review effort: 5 (Critical) | ~120 minutes 前端：策略选择、创建和只读展示
Estimated code review effort: 5 (Critical) | ~90 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/storage/remote_protocol/tests.rs (1)
775-780: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
tests/test_remote_storage.rs里的旧字段也要一起删掉
RemoteCreateLocalStorageTargetRequest现在只剩name/base_path/is_default，但tests/test_remote_storage.rs里还有多处max_file_size构造没清。这个文件会直接编译不过，把这些旧字段一并去掉。🤖 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 `@src/storage/remote_protocol/tests.rs` around lines 775 - 780, `RemoteCreateLocalStorageTargetRequest` no longer has `max_file_size`, so update the affected test setup in `tests/test_remote_storage.rs` to use only `name`, `base_path`, and `is_default`. Remove every leftover `max_file_size` initialization in the request constructors and any assertions tied to it so the test module compiles cleanly with the current `RemoteCreateLocalStorageTargetRequest` shape.src/storage/connectors/tests.rs (1)
77-93: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win夹具补字段没问题，但
reject_unexpected_remote_storage_target_key这条分支还没用例。直接在src/storage/connectors/tests.rs基于draft_connection加个非 remote driver + 非空remote_storage_target_key的断言，确认会回PolicyRemoteNodeUnexpected。🤖 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 `@src/storage/connectors/tests.rs` around lines 77 - 93, Add a test case in tests.rs using draft_connection to cover reject_unexpected_remote_storage_target_key: build a non-remote driver connection with a non-empty remote_storage_target_key and assert validation returns PolicyRemoteNodeUnexpected. Reuse draft_connection and the StorageConnectorConnectionInput / DriverType setup so the new case sits alongside the existing connector policy tests and exercises the unexpected remote_storage_target_key branch directly.Source: Coding guidelines
frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx (1)
224-307: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win非只读分支的创建按钮没吃到
canCreateTargets这道闸。只读分支（259-264行）老老实实检查了
!canCreateTargets，非只读分支（298-302行）却漏了——按钮亮着能点，点了也是白点，因为startCreate()内部会因为canCreateTargets为 false 直接返回。现在没人踩坑是因为RemoteNodeEditForm目前必传onCreateTarget，但这变量是你自己这次 diff 新加的，加了就该两处都用上，不然就是埋了颗迟早要绊人的钉子。🔧 补上这道闸
disabled={ loading || Boolean(errorMessage) || - firstSupportedDriverType == null + firstSupportedDriverType == null || + !canCreateTargets }🤖 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 `@frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx` around lines 224 - 307, The non-read-only create button in RemoteNodeRemoteStorageTargetSection is missing the canCreateTargets guard, so it can appear enabled even when startCreate will immediately return; update the create button disabled logic in the activeDraftMode == null branch to match the readOnly branch and include canCreateTargets alongside loading, errorMessage, and firstSupportedDriverType checks, using the existing startCreate and canCreateTargets symbols to locate the fix.frontend-panel/src/components/admin/StoragePolicyDialog.tsx (1)
296-310: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win把远程节点缺失和远程目标缺失拆开，别让编辑态一打开就报红
createRemoteTargetError现在把“没选 remote node”和“没选 remote target”两种语义硬塞进一个变量里；更要命的是编辑模式没有createStepTouched门槛，只要历史策略的remote_storage_target_key为空，RemoteNodeField就会直接亮错。拆成两个错误，或者给编辑态补一层明确的触发条件。🤖 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 `@frontend-panel/src/components/admin/StoragePolicyDialog.tsx` around lines 296 - 310, The remote node missing state and remote storage target missing state are conflated in the StoragePolicyDialog error logic, causing edit mode to show an error immediately when historical data lacks remote_storage_target_key. Update the createRemoteNodeError and createRemoteTargetError handling in StoragePolicyDialog so each validation has its own explicit condition, and ensure the RemoteNodeField only shows the target error after an appropriate user interaction trigger in both create and edit modes.
🧹 Nitpick comments (5)
migration/src/m20260704_000002_add_remote_storage_target_key_to_storage_policies.rs (1)
45-64: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuedown() 少了对称的幂等守卫
up()特意用has_column判断是否已存在该列才add_column，摆明是防着重复执行或状态不一致的情况。可down()里drop_column直接无脑执行，没有对应的has_column判断兜底。同一个 PR 里的m20260705_000001_drop_remote_storage_target_max_file_size.rs两侧（up/down）都做了对称守卫，这里风格不统一，回滚重放时容易在列已经被删过的情况下直接炸掉。小子，既然你都想到 up 要防重放了，down 也别偷懒。
🔧 建议补齐对称守卫
async fn down(&self, manager: &SchemaManager) -> Result<(), DbErr> { manager .drop_index( Index::drop() .name("idx_storage_policies_remote_target") .table(StoragePolicies::Table) .if_exists() .to_owned(), ) .await?; - manager - .alter_table( - Table::alter() - .table(StoragePolicies::Table) - .drop_column(StoragePolicies::RemoteStorageTargetKey) - .to_owned(), - ) - .await + if manager + .has_column( + StoragePolicies::Table.to_string(), + StoragePolicies::RemoteStorageTargetKey.to_string(), + ) + .await? + { + manager + .alter_table( + Table::alter() + .table(StoragePolicies::Table) + .drop_column(StoragePolicies::RemoteStorageTargetKey) + .to_owned(), + ) + .await?; + } + Ok(()) }🤖 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 `@migration/src/m20260704_000002_add_remote_storage_target_key_to_storage_policies.rs` around lines 45 - 64, The down() implementation in the migration is missing a symmetric idempotency guard for dropping the remote storage target key column. Update the down path in the migration struct that defines up()/down() so it checks has_column before calling alter_table(...).drop_column(...) on StoragePolicies::RemoteStorageTargetKey, matching the defensive pattern already used in up() and the sibling migration’s up/down symmetry.src/services/remote_storage_target_service/tests.rs (1)
756-808: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winresolve_target_by_key 没有专门的单测
这个文件里
resolve_effective_target的四种状态（required / default_missing / error / pending）都测得挺全，但target.rs新增的resolve_target_by_key——也就是策略显式指定target_key时真正决定文件落到哪个远程目标的函数——在这里一个直接测试都没有，只能靠间接推断它复用了build_resolved_target就当它没问题。它可是这次重构的核心路由逻辑，建了错目标可不是小事。补一组类似
resolve_effective_target_reports_required_default_and_pending_states的用例（key 不存在 / target 有 last_error / target 处于 pending revision），成本不高，但能防住以后有人手滑改坏路由。别指望我天天替你盯着这种坑，你自己补上。
🤖 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 `@src/services/remote_storage_target_service/tests.rs` around lines 756 - 808, Add direct unit coverage for resolve_target_by_key in the target resolution tests, since it currently has no explicit assertions and is only indirectly exercised through build_resolved_target. Create cases around the existing resolve_target_by_key flow for a missing target_key, a target with last_error set, and a target in pending revision state (using the same setup patterns as resolve_effective_target_reports_required_default_and_pending_states) so the core routing logic is verified directly.migration/src/m20260705_000001_drop_remote_storage_target_max_file_size.rs (1)
39-52: 🧹 Nitpick | 🔵 Trivial顺嘴提醒：回滚不是真的"恢复"
down()补列时用的是default(0)，这只是让 schema 形状复原，原来每个 target 的真实max_file_size数值在up()执行时已经被物理删除，回滚不会拿回原值。生产环境如果真跑到这步回滚，记得提前有备份或者接受这些值全部归零。这是删列迁移的通病，不是这段代码写错了。🤖 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 `@migration/src/m20260705_000001_drop_remote_storage_target_max_file_size.rs` around lines 39 - 52, The `down()` path in `drop_remote_storage_target_max_file_size` currently recreates `RemoteStorageTargets::MaxFileSize` with `default(0)`, but this only restores the schema shape and cannot recover the deleted per-row values from `up()`. Add an explicit note in the migration or `down()` implementation near `Table::alter()`/`ColumnDef::new(RemoteStorageTargets::MaxFileSize)` clarifying that rollback will reset the column to zero and does not restore original data, so operators know to rely on backups if they need the prior values.src/services/remote_storage_target_service/target.rs (1)
40-58: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win给 target 缺失单独分码
resolve_target_by_key找不到target_key时还在回ManagedIngressRequired，和“binding 没配任何 target”混在一起。补一个专门的 target-not-found 码，并在这里切过去。🤖 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 `@src/services/remote_storage_target_service/target.rs` around lines 40 - 58, The resolve_target_by_key flow currently maps a missing target_key to ManagedIngressRequired, which conflates it with a different configuration failure. Add a dedicated target-not-found ApiErrorCode and update resolve_target_by_key to use that code in the ok_or_else branch after remote_storage_target_repo::find_by_binding_and_target_key, while keeping build_resolved_target unchanged for the success path.frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx (1)
338-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value只读/非只读两段列表渲染基本是复制粘贴。
除了外面裹没裹
AnimatedCollapsible和readOnlyprop 有没有传，两段RemoteNodeRemoteStorageTargetsList的其余 props 一模一样。抽个公共 props 对象出来，以后同步维护能少挨一次骂。🤖 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 `@frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx` around lines 338 - 373, The read-only and non-read-only branches in RemoteNodeRemoteStorageTargetSection are duplicating the same RemoteNodeRemoteStorageTargetsList props, so refactor the shared props into a single local object and reuse it in both branches. Keep the branch-specific differences only where needed: AnimatedCollapsible and the readOnly prop in the read-only path, while preserving the existing handlers and target/loading/error props via the shared object.
🤖 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 `@frontend-panel/src/pages/admin/AdminPoliciesPage.tsx`:
- Around line 708-733: The remote node ID validation path in
createRemoteStorageTargetForPolicy is swallowing failures by only showing a
toast and returning, unlike the catch branch that rethrows. Make this failure
path behave like a real error so
RemoteNodeRemoteStorageTargetSection.handleSubmit can await and prevent draft
reset, by replacing the silent return with an exception flow after toast.error
while keeping the existing adminRemoteNodeService.createStorageTarget and
loadRemoteStorageTargetsForPolicy behavior unchanged.
In `@src/services/master_binding_service.rs`:
- Around line 375-393: The fallback in AuthorizedMasterBinding construction
treats a missing policy_max_file_size as 0, which downstream logic in
validate_ingress_object_size and client.rs interprets as unlimited. Update the
master_binding_service path that builds AuthorizedMasterBinding to preserve a
bounded default during rolling upgrades, or explicitly reject requests when
policy_max_file_size is absent, so older clients cannot bypass size validation.
Use the policy_max_file_size field and the target resolution flow in
resolve_target_by_key/resolve_effective_target to locate the change.
In `@src/services/policy_service/policies.rs`:
- Around line 398-405: Move the remote storage target validation out of the
write transaction in update so validate_remote_storage_policy_target and its
list_storage_targets network call run before begin, avoiding holding the
transaction open on slow remote I/O. Also add a guard in update (using
remote_node_id and remote_storage_target_key) so unchanged targets are not
revalidated; when the target hasn’t changed, reuse the existing
final_remote_storage_target_key instead of calling
validate_remote_storage_policy_target again.
- Around line 638-709: `validate_remote_storage_policy_target` is currently
re-validating the existing `remote_storage_target_key` even when it was not
changed, which blocks unrelated `update` operations. Adjust the call flow so
health checks for `last_error` and `applied_revision` are only enforced when
`remote_storage_target_key` is explicitly changed; if the key is unchanged,
preserve and reuse the existing value without failing the update. Use the
`update` path and `validate_remote_storage_policy_target` to locate the logic
and add the unchanged-key bypass there.
In `@tests/test_migrations.rs`:
- Around line 12-13: The migration test coverage in test_migrations.rs only
registers and validates
m20260705_000001_drop_remote_storage_target_max_file_size, so add the missing
round-trip test coverage for
m20260704_000002_add_remote_storage_target_key_to_storage_policies as well.
Update the migration registration/assertion list and extend the up/down column
checks in the migration test helper to verify
storage_policies.remote_storage_target_key is added on up and removed on down,
using the existing migration test symbols as the place to wire it in.
---
Outside diff comments:
In
`@frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx`:
- Around line 224-307: The non-read-only create button in
RemoteNodeRemoteStorageTargetSection is missing the canCreateTargets guard, so
it can appear enabled even when startCreate will immediately return; update the
create button disabled logic in the activeDraftMode == null branch to match the
readOnly branch and include canCreateTargets alongside loading, errorMessage,
and firstSupportedDriverType checks, using the existing startCreate and
canCreateTargets symbols to locate the fix.
In `@frontend-panel/src/components/admin/StoragePolicyDialog.tsx`:
- Around line 296-310: The remote node missing state and remote storage target
missing state are conflated in the StoragePolicyDialog error logic, causing edit
mode to show an error immediately when historical data lacks
remote_storage_target_key. Update the createRemoteNodeError and
createRemoteTargetError handling in StoragePolicyDialog so each validation has
its own explicit condition, and ensure the RemoteNodeField only shows the target
error after an appropriate user interaction trigger in both create and edit
modes.
In `@src/storage/connectors/tests.rs`:
- Around line 77-93: Add a test case in tests.rs using draft_connection to cover
reject_unexpected_remote_storage_target_key: build a non-remote driver
connection with a non-empty remote_storage_target_key and assert validation
returns PolicyRemoteNodeUnexpected. Reuse draft_connection and the
StorageConnectorConnectionInput / DriverType setup so the new case sits
alongside the existing connector policy tests and exercises the unexpected
remote_storage_target_key branch directly.
In `@src/storage/remote_protocol/tests.rs`:
- Around line 775-780: `RemoteCreateLocalStorageTargetRequest` no longer has
`max_file_size`, so update the affected test setup in
`tests/test_remote_storage.rs` to use only `name`, `base_path`, and
`is_default`. Remove every leftover `max_file_size` initialization in the
request constructors and any assertions tied to it so the test module compiles
cleanly with the current `RemoteCreateLocalStorageTargetRequest` shape.
---
Nitpick comments:
In
`@frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx`:
- Around line 338-373: The read-only and non-read-only branches in
RemoteNodeRemoteStorageTargetSection are duplicating the same
RemoteNodeRemoteStorageTargetsList props, so refactor the shared props into a
single local object and reuse it in both branches. Keep the branch-specific
differences only where needed: AnimatedCollapsible and the readOnly prop in the
read-only path, while preserving the existing handlers and target/loading/error
props via the shared object.
In
`@migration/src/m20260704_000002_add_remote_storage_target_key_to_storage_policies.rs`:
- Around line 45-64: The down() implementation in the migration is missing a
symmetric idempotency guard for dropping the remote storage target key column.
Update the down path in the migration struct that defines up()/down() so it
checks has_column before calling alter_table(...).drop_column(...) on
StoragePolicies::RemoteStorageTargetKey, matching the defensive pattern already
used in up() and the sibling migration’s up/down symmetry.
In `@migration/src/m20260705_000001_drop_remote_storage_target_max_file_size.rs`:
- Around line 39-52: The `down()` path in
`drop_remote_storage_target_max_file_size` currently recreates
`RemoteStorageTargets::MaxFileSize` with `default(0)`, but this only restores
the schema shape and cannot recover the deleted per-row values from `up()`. Add
an explicit note in the migration or `down()` implementation near
`Table::alter()`/`ColumnDef::new(RemoteStorageTargets::MaxFileSize)` clarifying
that rollback will reset the column to zero and does not restore original data,
so operators know to rely on backups if they need the prior values.
In `@src/services/remote_storage_target_service/target.rs`:
- Around line 40-58: The resolve_target_by_key flow currently maps a missing
target_key to ManagedIngressRequired, which conflates it with a different
configuration failure. Add a dedicated target-not-found ApiErrorCode and update
resolve_target_by_key to use that code in the ok_or_else branch after
remote_storage_target_repo::find_by_binding_and_target_key, while keeping
build_resolved_target unchanged for the success path.
In `@src/services/remote_storage_target_service/tests.rs`:
- Around line 756-808: Add direct unit coverage for resolve_target_by_key in the
target resolution tests, since it currently has no explicit assertions and is
only indirectly exercised through build_resolved_target. Create cases around the
existing resolve_target_by_key flow for a missing target_key, a target with
last_error set, and a target in pending revision state (using the same setup
patterns as
resolve_effective_target_reports_required_default_and_pending_states) so the
core routing logic is verified directly.
🪄 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 Plus
Run ID: 371948c4-4cdd-44e8-b4cd-dfa2f4841e00
📒 Files selected for processing (67)
frontend-panel/src/components/admin/StoragePolicyDialog.tsxfrontend-panel/src/components/admin/admin-policies-page/PolicyDialogs.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeEditForm.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetForm.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.test.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetsList.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/remoteNodeRemoteStorageTargetPresentation.test.tsfrontend-panel/src/components/admin/remoteStorageTargetDialogShared.test.tsfrontend-panel/src/components/admin/remoteStorageTargetDialogShared.tsfrontend-panel/src/components/admin/storage-policy-dialog/StoragePolicyCreateWizard.tsxfrontend-panel/src/components/admin/storage-policy-dialog/StoragePolicyEditForm.tsxfrontend-panel/src/components/admin/storage-policy-dialog/StoragePolicyRemoteFields.tsxfrontend-panel/src/components/admin/storage-policy-dialog/connectionNormalization.tsfrontend-panel/src/components/admin/storage-policy-dialog/formTypes.tsfrontend-panel/src/components/admin/storage-policy-dialog/payloadBuilders.tsfrontend-panel/src/components/admin/storagePolicyDialogShared.test.tsfrontend-panel/src/i18n/locales/en/admin/policies.jsonfrontend-panel/src/i18n/locales/en/admin/remote-nodes.jsonfrontend-panel/src/i18n/locales/zh/admin/policies.jsonfrontend-panel/src/i18n/locales/zh/admin/remote-nodes.jsonfrontend-panel/src/pages/admin/AdminPoliciesPage.test.tsxfrontend-panel/src/pages/admin/AdminPoliciesPage.tsxfrontend-panel/src/pages/admin/AdminRemoteNodesPage.test.tsxfrontend-panel/src/services/adminService.test.tsfrontend-panel/src/services/api.generated.tsmigration/src/lib.rsmigration/src/m20260704_000002_add_remote_storage_target_key_to_storage_policies.rsmigration/src/m20260705_000001_drop_remote_storage_target_max_file_size.rssrc/api/dto/admin.rssrc/api/routes/admin/policies.rssrc/entities/remote_storage_target.rssrc/entities/storage_policy.rssrc/services/master_binding_service.rssrc/services/media_metadata_service/tests.rssrc/services/policy_service/mod.rssrc/services/policy_service/models.rssrc/services/policy_service/policies.rssrc/services/remote_storage_target_service/driver.rssrc/services/remote_storage_target_service/local_profiles.rssrc/services/remote_storage_target_service/mod.rssrc/services/remote_storage_target_service/models.rssrc/services/remote_storage_target_service/normalization.rssrc/services/remote_storage_target_service/target.rssrc/services/remote_storage_target_service/tests.rssrc/services/task_service/storage_policy_cleanup.rssrc/services/task_service/types.rssrc/services/workspace_storage_service/multipart/streaming_direct.rssrc/storage/connectors/common.rssrc/storage/connectors/mod.rssrc/storage/connectors/models.rssrc/storage/connectors/tests.rssrc/storage/drivers/azure_blob/mod.rssrc/storage/drivers/local/tests.rssrc/storage/drivers/remote/mod.rssrc/storage/drivers/remote/tests.rssrc/storage/drivers/s3/tests.rssrc/storage/drivers/s3_compatible.rssrc/storage/drivers/tencent_cos/signing.rssrc/storage/drivers/tencent_cos/tests.rssrc/storage/registry.rssrc/storage/remote_protocol/client.rssrc/storage/remote_protocol/mod.rssrc/storage/remote_protocol/models.rssrc/storage/remote_protocol/tests.rstests/test_migrations.rstests/test_remote_storage.rs
💤 Files with no reviewable changes (12)
- src/services/remote_storage_target_service/models.rs
- frontend-panel/src/i18n/locales/zh/admin/remote-nodes.json
- frontend-panel/src/components/admin/admin-remote-nodes-page/remoteNodeRemoteStorageTargetPresentation.test.ts
- src/services/remote_storage_target_service/local_profiles.rs
- src/entities/remote_storage_target.rs
- frontend-panel/src/i18n/locales/en/admin/remote-nodes.json
- frontend-panel/src/components/admin/remoteStorageTargetDialogShared.ts
- frontend-panel/src/pages/admin/AdminRemoteNodesPage.test.tsx
- frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetForm.tsx
- src/storage/remote_protocol/models.rs
- frontend-panel/src/services/adminService.test.ts
- src/services/remote_storage_target_service/normalization.rs
- Add `remote_storage_target_key` column to `storage_policies` table with migration round-trip test - Introduce `index_helpers` module with cross-database helpers for drop, rename, and existence checks on indexes - Fix MySQL migration path by renaming indexes in-place instead of drop/recreate after table rename - Guard `down()` migration steps against idempotent column and index removal - Add `PolicyRemoteStorageTargetRequired` and `RemoteStorageTargetNotFound` error codes to API and i18n (en/zh) - Skip remote targetvalidation on policy update when node and target binding are unchanged - Split `authorize_internal_request` into read and write variants; make `max_file_size` required on write paths - Always append `max_file_size` query param in remote protocol client presigned URLs - Propagate `RemoteStorageTargetNotFound` through remote error kind mapping and status error builder - Add tests for target key validation, unchanged binding update, stale response races, and quick-create flow in policy wizard - Remove `max_file_size` field from `RemoteCreateLocalStorageTargetRequest`
AptS-1547
added
Enhancement
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/storage/remote_protocol/tests.rs (1)
856-862: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win断言精度倒退：
contains太松了。原来
ends_with("/compose")改成contains("/compose")之后，只要路径任何位置出现 "/compose" 子串（哪怕是 query 参数值或者对象 key 里的巧合字符串）都会被判定通过，起不到验证路由正确性的作用了。既然只是因为新增了 query 参数导致路径不再以 "/compose" 结尾，应该先按?切掉 query 部分再做ends_with判断，而不是直接放宽成子串匹配。🔧 建议修复
assert!(requests.iter().any(|request| { + let path = request + .path_and_query + .split('?') + .next() + .unwrap_or(request.path_and_query.as_str()); request.method == "POST" - && request.path_and_query.contains("/compose") + && path.ends_with("/compose") && serde_json::from_slice::<serde_json::Value>(&request.body) .expect("compose request body should be JSON")["expected_size"] == 6 }));🤖 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 `@src/storage/remote_protocol/tests.rs` around lines 856 - 862, The request assertion in the remote protocol test has become too permissive by using the path_and_query contains("/compose") check, which weakens route validation. Update the test around the request matching logic to strip off any query string from request.path_and_query and then verify the path ends with "/compose", keeping the JSON expected_size assertion unchanged. Use the existing request.iter().any closure and path_and_query field as the locating symbols when tightening the match.tests/test_migrations.rs (1)
268-307: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win测试只验证了列，没验证配套索引的往返。
storage_policy_remote_storage_target_key_migration_round_trips_column只断言了remote_storage_target_key列的存在性，但m20260704_000002的up/down同时也创建/删除了idx_storage_policies_remote_target索引（remote_node_id+remote_storage_target_key）。建议顺手加个索引存在性断言，覆盖面更完整。新增两段迁移集成测试：storage_policy_remote_storage_target_key_migration_round_trips_column：验证 up 后 storage_policies 新增列 remote_storage_target_key、down 后移除、再次 up 后恢复。
🤖 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 `@tests/test_migrations.rs` around lines 268 - 307, The migration round-trip test currently checks only the `remote_storage_target_key` column, but `m20260704_000002` also manages the `idx_storage_policies_remote_target` index. Update `storage_policy_remote_storage_target_key_migration_round_trips_column` in `tests/test_migrations.rs` to also assert the index exists after `CurrentMigrator::up`, is removed after `CurrentMigrator::down`, and is restored after reapplying, using the existing helpers and `sqlite_table_indexes`-style lookup if available.
🤖 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 `@tests/test_migrations.rs`:
- Around line 268-307: add coverage for the MySQL-specific migration path in the
migration tests: the existing storage policy remote target key round-trip test
in test_migrations.rs only exercises sqlite::memory() and never hits
rename_mysql_index_if_exists in
m20260704_000001_rename_managed_ingress_profiles_to_remote_storage_targets. Add
a test that runs with ASTER_TEST_DATABASE_BACKEND=mysql, or refactor the
migration suite to run this case per backend, so the MySQL branch in
CurrentMigrator is actually validated.
---
Nitpick comments:
In `@src/storage/remote_protocol/tests.rs`:
- Around line 856-862: The request assertion in the remote protocol test has
become too permissive by using the path_and_query contains("/compose") check,
which weakens route validation. Update the test around the request matching
logic to strip off any query string from request.path_and_query and then verify
the path ends with "/compose", keeping the JSON expected_size assertion
unchanged. Use the existing request.iter().any closure and path_and_query field
as the locating symbols when tightening the match.
In `@tests/test_migrations.rs`:
- Around line 268-307: The migration round-trip test currently checks only the
`remote_storage_target_key` column, but `m20260704_000002` also manages the
`idx_storage_policies_remote_target` index. Update
`storage_policy_remote_storage_target_key_migration_round_trips_column` in
`tests/test_migrations.rs` to also assert the index exists after
`CurrentMigrator::up`, is removed after `CurrentMigrator::down`, and is restored
after reapplying, using the existing helpers and `sqlite_table_indexes`-style
lookup if available.
🪄 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 Plus
Run ID: c06b37ed-eb6d-402d-a8ec-e7d31d96b08a
📒 Files selected for processing (35)
.gitignorefrontend-panel/src/components/admin/StoragePolicyDialog.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.test.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsxfrontend-panel/src/components/admin/storage-policy-dialog/StoragePolicyRemoteFields.test.tsxfrontend-panel/src/i18n/index.test.tsfrontend-panel/src/i18n/locales/en/errors/managed-ingress.jsonfrontend-panel/src/i18n/locales/en/errors/storage.jsonfrontend-panel/src/i18n/locales/zh/errors/managed-ingress.jsonfrontend-panel/src/i18n/locales/zh/errors/storage.jsonfrontend-panel/src/pages/admin/AdminPoliciesPage.test.tsxfrontend-panel/src/pages/admin/AdminPoliciesPage.tsxfrontend-panel/src/services/api.generated.tsfrontend-panel/src/types/api-helpers.tsmigration/src/index_helpers.rsmigration/src/lib.rsmigration/src/m20260704_000001_rename_managed_ingress_profiles_to_remote_storage_targets.rsmigration/src/m20260704_000002_add_remote_storage_target_key_to_storage_policies.rsmigration/src/m20260705_000001_drop_remote_storage_target_max_file_size.rssrc/api/api_error_code.rssrc/api/routes/internal_storage.rssrc/services/master_binding_service.rssrc/services/policy_service/policies.rssrc/services/remote_storage_target_service/target.rssrc/services/remote_storage_target_service/tests.rssrc/storage/connectors/tests.rssrc/storage/remote_protocol/client.rssrc/storage/remote_protocol/errors.rssrc/storage/remote_protocol/tests.rstests/test_azure_blob.rstests/test_local_driver_security.rstests/test_migrations.rstests/test_policies.rstests/test_remote_storage.rstests/test_s3.rs
✅ Files skipped from review due to trivial changes (7)
- frontend-panel/src/i18n/locales/zh/errors/managed-ingress.json
- .gitignore
- frontend-panel/src/i18n/locales/en/errors/managed-ingress.json
- tests/test_s3.rs
- frontend-panel/src/i18n/locales/zh/errors/storage.json
- frontend-panel/src/types/api-helpers.ts
- frontend-panel/src/services/api.generated.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- migration/src/m20260705_000001_drop_remote_storage_target_max_file_size.rs
- src/storage/connectors/tests.rs
- migration/src/lib.rs
- migration/src/m20260704_000002_add_remote_storage_target_key_to_storage_policies.rs
- src/services/remote_storage_target_service/target.rs
- src/storage/remote_protocol/client.rs
- frontend-panel/src/components/admin/StoragePolicyDialog.tsx
- src/services/remote_storage_target_service/tests.rs
- src/services/policy_service/policies.rs
- frontend-panel/src/pages/admin/AdminPoliciesPage.tsx
- frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx
…ests - Keep quick-create draft open on storage target creation failure in RemoteNodeRemoteStorageTargetSection by catching errors silently and letting parent handlers surface API errors - Add frontend test verifying draft form remains visible with original values when createStorageTarget rejects - Refactor sqlite_index_exists into sqlite_table_index_exists accepting a table name parameter; add mysql_table_index_exists helper using information_schema.statistics - Extend storage_policy_remote_storage_target_key migration test to assert idx_storage_policies_remote_target index is created, removed on rollback, and restored on reapply - Add mysql_remote_storage_target_rename_migration_round_trips_indexes test covering full up/down/up cycle for renamed indexes on remote_storage_targets vs managed_ingress_profiles tables - Fix remote_client_object_profile_and_compose_paths_roundtrip assertion to strip query string before checking /compose path suffix - Register RENAME_MANAGED_INGRESS_PROFILES_MIGRATION constant and steps_to_roll_back_rename_managed_ingress_profiles helper in test_migrations.rs
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
1 participant
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.
Summary
Validation
Closed #370
Summary by CodeRabbit