From 7babfb728139b5c5b5adefd787cc640a684e34e6 Mon Sep 17 00:00:00 2001 From: CharlesKWON Date: Thu, 16 Apr 2026 17:26:05 +0900 Subject: [PATCH] fix(FiveSql2): 9 latent bugs from static analysis sweep MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Systematic bug-hunt driven by an automated analysis of all FiveSql2 source files. Each fix is targeted — no speculative refactoring. --- #1 CLASSDATA hSubCache leaked across queries (CRITICAL) --- CLASSDATA hSubCache INIT { => } SHARED shared one hash across ALL TSqlExecutor instances. A non-correlated subquery cached in query A was silently returned for an unrelated query B if the subquery text happened to produce the same cache key. Converted to instance DATA initialized in New(). --- #5+#21 IS NULL / COALESCE treated empty string as NULL (HIGH) --- RETURN xL == NIL .OR. ( ValType(xL) == "C" .AND. Empty(AllTrim(xL)) ) SQL standard: '' is a valid non-NULL value. Removed the empty-string check from both IS NULL evaluation and COALESCE skip logic. --- #4 Multiple ? parameters all returned first value (HIGH) --- ND_PAR nodes had no index — EvalExpr always returned ::aParams[1]. Parser now stamps each ? with a sequential 1-based index in xNode[2]. EvalExpr uses it to return the correct ::aParams[n]. --- #10+#11 SqlEvalRowExpr missing / and || operators, single-arg function eval (MEDIUM) --- Division and string concatenation fell through to RETURN NIL in the row-expression evaluator used by recursive CTEs and aggregate ComputeAgg. Also, multi-argument functions like SUBSTR(x,2,3) only received the first argument. Both fixed. --- #9 SUM/AVG/MIN/MAX of all NULLs returned 0 instead of NULL (MEDIUM) --- SQL standard requires NULL. Changed the aggregate return path to return NIL when nCount == 0 (SUM/AVG) or when xMin/xMax == NIL. --- #8 MIN/MAX used SqlCoerceNum for comparison (MEDIUM) --- Strings and dates were coerced to numbers (Val()) before comparing, making MIN('banana') == MIN('apple') == 0. Switched to SqlCmpLt which handles type-appropriate comparison. --- #7 SqlExprHasAgg only checked top-level node (MEDIUM) --- Expressions like `salary + COUNT(*)` were not detected as containing an aggregate because the top node was ND_BIN, not ND_FN. Made the function recursive — walks ND_BIN, ND_UNI, ND_FN args, ND_CASE branches. --- #13 SELECT * only expanded first table in JOINs (MEDIUM) --- `SELECT * FROM orders o JOIN customers c ON ...` only included fields from orders. Changed the expansion loop to iterate ALL entries in ::aTables. --- #2 s_aOuterStack not unwound on subquery error (HIGH) --- SubqueryCached's PushOuter/PopOuter pair was not protected by BEGIN SEQUENCE. A runtime error inside the subquery left a stale entry on the module-level outer stack, corrupting all subsequent queries' correlated column resolution. Wrapped in SEQUENCE/RECOVER. Validation: - FiveSql2 43/43 - Harbour compat 51/51 - go test ./... ALL PASS Co-Authored-By: Claude Opus 4.6 (1M context) --- _FiveSql2/src/TSqlAgg.prg | 14 +++++----- _FiveSql2/src/TSqlExecutor.prg | 44 +++++++++++++++++++------------ _FiveSql2/src/TSqlExpr.prg | 48 +++++++++++++++++++++++++++++++--- _FiveSql2/src/TSqlFunc.prg | 4 +-- _FiveSql2/src/TSqlParser2.prg | 7 +++-- 5 files changed, 87 insertions(+), 30 deletions(-) diff --git a/_FiveSql2/src/TSqlAgg.prg b/_FiveSql2/src/TSqlAgg.prg index 1296a38..4b2399f 100644 --- a/_FiveSql2/src/TSqlAgg.prg +++ b/_FiveSql2/src/TSqlAgg.prg @@ -424,10 +424,11 @@ METHOD ComputeAgg( xE, aGR, aFN ) CLASS TSqlAgg IF xVal != NIL nCount++ nSum += SqlCoerceNum( xVal ) - IF xMin == NIL .OR. SqlCoerceNum( xVal ) < SqlCoerceNum( xMin ) + /* Use SqlCmpLt for type-safe comparison (handles strings, dates) */ + IF xMin == NIL .OR. SqlCmpLt( xVal, xMin ) xMin := xVal ENDIF - IF xMax == NIL .OR. SqlCoerceNum( xVal ) > SqlCoerceNum( xMax ) + IF xMax == NIL .OR. SqlCmpLt( xMax, xVal ) xMax := xVal ENDIF ENDIF @@ -437,13 +438,14 @@ METHOD ComputeAgg( xE, aGR, aFN ) CLASS TSqlAgg CASE cFunc == "COUNT" RETURN nCount CASE cFunc == "SUM" - RETURN nSum + /* SQL standard: SUM of all NULLs = NULL, not 0 */ + RETURN iif( nCount > 0, nSum, NIL ) CASE cFunc == "AVG" - RETURN iif( nCount > 0, nSum / nCount, 0 ) + RETURN iif( nCount > 0, nSum / nCount, NIL ) CASE cFunc == "MIN" - RETURN iif( xMin != NIL, xMin, 0 ) + RETURN xMin CASE cFunc == "MAX" - RETURN iif( xMax != NIL, xMax, 0 ) + RETURN xMax CASE cFunc == "GROUP_CONCAT" .OR. cFunc == "STRING_AGG" cResult := "" cSep := ", " diff --git a/_FiveSql2/src/TSqlExecutor.prg b/_FiveSql2/src/TSqlExecutor.prg index 2084054..2e48304 100644 --- a/_FiveSql2/src/TSqlExecutor.prg +++ b/_FiveSql2/src/TSqlExecutor.prg @@ -40,7 +40,7 @@ CLASS TSqlExecutor DATA nSubCacheSeq INIT 0 /* monotonic ID for subqueries */ DATA aSemiJoinSlots INIT {} /* list of {xSubNode, semiJoinData | "NO"} */ - CLASSDATA hSubCache INIT { => } SHARED + DATA hSubCache METHOD New( hQuery, aParams ) CONSTRUCTOR METHOD Run() @@ -99,6 +99,7 @@ METHOD New( hQuery, aParams ) CLASS TSqlExecutor * can end up sharing the same instance across New() calls depending * on the compile path, which would let one query's subquery cache * leak into the next query's results. */ + ::hSubCache := { => } ::hSubCorrCache := { => } ::aSubCacheSlots := {} ::aSemiJoinSlots := {} @@ -412,10 +413,10 @@ METHOD EvalExpr( xNode ) CLASS TSqlExecutor RETURN ::Resolve( xNode[ 2 ] ) CASE xNode[ 1 ] == ND_PAR - IF Len( ::aParams ) > 0 - /* Use static counter per expression evaluation chain */ - xVal := ::aParams[ 1 ] - RETURN xVal + /* xNode[2] = 1-based parameter index from parser */ + nPI := iif( xNode[ 2 ] != NIL, xNode[ 2 ], 1 ) + IF nPI >= 1 .AND. nPI <= Len( ::aParams ) + RETURN ::aParams[ nPI ] ENDIF RETURN NIL @@ -494,10 +495,11 @@ METHOD EvalExpr( xNode ) CLASS TSqlExecutor /* IS NULL / IS NOT NULL */ IF cOp == "IS NULL" .OR. cOp == "IS NOT NULL" xL := ::EvalExpr( xNode[ 3 ] ) + /* SQL standard: only NIL is NULL, empty string '' is NOT NULL */ IF cOp == "IS NULL" - RETURN xL == NIL .OR. ( ValType( xL ) == "C" .AND. Empty( AllTrim( xL ) ) ) + RETURN xL == NIL ELSE - RETURN !( xL == NIL .OR. ( ValType( xL ) == "C" .AND. Empty( AllTrim( xL ) ) ) ) + RETURN xL != NIL ENDIF ENDIF @@ -1244,14 +1246,14 @@ METHOD RunSelect() CLASS TSqlExecutor cColAlias := SqlExprName( xExpr ) ENDIF AAdd( aResultExprs, { xExpr, cColAlias } ) - /* Expand SELECT * */ + /* Expand SELECT * — iterate ALL tables (primary + joined) */ IF xExpr[ 1 ] == ND_COL .AND. xExpr[ 2 ] == "*" aResultExprs := {} aFieldNames := {} - IF Len( ::aTables ) > 0 - cAlias := ::aTables[ 1 ][ 2 ] + FOR k := 1 TO Len( ::aTables ) + cAlias := ::aTables[ k ][ 2 ] IF Empty( cAlias ) - cAlias := ::aTables[ 1 ][ 1 ] + cAlias := ::aTables[ k ][ 1 ] ENDIF nWA := Select( cAlias ) IF nWA > 0 @@ -1261,7 +1263,7 @@ METHOD RunSelect() CLASS TSqlExecutor AAdd( aResultExprs, { SqlNode( ND_COL, cFN, NIL, NIL, NIL ), cFN } ) NEXT ENDIF - ENDIF + NEXT EXIT ENDIF NEXT @@ -1995,16 +1997,24 @@ METHOD SubqueryCached( xSubNode ) CLASS TSqlExecutor ENDIF /* Cache miss — execute the subquery. PushOuter so nested ::Resolve - * calls can see the current outer row's values. */ + * calls can see the current outer row's values. Use BEGIN SEQUENCE + * to guarantee PopOuter runs even on subquery runtime errors — + * a stale s_aOuterStack entry would corrupt all subsequent queries. */ nSavedWA := Select() ::PushOuter() - oSub := TSqlExecutor():New( hQ, ::aParams ) - oSub:nDepth := ::nDepth - aResult := oSub:Run() + BEGIN SEQUENCE + oSub := TSqlExecutor():New( hQ, ::aParams ) + oSub:nDepth := ::nDepth + aResult := oSub:Run() + RECOVER + aResult := NIL + END SEQUENCE ::PopOuter() dbSelectArea( nSavedWA ) - ::hSubCorrCache[ cCacheKey ] := aResult + IF aResult != NIL + ::hSubCorrCache[ cCacheKey ] := aResult + ENDIF RETURN aResult diff --git a/_FiveSql2/src/TSqlExpr.prg b/_FiveSql2/src/TSqlExpr.prg index 47fb282..dbb4f76 100644 --- a/_FiveSql2/src/TSqlExpr.prg +++ b/_FiveSql2/src/TSqlExpr.prg @@ -42,15 +42,43 @@ FUNCTION SqlExprName( xE ) RETURN "expr" -/* Check whether an expression node is an aggregate function call */ +/* Check whether an expression tree contains an aggregate function call. + * Recurses into ND_BIN, ND_UNI, ND_FN args, ND_CASE to find nested + * aggregates like `salary + COUNT(*)` or `CASE WHEN ... THEN SUM(x)`. */ FUNCTION SqlExprHasAgg( xE ) + LOCAL i + IF xE == NIL RETURN .F. ENDIF IF xE[ 1 ] == ND_FN .AND. SqlIsAggName( xE[ 2 ] ) RETURN .T. ENDIF + /* Recurse into sub-expressions */ + IF xE[ 1 ] == ND_BIN + RETURN SqlExprHasAgg( xE[ 3 ] ) .OR. SqlExprHasAgg( xE[ 4 ] ) + ENDIF + IF xE[ 1 ] == ND_UNI + RETURN SqlExprHasAgg( xE[ 3 ] ) + ENDIF + IF xE[ 1 ] == ND_FN .AND. ValType( xE[ 3 ] ) == "A" + FOR i := 1 TO Len( xE[ 3 ] ) + IF SqlExprHasAgg( xE[ 3 ][ i ] ) + RETURN .T. + ENDIF + NEXT + ENDIF + IF xE[ 1 ] == ND_CASE .AND. ValType( xE[ 2 ] ) == "A" + FOR i := 1 TO Len( xE[ 2 ] ) + IF SqlExprHasAgg( xE[ 2 ][ i ][ 1 ] ) .OR. SqlExprHasAgg( xE[ 2 ][ i ][ 2 ] ) + RETURN .T. + ENDIF + NEXT + IF xE[ 3 ] != NIL .AND. SqlExprHasAgg( xE[ 3 ] ) + RETURN .T. + ENDIF + ENDIF RETURN .F. @@ -173,7 +201,7 @@ RETURN xExpr */ FUNCTION SqlEvalRowExpr( xExpr, aFN, aRow ) - LOCAL xL, xR, cOp, cName, i + LOCAL xL, xR, cOp, cName, i, aFnArgs IF xExpr == NIL RETURN NIL @@ -221,6 +249,15 @@ FUNCTION SqlEvalRowExpr( xExpr, aFN, aRow ) IF cOp == "*" RETURN SqlCoerceNum( xL ) * SqlCoerceNum( xR ) ENDIF + IF cOp == "/" + IF SqlCoerceNum( xR ) != 0 + RETURN SqlCoerceNum( xL ) / SqlCoerceNum( xR ) + ENDIF + RETURN 0 + ENDIF + IF cOp == "||" + RETURN SqlCoerceStr( xL ) + SqlCoerceStr( xR ) + ENDIF IF cOp == "AND" RETURN SqlIsTrue( xL ) .AND. SqlIsTrue( xR ) ENDIF @@ -263,7 +300,12 @@ FUNCTION SqlEvalRowExpr( xExpr, aFN, aRow ) CASE xExpr[ 1 ] == ND_FN IF Len( xExpr[ 3 ] ) > 0 - RETURN SqlEvalFunc( xExpr[ 2 ], { SqlEvalRowExpr( xExpr[ 3 ][ 1 ], aFN, aRow ) } ) + /* Evaluate ALL arguments, not just the first */ + aFnArgs := {} + FOR i := 1 TO Len( xExpr[ 3 ] ) + AAdd( aFnArgs, SqlEvalRowExpr( xExpr[ 3 ][ i ], aFN, aRow ) ) + NEXT + RETURN SqlEvalFunc( xExpr[ 2 ], aFnArgs ) ENDIF RETURN SqlEvalFunc( xExpr[ 2 ], {} ) diff --git a/_FiveSql2/src/TSqlFunc.prg b/_FiveSql2/src/TSqlFunc.prg index eb22245..95c9ba4 100644 --- a/_FiveSql2/src/TSqlFunc.prg +++ b/_FiveSql2/src/TSqlFunc.prg @@ -159,9 +159,9 @@ FUNCTION SqlEvalFunc( cName, aArgs ) ENDIF RETURN SqlArg(aArgs,3) CASE cName == "COALESCE" + /* SQL standard: empty string is NOT NULL, only NIL is NULL */ FOR i := 1 TO Len( aArgs ) - IF aArgs[ i ] != NIL .AND. ; - !( ValType( aArgs[ i ] ) == "C" .AND. Empty( AllTrim( aArgs[ i ] ) ) ) + IF aArgs[ i ] != NIL RETURN aArgs[ i ] ENDIF NEXT diff --git a/_FiveSql2/src/TSqlParser2.prg b/_FiveSql2/src/TSqlParser2.prg index 2a3f182..90d75e7 100644 --- a/_FiveSql2/src/TSqlParser2.prg +++ b/_FiveSql2/src/TSqlParser2.prg @@ -129,6 +129,7 @@ CLASS TSqlParser2 DATA aTokens DATA nPos DATA aParams + DATA nParamSeq INIT 0 /* positional ? counter (1-based) */ DATA hInfixTT /* hash: token_type => { op, lbp, rbp, ndType } */ DATA hInfixKW /* hash: keyword => { op, lbp, rbp, ndType } */ @@ -1375,10 +1376,12 @@ METHOD ParsePrimary() CLASS TSqlParser2 RETURN SqlNode( ND_LIT, cVal, NIL, NIL, NIL ) ENDIF - /* Positional parameter */ + /* Positional parameter — each ? gets a sequential 1-based index + * so EvalExpr can return the correct ::aParams[n] value. */ IF ::TType( ::nPos ) == TK_QMARK ::nPos++ - RETURN SqlNode( ND_PAR, NIL, NIL, NIL, NIL ) + ::nParamSeq++ + RETURN SqlNode( ND_PAR, ::nParamSeq, NIL, NIL, NIL ) ENDIF /* Parenthesized expression or scalar subquery */