Advanced SQL Analytics Framework#9
Conversation
… vulnerability (#94942) * disable sql expressions remove duckdb ref * Run `make update-workspace` --------- Co-authored-by: Scott Lepper <scott.lepper@gmail.com>
WalkthroughThis change removes several dependencies from the Go module, disables SQL expressions by default via a feature gate, and introduces a new in-memory SQL database abstraction with stub methods. Related code is updated to use the new local database implementation instead of the external Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExpressionQueryReader
participant SQLFeatureGate
participant SQLDB
User->>ExpressionQueryReader: Submit SQL query
ExpressionQueryReader->>SQLFeatureGate: Check if SQL expressions enabled
alt Not enabled
SQLFeatureGate-->>ExpressionQueryReader: Return false
ExpressionQueryReader-->>User: Return "sqlExpressions is not implemented" error
else Enabled
ExpressionQueryReader->>SQLDB: Execute SQL query
SQLDB-->>ExpressionQueryReader: Return result/error
ExpressionQueryReader-->>User: Return result/error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
|
@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: 3
🔭 Outside diff range comments (1)
pkg/expr/sql/parser.go (1)
22-40: Function will fail due to unimplemented database methods.The
TablesListfunction relies onRunCommandswhich currently returns "not implemented" error. This will cause all SQL parsing to fail until the database methods are properly implemented.Ensure the database implementation is completed before this code path is enabled, or add proper error handling for the transition period.
🧹 Nitpick comments (1)
pkg/expr/sql/parser.go (1)
23-23: Update variable name to reflect the new implementation.The variable name
duckDBis misleading since it no longer uses the DuckDB implementation. Consider renaming for clarity.- duckDB := NewInMemoryDB() + db := NewInMemoryDB()And update the subsequent usage:
- ret, err := duckDB.RunCommands([]string{cmd}) + ret, err := db.RunCommands([]string{cmd})
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.sumis excluded by!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (5)
go.mod(0 hunks)pkg/expr/reader.go(2 hunks)pkg/expr/sql/db.go(1 hunks)pkg/expr/sql/parser.go(1 hunks)pkg/expr/sql_command.go(1 hunks)
💤 Files with no reviewable changes (1)
- go.mod
🔇 Additional comments (3)
pkg/expr/sql/db.go (1)
20-22: Verify that theQueryFramesIntosignature matches the external go-duck interfaceBefore implementing this stub in
pkg/expr/sql/db.go, please confirm that your method signature aligns exactly with the originalQueryFramesIntodefinition in the go-duck package you’re replacing. Any mismatch in parameter order, types (e.g. missingcontext.Context), or return value will break compatibility.Checklist for verification:
Locate the go-duck package’s
QueryFramesIntointerface in its source or GoDoc.Compare its full signature (including any
context.Contextor other additional parameters) against this local stub:func (db *DB) QueryFramesInto( name string, query string, frames []*data.Frame, f *data.Frame, ) errorUpdate the local stub to match exactly (order, types, names) if there are any discrepancies.
pkg/expr/sql_command.go (1)
96-96: Good variable naming improvement.Renaming from
duckDBtodbbetter reflects the abstracted database interface.pkg/expr/reader.go (1)
129-132: Feature gate protects against unimplemented functionality.The feature gate check appropriately prevents SQL queries from reaching the unimplemented database methods. However, the gate logic itself is flawed (see comment on
enableSqlExpressionsfunction).
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | ||
| if enabled { | ||
| return false | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Critical logic error in feature gate implementation.
The enableSqlExpressions function has flawed logic that always returns false, effectively disabling SQL expressions unconditionally:
- Line 195 negates the feature flag check
- Both conditional branches return
false - SQL expressions will never be enabled regardless of the feature flag state
Fix the logic to properly respect the feature flag:
func enableSqlExpressions(h *ExpressionQueryReader) bool {
- enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
- if enabled {
- return false
- }
- return false
+ return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | |
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | |
| if enabled { | |
| return false | |
| } | |
| return false | |
| } | |
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | |
| return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | |
| } |
🤖 Prompt for AI Agents
In pkg/expr/reader.go around lines 194 to 200, the enableSqlExpressions function
incorrectly negates the feature flag check and returns false in all cases,
causing SQL expressions to always be disabled. Fix this by removing the negation
on the feature flag check and returning true when the feature flag is enabled,
otherwise false, so the function correctly reflects the feature flag state.
| db := sql.NewInMemoryDB() | ||
| var frame = &data.Frame{} | ||
|
|
||
| logger.Debug("Executing query", "query", gr.query, "frames", len(allFrames)) | ||
| err := duckDB.QueryFramesInto(gr.refID, gr.query, allFrames, frame) | ||
| err := db.QueryFramesInto(gr.refID, gr.query, allFrames, frame) |
There was a problem hiding this comment.
SQL query execution will fail due to unimplemented database methods.
The migration from duck.NewInMemoryDB() to sql.NewInMemoryDB() replaces a working implementation with stub methods. The QueryFramesInto call on line 100 will always return "not implemented" error.
Ensure either:
- The database methods are implemented before enabling this code path
- The feature gate in
reader.goproperly blocks SQL queries until implementation is complete
🤖 Prompt for AI Agents
In pkg/expr/sql_command.go around lines 96 to 100, the code uses
sql.NewInMemoryDB() whose methods like QueryFramesInto are not yet implemented,
causing SQL query execution to fail. To fix this, either implement the required
database methods in sql.NewInMemoryDB before enabling this code path, or ensure
that the feature gate in reader.go correctly blocks SQL queries until these
implementations are complete.
| type DB struct { | ||
| } | ||
|
|
||
| func (db *DB) TablesList(rawSQL string) ([]string, error) { | ||
| return nil, errors.New("not implemented") | ||
| } | ||
|
|
||
| func (db *DB) RunCommands(commands []string) (string, error) { | ||
| return "", errors.New("not implemented") | ||
| } | ||
|
|
||
| func (db *DB) QueryFramesInto(name string, query string, frames []*data.Frame, f *data.Frame) error { | ||
| return errors.New("not implemented") | ||
| } | ||
|
|
||
| func NewInMemoryDB() *DB { | ||
| return &DB{} | ||
| } |
There was a problem hiding this comment.
Critical: All database methods are unimplemented stubs.
This implementation replaces the external go-duck dependency with stub methods that return "not implemented" errors. This will break existing SQL functionality until proper implementations are provided.
Consider one of the following approaches:
- Implement the actual database functionality before merging
- Add feature flags to prevent SQL queries from reaching this code
- Return more descriptive errors indicating the feature is temporarily disabled
func (db *DB) TablesList(rawSQL string) ([]string, error) {
- return nil, errors.New("not implemented")
+ return nil, errors.New("SQL expressions are temporarily disabled - implementation in progress")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type DB struct { | |
| } | |
| func (db *DB) TablesList(rawSQL string) ([]string, error) { | |
| return nil, errors.New("not implemented") | |
| } | |
| func (db *DB) RunCommands(commands []string) (string, error) { | |
| return "", errors.New("not implemented") | |
| } | |
| func (db *DB) QueryFramesInto(name string, query string, frames []*data.Frame, f *data.Frame) error { | |
| return errors.New("not implemented") | |
| } | |
| func NewInMemoryDB() *DB { | |
| return &DB{} | |
| } | |
| func (db *DB) TablesList(rawSQL string) ([]string, error) { | |
| return nil, errors.New("SQL expressions are temporarily disabled - implementation in progress") | |
| } |
🤖 Prompt for AI Agents
In pkg/expr/sql/db.go from lines 9 to 26, all database methods are currently
unimplemented stubs returning generic "not implemented" errors, which will break
SQL functionality. To fix this, either implement the actual database logic for
these methods before merging, or add feature flags to disable SQL query
execution through this code path. Alternatively, update the error messages to
clearly indicate that the feature is temporarily disabled to improve clarity for
users.
|
This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 2 weeks if no further activity occurs. Please feel free to give a status update or ping for review. Thank you for your contributions! |
|
This pull request has been automatically closed because it has not had any further activity in the last 2 weeks. Thank you for your contributions! |
2 participants
Test 9
Summary by CodeRabbit
New Features
Bug Fixes
Chores