fix: unquote flow_name in create flow expr #5483
Conversation
WalkthroughThis pull request updates the handling of flow names across multiple modules. A new static regex for compatibility is added to allow older, less strict naming conventions, and the behavior of the flow name parser is modified to log warnings when falling back to the compatibility pattern. Additionally, a new error variant is introduced for invalid flow names and a sanitization function is implemented to validate and clean flow names before creating flow expressions. The SQL parser is also updated to consistently handle quoted flow names and case-insensitive matching for the SINK keyword. Changes
Sequence Diagram(s)sequenceDiagram
participant F as FlowNameKeyInner
participant SR as StandardRegex
participant CR as CompatRegex
participant W as Telemetry (warn)
participant C as Caller
C->>F: Call from_bytes(input)
F->>SR: Try matching input with standard pattern
alt Standard Pattern Matched
SR-->>F: Match result
F-->>C: Return parsed flow name
else Standard Pattern Failed
F->>W: Log warning message
F->>CR: Try matching input with compatibility pattern
alt Compatibility Pattern Matched
CR-->>F: Match result
F-->>C: Return parsed flow name
else
CR-->>F: No match
F-->>C: Return error
end
end
sequenceDiagram
participant U as User Query
participant P as Parser (parse_create_flow)
participant E as ExprFactory (to_create_flow_task_expr)
participant S as sanitize_flow_name
participant T as Test
U->>P: Submit CREATE FLOW query with flow name
P->>E: Invoke to_create_flow_task_expr with flow name
E->>S: Call sanitize_flow_name(flow name)
alt Flow name valid and sanitized
S-->>E: Return sanitized name
E-->>P: Return CreateFlowExpr with sanitized name
P-->>U: Successfully parse and process flow
else Flow name invalid
S-->>E: Return error
E-->>P: Propagate error
P-->>U: Return error response
end
T-->>E: Validate behavior with test case
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🪧 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
Documentation and Community
|
|
What if users already have invalid flow keys like |
added compat key pattern for older flow_name, and tests, should be ok now |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/common/meta/src/key/flow/flow_name.rs (1)
124-135: Consider enhancing the warning message.The warning message could be more informative by including details about the expected pattern.
- "FlowNameKeyInner '{}' is not a valid flow name in newer version.", + "FlowNameKeyInner '{}' is not a valid flow name in newer version. Expected pattern: {}/catalog/flow_name",
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/common/meta/src/key/flow/flow_name.rs(4 hunks)src/operator/src/error.rs(2 hunks)src/operator/src/expr_factory.rs(5 hunks)src/sql/src/parsers/create_parser.rs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (18)
- GitHub Check: Fuzz Test (Distributed, Remote WAL, fuzz_insert_logical_table)
- GitHub Check: Fuzz Test (Distributed, Remote WAL, fuzz_insert)
- GitHub Check: Fuzz Test with Chaos (Distributed, Local WAL, fuzz_migrate_metric_regions)
- GitHub Check: Fuzz Test (Distributed, Remote WAL, fuzz_alter_logical_table)
- GitHub Check: Fuzz Test with Chaos (Distributed, Local WAL, fuzz_migrate_mito_regions)
- GitHub Check: Fuzz Test (Distributed, Remote WAL, fuzz_create_logical_table)
- GitHub Check: Fuzz Test with Chaos (Distributed, Remote WAL, fuzz_failover_metric_regions)
- GitHub Check: Fuzz Test (Distributed, Remote WAL, fuzz_create_database)
- GitHub Check: Fuzz Test with Chaos (Distributed, Remote WAL, fuzz_failover_mito_regions)
- GitHub Check: Fuzz Test (Distributed, Remote WAL, fuzz_alter_table)
- GitHub Check: Fuzz Test with Chaos (Distributed, Remote WAL, fuzz_migrate_metric_regions)
- GitHub Check: Fuzz Test (Distributed, Remote WAL, fuzz_create_table)
- GitHub Check: Fuzz Test with Chaos (Distributed, Remote WAL, fuzz_migrate_mito_regions)
- GitHub Check: Fuzz Test (fuzz_insert)
- GitHub Check: Sqlness Test (Pg Kvbackend)
- GitHub Check: Sqlness Test (Remote WAL)
- GitHub Check: Sqlness Test (Basic)
- GitHub Check: test
🔇 Additional comments (7)
src/common/meta/src/key/flow/flow_name.rs (2)
42-46: LGTM! Good addition of backward compatibility.The new regex pattern
COMPAT_FLOW_NAME_KEY_PATTERNallows for less strict name patterns while maintaining compatibility with older flow names. The documentation clearly explains its purpose.
301-305: LGTM! Good test coverage for compatibility.The test case verifies the backward compatibility with older flow name formats, specifically testing the
a/\b`` pattern mentioned in the PR comments.src/operator/src/error.rs (2)
707-712: LGTM! Well-structured error variant.The new
InvalidFlowNameerror variant follows the established pattern and includes appropriate fields for error reporting.
831-831: LGTM! Consistent error code mapping.The
InvalidFlowNameerror is correctly mapped toStatusCode::InvalidArguments, consistent with similar validation errors.src/operator/src/expr_factory.rs (2)
748-757: LGTM! Clean and efficient flow name sanitization.The
sanitize_flow_namefunction effectively validates and sanitizes flow names. The use ofswap_removeis efficient as it avoids unnecessary cloning.
772-825: LGTM! Comprehensive test coverage.The test cases cover both successful and error scenarios, including the specific case of invalid flow names with dots (e.g.,
abc.task_2).src/sql/src/parsers/create_parser.rs (1)
1309-1325: LGTM! Updated test case for quoted flow names.The test case now correctly verifies the handling of quoted flow names, ensuring consistency with the new flow name requirements.
I hereby agree to the terms of the GreptimeDB CLA.
Refer to a related PR or issue link (optional)
What's changed and what's your intention?
fix and tested, flow name with quote will now be sanitize as normal
PR Checklist
Please convert it to a draft if some of the following conditions are not met.
Summary by CodeRabbit