refactor: formula builder parsed tree and ltar builder #10977
Conversation
📝 WalkthroughWalkthroughThis update refactors and extends the formula query building functionality. A new helper function, Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client
participant F as formulaQueryBuilderv2
participant E as callExpressionBuilder
participant B as binaryExpressionBuilder
participant L as lookupOrLtarBuilder
C->>F: Submit formula query
alt Function Call
F->>E: Invoke callExpressionBuilder
E-->>F: Return function query SQL snippet
end
alt Binary Expression
F->>B: Invoke binaryExpressionBuilder
B-->>F: Return binary expression SQL snippet
end
alt Lookup/Link Operation
F->>L: Invoke lookupOrLtarBuilder
L-->>F: Return lookup query builder
end
F-->>C: Return constructed query
Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
packages/nocodb/src/db/formulav2/formula-query-builder.helpers.ts (2)
6-16: Unify similar cases for sum-based operations.
Handling multiple synonyms (ADD,SUM,FLOAT,NUMBER,ARITH) in separatecasestatements is fine, but they ultimately produce the samesumlogic. You might consolidate them into a single grouped case for clarity.case 'ADD': case 'SUM': case 'FLOAT': case 'NUMBER': case 'ARITH': - return ({ qb, cn }) => qb.clear('select').sum(cn); + // Single combined case to avoid repetition + return ({ qb, cn }) => qb.clear('select').sum(cn);
28-29: Remove the useless'CONCAT'case or differentiate it fromdefault.
Since'CONCAT'falls through todefaultlogic, remove this redundant case for cleaner code.- case 'CONCAT': default: return ({ qb, cn }) => qb.clear('select').concat(cn);🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/nocodb/src/db/formulav2/lookup-or-ltar-builder.ts (2)
21-43: Function signature clarity.
lookupOrLtarBuilderreceives many parameters that affect control flow (model, tableAlias,_formulaQueryBuilder, etc.). Consider documenting each parameter thoroughly or refactoring to reduce complexity.
401-409: Circular reference detection.
This block throws an error when a formula references itself. Consider adding a direct unit test to ensure this logic catches circular references as expected.Would you like help drafting a unit test for circular reference scenarios?
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts (2)
49-81: Remove unreachablebreak.Within the
case 'ADD'/'SUM'block, thereturnstatements in both theifandelseclauses make thebreak;at line 81 unreachable.if (pt.arguments.length > 1) { return fn(...) } else { return fn(...) } - break; + // break not needed since the code returns unconditionally above🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This code is unreachable
... because either this statement ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
109-249: Remove unreachablebreak.Inside the
case 'URL'block, there's areturnat line 248, causing the subsequentbreak;at line 249 to be unreachable.return fn( { ... }, prevBinaryOp, ); - break; + // break not needed since the code returns unconditionally above🧰 Tools
🪛 Biome (1.9.4)
[error] 249-249: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nocodb/src/db/formulav2/formula-query-builder.helpers.ts(1 hunks)packages/nocodb/src/db/formulav2/formula-query-builder.types.ts(1 hunks)packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts(5 hunks)packages/nocodb/src/db/formulav2/lookup-or-ltar-builder.ts(1 hunks)packages/nocodb/src/db/formulav2/parsed-tree-builder.ts(1 hunks)packages/nocodb/src/db/genRollupSelectv2.ts(2 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts (1)
packages/nocodb/src/db/formulav2/formula-query-builder.types.ts (2)
FnParsedTreeNode(38-46)TAliasToColumn(21-24)
🪛 Biome (1.9.4)
packages/nocodb/src/db/formulav2/formula-query-builder.helpers.ts
[error] 28-28: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts
[error] 81-81: This code is unreachable
... because either this statement ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 249-249: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts
[error] 459-460: Expected a parameter but instead found '}'.
Expected a parameter here.
(parse)
[error] 464-464: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 465-465: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 466-466: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 467-467: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 468-468: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 469-469: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 469-469: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 470-470: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 461-461: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 463-463: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.
(lint/suspicious/noConfusingLabels)
[error] 460-470: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: jest-unit-test
🔇 Additional comments (10)
packages/nocodb/src/db/genRollupSelectv2.ts (1)
2-2: Adopt new type import and parameter name.
Switching fromBaseModelSqlv2toIBaseModelSqlV2aligns with the ongoing refactor and naming convention changes, ensuring consistency across the codebase.Also applies to: 24-24
packages/nocodb/src/db/formulav2/formula-query-builder.types.ts (2)
13-17: Consistent usage ofIBaseModelSqlV2.
Renaming fromBaseModelSqlv2toIBaseModelSqlV2here and in other files ensures a clearer and more descriptive interface naming convention.
21-23: Corrected spelling forTAliasToColumnand usage in interface.
Good fix to renameTAliasToClumn→TAliasToColumn. This prevents type mismatches and clarifies its usage inaliasToColumn.Also applies to: 29-29
packages/nocodb/src/db/formulav2/lookup-or-ltar-builder.ts (1)
370-378:❓ Verification inconclusive
Verify repeated joins with
parentModel.
At line 303 (and beyond) we already join withparentModel.table_name. At lines 370-378, there's a second join referencingparentModel.table_nameagain. Please confirm that a second join is necessary or if this might unintentionally produce duplicate rows.
🏁 Script executed:
#!/bin/bash # Identify all joins referencing parentModel in this file to confirm correctness rg -A 3 $'\\.join\\(' packages/nocodb/src/db/formulav2/lookup-or-ltar-builder.tsLength of output: 1937
Double-Check Repeated Join on
parentModel.table_nameThe grep results confirm that there are multiple instances where
parentModel.table_nameis joined using the aliasnestedAlias(including around line 303 and again at lines 370–378). Please verify that the insertions of these joins are intentional and that they won’t lead to duplicate rows.
- Check if both joins using
parentModel.table_name(viaknex.rawwithnestedAlias) are needed.- Confirm that any repeated joining does not inadvertently create duplicate records in the results.
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts (2)
320-526: Implementation appears robust.These lines handle various operators (
&,+,==,!=,/) comprehensively, including special cases for multiple DB clients. No immediate issues found.
83-108:❓ Verification inconclusive
Verify database behavior for
CONCAT().For non-SQLite databases, the code relies on native
CONCAT()or a mapped function to handleNULLarguments (e.g., skippingNULL). Ensure that this behavior aligns with your requirements in PostgreSQL, MSSQL, etc.
🏁 Script executed:
#!/bin/bash # Checks how Postgres handles CONCAT with NULL arguments. Adjust the container & user as needed. cat <<EOF | docker exec -i my-postgres psql -U postgres SELECT CONCAT('a', NULL, 'b') AS test_concat; EOF # Expecting "ab" if Postgres CONCAT ignores NULL. Verify it matches your intended behavior.Length of output: 148
Action Required: Manual Verification of CONCAT() Behavior on Non-SQLite Databases
The current automated test for PostgreSQL's handling of
CONCAT('a', NULL, 'b')did not execute (error:docker: command not found). Please verify manually that the nativeCONCAT()function or your mapped implementation behaves as intended—specifically, thatNULLarguments are treated as empty strings (i.e., the result should be'ab'). This behavior is critical for consistency across PostgreSQL, MSSQL, and other supported databases.
- Next Steps:
- Run the test manually on your PostgreSQL instance (e.g., using
psql) to confirm the output.- Verify similar behavior (or adjust mapping functions) for any other non-SQLite databases used in your project.
packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (4)
13-17: Modular import approach looks good.Importing
lookupOrLtarBuilder,binaryExpressionBuilder, andcallExpressionBuilderaligns well with the new refactored structure, promoting better maintainability.
186-195: Clearer handling of lookups and linking.Using
lookupOrLtarBuilderforLookup/LinkToAnotherRecordlogic centralizes and clarifies these behaviors, reducing inline complexity.
353-353: Consistent function invocation.The
_formulaQueryBuildercall using the newly introducedfnapproach correctly implements the modular builder pattern.
458-470:✅ Verification successful
Address static analysis parse warnings.
The linter flags potential parse issues around lines 459-470. Verify compilation to confirm that your TypeScript/JavaScript syntax is valid. The code may be correct but unrecognized by the static analysis tool.
🏁 Script executed:
#!/bin/bash # Check TypeScript compilation npm install --no-save typescript npx tsc --noEmitLength of output: 5027
Static Analysis Verification Passed
The code in
packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts(lines 458–470) compiles cleanly with TypeScript (v5.8.2). The successful compilation indicates that the syntax is valid despite the linter flagging potential parse issues. It appears that these static analysis warnings are likely false positives or a configuration mismatch with the linting tool.
- Verification: The
npx tsc --noEmitcommand completed without any errors.- Recommendation: If the linter continues to issue warnings, consider reviewing its configuration or suppressing known false positives.
No changes to the code are required.
🧰 Tools
🪛 Biome (1.9.4)
[error] 459-460: Expected a parameter but instead found '}'.
Expected a parameter here.
(parse)
[error] 464-464: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 465-465: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 466-466: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 467-467: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 468-468: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 469-469: Expected an expression, or an assignment but instead found ':'.
Expected an expression, or an assignment here.
(parse)
[error] 469-469: Expected an expression but instead found ']'.
Expected an expression here.
(parse)
[error] 470-470: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 461-461: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 463-463: Unexpected label.
Only loops should be labeled.
The use of labels for other statements is suspicious and unfamiliar.(lint/suspicious/noConfusingLabels)
[error] 460-470: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
| case 'AVG': | ||
| return ({ qb, cn }) => qb.clear('select').sum(cn); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement true average logic or remove the placeholder.
Currently, 'AVG' returns the sum instead of an actual average. Consider implementing the average calculation now or removing the placeholder to avoid confusion.
case 'AVG':
- return ({ qb, cn }) => qb.clear('select').sum(cn);
+ return ({ qb, cn, knex }) =>
+ qb
+ .clear('select')
+ .select(knex.raw('AVG(??)', [cn]));📝 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.
| case 'AVG': | |
| return ({ qb, cn }) => qb.clear('select').sum(cn); | |
| case 'AVG': | |
| return ({ qb, cn, knex }) => | |
| qb | |
| .clear('select') | |
| .select(knex.raw('AVG(??)', [cn])); |
| switch (relationType) { | ||
| case RelationTypes.BELONGS_TO: | ||
| selectQb = knex( | ||
| knex.raw(`?? as ??`, [ | ||
| baseModelSqlv2.getTnPath(parentModel.table_name), | ||
| alias, | ||
| ]), | ||
| ).where( | ||
| `${alias}.${parentColumn.column_name}`, | ||
| knex.raw(`??`, [ | ||
| `${ | ||
| tableAlias ?? baseModelSqlv2.getTnPath(childModel.table_name) | ||
| }.${childColumn.column_name}`, | ||
| ]), | ||
| ); | ||
| lookupColumn = lookupColumn ?? parentModel.displayValue; | ||
| break; | ||
| case RelationTypes.HAS_MANY: | ||
| isArray = relation.type !== RelationTypes.ONE_TO_ONE; | ||
| selectQb = knex( | ||
| knex.raw(`?? as ??`, [ | ||
| baseModelSqlv2.getTnPath(childModel.table_name), | ||
| alias, | ||
| ]), | ||
| ).where( | ||
| `${alias}.${childColumn.column_name}`, | ||
| knex.raw(`??`, [ | ||
| `${ | ||
| tableAlias ?? baseModelSqlv2.getTnPath(parentModel.table_name) | ||
| }.${parentColumn.column_name}`, | ||
| ]), | ||
| ); | ||
| lookupColumn = lookupColumn ?? childModel.displayValue; | ||
| break; | ||
| case RelationTypes.MANY_TO_MANY: | ||
| { | ||
| isArray = true; | ||
| const mmModel = await relation.getMMModel(context); | ||
| const mmParentColumn = await relation.getMMParentColumn(context); | ||
| const mmChildColumn = await relation.getMMChildColumn(context); | ||
|
|
||
| const assocAlias = `__nc${aliasCount++}`; | ||
| selectQb = knex( | ||
| knex.raw(`?? as ??`, [ | ||
| baseModelSqlv2.getTnPath(parentModel.table_name), | ||
| alias, | ||
| ]), | ||
| ) | ||
| .join( | ||
| knex.raw(`?? as ??`, [ | ||
| baseModelSqlv2.getTnPath(mmModel.table_name), | ||
| assocAlias, | ||
| ]), | ||
| `${assocAlias}.${mmParentColumn.column_name}`, | ||
| `${alias}.${parentColumn.column_name}`, | ||
| ) | ||
| .where( | ||
| `${assocAlias}.${mmChildColumn.column_name}`, | ||
| knex.raw(`??`, [ | ||
| `${ | ||
| tableAlias ?? | ||
| baseModelSqlv2.getTnPath(childModel.table_name) | ||
| }.${childColumn.column_name}`, | ||
| ]), | ||
| ); | ||
| lookupColumn = lookupColumn ?? parentModel.displayValue; | ||
| } | ||
| break; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate large switch logic for relation types.
This block handles BELONGS_TO, HAS_MANY, and MANY_TO_MANY with very similar patterns: building query aliases, setting up joins, etc. Refactoring each relation type’s logic into dedicated helper functions can reduce duplication and improve readability.
c43894c to
e8b3a72
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/nocodb/src/db/formulav2/formula-query-builder.helpers.ts (1)
18-20:⚠️ Potential issueImplement true average logic instead of using sum
The
AVGcase returns a sum operation instead of an actual average calculation, which is misleading and incorrect.case 'AVG': - return ({ qb, cn }) => qb.clear('select').sum(cn); + return ({ qb, cn, knex }) => + qb + .clear('select') + .select(knex.raw('AVG(??)', [cn]));Consider implementing the more complex average calculation that's outlined in the TODO comments (lines 21-27) if needed for specific use cases.
🧹 Nitpick comments (4)
packages/nocodb/src/db/formulav2/formula-query-builder.helpers.ts (1)
28-30: Remove redundant CONCAT caseThe CONCAT case is redundant since it performs the same operation as the default case that follows it.
- case 'CONCAT': default: return ({ qb, cn }) => qb.clear('select').concat(cn);🧰 Tools
🪛 Biome (1.9.4)
[error] 28-28: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts (3)
81-81: Remove unreachable break statementThis break statement is unreachable because the function will always return before reaching this line.
} - break;🧰 Tools
🪛 Biome (1.9.4)
[error] 81-81: This code is unreachable
... because either this statement ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
249-249: Remove unreachable break statementThis break statement is unreachable because the function will always return before reaching this line.
} - break;🧰 Tools
🪛 Biome (1.9.4)
[error] 249-249: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
302-574: Complex binary expression handling with thorough database-specific logicThe binaryExpressionBuilder function is well-structured with clear handling for different database types and comparison scenarios. The special cases for comparing with empty strings, handling NULL values, and converting types are properly implemented.
However, the function is quite complex (270+ lines). Consider breaking it down into smaller helper functions for specific operations to improve maintainability and testability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/nocodb/src/db/formulav2/formula-query-builder.helpers.ts(1 hunks)packages/nocodb/src/db/formulav2/formula-query-builder.types.ts(1 hunks)packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts(5 hunks)packages/nocodb/src/db/formulav2/lookup-or-ltar-builder.ts(1 hunks)packages/nocodb/src/db/formulav2/parsed-tree-builder.ts(1 hunks)packages/nocodb/src/db/genRollupSelectv2.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/nocodb/src/db/formulav2/formula-query-builder.types.ts
- packages/nocodb/src/db/formulav2/lookup-or-ltar-builder.ts
- packages/nocodb/src/db/genRollupSelectv2.ts
🧰 Additional context used
🧬 Code Definitions (1)
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts (1)
packages/nocodb/src/db/formulav2/formula-query-builder.types.ts (2)
FnParsedTreeNode(38-46)TAliasToColumn(21-24)
🪛 Biome (1.9.4)
packages/nocodb/src/db/formulav2/formula-query-builder.helpers.ts
[error] 28-28: Useless case clause.
because the default clause is present:
Unsafe fix: Remove the useless case.
(lint/complexity/noUselessSwitchCase)
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts
[error] 81-81: This code is unreachable
... because either this statement ...
... or this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 249-249: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: release-docker / buildx
- GitHub Check: pre-build-for-playwright / playwright
- GitHub Check: unit-tests
- GitHub Check: unit-tests-pg
🔇 Additional comments (6)
packages/nocodb/src/db/formulav2/parsed-tree-builder.ts (1)
109-131: Well-documented URL handling with whitespace for regex parsingThe detailed comments explaining the URL formatting are excellent. They clearly document the problem, solution, and provide examples of why whitespace is needed to avoid regex parsing issues.
packages/nocodb/src/db/formulav2/formulaQueryBuilderv2.ts (5)
13-17: Good refactoring to use modular builder functionsThe imports from separate modules show a good refactoring approach that improves modularity and separation of concerns.
19-20: Type update from BaseModelSqlv2 to IBaseModelSqlV2The change from concrete class to interface type increases flexibility and follows better interface-based design principles.
149-160: Simplified lookup/LTAR logic with dedicated builderReplacing the inline implementation with a dedicated builder function makes the code more maintainable and easier to understand.
342-381: Refactored expression builders improve code organizationMoving the complex expression building logic to dedicated functions improves maintainability and readability.
420-425: Updated type definitions for better type safetyThe parameter types have been updated to provide more explicit definitions, which enhances type safety and clarity.
Uffizzi Ephemeral Environment Deploying☁️ https://app.uffizzi.com/github.com/nocodb/nocodb/pull/10977 ⚙️ Updating now by workflow run 14057829408. What is Uffizzi? Learn more! |
Change Summary
refactor: formula builder parsed tree and ltar builder
Change type