[ FIX ] Support top-level And Condition in where query
#335
Conversation
WalkthroughThe changes involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant QueryProcessor
participant ClauseBuilder
User->>QueryProcessor: Send query
QueryProcessor->>ClauseBuilder: Convert query to clause
ClauseBuilder->>ClauseBuilder: Evaluate conditions
ClauseBuilder->>ClauseBuilder: Determine composite operator
ClauseBuilder-->>QueryProcessor: Return generated clauses
QueryProcessor-->>User: Return results
Warning Rate limit exceeded@posaune0423 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 54 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 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 using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
packages/sdk/src/__tests__/convertQueryToClause.test.ts (2)
Line range hint
193-263: Well-structured test case for top-level AND conditions.The test case effectively validates the PR's main objective of supporting top-level And conditions. It provides good coverage with a mix of:
- Simple equality conditions
- Numeric comparisons
- Nested Or conditions
Consider adding these additional test cases to improve coverage:
- Empty And condition array
- Single condition in And array
- Multiple nested And conditions
- Mixed And/Or conditions at the same level
Example structure:
it("should handle edge cases in AND conditions", () => { const cases = [ { And: [] }, // empty array { And: [{ score: { $gt: 100 } }] }, // single condition { And: [{ And: [...] }, { And: [...] }] }, // nested And { And: [...], Or: [...] } // mixed at same level ]; // ... test implementation });
Line range hint
1-263: Consider implementing a test helper for complex query structures.As the query structures become more complex with nested conditions, consider creating a test helper function to build these structures. This would:
- Reduce duplication in test cases
- Make test cases more readable
- Make it easier to add new test cases
Example helper structure:
function buildQueryWithConditions(conditions: Condition[]) { return { world: { player: { $: { where: { And: conditions } } } } }; }packages/sdk/src/convertQuerytoClause.ts (2)
23-23: Clarify the purpose of skipping 'entityIds' namespaceThe line
if (namespace === "entityIds") continue;skips processing for the"entityIds"namespace. To improve code readability, please add a comment explaining why"entityIds"is being skipped.
Line range hint
157-218: Consider standardizing the return type ofbuildWhereClausefunctionThe
buildWhereClausefunction currently returnstorii.Clause | torii.Clause[] | undefined, which leads to additional type checks and complexity in the consuming code. To simplify the logic and improve maintainability, consider refactoringbuildWhereClauseto always return an array oftorii.Clause(even if empty).Suggested changes:
Update the function signature to consistently return an array:
-function buildWhereClause( +function buildWhereClause( namespaceModel: string, where: Record<string, any> -): torii.Clause | torii.Clause[] | undefined { +): torii.Clause[] {Modify return statements within
buildWhereClauseto return arrays:- if (subClauses.length === 0) return undefined; - if (subClauses.length === 1) return subClauses[0]; + if (subClauses.length === 0) return []; return subClauses;Adjust the calling code to handle the array of clauses without checking for single clauses or undefined values:
In the
processModelsfunction:-if (clause) { - if (Array.isArray(clause)) { - clauses.push(...clause); - } else { - clauses.push(clause); - } -} +clauses.push(...clause);And similarly in other places where
buildWhereClauseis called.This refactor will reduce complexity and make the codebase more consistent.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/sdk/src/__tests__/convertQueryToClause.test.ts(4 hunks)packages/sdk/src/convertQuerytoClause.ts(5 hunks)
🔇 Additional comments (4)
packages/sdk/src/__tests__/convertQueryToClause.test.ts (2)
7-7: LGTM! Import changes are appropriate.
The removal of unused SchemaType import and addition of QueryType import aligns with the actual types being used in the test cases.
141-189: Test case properly validates nested composite conditions.
The updated test case effectively covers:
- Nested And/Or composite conditions
- Multiple clauses within each composite
- Proper model and member field handling
Let's verify the corresponding implementation matches this test case:
✅ Verification successful
Test case aligns with implementation's composite condition handling
The implementation in convertQuerytoClause.ts correctly supports the test case's nested composite conditions through:
- The
buildWhereClausefunction that handles nested And/Or operators - Proper construction of composite clauses with multiple conditions
- Support for Member clauses with various operators and primitive values
- Correct handling of hierarchical structure matching the test case format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for the implementation of composite condition handling
ast-grep --pattern 'function convertQueryToClause($_) {
$$$
Composite: {
operator: $_,
clauses: $_
}
$$$
}'
Length of output: 125
Script:
#!/bin/bash
# Let's try a different approach to find the implementation
# First, let's locate the convertQueryToClause implementation file
fd "convertQueryToClause" --type f
# Then let's examine its content
rg -A 10 "function convertQueryToClause" --type ts
Length of output: 965
Script:
#!/bin/bash
# Let's examine the full implementation of convertQueryToClause
cat packages/sdk/src/convertQuerytoClause.ts
Length of output: 9368
packages/sdk/src/convertQuerytoClause.ts (2)
28-37: Verify the logic for determining hasOnlyAndCondition
The logic that sets hasOnlyAndCondition to false when any clause does not meet the 'Composite' with 'And' operator condition may not handle all scenarios correctly. Please verify that this logic accurately reflects the intended behavior, especially when dealing with mixed operators or nested composite clauses.
46-46: LGTM
The operator selection logic using hasOnlyAndCondition correctly sets the top-level operator to "And" or "Or" based on the clauses.
posaune0423
changed the title
And Condition in where query
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/sdk/src/convertQuerytoClause.ts (2)
28-37: Simplify the complex condition check for better readabilityThe nested condition check for determining
hasOnlyAndConditioncan be simplified using a helper function.Consider refactoring to:
-if ( - !modelClauses.every( - (clause) => - clause && - "Composite" in clause && - clause.Composite.operator === "And" - ) -) { - hasOnlyAndCondition = false; -} +const isAndCompositeClause = (clause: torii.Clause) => + clause && "Composite" in clause && clause.Composite.operator === "And"; + +if (!modelClauses.every(isAndCompositeClause)) { + hasOnlyAndCondition = false; +}
Line range hint
152-218: Consider breaking down the complex buildWhereClause functionThe function has grown to handle multiple responsibilities:
- Logical operator processing
- Member clause generation
- Array result handling
This complexity makes it harder to maintain and test.
Consider splitting into smaller, focused functions:
+function buildLogicalClause( + namespaceModel: string, + operator: torii.LogicalOperator, + conditions: Array<Record<string, any>> +): torii.Clause | undefined { + const subClauses = conditions + .map(condition => buildWhereClause(namespaceModel, condition)) + .filter((clause): clause is torii.Clause => clause !== undefined); + + if (subClauses.length === 0) return undefined; + if (subClauses.length === 1) return subClauses[0]; + + return { + Composite: { + operator, + clauses: subClauses, + }, + }; +} + +function buildMemberClauses( + namespaceModel: string, + where: Record<string, any> +): torii.Clause[] { + return Object.entries(where) + .flatMap(([member, memberValue]) => { + if (typeof memberValue !== 'object' || memberValue === null) return []; + + return Object.entries(memberValue) + .filter(([op]) => op.startsWith('$')) + .map(([op, val]) => ({ + Member: { + model: namespaceModel, + member, + operator: convertOperator(op), + value: convertToPrimitive(val), + }, + })); + }); +} function buildWhereClause( namespaceModel: string, where: Record<string, any> ): torii.Clause | torii.Clause[] | undefined { const logicalOperators: Record<string, torii.LogicalOperator> = { And: "And", Or: "Or", }; const logicalKey = Object.keys(where).find((key) => key in logicalOperators); if (logicalKey) { - // Handle explicit And/Or conditions - const operator = logicalOperators[logicalKey]; - const conditions = where[logicalKey] as Array<Record<string, any>>; - - const subClauses: torii.Clause[] = []; - for (const condition of conditions) { - const clause = buildWhereClause(namespaceModel, condition); - if (clause) { - if (Array.isArray(clause)) { - subClauses.push(...clause); - } else { - subClauses.push(clause); - } - } - } - - if (subClauses.length === 0) return undefined; - if (subClauses.length === 1) return subClauses[0]; - - return { - Composite: { - operator: operator, - clauses: subClauses, - }, - }; + return buildLogicalClause( + namespaceModel, + logicalOperators[logicalKey], + where[logicalKey] as Array<Record<string, any>> + ); } - // If no logical operator, build Member clauses - const memberClauses: torii.Clause[] = []; - - for (const [member, memberValue] of Object.entries(where)) { - if (typeof memberValue === "object" && memberValue !== null) { - for (const [op, val] of Object.entries(memberValue)) { - if (!op.startsWith("$")) continue; - memberClauses.push({ - Member: { - model: namespaceModel, - member, - operator: convertOperator(op), - value: convertToPrimitive(val), - }, - }); - } - } - } - - if (memberClauses.length === 0) return undefined; - if (memberClauses.length === 1) return memberClauses[0]; - - return memberClauses; + const memberClauses = buildMemberClauses(namespaceModel, where); + return memberClauses.length === 0 ? undefined : + memberClauses.length === 1 ? memberClauses[0] : + memberClauses; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/src/convertQuerytoClause.ts(5 hunks)
🔇 Additional comments (2)
packages/sdk/src/convertQuerytoClause.ts (2)
125-126: Add type checking before spreading array elements
While spreading array elements from buildWhereClause, ensure all elements are valid clauses.
Consider adding runtime type checks:
-if (Array.isArray(clause)) {
- clauses.push(...clause);
+if (Array.isArray(clause)) {
+ const validClauses = clause.filter((c): c is torii.Clause => c !== undefined);
+ clauses.push(...validClauses);Let's verify the usage of array spreading in the codebase:
Line range hint 1-218: Verify handling of nested conditions
The implementation supports top-level "And" conditions as intended. However, let's verify the handling of deeply nested conditions.
Run this script to check for potential edge cases in the test suite:
✅ Verification successful
Nested conditions are properly handled and tested
The test suite demonstrates comprehensive coverage of nested conditions, including:
- Complex nested AND/OR combinations in "should handle complex conditions with multiple operators"
- Deep nesting with mixed operators in "should handle top level AND conditions"
- Various edge cases and combinations of conditions
The implementation correctly processes:
- Top-level AND conditions
- Nested AND within OR conditions
- Multiple levels of nesting with different operators
- Mixed conditions across different models and namespaces
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check test coverage for nested conditions
# Search for test cases with nested conditions
rg --type typescript 'describe|test|it.*nested.*condition' -A 5 -g '*test*'
# Search for complex query objects in test files
ast-grep --pattern 'const $_ = {
$$$
And: [
$$$
{
$$$
And: [$$$]
$$$
}
$$$
]
$$$
}'
Length of output: 240
Script:
#!/bin/bash
# Let's try a different approach to find test cases and nested conditions
# Search for test files related to query conversion
fd -e ts -e js "test|spec" | grep -i "query\|clause"
# Look for nested condition patterns in test files
rg "And.*\[.*And.*\]" --type ts --type js
# Search for test cases with multiple conditions
rg "test|it\(" -A 10 --type ts --type js | grep -A 10 "operator.*And"
# Look for actual usage of convertQueryToClause with nested structures
ast-grep --pattern 'convertQueryToClause({
$$$
And: [
$$$
]
$$$
})'
Length of output: 374
Script:
#!/bin/bash
# Let's examine the test file content directly
# Display the content of the test file
cat packages/sdk/src/__tests__/convertQueryToClause.test.ts
# Also check the entity key clauses test as it might contain relevant test cases
cat packages/sdk/src/__tests__/convertQueryToEntityKeyClauses.test.ts
Length of output: 16762
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/sdk/src/__tests__/convertQueryToClause.test.ts (1)
Line range hint
193-262: Comprehensive test case for top-level AND conditionsThe new test case effectively validates the main PR objective with a good mix of conditions. Consider adding edge cases to strengthen the test suite:
- Empty AND array
- Single condition in AND array
- Deeply nested AND/OR combinations
Would you like me to provide example test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/sdk/src/__tests__/convertQueryToClause.test.ts(3 hunks)
🔇 Additional comments (3)
packages/sdk/src/__tests__/convertQueryToClause.test.ts (3)
7-7: LGTM! Clean import statements
The removal of unused SchemaType import and retention of only necessary imports improves code cleanliness.
141-189: Well-structured test expectations for complex conditions
The updated test expectations effectively validate:
- Nested composite conditions (AND within OR)
- Multiple field conditions (score and name)
- Multiple model conditions (player and item)
- Various operators (Gt, Eq, Lt)
Line range hint 1-262: Verify test coverage for the new functionality
Let's ensure we have comprehensive test coverage for the new AND condition handling.
✅ Verification successful
Test coverage appears comprehensive for AND condition handling
Looking at the implementation and test cases, the test coverage for AND conditions is thorough:
- The test "should handle complex conditions with multiple operators" covers nested AND conditions with OR combinations
- The test "should handle top level AND conditions" specifically tests the new AND condition handling
- The implementation correctly processes both top-level and nested AND conditions in
buildWhereClausefunction - All operator cases including AND/OR compositions are properly tested with various combinations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check test coverage for convertQueryToClause
npx vitest run --coverage packages/sdk/src/__tests__/convertQueryToClause.test.ts | grep -A 5 "Coverage summary"
Length of output: 208
Script:
#!/bin/bash
# Let's analyze the test coverage by examining the test cases and implementation
# First, let's check the implementation of convertQueryToClause
ast-grep --pattern 'export function convertQueryToClause' -A 10
# Then, let's check all test cases
rg "it\(" packages/sdk/src/__tests__/convertQueryToClause.test.ts -A 2
Length of output: 834
Script:
#!/bin/bash
# Let's examine the implementation file to understand what functionality needs to be tested
rg -l "convertQueryToClause" --type ts
# Then get the content of the implementation file
fd "convertQuerytoClause.ts" --type f --exec cat {}
Length of output: 9958
|
Thank's for contribution ser, would you mind create fill in a PR in the book to document how query should be built ? |
|
@MartianGreed I realized that the implementation approach in this PR was slightly mistaken, so I have made new fixes in this PR. Could you please take a look at it? |
Closes #
Introduced changes
Andconditions in where query logically workshasOnlyAndConditionnot to make root Composite logical operator fixed toOrconsole.logand type importChecklist
Summary by CodeRabbit
Summary by CodeRabbit
New Features
convertQueryToClausefunction.Bug Fixes
Documentation