From 6f368bbce57685ae572f4f3cac023fa1f58411aa Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Dec 2025 13:18:17 +0000 Subject: [PATCH 1/4] Initial plan From dca43b0e050ec18e542af5fb2bce279f831eeb99 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Dec 2025 13:26:37 +0000 Subject: [PATCH 2/4] Initial analysis: identified bug in AddTablePrefixToColumns Co-authored-by: warkanum <208308+warkanum@users.noreply.github.com> --- pkg/common/test_addprefix_test.go | 63 ++++++++++++++++++++++++++++ pkg/common/test_unregistered_test.go | 51 ++++++++++++++++++++++ 2 files changed, 114 insertions(+) create mode 100644 pkg/common/test_addprefix_test.go create mode 100644 pkg/common/test_unregistered_test.go diff --git a/pkg/common/test_addprefix_test.go b/pkg/common/test_addprefix_test.go new file mode 100644 index 0000000..377040e --- /dev/null +++ b/pkg/common/test_addprefix_test.go @@ -0,0 +1,63 @@ +package common + +import ( +"fmt" +"testing" +) + +func TestAddTablePrefixToColumns_BugRepro(t *testing.T) { +tests := []struct { +name string +where string +tableName string +expected string +}{ +{ +name: "SQL literal true - should not add prefix", +where: "true", +tableName: "mastertask", +expected: "true", +}, +{ +name: "SQL literal TRUE uppercase - should not add prefix", +where: "TRUE", +tableName: "mastertask", +expected: "TRUE", +}, +{ +name: "SQL literal false - should not add prefix", +where: "false", +tableName: "mastertask", +expected: "false", +}, +{ +name: "SQL literal null - should not add prefix", +where: "null", +tableName: "mastertask", +expected: "null", +}, +{ +name: "SQL literal NULL uppercase - should not add prefix", +where: "NULL", +tableName: "mastertask", +expected: "NULL", +}, +{ +name: "Multiple true conditions", +where: "true AND true", +tableName: "mastertask", +expected: "true AND true", +}, +} + +for _, tt := range tests { +t.Run(tt.name, func(t *testing.T) { +result := AddTablePrefixToColumns(tt.where, tt.tableName) +if result != tt.expected { +t.Errorf("AddTablePrefixToColumns(%q, %q) = %q; want %q", tt.where, tt.tableName, result, tt.expected) +} else { +fmt.Printf("✓ Test passed: %s\n", tt.name) +} +}) +} +} diff --git a/pkg/common/test_unregistered_test.go b/pkg/common/test_unregistered_test.go new file mode 100644 index 0000000..4e4d4a2 --- /dev/null +++ b/pkg/common/test_unregistered_test.go @@ -0,0 +1,51 @@ +package common + +import ( +"fmt" +"testing" +) + +func TestAddTablePrefixToColumns_UnregisteredModel(t *testing.T) { +tests := []struct { +name string +where string +tableName string +expected string +}{ +{ +name: "SQL literal true with unregistered model - should not add prefix", +where: "true", +tableName: "unregistered_table", +expected: "true", +}, +{ +name: "SQL literal false with unregistered model - should not add prefix", +where: "false", +tableName: "unregistered_table", +expected: "false", +}, +{ +name: "SQL literal null with unregistered model - should not add prefix", +where: "null", +tableName: "unregistered_table", +expected: "null", +}, +{ +name: "Valid column with unregistered model - should not add prefix (no validation)", +where: "status = 'active'", +tableName: "unregistered_table", +expected: "unregistered_table.status = 'active'", +}, +} + +for _, tt := range tests { +t.Run(tt.name, func(t *testing.T) { +result := AddTablePrefixToColumns(tt.where, tt.tableName) +if result != tt.expected { +t.Errorf("AddTablePrefixToColumns(%q, %q) = %q; want %q", tt.where, tt.tableName, result, tt.expected) +} else { +fmt.Printf("✓ Test passed: %s\n", tt.name) +} +}) +} +} From aa9b7312f601d52ed924e0fd52c907bf598e6e71 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Dec 2025 13:31:18 +0000 Subject: [PATCH 3/4] Fix AddTablePrefixToColumns to handle parenthesized AND conditions correctly Co-authored-by: warkanum <208308+warkanum@users.noreply.github.com> --- pkg/common/sql_helpers.go | 67 ++++++++++++++++++++++++- pkg/common/sql_helpers_test.go | 73 ++++++++++++++++++++++++++++ pkg/common/test_addprefix_test.go | 63 ------------------------ pkg/common/test_unregistered_test.go | 51 ------------------- 4 files changed, 138 insertions(+), 116 deletions(-) delete mode 100644 pkg/common/test_addprefix_test.go delete mode 100644 pkg/common/test_unregistered_test.go diff --git a/pkg/common/sql_helpers.go b/pkg/common/sql_helpers.go index 6730db6..88142e1 100644 --- a/pkg/common/sql_helpers.go +++ b/pkg/common/sql_helpers.go @@ -265,6 +265,41 @@ func stripOuterParentheses(s string) string { } } +// stripOneOuterParentheses removes only one level of matching outer parentheses from a string +// Unlike stripOuterParentheses, this only strips once, preserving nested parentheses +func stripOneOuterParentheses(s string) string { + s = strings.TrimSpace(s) + + if len(s) < 2 || s[0] != '(' || s[len(s)-1] != ')' { + return s + } + + // Check if these parentheses match (i.e., they're the outermost pair) + depth := 0 + matched := false + for i := 0; i < len(s); i++ { + switch s[i] { + case '(': + depth++ + case ')': + depth-- + if depth == 0 && i == len(s)-1 { + matched = true + } else if depth == 0 { + // Found a closing paren before the end, so outer parens don't match + return s + } + } + } + + if !matched { + return s + } + + // Strip only the outer parentheses + return strings.TrimSpace(s[1 : len(s)-1]) +} + // splitByAND splits a WHERE clause by AND operators (case-insensitive) // This is parenthesis-aware and won't split on AND operators inside subqueries func splitByAND(where string) []string { @@ -683,8 +718,8 @@ func AddTablePrefixToColumns(where string, tableName string) string { // - No valid column reference is found // - The column doesn't exist in the table (when validColumns is provided) func addPrefixToSingleCondition(cond string, tableName string, validColumns map[string]bool) string { - // Strip outer grouping parentheses to get to the actual condition - strippedCond := stripOuterParentheses(cond) + // Strip one level of outer grouping parentheses to get to the actual condition + strippedCond := stripOneOuterParentheses(cond) // Skip SQL literals and trivial conditions (true, false, null, 1=1, etc.) if IsSQLExpression(strippedCond) || IsTrivialCondition(strippedCond) { @@ -692,6 +727,34 @@ func addPrefixToSingleCondition(cond string, tableName string, validColumns map[ return cond } + // After stripping outer parentheses, check if there are multiple AND-separated conditions + // at the top level. If so, split and process each separately to avoid incorrectly + // treating "true AND status" as a single column name. + subConditions := splitByAND(strippedCond) + if len(subConditions) > 1 { + // Multiple conditions found - process each separately + logger.Debug("Found %d sub-conditions after stripping parentheses, processing separately", len(subConditions)) + processedConditions := make([]string, 0, len(subConditions)) + for _, subCond := range subConditions { + // Recursively process each sub-condition + processed := addPrefixToSingleCondition(subCond, tableName, validColumns) + processedConditions = append(processedConditions, processed) + } + result := strings.Join(processedConditions, " AND ") + // Preserve original outer parentheses if they existed + if cond != strippedCond { + result = "(" + result + ")" + } + return result + } + + // If we stripped parentheses and still have more parentheses, recursively process + if cond != strippedCond && strings.HasPrefix(strippedCond, "(") && strings.HasSuffix(strippedCond, ")") { + // Recursively handle nested parentheses + processed := addPrefixToSingleCondition(strippedCond, tableName, validColumns) + return "(" + processed + ")" + } + // Extract the left side of the comparison (before the operator) columnRef := extractLeftSideOfComparison(strippedCond) if columnRef == "" { diff --git a/pkg/common/sql_helpers_test.go b/pkg/common/sql_helpers_test.go index d4a0706..6f2a4ca 100644 --- a/pkg/common/sql_helpers_test.go +++ b/pkg/common/sql_helpers_test.go @@ -658,3 +658,76 @@ func TestSanitizeWhereClauseWithModel(t *testing.T) { }) } } + +func TestAddTablePrefixToColumns_ComplexConditions(t *testing.T) { +tests := []struct { +name string +where string +tableName string +expected string +}{ +{ +name: "Parentheses with true AND condition - should not prefix true", +where: "(true AND status = 'active')", +tableName: "mastertask", +expected: "(true AND mastertask.status = 'active')", +}, +{ +name: "Parentheses with multiple conditions including true", +where: "(true AND status = 'active' AND id > 5)", +tableName: "mastertask", +expected: "(true AND mastertask.status = 'active' AND mastertask.id > 5)", +}, +{ +name: "Nested parentheses with true", +where: "((true AND status = 'active'))", +tableName: "mastertask", +expected: "((true AND mastertask.status = 'active'))", +}, +{ +name: "Mixed: false AND valid conditions", +where: "(false AND name = 'test')", +tableName: "mastertask", +expected: "(false AND mastertask.name = 'test')", +}, +{ +name: "Mixed: null AND valid conditions", +where: "(null AND status = 'active')", +tableName: "mastertask", +expected: "(null AND mastertask.status = 'active')", +}, +{ +name: "Multiple true conditions in parentheses", +where: "(true AND true AND status = 'active')", +tableName: "mastertask", +expected: "(true AND true AND mastertask.status = 'active')", +}, +{ +name: "Simple true without parens - should not prefix", +where: "true", +tableName: "mastertask", +expected: "true", +}, +{ +name: "Simple condition without parens - should prefix", +where: "status = 'active'", +tableName: "mastertask", +expected: "mastertask.status = 'active'", +}, +{ +name: "Unregistered table with true - should not prefix true", +where: "(true AND status = 'active')", +tableName: "unregistered_table", +expected: "(true AND unregistered_table.status = 'active')", +}, +} + +for _, tt := range tests { +t.Run(tt.name, func(t *testing.T) { +result := AddTablePrefixToColumns(tt.where, tt.tableName) +if result != tt.expected { +t.Errorf("AddTablePrefixToColumns(%q, %q) = %q; want %q", tt.where, tt.tableName, result, tt.expected) +} +}) +} +} diff --git a/pkg/common/test_addprefix_test.go b/pkg/common/test_addprefix_test.go deleted file mode 100644 index 377040e..0000000 --- a/pkg/common/test_addprefix_test.go +++ /dev/null @@ -1,63 +0,0 @@ -package common - -import ( -"fmt" -"testing" -) - -func TestAddTablePrefixToColumns_BugRepro(t *testing.T) { -tests := []struct { -name string -where string -tableName string -expected string -}{ -{ -name: "SQL literal true - should not add prefix", -where: "true", -tableName: "mastertask", -expected: "true", -}, -{ -name: "SQL literal TRUE uppercase - should not add prefix", -where: "TRUE", -tableName: "mastertask", -expected: "TRUE", -}, -{ -name: "SQL literal false - should not add prefix", -where: "false", -tableName: "mastertask", -expected: "false", -}, -{ -name: "SQL literal null - should not add prefix", -where: "null", -tableName: "mastertask", -expected: "null", -}, -{ -name: "SQL literal NULL uppercase - should not add prefix", -where: "NULL", -tableName: "mastertask", -expected: "NULL", -}, -{ -name: "Multiple true conditions", -where: "true AND true", -tableName: "mastertask", -expected: "true AND true", -}, -} - -for _, tt := range tests { -t.Run(tt.name, func(t *testing.T) { -result := AddTablePrefixToColumns(tt.where, tt.tableName) -if result != tt.expected { -t.Errorf("AddTablePrefixToColumns(%q, %q) = %q; want %q", tt.where, tt.tableName, result, tt.expected) -} else { -fmt.Printf("✓ Test passed: %s\n", tt.name) -} -}) -} -} diff --git a/pkg/common/test_unregistered_test.go b/pkg/common/test_unregistered_test.go deleted file mode 100644 index 4e4d4a2..0000000 --- a/pkg/common/test_unregistered_test.go +++ /dev/null @@ -1,51 +0,0 @@ -package common - -import ( -"fmt" -"testing" -) - -func TestAddTablePrefixToColumns_UnregisteredModel(t *testing.T) { -tests := []struct { -name string -where string -tableName string -expected string -}{ -{ -name: "SQL literal true with unregistered model - should not add prefix", -where: "true", -tableName: "unregistered_table", -expected: "true", -}, -{ -name: "SQL literal false with unregistered model - should not add prefix", -where: "false", -tableName: "unregistered_table", -expected: "false", -}, -{ -name: "SQL literal null with unregistered model - should not add prefix", -where: "null", -tableName: "unregistered_table", -expected: "null", -}, -{ -name: "Valid column with unregistered model - should not add prefix (no validation)", -where: "status = 'active'", -tableName: "unregistered_table", -expected: "unregistered_table.status = 'active'", -}, -} - -for _, tt := range tests { -t.Run(tt.name, func(t *testing.T) { -result := AddTablePrefixToColumns(tt.where, tt.tableName) -if result != tt.expected { -t.Errorf("AddTablePrefixToColumns(%q, %q) = %q; want %q", tt.where, tt.tableName, result, tt.expected) -} else { -fmt.Printf("✓ Test passed: %s\n", tt.name) -} -}) -} -} From 8e8c3c6de6cb8cc72657352c5dd43724a8b9eaaf Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 30 Dec 2025 13:36:29 +0000 Subject: [PATCH 4/4] Refactor: Extract common logic from stripOuterParentheses functions Co-authored-by: warkanum <208308+warkanum@users.noreply.github.com> --- pkg/common/sql_helpers.go | 46 ++++++++++++--------------------------- 1 file changed, 14 insertions(+), 32 deletions(-) diff --git a/pkg/common/sql_helpers.go b/pkg/common/sql_helpers.go index 88142e1..0af6616 100644 --- a/pkg/common/sql_helpers.go +++ b/pkg/common/sql_helpers.go @@ -234,44 +234,26 @@ func stripOuterParentheses(s string) string { s = strings.TrimSpace(s) for { - if len(s) < 2 || s[0] != '(' || s[len(s)-1] != ')' { + stripped, wasStripped := stripOneMatchingOuterParen(s) + if !wasStripped { return s } - - // Check if these parentheses match (i.e., they're the outermost pair) - depth := 0 - matched := false - for i := 0; i < len(s); i++ { - switch s[i] { - case '(': - depth++ - case ')': - depth-- - if depth == 0 && i == len(s)-1 { - matched = true - } else if depth == 0 { - // Found a closing paren before the end, so outer parens don't match - return s - } - } - } - - if !matched { - return s - } - - // Strip the outer parentheses and continue - s = strings.TrimSpace(s[1 : len(s)-1]) + s = stripped } } // stripOneOuterParentheses removes only one level of matching outer parentheses from a string // Unlike stripOuterParentheses, this only strips once, preserving nested parentheses func stripOneOuterParentheses(s string) string { - s = strings.TrimSpace(s) + stripped, _ := stripOneMatchingOuterParen(strings.TrimSpace(s)) + return stripped +} +// stripOneMatchingOuterParen is a helper that strips one matching pair of outer parentheses +// Returns the stripped string and a boolean indicating if stripping occurred +func stripOneMatchingOuterParen(s string) (string, bool) { if len(s) < 2 || s[0] != '(' || s[len(s)-1] != ')' { - return s + return s, false } // Check if these parentheses match (i.e., they're the outermost pair) @@ -287,17 +269,17 @@ func stripOneOuterParentheses(s string) string { matched = true } else if depth == 0 { // Found a closing paren before the end, so outer parens don't match - return s + return s, false } } } if !matched { - return s + return s, false } - // Strip only the outer parentheses - return strings.TrimSpace(s[1 : len(s)-1]) + // Strip the outer parentheses + return strings.TrimSpace(s[1 : len(s)-1]), true } // splitByAND splits a WHERE clause by AND operators (case-insensitive)