fix: throw error if invalid operator used in xcCondition #8568
Conversation
WalkthroughWalkthroughThe recent changes to the Changes
Sequence Diagram(s) (Beta)sequenceDiagram
participant Client
participant CustomKnex
Client->>CustomKnex: call parseCondition(obj, columnAliases, qb, pKey?)
CustomKnex->>CustomKnex: Evaluate condition
alt Valid operator
CustomKnex->>Client: Return parsed condition
else Invalid operator
CustomKnex->>Client: Throw error
end
Tip Early Access Features
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (6)
packages/nocodb/src/db/CustomKnex.ts (6)
Line range hint
72-72: Replace!=with!==for strict comparison.- if (str && str != '~not') + if (str && str !== '~not')Using strict equality (
!==) avoids type coercion and should be used instead of loose equality (!=) to ensure more predictable code behavior.Also applies to: 83-83, 98-98, 227-227, 262-262
Line range hint
134-491: Consider replacingforEachwithfor...of.Using
for...ofprovides better readability and is generally more performant in scenarios where the loop might be broken out of, which is not possible withforEach.
Line range hint
489-489: Use template literals instead of string concatenation.- alias: `${this._tn[relation.tn] ? this._tn[relation.tn] + '___' : ''}${relation.tn}`, + alias: `${this._tn[relation.tn] ? `${this._tn[relation.tn]}___` : ''}${relation.tn}`,Template literals provide clearer syntax and improved readability, especially when dealing with complex expressions.
Line range hint
223-223: Remove or refactor unreachable code.Unreachable code can lead to confusion and clutter in the codebase. It's important to either remove it or refactor the logic to make the code paths reachable.
Also applies to: 258-258, 295-295, 352-352, 414-414, 484-484
Line range hint
118-494: Consider converting function expressions to arrow functions.Arrow functions provide a cleaner and more concise syntax and lexically bind the
thisvalue, which is often desirable in modular JavaScript code.
Line range hint
173-173: Specify a more precise type instead ofany.Using
anytype can lead to potential runtime errors and makes the code less maintainable and understandable. Where possible, replaceanywith a more specific type to enhance type safety and clarity.Also applies to: 499-499, 541-541, 567-567
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/src/db/CustomKnex.ts (1 hunks)
Additional Context Used
Biome (20)
packages/nocodb/src/db/CustomKnex.ts (20)
72-72: Use !== instead of !=.
!= is only allowed when comparing againstnull
81-83: The assignment should not be in an expression.
83-83: Use !== instead of !=.
!= is only allowed when comparing againstnull
98-98: Use !== instead of !=.
!= is only allowed when comparing againstnull
134-491: Prefer for...of instead of forEach.
173-173: Unexpected any. Specify a different type.
227-227: Use !== instead of !=.
!= is only allowed when comparing againstnull
262-262: Use !== instead of !=.
!= is only allowed when comparing againstnull
470-470: Use === instead of ==.
== is only allowed when comparing againstnull
489-489: Template literals are preferred over string concatenation.
223-223: This code is unreachable
258-258: This code is unreachable
295-295: This code is unreachable
352-352: This code is unreachable
414-414: This code is unreachable
484-484: This code is unreachable
118-494: This function expression can be turned into an arrow function.
499-499: Unexpected any. Specify a different type.
541-541: Unexpected any. Specify a different type.
567-567: Unexpected any. Specify a different type.
a34cfac to
518801d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (2)
packages/nocodb/src/db/CustomKnex.ts (2)
Line range hint
72-72: Replace!=with!==and==with===to avoid type coercion issues.- if (openIndex == -1) openIndex = str.indexOf('(~'); + if (openIndex === -1) openIndex = str.indexOf('(~'); - if (closingIndex == -1) + if (closingIndex === -1) - if (operandStartIndex != -1) + if (operandStartIndex !== -1) - if (matches[3] != 'null') + if (matches[3] !== 'null') - if (matches[4] != 'null') + if (matc 8000 hes[4] !== 'null') - if (matches[3] == 'like' && clientType === 'pg') { + if (matches[3] === 'like' && clientType === 'pg') {Also applies to: 83-83, 98-98, 227-227, 262-262, 470-470
Line range hint
134-491: Consider refactoringforEachloops tofor...ofloops for better performance and readability.- conditions.forEach((condition) => { + for (const condition of conditions) {
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/src/db/CustomKnex.ts (1 hunks)
Additional Context Used
Biome (20)
packages/nocodb/src/db/CustomKnex.ts (20)
72-72: Use !== instead of !=.
!= is only allowed when comparing againstnull
81-83: The assignment should not be in an expression.
83-83: Use !== instead of !=.
!= is only allowed when comparing againstnull
98-98: Use !== instead of !=.
!= is only allowed when comparing againstnull
134-491: Prefer for...of instead of forEach.
173-173: Unexpected any. Specify a different type.
227-227: Use !== instead of !=.
!= is only allowed when comparing againstnull
262-262: Use !== instead of !=.
!= is only allowed when comparing againstnull
470-470: Use === instead of ==.
== is only allowed when comparing againstnull
489-489: Template literals are preferred over string concatenation.
223-223: This code is unreachable
258-258: This code is unreachable
295-295: This code is unreachable
352-352: This code is unreachable
414-414: This code is unreachable
484-484: This code is unreachable
118-494: This function expression can be turned into an arrow function.
499-499: Unexpected any. Specify a different type.
541-541: Unexpected any. Specify a different type.
567-567: Unexpected any. Specify a different type.
|
Uffizzi Preview |
c627141 to
a6e477b
Compare
a6e477b to
2e8f65b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (6)
packages/nocodb/src/db/CustomKnex.ts (6)
Line range hint
73-73: Replace!=with!==to ensure strict type comparison.- if (openIndex == -1) openIndex = str.indexOf('(~'); + if (openIndex === -1) openIndex = str.indexOf('(~'); - if (closingIndex == -1) + if (closingIndex === -1) - if (operandStartIndex != -1) + if (operandStartIndex !== -1) - if (matches[1] != '') + if (matches[1] !== '') - if (matches[4] != 'null') + if (matches[4] !== 'null')Also applies to: 84-84, 99-99, 228-228, 263-263
Line range hint
135-492: Consider usingfor...ofinstead offorEachfor better performance and readability in modern JavaScript.- conditions.forEach((condition) => { + for (const condition of conditions) {
Line range hint
471-471: Use===for strict equality comparison to avoid unexpected type coercion.- if (matches[3] == 'like' && clientType === 'pg') { + if (matches[3] === 'like' && clientType === 'pg') {
Line range hint
490-490: Use template literals for string concatenation to improve readability and maintainability.- alias: `${ - this._tn[relation.tn] ? this._tn[relation.tn] + '___' : '' - }${relation.tn}`, + alias: `${this._tn[relation.tn] ? `${this._tn[relation.tn]}___` : ''}${relation.tn}`,
Line range hint
174-174: Specify a more explicit type instead ofanyto enhance type safety and code clarity.- const matches: any[] = condition.match( + const matches: Array<string | null> = condition.match(Also applies to: 546-546, 574-574, 584-584
Line range hint
119-495: Convert this function expression to an arrow function for better readability and to maintain the lexicalthis.- conditions.forEach((condition) => { + conditions.forEach((condition) => {
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/src/db/CustomKnex.ts (2 hunks)
Additional Context Used
Biome (20)
packages/nocodb/src/db/CustomKnex.ts (20)
73-73: Use !== instead of !=.
!= is only allowed when comparing againstnull
82-84: The assignment should not be in an expression.
84-84: Use !== instead of !=.
!= is only allowed when comparing againstnull
99-99: Use !== instead of !=.
!= is only allowed when comparing againstnull
135-492: Prefer for...of instead of forEach.
174-174: Unexpected any. Specify a different type.
228-228: Use !== instead of !=.
!= is only allowed when comparing againstnull
263-263: Use !== instead of !=.
!= is only allowed when comparing againstnull
471-471: Use === instead of ==.
== is only allowed when comparing againstnull
490-490: Template literals are preferred over string concatenation.
224-224: This code is unreachable
259-259: This code is unreachable
296-296: This code is unreachable
353-353: This code is unreachable
415-415: This code is unreachable
485-485: This code is unreachable
119-495: This function expression can be turned into an arrow function.
546-546: Unexpected any. Specify a different type.
574-574: Unexpected any. Specify a different type.
584-584: Unexpected any. Specify a different type.
Additional comments not posted (1)
packages/nocodb/src/db/CustomKnex.ts (1)
695-699: The error handling for invalid operators is a crucial improvement for robustness. Consider providing more context in the error message to aid debugging.- throw new Error(`Found invalid conditional operator "${key}" in expression`); + throw new Error(`Found invalid conditional operator "${key}" in expression. Please check the condition object for correct syntax.`);
|
merging sans approval from @pranavxc as the corresponding PR was approved in hub already |
Change Summary
Provide summary of changes with issue number if any.
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of