Clarify storage descriptor and normalization contracts#383
Merged
Conversation
Add comprehensive documentation and shared normalization utilities for storage descriptor fields across storage policies and remote storage targets. **Documentation:** - Add storage descriptor and field normalization contract document (EN/ZH-CN) - Document descriptor separation rationale and normalization boundaries - Add descriptor rules, normalization rules, implementation boundaries, and test guidelines - Update developer docs index to reference new contract document **Shared field semantics:** - Add `src/storage/field_contract.rs` with shared field kind, semantics, and normalization helpers - Add `StorageDescriptorFieldSemantics` to unify required/secret/boolean field contracts - Add `normalize_required_storage_field` for trimming and blank validation - Add `preserve_secret_when_omitted` for secret edit-flow preservation - Add `normalize_object_storage_prefix` for S3/object-storage prefix normalization - Add `normalize_storage_policy_max_file_size` with negative value rejection - Add `normalize_relative_local_target_path` with escape-segment detection - Add comprehensive unit tests for all normalization helpers **Backend integration:** - Integrate field contract into storage connector and remote target driver descriptors - Apply `max_file_size` normalization in storage policy create/update services - Replace inline `normalize_non_blank` with shared `normalize_required_storage_field` - Apply `normalize_object_storage_prefix` in S3 remote target driver - Apply `preserve_secret_when_omitted` in remote target update flows - Add service-level tests for negative `max_file_size` rejection **Frontend integration:** - Update remote storage target form to use descriptor-based secret preservation logic - Replace hardcoded `driver_type === "s3"` checks with `secretKeyField?.secret === true` - Update validation to check `basePathField?.required` and `basePathField?.help_key` semantics - Simplify credential preservation check to generic same-driver-type + secret-field pattern
|
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 (1)
📝 WalkthroughWalkthrough新增共享存储字段契约与归一化 helper，后端 descriptor、远程存储目标、storage policy 和前端表单都改为读取这些契约与验证元数据，并补充了对应文档与测试。 Changes字段契约与归一化统一
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant 管理端表单
participant 后端驱动描述符
participant field_contract.rs
participant remote_storage_target_service
管理端表单->>后端驱动描述符: 读取字段描述符与 validation
后端驱动描述符->>field_contract.rs: 派生字段语义/归一化规则
remote_storage_target_service->>field_contract.rs: 规范化名称、secret、路径、前缀
field_contract.rs-->>remote_storage_target_service: 返回归一化结果或错误
remote_storage_target_service-->>管理端表单: 提供描述符与校验元数据
Possibly related PRs
Suggested labels: 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.
🧹 Nitpick comments (2)
frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx (1)
161-195: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win别拿
help_key当语义开关（frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx:161-172）
help_key只是提示文案 key，不该拿来决定本地路径校验。这里改成后端 descriptor 里的显式语义位，不然 key 一改，校验逻辑就跟着飘。🤖 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 161 - 195, The local path validation in RemoteNodeRemoteStorageTargetSection is incorrectly using basePathField.help_key as the semantic switch for whether to enforce relative-path rules. Update the validation logic around requiresLocalRelativePath/localPathError to rely on an explicit descriptor flag from the backend schema instead of the help_key string, and keep the rest of the form-required checks unchanged.Source: Coding guidelines
src/services/remote_storage_target_service/driver.rs (1)
51-63: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
From里塞panic!，这不是"转换"，是埋雷。
RemoteStorageTargetDriverFieldKind压根没有Select变体，From<StorageDescriptorFieldKind>却硬着头皮实现全量转换，遇到Select直接炸。现在两个调用点（remote_storage_target_text_field/remote_storage_target_boolean_field）确实不会喂Select进来，所以暂时安全——但From语义上就该是保证成功的转换，写成"大概率成功，偶尔崩"的样子，谁用谁倒霉。字段契约本来就是给 storage policy 和 managed ingress 共享的，以后有人复制粘贴出个下拉框字段，这颗雷就爆了。改用
TryFrom，让"managed ingress 不支持 Select"这件事在类型层面显式暴露出来，而不是靠运行时炸给你看。♻️ 建议改用 TryFrom
-impl From<StorageDescriptorFieldKind> for RemoteStorageTargetDriverFieldKind { - fn from(value: StorageDescriptorFieldKind) -> Self { - match value { - StorageDescriptorFieldKind::Text => Self::Text, - StorageDescriptorFieldKind::Secret => Self::Secret, - StorageDescriptorFieldKind::Boolean => Self::Boolean, - StorageDescriptorFieldKind::Number => Self::Number, - StorageDescriptorFieldKind::Select => { - panic!("managed ingress target descriptors do not support select fields") - } - } - } -} +impl TryFrom<StorageDescriptorFieldKind> for RemoteStorageTargetDriverFieldKind { + type Error = &'static str; + + fn try_from(value: StorageDescriptorFieldKind) -> Result<Self, Self::Error> { + match value { + StorageDescriptorFieldKind::Text => Ok(Self::Text), + StorageDescriptorFieldKind::Secret => Ok(Self::Secret), + StorageDescriptorFieldKind::Boolean => Ok(Self::Boolean), + StorageDescriptorFieldKind::Number => Ok(Self::Number), + StorageDescriptorFieldKind::Select => { + Err("managed ingress target descriptors do not support select fields") + } + } + } +}调用处
semantics.kind.into()改成semantics.kind.try_into().expect("...")，两个调用点都是内部硬编码不会触发 Select，.expect的成本不变，但类型契约更诚实。🤖 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/driver.rs` around lines 51 - 63, The impl of From<StorageDescriptorFieldKind> for RemoteStorageTargetDriverFieldKind is not a total conversion because the Select branch panics, so replace it with TryFrom to make the unsupported case explicit. Update the conversion in driver.rs and adjust the call sites in remote_storage_target_text_field and remote_storage_target_boolean_field to use try_into() with a clear expect message, since those paths are internal and should never hit Select. This keeps the type contract honest and avoids runtime panic hidden inside From.
🤖 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
`@frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx`:
- Around line 161-195: The local path validation in
RemoteNodeRemoteStorageTargetSection is incorrectly using basePathField.help_key
as the semantic switch for whether to enforce relative-path rules. Update the
validation logic around requiresLocalRelativePath/localPathError to rely on an
explicit descriptor flag from the backend schema instead of the help_key string,
and keep the rest of the form-required checks unchanged.
In `@src/services/remote_storage_target_service/driver.rs`:
- Around line 51-63: The impl of From<StorageDescriptorFieldKind> for
RemoteStorageTargetDriverFieldKind is not a total conversion because the Select
branch panics, so replace it with TryFrom to make the unsupported case explicit.
Update the conversion in driver.rs and adjust the call sites in
remote_storage_target_text_field and remote_storage_target_boolean_field to use
try_into() with a clear expect message, since those paths are internal and
should never hit Select. This keeps the type contract honest and avoids runtime
panic hidden inside From.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ead29429-5be1-4d86-b6c9-ab7dd6fb896f
📒 Files selected for processing (12)
developer-docs/en/README.mddeveloper-docs/en/storage-descriptor-normalization-contract.mddeveloper-docs/zh-CN/storage-descriptor-normalization-contract.mdfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetForm.tsxfrontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsxsrc/services/policy_service/policies.rssrc/services/remote_storage_target_service/driver.rssrc/services/remote_storage_target_service/normalization.rssrc/services/remote_storage_target_service/paths.rssrc/storage/connector_descriptor.rssrc/storage/field_contract.rssrc/storage/mod.rs
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx (1)
183-197: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
access_key不能跟secret_key一起跳过编辑态必填校验。 这里的access_key不是secret字段，编辑时清空后应该立刻报必填错；现在只要 driver 不变就一律放行，会把空值拖到后端才被拒掉。按 field.secret 区分
const accessKeyError = accessKeyField?.required && - !preservesExistingCredentialValues && + !(accessKeyField?.secret && preservesExistingCredentialValues) && !form.access_key.trim() ? t("remote_node_ingress_profile_access_key_required") : null;🤖 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 183 - 197, The edit-mode required check in RemoteNodeRemoteStorageTargetSection should not skip validation for access_key just because the driver_type is unchanged; update the preservesExistingCredentialValues logic or split the conditions so only secret fields can preserve existing values in edit mode. Keep secret_key behavior tied to field.secret, but make access_key revalidate immediately when cleared, using the existing form/accessKeyField/secretKeyField checks in this component.
🧹 Nitpick comments (1)
src/services/remote_storage_target_service/driver.rs (1)
51-65: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTryFrom +
.expect()组合有点绕，容易埋雷。
Select分支被拒绝，然后text_field_with_validation/boolean_field里立刻.expect()兜底——这两处目前确实不可能命中Select（text/secret/boolean语义函数只产出对应 kind），但把"不可能发生"的分支从编译期保证退化成运行时 panic，后续如果有人改了field_contract里的语义函数，这里就直接崩，而不是编译不过。♻️ 可选写法：避免 expect
-impl TryFrom<StorageDescriptorFieldKind> for RemoteStorageTargetDriverFieldKind { - type Error = &'static str; - - fn try_from(value: StorageDescriptorFieldKind) -> std::result::Result<Self, Self::Error> { - match value { - StorageDescriptorFieldKind::Text => Ok(Self::Text), - StorageDescriptorFieldKind::Secret => Ok(Self::Secret), - StorageDescriptorFieldKind::Boolean => Ok(Self::Boolean), - StorageDescriptorFieldKind::Number => Ok(Self::Number), - StorageDescriptorFieldKind::Select => { - Err("managed ingress target descriptors do not support select fields") - } - } - } -} +fn non_select_field_kind(kind: StorageDescriptorFieldKind) -> RemoteStorageTargetDriverFieldKind { + match kind { + StorageDescriptorFieldKind::Text => RemoteStorageTargetDriverFieldKind::Text, + StorageDescriptorFieldKind::Secret => RemoteStorageTargetDriverFieldKind::Secret, + StorageDescriptorFieldKind::Boolean => RemoteStorageTargetDriverFieldKind::Boolean, + StorageDescriptorFieldKind::Number => RemoteStorageTargetDriverFieldKind::Number, + StorageDescriptorFieldKind::Select => unreachable!( + "managed ingress target descriptors only construct text/secret/boolean fields" + ), + } +}不影响功能，纯粹图个心安，爱改不改。
Also applies to: 106-144, 146-166
🤖 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/driver.rs` around lines 51 - 65, The TryFrom + expect flow in RemoteStorageTargetDriverFieldKind is too fragile because Select is rejected only at runtime and then unwrapped in field builders. Update the conversion and the callers in RemoteStorageTargetDriverFieldKind, text_field_with_validation, and boolean_field so they do not rely on .expect for impossible kinds; instead, make the unsupported case handled explicitly at the type/Result boundary so future changes to field_contract semantics fail safely without a panic.
🤖 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.
Outside diff comments:
In
`@frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsx`:
- Around line 183-197: The edit-mode required check in
RemoteNodeRemoteStorageTargetSection should not skip validation for access_key
just because the driver_type is unchanged; update the
preservesExistingCredentialValues logic or split the conditions so only secret
fields can preserve existing values in edit mode. Keep secret_key behavior tied
to field.secret, but make access_key revalidate immediately when cleared, using
the existing form/accessKeyField/secretKeyField checks in this component.
---
Nitpick comments:
In `@src/services/remote_storage_target_service/driver.rs`:
- Around line 51-65: The TryFrom + expect flow in
RemoteStorageTargetDriverFieldKind is too fragile because Select is rejected
only at runtime and then unwrapped in field builders. Update the conversion and
the callers in RemoteStorageTargetDriverFieldKind, text_field_with_validation,
and boolean_field so they do not rely on .expect for impossible kinds; instead,
make the unsupported case handled explicitly at the type/Result boundary so
future changes to field_contract semantics fail safely without a panic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 44b5f1b8-524f-430c-b15b-644a900c1ae8
📒 Files selected for processing (4)
frontend-panel/src/components/admin/admin-remote-nodes-page/RemoteNodeRemoteStorageTargetSection.tsxfrontend-panel/src/services/api.generated.tssrc/services/remote_storage_target_service/driver.rssrc/services/remote_storage_target_service/tests.rs
✅ Files skipped from review due to trivial changes (1)
- frontend-panel/src/services/api.generated.ts
…rving secret Changed S3 remote storage target edit flow to require access_key input on every edit while continuing to preserve the existing secret_key value when unchanged. Updated validation logic to distinguish between public credentials (access_key) and secret credentials (secret_key), with only the latter being preserved during edits. Backend changes: - Renamed `preservesExistingCredentialValues` to `preservesExistingSecretValue` with stricter logic - Added `remote_storage_target_field_kind` helper to centralize field kind conversion with proper error handling - Changed field builder functions to return `Result<RemoteStorageTargetDriverFieldDescriptor>` for consistent error propagation - Updated `descriptor()` methods across connector traits to return `Result<RemoteStorageTargetDriverDescriptor>` Frontend changes: - Updated test to verify access_key is now required on edit and included in update payload - Modified validation to always require access_key input regardless of edit mode - Retained secret_key preservation behavior (only validated when field is non-secret or value changed) Test changes: - Updated test name and assertions to reflect new access_key requirement - Added validation checks for disabled save button and required field message - Verified rotated access_key appears in update payload
Member
|
@coderabbitai review |
✅ Action performedReview finished.
|
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.
Summary
Verification
Closes #372
Summary by CodeRabbit
max_file_size创建与更新均拒绝负值，并覆盖边界校验。