Advanced SQL Analytics Framework - #770
malinosqui wants to merge 15 commits into
Conversation
… vulnerability (#94942) * disable sql expressions remove duckdb ref * Run `make update-workspace` --------- Co-authored-by: Scott Lepper <scott.lepper@gmail.com>
Code Review Completed! 🔥The code review was successfully completed based on your current configurations. Kody Guide: Usage and ConfigurationInteracting with Kody
Current Kody ConfigurationReview OptionsThe following review options are enabled or disabled:
|
| func enableSqlExpressions(h *ExpressionQueryReader) bool { | ||
| enabled := !h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions) | ||
| if enabled { | ||
| return false | ||
| } | ||
| return false | ||
| } |
There was a problem hiding this comment.
Logic error in enableSqlExpressions unconditionally returns false, disabling the SQL expressions feature regardless of configuration. Return the featuremgmt.FlagSqlExpressions status directly via h.features.IsEnabledGlobally.
func enableSqlExpressions(h *ExpressionQueryReader) bool {
return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
}Prompt for LLM
File pkg/expr/reader.go:
Line 194 to 200:
WHAT: The `enableSqlExpressions` function unconditionally returns `false` due to flawed boolean logic. WHY: This completely disables the SQL expressions feature and causes it to fail with an error even when explicitly enabled via configuration. HOW: Return the feature flag directly: `return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)`
Suggested Code:
func enableSqlExpressions(h *ExpressionQueryReader) bool {
return h.features.IsEnabledGlobally(featuremgmt.FlagSqlExpressions)
}
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
| "github.com/scottlepp/go-duck/duck" | ||
|
|
||
| "github.com/grafana/grafana/pkg/apimachinery/errutil" | ||
| "github.com/grafana/grafana/pkg/expr/mathexp" |
There was a problem hiding this comment.
Implementation error masking in NewSQLCommand converts the "not implemented" status from sql.TablesList() into a misleading 400 Bad Request validation error. Return a 501 Not Implemented error or bypass parsing when the backend is missing to surface the true cause of the failure.
Prompt for LLM
File pkg/expr/sql_command.go:
Line 12:
**WHAT:** `sql.TablesList()` now consistently fails because the `NewInMemoryDB` implementation was replaced with a stub that always returns `"not implemented"`. However, `NewSQLCommand` swallows this implementation error and converts it to a `400 Bad Request` validation error (`"sql-invalid-sql"`).
**WHY:** If the SQL Expressions feature flag is enabled, every SQL query will immediately fail during parsing with a misleading "error reading SQL command" validation message, hiding the true cause that the underlying database execution engine (duckdb) has been removed.
**HOW:** Either explicitly check for the "not implemented" error and return a `501 Not Implemented` error, or bypass parsing entirely when the backend isn't implemented so the user gets a clear failure reason.
Talk to Kody by mentioning @kody
Was this suggestion helpful? React with 👍 or 👎 to help Kody learn from this interaction.
Here is a precise description of the pull request based on the provided code changes:
Overview
This pull request temporarily disables the SQL expressions functionality and removes the external DuckDB dependency from the expression engine. It replaces the active SQL execution engine with a stubbed local implementation, effectively putting the Advanced SQL Analytics feature into a "not implemented" state.
Key Changes
FlagSqlExpressions) in the expression query reader (pkg/expr/reader.go). However, the logic is currently hardcoded to always returnfalse, completely disabling the reading and execution of SQL queries.github.com/scottlepp/go-duck/duckdependency from both the SQL parser (pkg/expr/sql/parser.go) and the SQL command executor (pkg/expr/sql_command.go).pkg/expr/sql/db.go) to replace the DuckDB implementation. All core methods (TablesList,RunCommands,QueryFramesInto) are currently stubbed out and will return a"not implemented"error.go.work.sumto reflect the removal of unused dependencies (like Azure storage and Google HTTP replayers) and the addition of new underlying libraries (like Apache Thrift and Minio ASM tools).Functional Impact
From a user or system perspective, SQL-based expression queries are now entirely disabled. Any attempt to parse or execute a SQL expression query will immediately fail with a
"sqlExpressions is not implemented"or"not implemented"error. This appears to be a transitional update to safely gate the feature or prepare for a different underlying SQL execution engine.