feat: implement clickhouse-user-query
#2554
Conversation
Deploying rivet with
|
| Latest commit: |
96dc117
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0335fa33.rivet.pages.dev |
| Branch Preview URL: | https://06-05-feat-implement-clickho.rivet.pages.dev |
NathanFlurry
force-pushed
the
06-05-feat_implement_clickhouse-user-query_
branch
2 times, most recently
from
6164225 to
1a2c0b8
Compare
NathanFlurry
force-pushed
the
chore_add_Linux_ARM64_and_Windows_build_targets
branch
from
fa06adb to
c360977
Compare
Deploying rivet-hub with
|
| Latest commit: |
96dc117
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0ab41450.rivet-hub-7jb.pages.dev |
| Branch Preview URL: | https://06-05-feat-implement-clickho.rivet-hub-7jb.pages.dev |
Deploying rivet-studio with
|
| Latest commit: |
96dc117
|
| Status: | 🚫 Build failed. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Implements a new clickhouse-user-query package that provides a type-safe, SQL-injection-protected way to build ClickHouse queries from user-defined expressions.
- Implementation includes comprehensive schema validation and type checking in
src/schema.rsto ensure query safety - Builder pattern in
src/builder.rssupports complex WHERE clauses with boolean, string, numeric, and array comparisons - Robust error handling system in
src/error.rswith custom error types for schema validation, property checking, and query validation - Full test coverage with integration tests using testcontainers for isolated ClickHouse instance testing
- QueryExpr system in
src/query.rsenables type-safe construction of parameterized queries with support for nested properties
11 file(s) reviewed, 5 comment(s)
Edit PR Review Bot Settings | Greptile
MasterPtato
changed the base branch from
chore_add_Linux_ARM64_and_Windows_build_targets
to
graphite-base/2554
NathanFlurry
force-pushed
the
06-05-feat_implement_clickhouse-user-query_
branch
from
1a2c0b8 to
ec1d934
Compare
NathanFlurry
force-pushed
the
graphite-base/2554
branch
from
c360977 to
62fd12b
Compare
NathanFlurry
changed the base branch from
graphite-base/2554
to
chore_add_Linux_ARM64_and_Windows_build_targets
MasterPtato
force-pushed
the
chore_add_Linux_ARM64_and_Windows_build_targets
branch
from
62fd12b to
6f45f25
Compare
MasterPtato
force-pushed
the
06-05-feat_implement_clickhouse-user-query_
branch
from
ec1d934 to
5d1b5e2
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review: clickhouse-user-query PackageTodo List:
🔒 Critical Security Issues1. SQL Injection Vulnerability (HIGH SEVERITY)Location: Ok(format!("{}[{}]", property_ident.0, format!("'{}'", subprop.replace("'", "\\'"))))Issue: The subproperty escaping only handles single quotes but is vulnerable to other injection vectors:
Fix Required: fn escape_clickhouse_string(s: &str) -> String {
s.replace('\\', "\\\\")
.replace('\'', "\\'")
.replace('`', "\\`")
.replace('\n', "\\n")
.replace('\r', "\\r")
.replace('\t', "\\t")
}
// Use in build_column_reference:
Ok(format!("{}[{}]", property_ident.0, format!("'{}'", escape_clickhouse_string(subprop))))2. Potential Panic (MEDIUM SEVERITY)Location: if name.chars().next().unwrap().is_numeric() {Issue: This will panic if Fix Required: if name.chars().next().map_or(false, |c| c.is_numeric()) {
return Err(UserQueryError::InvalidPropertyName(name.to_string()));
}🛠️ Code Quality Issues3. Missing Duplicate Property ValidationLocation: Issue: Fix Required: pub fn new(properties: Vec<Property>) -> Result<Self> {
let mut seen_names = std::collections::HashSet::new();
for prop in &properties {
if !seen_names.insert(&prop.name) {
return Err(UserQueryError::DuplicatePropertyName(prop.name.clone()));
}
}
Ok(Self { properties })
}4. Missing Future CompatibilityLocation: Issue: The Fix Required: #[derive(Error, Debug)]
#[non_exhaustive]
pub enum UserQueryError {
// ... existing variants
}5. Identifier Escaping InconsistencyLocation: Issue: The code uses Recommendation: Use consistent escaping approach throughout or document why different methods are used. 📊 Test Coverage AnalysisStrengths:
Areas for Improvement:
🎯 Architecture & DesignPositive Aspects:
Suggestions:
📝 Minor Issues
✅ Recommendations
🔍 Overall AssessmentThe implementation demonstrates solid understanding of type safety and parameterized queries, but has a critical security vulnerability that must be addressed before merging. The architecture is well-designed with good separation of concerns and comprehensive testing, but needs immediate security fixes. Status: ❌ CHANGES REQUESTED - Critical security issue must be resolved |
|
Claude finished @NathanFlurry's task —— View job Code Review: clickhouse-user-query PackageTodo List:
🔒 Critical Security Issues1. SQL Injection Vulnerability (HIGH SEVERITY)Location: Ok(format!("{}[{}]", property_ident.0, format!("'{}'", subprop.replace("'", "\\'"))))Issue: The subproperty escaping only handles single quotes but is vulnerable to other injection vectors:
Fix Required: fn escape_clickhouse_string(s: &str) -> String {
s.replace('\\', "\\\\")
.replace('\'', "\\'")
.replace('`', "\\`")
.replace('\n', "\\n")
.replace('\r', "\\r")
.replace('\t', "\\t")
}
// Use in build_column_reference:
Ok(format!("{}[{}]", property_ident.0, format!("'{}'", escape_clickhouse_string(subprop))))2. Potential Panic (MEDIUM SEVERITY)Location: if name.chars().next().unwrap().is_numeric() {Issue: This will panic if Fix Required: if name.chars().next().map_or(false, |c| c.is_numeric()) {
return Err(UserQueryError::InvalidPropertyName(name.to_string()));
}🛠️ Code Quality Issues3. Missing Duplicate Property ValidationLocation: Issue: Fix Required: pub fn new(properties: Vec<Property>) -> Result<Self> {
let mut seen_names = std::collections::HashSet::new();
for prop in &properties {
if !seen_names.insert(&prop.name) {
return Err(UserQueryError::DuplicatePropertyName(prop.name.clone()));
}
}
Ok(Self { properties })
}4. Missing Future CompatibilityLocation: Issue: The Fix Required: #[derive(Error, Debug)]
#[non_exhaustive]
pub enum UserQueryError {
// ... existing variants
}5. Identifier Escaping InconsistencyLocation: Issue: The code uses Recommendation: Use consistent escaping approach throughout or document why different methods are used. 📊 Test Coverage AnalysisStrengths:
Areas for Improvement:
🎯 Architecture & DesignPositive Aspects:
Suggestions:
📝 Minor Issues
✅ Recommendations
🔍 Overall AssessmentThe implementation demonstrates solid understanding of type safety and parameterized queries, but has a critical security vulnerability that must be addressed before merging. The architecture is well-designed with good separation of concerns and comprehensive testing, but needs immediate security fixes. Status: ❌ CHANGES REQUESTED - Critical security issue must be resolved |
NathanFlurry
force-pushed
the
07-03-fix_toolchain_mark_external_deps
branch
from
34d28a2 to
ecc699b
Compare
NathanFlurry
force-pushed
the
06-05-feat_implement_clickhouse-user-query_
branch
from
4511ae5 to
96dc117
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review: clickhouse-user-query Package Todo List:
🔒 Critical Security Issues1. SQL Injection Vulnerability (HIGH SEVERITY)Location: Ok(format!("{}[{}]", property_ident.0, format!("'{}'", subprop.replace("'", "\\'"))))Issue: The subproperty escaping only handles single quotes but is vulnerable to other injection vectors:
Fix Required: fn escape_clickhouse_string(s: &str) -> String {
s.replace('\\', "\\\\")
.replace('\'', "\\'")
.replace('`', "\\`")
.replace('\n', "\\n")
.replace('\r', "\\r")
.replace('\t', "\\t")
}
// Use in build_column_reference:
Ok(format!("{}[{}]", property_ident.0, format!("'{}'", escape_clickhouse_string(subprop))))2. Potential Panic (MEDIUM SEVERITY)Location: if name.chars().next().unwrap().is_numeric() {Issue: This will panic if Fix Required: if name.chars().next().map_or(false, |c| c.is_numeric()) {
return Err(UserQueryError::InvalidPropertyName(name.to_string()));
}🛠️ Code Quality Issues3. Missing Duplicate Property ValidationLocation: Issue: Fix Required: pub fn new(properties: Vec<Property>) -> Result<Self> {
let mut seen_names = std::collections::HashSet::new();
for prop in &properties {
if !seen_names.insert(&prop.name) {
return Err(UserQueryError::DuplicatePropertyName(prop.name.clone()));
}
}
Ok(Self { properties })
}4. Missing Future CompatibilityLocation: Issue: The Fix Required: #[derive(Error, Debug)]
#[non_exhaustive]
pub enum UserQueryError {
// ... existing variants
}5. Identifier Escaping InconsistencyLocation: Issue: The code uses Recommendation: Use consistent escaping approach throughout or document why different methods are used. 📊 Test Coverage AnalysisStrengths:
Areas for Improvement:
🎯 Architecture & DesignPositive Aspects:
Suggestions:
📝 Minor Issues
✅ Recommendations
🔍 Overall AssessmentThe implementation demonstrates solid understanding of type safety and parameterized queries, but has a critical security vulnerability that must be addressed before merging. The architecture is well-designed with good separation of concerns and comprehensive testing, but needs immediate security fixes. Status: ❌ CHANGES REQUESTED - Critical security issue must be resolved |
|
Claude finished @NathanFlurry's task —— View job Code Review: clickhouse-user-query PackageTodo List:
🔒 Critical Security Issues1. SQL Injection Vulnerability (HIGH SEVERITY)Location: Ok(format!("{}[{}]", property_ident.0, format!("'{}'", subprop.replace("'", "\\'"))))Issue: The subproperty escaping only handles single quotes but is vulnerable to other injection vectors:
Fix Required: fn escape_clickhouse_string(s: &str) -> String {
s.replace('\\', "\\\\")
.replace('\'', "\\'")
.replace('`', "\\`")
.replace('\n', "\\n")
.replace('\r', "\\r")
.replace('\t', "\\t")
}
// Use in build_column_reference:
Ok(format!("{}[{}]", property_ident.0, format!("'{}'", escape_clickhouse_string(subprop))))2. Potential Panic (MEDIUM SEVERITY)Location: if name.chars().next().unwrap().is_numeric() {Issue: This will panic if Fix Required: if name.chars().next().map_or(false, |c| c.is_numeric()) {
return Err(UserQueryError::InvalidPropertyName(name.to_string()));
}🛠️ Code Quality Issues3. Missing Duplicate Property ValidationLocation: Issue: Fix Required: pub fn new(properties: Vec<Property>) -> Result<Self> {
let mut seen_names = std::collections::HashSet::new();
for prop in &properties {
if !seen_names.insert(&prop.name) {
return Err(UserQueryError::DuplicatePropertyName(prop.name.clone()));
}
}
Ok(Self { properties })
}4. Missing Future CompatibilityLocation: Issue: The Fix Required: #[derive(Error, Debug)]
#[non_exhaustive]
pub enum UserQueryError {
// ... existing variants
}5. Identifier Escaping InconsistencyLocation: Issue: The code uses Recommendation: Use consistent escaping approach throughout or document why different methods are used. 📊 Test Coverage AnalysisStrengths:
Areas for Improvement:
🎯 Architecture & DesignPositive Aspects:
Suggestions:
📝 Minor Issues
✅ Recommendations
🔍 Overall AssessmentThe implementation demonstrates solid understanding of type safety and parameterized queries, but has a critical security vulnerability that must be addressed before merging. The architecture is well-designed with good separation of concerns and comprehensive testing, but needs immediate security fixes. Status: ❌ CHANGES REQUESTED - Critical security issue must be resolved |
MasterPtato
force-pushed
the
07-03-fix_toolchain_mark_external_deps
branch
from
ecc699b to
34d28a2
Compare
MasterPtato
force-pushed
the
06-05-feat_implement_clickhouse-user-query_
branch
from
96dc117 to
4511ae5
Compare
|
Claude finished @NathanFlurry's task —— View job Code Review: clickhouse-user-query Package Todo List:
🔒 Critical Security Issues1. SQL Injection Vulnerability (HIGH SEVERITY)Location: Ok(format!("{}[{}]", property_ident.0, format!("'{}'", subprop.replace("'", "\\'"))))Issue: The subproperty escaping only handles single quotes but is vulnerable to other injection vectors:
Fix Required: fn escape_clickhouse_string(s: &str) -> String {
s.replace('\\', "\\\\")
.replace('\'', "\\'")
.replace('`', "\\`")
.replace('\n', "\\n")
.replace('\r', "\\r")
.replace('\t', "\\t")
}
// Use in build_column_reference:
Ok(format!("{}[{}]", property_ident.0, format!("'{}'", escape_clickhouse_string(subprop))))2. Potential Panic (MEDIUM SEVERITY)Location: if name.chars().next().unwrap().is_numeric() {Issue: This will panic if Fix Required: if name.chars().next().map_or(false, |c| c.is_numeric()) {
return Err(UserQueryError::InvalidPropertyName(name.to_string()));
}🛠️ Code Quality Issues3. Missing Duplicate Property ValidationLocation: Issue: Fix Required: pub fn new(properties: Vec<Property>) -> Result<Self> {
let mut seen_names = std::collections::HashSet::new();
for prop in &properties {
if !seen_names.insert(&prop.name) {
return Err(UserQueryError::DuplicatePropertyName(prop.name.clone()));
}
}
Ok(Self { properties })
}4. Missing Future CompatibilityLocation: Issue: The Fix Required: #[derive(Error, Debug)]
#[non_exhaustive]
pub enum UserQueryError {
// ... existing variants
}5. Identifier Escaping InconsistencyLocation: Issue: The code uses Recommendation: Use consistent escaping approach throughout or document why different methods are used. 📊 Test Coverage AnalysisStrengths:
Areas for Improvement:
🎯 Architecture & DesignPositive Aspects:
Suggestions:
📝 Minor Issues
✅ Recommendations
🔍 Overall AssessmentThe implementation demonstrates solid understanding of type safety and parameterized queries, but has a critical security vulnerability that must be addressed before merging. The architecture is well-designed with good separation of concerns and comprehensive testing, but needs immediate security fixes. Status: ❌ CHANGES REQUESTED - Critical security issue must be resolved |
|
Claude finished @NathanFlurry's task —— View job Code Review: clickhouse-user-query Package Todo List:
🔒 Critical Security Issues1. SQL Injection Vulnerability (HIGH SEVERITY)Location: Ok(format!("{}[{}]", property_ident.0, format!("'{}'", subprop.replace("'", "\\'"))))Issue: The subproperty escaping only handles single quotes but is vulnerable to other injection vectors:
Fix Required: fn escape_clickhouse_string(s: &str) -> String {
s.replace('\\', "\\\\")
.replace('\'', "\\'")
.replace('`', "\\`")
.replace('\n', "\\n")
.replace('\r', "\\r")
.replace('\t', "\\t")
}
// Use in build_column_reference:
Ok(format!("{}[{}]", property_ident.0, format!("'{}'", escape_clickhouse_string(subprop))))2. Potential Panic (MEDIUM SEVERITY)Location: if name.chars().next().unwrap().is_numeric() {Issue: This will panic if Fix Required: if name.chars().next().map_or(false, |c| c.is_numeric()) {
return Err(UserQueryError::InvalidPropertyName(name.to_string()));
}🛠️ Code Quality Issues3. Missing Duplicate Property ValidationLocation: Issue: Fix Required: pub fn new(properties: Vec<Property>) -> Result<Self> {
let mut seen_names = std::collections::HashSet::new();
for prop in &properties {
if !seen_names.insert(&prop.name) {
return Err(UserQueryError::DuplicatePropertyName(prop.name.clone()));
}
}
Ok(Self { properties })
}4. Missing Future CompatibilityLocation: Issue: The Fix Required: #[derive(Error, Debug)]
#[non_exhaustive]
pub enum UserQueryError {
// ... existing variants
}5. Identifier Escaping InconsistencyLocation: Issue: The code uses Recommendation: Use consistent escaping approach throughout or document why different methods are used. 📊 Test Coverage AnalysisStrengths:
Areas for Improvement:
🎯 Architecture & DesignPositive Aspects:
Suggestions:
📝 Minor Issues
✅ Recommendations
🔍 Overall AssessmentThe implementation demonstrates solid understanding of type safety and parameterized queries, but has a critical security vulnerability that must be addressed before merging. The architecture is well-designed with good separation of concerns and comprehensive testing, but needs immediate security fixes. Status: ❌ CHANGES REQUESTED - Critical security issue must be resolved |
Merge activity
|
<!-- Please make sure there is an issue that this PR is correlated to. --> ## Changes <!-- If there are frontend changes, please include screenshots. -->
Changes