-
-
Notifications
You must be signed in to change notification settings - Fork 137
feat: dont allow custom and time partition together #1205
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: dont allow custom and time partition together #1205
Conversation
stream creation should fail if time and custom partition flags are set also, allow only one field for custom partition update stream should fail if user tries to set custom partition when time partition already exists needs change in console as well currently console allows 3 custom partition fields that need to be updated to allow 1 field
WalkthroughThe changes adjust the error handling for partition validation during stream creation. The code now returns a direct error if both a time partition and a custom partition are provided, rather than performing an intermediate list check. A new validation in the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant S as Stream Creation
participant V as Validator
U->>S: Request stream creation with time & custom partitions
S->>V: Call validate_and_update_custom_partition()
alt Both time & custom partitions provided
V-->>S: Error: "Both partitions cannot be set simultaneously"
else Custom partition keys exceed limit
V-->>S: Error: "Custom partition key limit exceeded (max 1)"
else
V-->>S: Validation success
end
S-->>U: Return stream creation result or error
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/parseable/mod.rs (1)
827-836
: Consider adding validation for empty or whitespace-only partition keys.While the current implementation handles the number of partition keys correctly, it might be worth adding validation to ensure that partition keys are not empty or whitespace-only strings.
pub fn validate_custom_partition(custom_partition: &str) -> Result<(), CreateStreamError> { let custom_partition_list = custom_partition.split(',').collect::<Vec<&str>>(); if custom_partition_list.len() > 1 { return Err(CreateStreamError::Custom { msg: "Maximum 1 custom partition key is supported".to_string(), status: StatusCode::BAD_REQUEST, }); } + if custom_partition_list.iter().any(|key| key.trim().is_empty()) { + return Err(CreateStreamError::Custom { + msg: "Custom partition key cannot be empty".to_string(), + status: StatusCode::BAD_REQUEST, + }); + } Ok(()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/parseable/mod.rs
(3 hunks)
🔇 Additional comments (3)
src/parseable/mod.rs (3)
470-475
: LGTM! Clear error handling for partition validation.The error handling correctly prevents setting both time and custom partitions simultaneously, which aligns with the PR objectives.
622-628
: LGTM! Proper validation for stream updates.The validation correctly prevents setting a custom partition when a time partition already exists, which aligns with the PR objectives.
829-832
: LGTM! Clear restriction on custom partition keys.The validation correctly limits custom partitioning to one field, which aligns with the PR objectives.
stream creation should fail if time and custom partition flags are set also, allow only one field for custom partition update stream should fail if user tries to set custom partition when time partition already exists needs change in console as well currently console allows 3 custom partition fields that need to be updated to allow 1 field
stream creation should fail if time and custom partition flags are set also, allow only one field for custom partition update stream should fail if user tries to set custom partition when time partition already exists needs change in console as well currently console allows 3 custom partition fields that need to be updated to allow 1 field
stream creation should fail if time and custom partition flags are set
also, allow only one field for custom partition
update stream should fail if user tries to set custom partition when time partition already exists
needs change in console as well
currently console allows 3 custom partition fields that need to be updated to allow 1 field
Summary by CodeRabbit