Add table existence check and drop utilities to ClickHouse client#3422
Add table existence check and drop utilities to ClickHouse client#3422
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary by CodeRabbit
WalkthroughAdds table existence check and conditional DROP to the ClickHouse client, centralizes identifier validation with a new InvalidIdentifier error and validation helpers, extends query parameter handling for optional database and DDL wait behavior, updates the client trait insert signature, and adds tests for formatting, validation, and injection resistance. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Client Code
participant CHClient as ClickHouseClient
participant Validator as Identifier Validator
participant Builder as Query Builder
participant CH as ClickHouse Server
Caller->>CHClient: table_exists(database, table)
CHClient->>Validator: validate_clickhouse_identifier(database)
CHClient->>Validator: validate_clickhouse_identifier(table)
CHClient->>Builder: build_exists_table_query(database, table)
Builder->>CH: Execute "EXISTS TABLE \"db\".\"table\""
CH-->>CHClient: "1" or "0"
CHClient-->>Caller: Ok(true/false)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)**/*.rs📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
apps/framework-cli/**/*.rs📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
Files:
**/framework-cli/src/**⚙️ CodeRabbit configuration file
Files:
🧠 Learnings (16)📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2026-02-01T23:07:12.361ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:08:31.521ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2025-12-16T23:08:31.521ZApplied to files:
📚 Learning: 2025-12-16T23:09:10.917ZApplied to files:
📚 Learning: 2026-02-01T23:07:12.361ZApplied to files:
📚 Learning: 2025-12-17T23:48:30.295ZApplied to files:
🧬 Code graph analysis (1)apps/framework-cli/src/utilities/docker.rs (1)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
🔇 Additional comments (7)
✏️ Tip: You can disable this entire section by setting 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs`:
- Around line 229-266: Add unit tests in a #[cfg(test)] mod that exercise the
public methods table_exists and drop_table_if_exists: create a ClickhouseClient
test double or instantiate the client with a mocked/overridden execute_sql so
you can assert the exact SQL string produced (e.g. "EXISTS TABLE `{}`.`{}`" and
"DROP TABLE IF EXISTS `{}`.`{}`") and the return handling, include tests for
true/false responses for table_exists and for error propagation (simulate
execute_sql returning Err) and edge cases like empty database or table_name to
ensure formatting/escaping behavior is correct.
- Around line 239-244: Replace the forbidden anyhow::Result in table_exists with
a module-local thiserror-based error type (e.g., ClickHouseError) and return
Result<bool, ClickHouseError>; update table_exists (and any call sites) to
map/propagate execute_sql errors into that ClickHouseError (use a QueryFailed
variant). Define the ClickHouseError enum near this client module with
appropriate variants (e.g., QueryFailed(String), RequestFailed(...)) using
#[derive(thiserror::Error)]. Also standardize identifier quoting across methods
(change the backtick quoting in table_exists to match build_insert_query's
double-quote style) so all SQL identifiers use the same quoting convention.
- Around line 255-266: The drop_table_if_exists function uses anyhow::Result and
backtick quoting like the other method; change its signature to return the
crate-specific Result type (the same one used by table_exists) instead of
anyhow::Result, and update the SQL to use consistent double-quote identifier
quoting (e.g. format!("DROP TABLE IF EXISTS \"{}\".\"{}\"", database,
table_name)); keep the call to self.execute_sql(...).await? and propagate errors
with ? so the function body returns the appropriate Result<()> type used across
this client.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
cargo clippyto ensure Rust code passes Clippy's linting standards before each commit
**/*.rs: Usethiserrorwith#[derive(thiserror::Error)]for error handling in Rust; define errors near the fallibility unit with NO globalErrortype
NEVER useanyhow::Resultin Rust; define custom error types instead
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants inconstants.rsat the appropriate module level in Rust
Use tuple structs with validation constructors as newtypes in Rust (e.g.,struct UserId(String))
Write inline tests with#[cfg(test)]modules in Rust
Documentation is required for all public APIs in Rust
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/**/*.rs
📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
apps/framework-cli/**/*.rs: Always runcargo clippy --all-targets -- -D warningsbefore commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Userustfmt --edition 2021for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Usethiserrorcrate instead ofanyhow::Resultfor error handling
Define errors near their unit of fallibility (no global Error types)
Use#[derive(thiserror::Error)]with#[error()]messages for error definitions
Define newtypes as tuple structs:struct UserId(u64);
Add validation constructors for newtypes:UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes:#[derive(Debug, Clone, PartialEq)]
ImplementFrom/TryFromfor newtype conversions
Usederive_moreornutypeto reduce newtype boilerplate
Useconstfor static values (prefer overstatic)
UseUPPER_SNAKE_CASEnaming for constants
Scope constant visibility with preference:pub(crate)>pub(super)>pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
**/framework-cli/src/**
⚙️ CodeRabbit configuration file
**/framework-cli/src/**: When reviewing changes to Moose CLI:
- Check if any user-facing changes were made (commands, flags, configs, apis, etc)
- If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content
- If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.
- In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test E2E TypeScript Tests Template (Node 20, npm)
- GitHub Check: Test E2E Python Tests Template (Python 3.12)
- GitHub Check: Test E2E TypeScript Tests Template (Node 22, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 24, npm)
- GitHub Check: Test E2E Python Tests Template (Python 3.13)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
callicles
force-pushed
the
claude/add-clickhouse-helpers-QFnkh
branch
from
efb12d8 to
6198927
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs`:
- Around line 294-308: The build_exists_table_query, build_drop_table_query and
build_insert_query functions currently interpolate user-controlled
database/table identifiers directly, enabling SQL injection; replace that by
validating identifiers with an allowlist (e.g. regex `^[A-Za-z0-9_]+$`) before
using them in queries in build_exists_table_query, build_drop_table_query and
build_insert_query and return an error if validation fails; if you must support
broader names instead, implement a single escape_identifier helper that first
escapes backslashes then double quotes (escape '\' as '\\\\' and '"' as '\\"')
and call that helper for database and table_name in those three builders to
ensure consistent ClickHouse-compatible escaping.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
cargo clippyto ensure Rust code passes Clippy's linting standards before each commit
**/*.rs: Usethiserrorwith#[derive(thiserror::Error)]for error handling in Rust; define errors near the fallibility unit, never useanyhow::Result
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants inconstants.rsat the appropriate module level in Rust
Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g.,struct UserId(String))
Write inline tests with#[cfg(test)]modules in Rust
Documentation is required for all public APIs in Rust
Runcargo clippy --all-targets -- -D warningsbefore committing Rust code; no warnings allowed
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/**/*.rs
📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
apps/framework-cli/**/*.rs: Always runcargo clippy --all-targets -- -D warningsbefore commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Userustfmt --edition 2021for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Usethiserrorcrate instead ofanyhow::Resultfor error handling
Define errors near their unit of fallibility (no global Error types)
Use#[derive(thiserror::Error)]with#[error()]messages for error definitions
Define newtypes as tuple structs:struct UserId(u64);
Add validation constructors for newtypes:UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes:#[derive(Debug, Clone, PartialEq)]
ImplementFrom/TryFromfor newtype conversions
Usederive_moreornutypeto reduce newtype boilerplate
Useconstfor static values (prefer overstatic)
UseUPPER_SNAKE_CASEnaming for constants
Scope constant visibility with preference:pub(crate)>pub(super)>pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
**/framework-cli/src/**
⚙️ CodeRabbit configuration file
**/framework-cli/src/**: When reviewing changes to Moose CLI:
- Check if any user-facing changes were made (commands, flags, configs, apis, etc)
- If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content
- If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.
- In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧠 Learnings (7)
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Write unit tests for all public functions
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Test error conditions and edge cases
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Write tests for new functionality
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/tests/**/*.rs : Use integration tests for CLI commands
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Ensure all tests pass: `cargo test` for Rust components and appropriate test commands for TS/JS
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Use `thiserror` crate instead of `anyhow::Result` for error handling
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2026-02-01T23:07:12.348Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T23:07:12.348Z
Learning: Applies to **/*.rs : Use `thiserror` with `#[derive(thiserror::Error)]` for error handling in Rust; define errors near the fallibility unit, never use `anyhow::Result`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test E2E TypeScript Tests Template (Node 22, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 20, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 24, npm)
- GitHub Check: Test E2E Python Tests Template (Python 3.13)
- GitHub Check: Test E2E Python Tests Template (Python 3.12)
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs (1)
301-308:⚠️ Potential issue | 🟠 MajorInconsistent validation:
build_insert_querylacks identifier validation.New query builders (
build_exists_table_query,build_drop_table_query) validate identifiers, but this function does not. Theinsert()method passes potentially user-controlleddatabaseandtable_namedirectly here.Suggested fix
-fn build_insert_query(database: &str, table_name: &str, columns: &[String]) -> String { +fn build_insert_query(database: &str, table_name: &str, columns: &[String]) -> anyhow::Result<String> { + validate_identifier(database, "Database name")?; + validate_identifier(table_name, "Table name")?; - format!( + Ok(format!( "INSERT INTO \"{}\".\"{}\" ({}) VALUES", database, table_name, wrap_and_join_column_names(columns, ","), - ) + )) }Then update the call site at line 150 to handle the
Result.
🤖 Fix all issues with AI agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs`:
- Around line 275-290: The function validate_identifier currently returns
anyhow::Result and uses anyhow::bail!, which violates the project's
error-handling guideline; change validate_identifier to return a concrete error
Result<(), ValidationError> (or the project's canonical error type, e.g.,
CliError) and replace all anyhow::bail! calls with returning
Err(ValidationError::InvalidIdentifier { identifier_type:
identifier_type.to_string(), name: name.to_string(), message: format!(... ) })
(or the equivalent constructor on your project's error enum) so callers get a
typed error; keep the same validation checks and messages but construct and
return the typed error from validate_identifier instead of using anyhow macros.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
cargo clippyto ensure Rust code passes Clippy's linting standards before each commit
**/*.rs: Usethiserrorwith#[derive(thiserror::Error)]for error handling in Rust; define errors near the fallibility unit, never useanyhow::Result
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants inconstants.rsat the appropriate module level in Rust
Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g.,struct UserId(String))
Write inline tests with#[cfg(test)]modules in Rust
Documentation is required for all public APIs in Rust
Runcargo clippy --all-targets -- -D warningsbefore committing Rust code; no warnings allowed
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/**/*.rs
📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
apps/framework-cli/**/*.rs: Always runcargo clippy --all-targets -- -D warningsbefore commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Userustfmt --edition 2021for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Usethiserrorcrate instead ofanyhow::Resultfor error handling
Define errors near their unit of fallibility (no global Error types)
Use#[derive(thiserror::Error)]with#[error()]messages for error definitions
Define newtypes as tuple structs:struct UserId(u64);
Add validation constructors for newtypes:UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes:#[derive(Debug, Clone, PartialEq)]
ImplementFrom/TryFromfor newtype conversions
Usederive_moreornutypeto reduce newtype boilerplate
Useconstfor static values (prefer overstatic)
UseUPPER_SNAKE_CASEnaming for constants
Scope constant visibility with preference:pub(crate)>pub(super)>pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
**/framework-cli/src/**
⚙️ CodeRabbit configuration file
**/framework-cli/src/**: When reviewing changes to Moose CLI:
- Check if any user-facing changes were made (commands, flags, configs, apis, etc)
- If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content
- If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.
- In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧠 Learnings (8)
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Write unit tests for all public functions
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Test error conditions and edge cases
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Write tests for new functionality
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/tests/**/*.rs : Use integration tests for CLI commands
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Ensure all tests pass: `cargo test` for Rust components and appropriate test commands for TS/JS
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Use `thiserror` crate instead of `anyhow::Result` for error handling
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2026-02-01T23:07:12.348Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T23:07:12.348Z
Learning: Applies to **/*.rs : Use `thiserror` with `#[derive(thiserror::Error)]` for error handling in Rust; define errors near the fallibility unit, never use `anyhow::Result`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-17T23:48:30.295Z
Learnt from: callicles
Repo: 514-labs/moosestack PR: 3143
File: apps/framework-cli/src/cli/routines/seed_data.rs:614-641
Timestamp: 2025-12-17T23:48:30.295Z
Learning: In apps/framework-cli/src/cli/routines/seed_data.rs, the create_mirror_schema function uses CREATE TABLE ... AS SELECT ... LIMIT 0 to create dev mirrors for EXTERNALLY_MANAGED tables. This intentionally does not preserve column codecs, TTL expressions, or materialized column definitions from the remote table, which is acceptable for local dev-only mirrors where matching column types is sufficient for materialized view sources.
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test E2E TypeScript Tests Template (Node 20, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 22, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 24, npm)
🔇 Additional comments (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs (1)
548-671: Good test coverage.Tests cover:
- Valid identifiers
- Empty input
- Invalid characters
- Leading digits
- SQL injection attempts
- Query formatting with double quotes
wait_end_of_querybehavior for EXISTS vs DROP
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
Outdated
Show resolved
Hide resolved
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs (1)
277-284:⚠️ Potential issue | 🟠 Major
build_insert_querylacks validation likebuild_exists_table_queryandbuild_drop_table_query.The inconsistency is real:
build_exists_table_queryandbuild_drop_table_queryvalidate identifiers and returnResult, butbuild_insert_queryskips validation. Sincetable_nameanddatabaseare user-controlled parameters to theinsertmethod, this leaves the query vulnerable to malformed identifiers.Changing the return type to
Resultwill also require updating:
- The call site at line 151 to propagate the error with
?- The three existing tests (lines 495–520) to handle
Result
🤖 Fix all issues with AI agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs`:
- Around line 30-34: The two functions duplicate validation logic—refactor
validate_clickhouse_identifier to call is_valid_clickhouse_identifier(name) for
the boolean check and only build/return the detailed error string when that call
returns false; update validate_clickhouse_identifier (and any callers if needed)
to delegate the predicate to is_valid_clickhouse_identifier, keeping the
error-message construction (currently in validate_clickhouse_identifier) as the
only place that formats/returns the human-readable error.
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rsapps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
cargo clippyto ensure Rust code passes Clippy's linting standards before each commit
**/*.rs: Usethiserrorwith#[derive(thiserror::Error)]for error handling in Rust; define errors near the fallibility unit, never useanyhow::Result
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants inconstants.rsat the appropriate module level in Rust
Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g.,struct UserId(String))
Write inline tests with#[cfg(test)]modules in Rust
Documentation is required for all public APIs in Rust
Runcargo clippy --all-targets -- -D warningsbefore committing Rust code; no warnings allowed
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/**/*.rs
📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
apps/framework-cli/**/*.rs: Always runcargo clippy --all-targets -- -D warningsbefore commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Userustfmt --edition 2021for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Usethiserrorcrate instead ofanyhow::Resultfor error handling
Define errors near their unit of fallibility (no global Error types)
Use#[derive(thiserror::Error)]with#[error()]messages for error definitions
Define newtypes as tuple structs:struct UserId(u64);
Add validation constructors for newtypes:UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes:#[derive(Debug, Clone, PartialEq)]
ImplementFrom/TryFromfor newtype conversions
Usederive_moreornutypeto reduce newtype boilerplate
Useconstfor static values (prefer overstatic)
UseUPPER_SNAKE_CASEnaming for constants
Scope constant visibility with preference:pub(crate)>pub(super)>pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
**/framework-cli/src/**
⚙️ CodeRabbit configuration file
**/framework-cli/src/**: When reviewing changes to Moose CLI:
- Check if any user-facing changes were made (commands, flags, configs, apis, etc)
- If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content
- If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.
- In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧠 Learnings (15)
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Add validation constructors for newtypes: `UserId::new(id: u64) -> Result<Self, Error>`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Define errors near their unit of fallibility (no global Error types)
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Define newtypes as tuple structs: `struct UserId(u64);`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Document all public APIs and breaking changes
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Use `#[derive(thiserror::Error)]` with `#[error()]` messages for error definitions
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2026-02-01T23:07:12.348Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T23:07:12.348Z
Learning: Applies to **/*.rs : Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g., `struct UserId(String)`)
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Write unit tests for all public functions
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Test error conditions and edge cases
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Write tests for new functionality
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/tests/**/*.rs : Use integration tests for CLI commands
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Ensure all tests pass: `cargo test` for Rust components and appropriate test commands for TS/JS
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Use `thiserror` crate instead of `anyhow::Result` for error handling
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2026-02-01T23:07:12.348Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T23:07:12.348Z
Learning: Applies to **/*.rs : Use `thiserror` with `#[derive(thiserror::Error)]` for error handling in Rust; define errors near the fallibility unit, never use `anyhow::Result`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-17T23:48:30.295Z
Learnt from: callicles
Repo: 514-labs/moosestack PR: 3143
File: apps/framework-cli/src/cli/routines/seed_data.rs:614-641
Timestamp: 2025-12-17T23:48:30.295Z
Learning: In apps/framework-cli/src/cli/routines/seed_data.rs, the create_mirror_schema function uses CREATE TABLE ... AS SELECT ... LIMIT 0 to create dev mirrors for EXTERNALLY_MANAGED tables. This intentionally does not preserve column codecs, TTL expressions, or materialized column definitions from the remote table, which is acceptable for local dev-only mirrors where matching column types is sufficient for materialized view sources.
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Write meaningful names for functions, variables, and types
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧬 Code graph analysis (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs (1)
validate_clickhouse_identifier(40-67)
🔇 Additional comments (4)
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs (1)
13-18: LGTM!Error variant follows
thiserrorconventions with structured fields for detailed error reporting. Good use of typed errors for SQL injection prevention.apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs (3)
14-14: LGTM!Clean import of shared validation utilities from the errors module.
297-321: LGTM!Query builders correctly validate identifiers before constructing SQL. Good defense against injection.
524-647: Good test coverage for query builders and validation.Tests cover happy paths, edge cases (underscores, empty, invalid chars, digit-start), and SQL injection attempts.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs`:
- Around line 30-34: The validator is allowing Unicode letters because
is_alphanumeric() accepts non-ASCII; update is_valid_clickhouse_identifier to
use ASCII-only checks: replace c.is_alphanumeric() with
c.is_ascii_alphanumeric() and replace the leading-character check .is_numeric()
with .is_ascii_digit() so the function enforces ClickHouse's
^[a-zA-Z_][0-9a-zA-Z_]*$ rule (retain the existing non-empty check and
underscore allowance).
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
cargo clippyto ensure Rust code passes Clippy's linting standards before each commit
**/*.rs: Usethiserrorwith#[derive(thiserror::Error)]for error handling in Rust; define errors near the fallibility unit, never useanyhow::Result
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants inconstants.rsat the appropriate module level in Rust
Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g.,struct UserId(String))
Write inline tests with#[cfg(test)]modules in Rust
Documentation is required for all public APIs in Rust
Runcargo clippy --all-targets -- -D warningsbefore committing Rust code; no warnings allowed
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
apps/framework-cli/**/*.rs
📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
apps/framework-cli/**/*.rs: Always runcargo clippy --all-targets -- -D warningsbefore commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Userustfmt --edition 2021for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Usethiserrorcrate instead ofanyhow::Resultfor error handling
Define errors near their unit of fallibility (no global Error types)
Use#[derive(thiserror::Error)]with#[error()]messages for error definitions
Define newtypes as tuple structs:struct UserId(u64);
Add validation constructors for newtypes:UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes:#[derive(Debug, Clone, PartialEq)]
ImplementFrom/TryFromfor newtype conversions
Usederive_moreornutypeto reduce newtype boilerplate
Useconstfor static values (prefer overstatic)
UseUPPER_SNAKE_CASEnaming for constants
Scope constant visibility with preference:pub(crate)>pub(super)>pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
**/framework-cli/src/**
⚙️ CodeRabbit configuration file
**/framework-cli/src/**: When reviewing changes to Moose CLI:
- Check if any user-facing changes were made (commands, flags, configs, apis, etc)
- If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content
- If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.
- In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
🧠 Learnings (6)
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Add validation constructors for newtypes: `UserId::new(id: u64) -> Result<Self, Error>`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Define errors near their unit of fallibility (no global Error types)
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Define newtypes as tuple structs: `struct UserId(u64);`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Document all public APIs and breaking changes
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Use `#[derive(thiserror::Error)]` with `#[error()]` messages for error definitions
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2026-02-01T23:07:12.348Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T23:07:12.348Z
Learning: Applies to **/*.rs : Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g., `struct UserId(String)`)
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test E2E TypeScript Tests Template (Node 20, npm)
🔇 Additional comments (2)
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs (2)
13-18: LGTM!Error variant well-structured with meaningful fields for debugging.
40-62: LGTM!Clean delegation to
is_valid_clickhouse_identifierwith detailed error construction only on failure. Follows the suggested refactor from prior review.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
callicles
force-pushed
the
claude/add-clickhouse-helpers-QFnkh
branch
from
f83eb14 to
27e6343
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
…thods Add convenience methods to ClickHouseClient for checking table existence and dropping tables: - table_exists: Checks if a table exists in the specified database using the EXISTS TABLE query, returns true/false - drop_table_if_exists: Drops a table if it exists, idempotent operation Both methods use backticks for quoting identifiers to handle special characters in database/table names. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
The table_exists and drop_table_if_exists methods are not yet used in the codebase, so add the dead_code lint allow attribute to prevent warnings until they are integrated. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
- Fix inconsistent identifier quoting: use double quotes instead of backticks to match existing codebase conventions - Extract SQL query building into separate helper functions (build_exists_table_query, build_drop_table_query) for testability - Add comprehensive unit tests for: - EXISTS TABLE query formatting - DROP TABLE IF EXISTS query formatting - Verify wait_end_of_query parameter handling for both query types https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
Address PR feedback regarding potential SQL injection vulnerability: - Add validate_identifier() function that validates ClickHouse identifiers: - Must be non-empty - Can only contain alphanumeric characters and underscores - Cannot start with a digit - Update build_exists_table_query() and build_drop_table_query() to validate inputs and return Result types - Add comprehensive tests for identifier validation including: - Valid identifier patterns - Empty identifiers - Invalid characters (hyphens, dots, spaces) - SQL injection attempts - Identifiers starting with digits This follows the same validation pattern used in docker.rs for is_valid_clickhouse_identifier(). https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
…tion Replace anyhow::Result with concrete ClickhouseError type for identifier validation, following project error-handling guidelines: - Add InvalidIdentifier variant to ClickhouseError enum with fields: - identifier_type: The type of identifier (e.g., "Database name") - name: The actual invalid identifier value - reason: Description of why validation failed - Update validate_identifier() to return Result<(), ClickhouseError> - Update build_exists_table_query() and build_drop_table_query() to return Result<String, ClickhouseError> - Callers using anyhow::Result can still use ? operator due to automatic From conversion This provides typed errors for better error handling and follows the pattern used elsewhere in the codebase (e.g., ClickhouseError::InvalidParameters). https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
Move validation functions to errors.rs for reuse across modules: - Add is_valid_clickhouse_identifier() - returns bool, same logic as the existing function in utilities/docker.rs - Add validate_clickhouse_identifier() - returns typed ClickhouseError for detailed error information This consolidates the validation logic that was duplicated between docker.rs and client.rs into a shared location in the clickhouse module. The client.rs now imports and uses validate_clickhouse_identifier from errors.rs instead of having its own local copy. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
…dule Remove duplicate build_drop_table_query and use queries::drop_table_query which already supports non-cluster operations when cluster_name is None. Changes: - Import drop_table_query from super::queries - Update drop_table_if_exists to use drop_table_query(db, table, None) - Add identifier validation before calling drop_table_query - Remove build_drop_table_query function and its tests The existing queries::drop_table_query handles both clustered and non-clustered operations via the optional cluster_name parameter. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
Refactor validate_clickhouse_identifier to delegate the boolean check to is_valid_clickhouse_identifier, avoiding duplicated validation logic. The error message construction now only happens when validation fails: - Call is_valid_clickhouse_identifier(name) first - If valid, return Ok(()) immediately - If invalid, determine the specific reason and construct the error This keeps the validation predicate in one place (is_valid_clickhouse_identifier) while validate_clickhouse_identifier focuses solely on error formatting. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
…rors module Remove duplicate is_valid_clickhouse_identifier from utilities/docker.rs and have it import the function from infrastructure/olap/clickhouse/errors.rs. Changes: - docker.rs now imports is_valid_clickhouse_identifier from clickhouse errors - Removed duplicate function definition from docker.rs - Removed duplicate test (test_is_valid_clickhouse_identifier) from docker.rs - Updated doc comment in errors.rs to reflect shared usage The function is now the single source of truth for identifier validation, used by both ClickHouse client operations and Docker XML generation. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
…ntifiers Use is_ascii_alphanumeric() and is_ascii_digit() instead of is_alphanumeric() and is_numeric() to enforce ClickHouse's strict ^[a-zA-Z_][0-9a-zA-Z_]* rule. This prevents Unicode letters from being accepted as valid identifier characters. https://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
callicles
force-pushed
the
claude/add-clickhouse-helpers-QFnkh
branch
from
27e6343 to
f29babb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs (1)
281-288:⚠️ Potential issue | 🟠 Major
build_insert_querylacks identifier validation.Unlike
build_exists_table_query(line 301-303), this function doesn't validatedatabaseortable_name. Inconsistent security posture.🛡️ Proposed fix
fn build_insert_query(database: &str, table_name: &str, columns: &[String]) -> String { + // Note: Callers should validate identifiers before calling this function + // Consider adding validation here for consistency with build_exists_table_query format!( "INSERT INTO \"{}\".\"{}\" ({}) VALUES", database,Or add full validation:
-fn build_insert_query(database: &str, table_name: &str, columns: &[String]) -> String { +fn build_insert_query(database: &str, table_name: &str, columns: &[String]) -> Result<String, ClickhouseError> { + validate_clickhouse_identifier(database, "Database name")?; + validate_clickhouse_identifier(table_name, "Table name")?; - format!( + Ok(format!( "INSERT INTO \"{}\".\"{}\" ({}) VALUES", database, table_name, wrap_and_join_column_names(columns, ","), - ) + )) }
📜 Review details
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rsapps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/utilities/docker.rs
🧰 Additional context used
📓 Path-based instructions (3)
**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Run
cargo clippyto ensure Rust code passes Clippy's linting standards before each commit
**/*.rs: Usethiserrorwith#[derive(thiserror::Error)]for error handling in Rust; define errors near the fallibility unit, never useanyhow::Result
Use snake_case for functions and variables, PascalCase for types and traits, SCREAMING_SNAKE_CASE for constants in Rust
Place constants inconstants.rsat the appropriate module level in Rust
Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g.,struct UserId(String))
Write inline tests with#[cfg(test)]modules in Rust
Documentation is required for all public APIs in Rust
Runcargo clippy --all-targets -- -D warningsbefore committing Rust code; no warnings allowed
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/utilities/docker.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
apps/framework-cli/**/*.rs
📄 CodeRabbit inference engine (apps/framework-cli/AGENTS.md)
apps/framework-cli/**/*.rs: Always runcargo clippy --all-targets -- -D warningsbefore commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Userustfmt --edition 2021for consistent formatting
Write meaningful names for functions, variables, and types
Document all public APIs and breaking changes
Usethiserrorcrate instead ofanyhow::Resultfor error handling
Define errors near their unit of fallibility (no global Error types)
Use#[derive(thiserror::Error)]with#[error()]messages for error definitions
Define newtypes as tuple structs:struct UserId(u64);
Add validation constructors for newtypes:UserId::new(id: u64) -> Result<Self, Error>
Derive standard traits for newtypes:#[derive(Debug, Clone, PartialEq)]
ImplementFrom/TryFromfor newtype conversions
Usederive_moreornutypeto reduce newtype boilerplate
Useconstfor static values (prefer overstatic)
UseUPPER_SNAKE_CASEnaming for constants
Scope constant visibility with preference:pub(crate)>pub(super)>pub
Group related constants together
Write unit tests for all public functions
Test error conditions and edge cases
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/utilities/docker.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
**/framework-cli/src/**
⚙️ CodeRabbit configuration file
**/framework-cli/src/**: When reviewing changes to Moose CLI:
- Check if any user-facing changes were made (commands, flags, configs, apis, etc)
- If yes, verify the documentation for THAT SPECIFIC feature is updated in apps/framework-docs-v2/content
- If docs for that feature doesn't exist yet, it should be added. If the change removes public apis, the documentation for those should also be removed. Changing unrelated docs doesn't satisfy this requirement.
- In addition to reviewing for docs discrepancies, you should also review the code as per usual guidelines.
Files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/utilities/docker.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧠 Learnings (16)
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Add validation constructors for newtypes: `UserId::new(id: u64) -> Result<Self, Error>`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Define newtypes as tuple structs: `struct UserId(u64);`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Define errors near their unit of fallibility (no global Error types)
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Use `#[derive(thiserror::Error)]` with `#[error()]` messages for error definitions
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2026-02-01T23:07:12.361Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T23:07:12.361Z
Learning: Applies to **/*.rs : Use tuple structs with validation constructors (newtypes) for Rust type safety (e.g., `struct UserId(String)`)
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Document all public APIs and breaking changes
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/utilities/docker.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Write meaningful names for functions, variables, and types
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rsapps/framework-cli/src/utilities/docker.rsapps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Always run `cargo clippy --all-targets -- -D warnings` before commits; fix all warnings - no Clippy warnings may remain (treat warnings as errors)
Applied to files:
apps/framework-cli/src/utilities/docker.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Write unit tests for all public functions
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Write tests for new functionality
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Test error conditions and edge cases
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/tests/**/*.rs : Use integration tests for CLI commands
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:08:31.521Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-16T23:08:31.521Z
Learning: Ensure all tests pass: `cargo test` for Rust components and appropriate test commands for TS/JS
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-16T23:09:10.917Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: apps/framework-cli/AGENTS.md:0-0
Timestamp: 2025-12-16T23:09:10.917Z
Learning: Applies to apps/framework-cli/**/*.rs : Use `thiserror` crate instead of `anyhow::Result` for error handling
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2026-02-01T23:07:12.361Z
Learnt from: CR
Repo: 514-labs/moosestack PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-02-01T23:07:12.361Z
Learning: Applies to **/*.rs : Use `thiserror` with `#[derive(thiserror::Error)]` for error handling in Rust; define errors near the fallibility unit, never use `anyhow::Result`
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
📚 Learning: 2025-12-17T23:48:30.295Z
Learnt from: callicles
Repo: 514-labs/moosestack PR: 3143
File: apps/framework-cli/src/cli/routines/seed_data.rs:614-641
Timestamp: 2025-12-17T23:48:30.295Z
Learning: In apps/framework-cli/src/cli/routines/seed_data.rs, the create_mirror_schema function uses CREATE TABLE ... AS SELECT ... LIMIT 0 to create dev mirrors for EXTERNALLY_MANAGED tables. This intentionally does not preserve column codecs, TTL expressions, or materialized column definitions from the remote table, which is acceptable for local dev-only mirrors where matching column types is sufficient for materialized view sources.
Applied to files:
apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs
🧬 Code graph analysis (1)
apps/framework-cli/src/utilities/docker.rs (1)
apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs (1)
is_valid_clickhouse_identifier(31-35)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Test E2E TypeScript Tests Template (Node 24, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 20, npm)
- GitHub Check: Test E2E TypeScript Tests Template (Node 22, npm)
- GitHub Check: Test E2E Python Tests Template (Python 3.12)
- GitHub Check: Test E2E Python Tests Template (Python 3.13)
🔇 Additional comments (7)
apps/framework-cli/src/utilities/docker.rs (1)
12-12: LGTM!Clean consolidation of identifier validation to a single source of truth. Usage in
generate_clickhouse_clusters_xml(line 389) correctly validates cluster names before XML generation.apps/framework-cli/src/infrastructure/olap/clickhouse/errors.rs (3)
13-18: LGTM!Error variant follows
thiserrorguidelines with descriptive fields. Good structured error for identifier validation failures.
31-35: Validation logic is correct.Properly enforces ClickHouse's
^[a-zA-Z_][0-9a-zA-Z_]*$constraint:
- Non-empty check
- ASCII alphanumeric + underscore only
- No leading digit (underscore allowed at start)
The
unwrap()on line 34 is safe due to short-circuit evaluation after!name.is_empty().
41-63: LGTM!Correctly delegates to
is_valid_clickhouse_identifierand only constructs the detailed error on failure. Good separation of boolean check vs. error construction.apps/framework-cli/src/infrastructure/olap/clickhouse/client.rs (3)
301-305: LGTM!Proper identifier validation before query construction. Double-quoted identifiers are consistent with
build_insert_query.
508-602: Good test coverage.Tests cover:
- Query formatting (lines 509-524)
wait_end_of_querybehavior for EXISTS vs DROP (lines 527-546)- Identifier validation: valid, empty, invalid chars, leading digit (lines 548-591)
- SQL injection rejection (lines 593-602)
256-267: No double validation.drop_table_querydoes not validate identifiers—it only renders a template. Validation occurs once indrop_table_if_exists.Likely an incorrect or invalid review comment.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
3 participants
Summary
Added two new utility methods to the ClickHouse client for managing table operations: checking if a table exists and dropping a table if it exists.
Changes
table_exists(): New async method that checks whether a table exists in a specified database by executing a ClickHouseEXISTS TABLEquery. ReturnsOk(true)if the table exists,Ok(false)otherwise.drop_table_if_exists(): New async method that safely drops a table from a specified database using theDROP TABLE IF EXISTSstatement, preventing errors if the table doesn't exist.Implementation Details
#[allow(dead_code)]to suppress warnings during development, indicating they may be used in future features or other parts of the codebaseanyhow::Resultexecute_sql()infrastructure for consistencyhttps://claude.ai/code/session_01NUEsWQtrCXM2QMJ9vccK5B
Note
Medium Risk
Touches ClickHouse SQL generation/execution paths and adds new identifier validation that could reject previously accepted names; mistakes here could break migrations or table management operations.
Overview
Adds
table_existsanddrop_table_if_existsto the ClickHouse client, including a newEXISTS TABLEquery builder.Introduces centralized ClickHouse identifier validation (
is_valid_clickhouse_identifier+validate_clickhouse_identifierand newClickhouseError::InvalidIdentifier), reusing it in Docker cluster XML generation and adding tests to cover query param behavior (e.g.,wait_end_of_query) and invalid identifiers/injection attempts.Written by Cursor Bugbot for commit f29babb. This will update automatically on new commits. Configure here.