fix: value "is" or "is not" null, not working
#62
Conversation
Signed-off-by: Jim Ezesinachi <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThis pull request introduces changes across multiple files, primarily focusing on enhancing the test coverage for the Changes
Possibly related PRs
Suggested reviewers
Poem
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 (
|
jimezesinachi
changed the title
null, not working
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
packages/backend/src/execution/executors/PgExecutor/PgExecutor.test.ts (2)
108-130: LGTM! Consider enhancing test coverage.The test case effectively verifies the
is: nullcondition. However, consider adding the following improvements:
- Add a test to verify the actual SQL query generated using
executor.compile(), similar to the existing "Film table SynthQL query compiles to expected SQL query" test.- Add an edge case test with a condition that returns no results to ensure proper handling of empty result sets.
Example structure for SQL verification:
const { sql } = executor.compile(q); expect(sql).toMatchInlineSnapshot();
132-154: LGTM! Consider additional validations.The test case effectively verifies the
is not: nullcondition. To make it more robust:
- Add assertions to verify the email format consistency:
expect(result).toEqual( expect.arrayContaining([ expect.objectContaining({ email: expect.stringMatching(/^[A-Z.]+@sakilacustomer\.org$/), }), ]) );
- Consider adding a test case with a mix of null and non-null values to verify proper filtering.
packages/backend/src/execution/executors/PgExecutor/queryBuilder/exp.ts (1)
Line range hint
397-461: Consider consolidating null value handling logic.The code currently has multiple places handling null values:
- Direct null comparison:
if (op === null)- Null value in operator object:
if (value === null)This could be consolidated to make the code more maintainable and consistent.
Consider refactoring the null handling logic into a separate method:
private handleNullComparison(quotedColumn: string, opName: string = 'is'): Exp { return ['op', opName, quotedColumn, ['const', null]]; }Then use it in both places:
if (op === null) { - return ['op', 'is', quotedColumn, ['const', null]]; + return this.handleNullComparison(quotedColumn); } // ... if (value === null) { - return ['op', opName, quotedColumn, ['const', null]]; + return this.handleNullComparison(quotedColumn, opName); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- packages/backend/src/execution/executors/PgExecutor/PgExecutor.test.ts (1 hunks)
- packages/backend/src/execution/executors/PgExecutor/queryBuilder/exp.ts (1 hunks)
- packages/docs/static/reference/assets/navigation.js (1 hunks)
- packages/docs/static/reference/assets/search.js (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/docs/static/reference/assets/search.js
🧰 Additional context used
🔇 Additional comments (2)
packages/docs/static/reference/assets/navigation.js (1)
1-1: Verify the documentation update.This change modifies the encoded navigation data for the documentation site. While the main PR focuses on fixing null value handling in SQL queries, it's unclear if this documentation change is intentional or auto-generated.
Let's decode and verify the content:
Consider documenting the reason for this navigation data update in the commit message or PR description.
✅ Verification successful
Let me gather more information about the documentation changes to understand if this navigation update is related to the null handling changes.
Based on the gathered information, I can now provide a final response:
Navigation update is auto-generated and consistent with PR changes
The navigation.js change is an auto-generated update that coincides with the documentation of runtime validation RFC, which is relevant to the PR's focus on null value handling. The git history shows this file was recently added and modified as part of the same PR that fixes "is" or "is not" null handling.
The blog post content (2024-09-15-rfc-runtime-validation) discusses runtime validation and schema handling, which aligns with the PR's objectives. This explains why the navigation data needed to be updated to include the new documentation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Decode and compare the base64 content to verify documentation updates # Note: Using only the first part of the string for brevity and avoiding potential binary data echo "Decoding navigation data..." echo "H4sIAAAAAAAAE5WXXW/TMBRA/0ueC2ODDehb11ZigLbSTiA0TZPn3K7WXCe1ndEK8d9RnbR2Yufae5xyfGzfD+/27m+mYauzYfZI6DOI/ERJmg2ykuhVNszWRV5xUCfNxwcl6duVXvNskD0zkWfDs0FGV4znEkQ2vDvKflQgd1PxxARYGeVEqY7MAdvi07NP/wZH32In9GrDp1IWEhe6JGZ0Np7JolTWyoQGuSS0/6RmQScO5xdd+YwTkWbdk5iOFpwD1d+J0la4rATVrBBtn4O2jRcfWsJ1WSgwm8eNlkWUa5bnHP4QCTGhJT3dvXtGzsK1SDkL1KGzckVEzkG+gW0pQamwpYEeGii5sqkEomFar2qK7UvtCl08tA2iQCNyuJeArcYvtSeSb3QNW913Eb0rQ1p/SWend58/np67OfFXzGFTgVvQyXs1K2Nb1mHGbuen6bhr32I0QUxoWagSaE967Pfk5DyBAEl0sKk6ugOKHnFTgWTQ0xPNx+TDjQtercUElsFnzrUdSeyZO0L79xWkZhB+lYNiuyZpi1eYUeGCrmBNorIaw0S35JFDSigPICYbid24EOaPTnu5JkvFWmkkdpPLiGpymWDp/McJigL/aYIuE4iIyzCprlb0+3V+7D3jJRNE7m5KTHdgUl373i68pzlkrMmYd0xkzgThTKMJcbCo0XQMKjNEmud2V6LZtVSa7yfhVYLQYDHjBJYgJeRzUBVHe6xNxrxXgvIqR0/ZIDHT14IJvP5qIuaJNmxSt9ZTLpFkDdofLzzdkUzyxpPgYDHjHJZ42AwQsyxgP4RjmpqIeaJPXNL79msFErUYIMky5qRScVeNRV+2q+vR/PfDzWw6H93ezBdW+0Ik29+s8751+Lb/fWv2K3hoYnJttODIzxmm+mo2bOvyqLpTZH3GQKm1ROV+s5jGQIhk0/cb0JUY6M0pOllKIH1zr/mUPFU2A7c3wDjzkBW2YXRSq0mTpm/gvWme8gDGqlj1HNfWsFW32W5AWxXcnGImixeWh+vPO/IBRouvNw4hsYcjavUacQcO1JYVVwqaU+BOyyXJvIzhzmCJmR64/w/9nO/IRRMAAA==" | base64 -d | gunzip -c 2>/dev/null || echo "Binary content detected" # Look for relevant changes in documentation files echo -e "\nSearching for null-related documentation changes..." rg -l "is.*null|null.*handling" --type mdLength of output: 1548
Script:
#!/bin/bash # Check the content of the blog post that mentions null handling echo "Checking blog post content..." cat "packages/docs/blog/2024-09-15-rfc-runtime-validation/index.md" # Look for recent changes to navigation-related files echo -e "\nChecking recent changes to navigation files..." git log -p --since="1 month ago" packages/docs/static/reference/assets/navigation.js # Check if there are any other navigation data files echo -e "\nLooking for other navigation-related files..." fd -e js -e json navigationLength of output: 9060
packages/backend/src/execution/executors/PgExecutor/queryBuilder/exp.ts (1)
458-461: 🛠️ Refactor suggestionVerify the null value handling implementation.
The added code correctly handles null values in operator conditions, but we should verify that it works correctly with all SQL operators, especially 'is' and 'is not'.
Let's verify the operator handling with null values:
Consider adding explicit type checking for the operator to ensure type safety:
if (value === null) { + // Ensure operator is compatible with null values + if (!['is', 'is not', '=', '!='].includes(opName)) { + throw new Error(`Operator ${opName} is not compatible with null values`); + } return ['op', opName, quotedColumn, ['const', null]]; }✅ Verification successful
Let me try a different search strategy to verify the null value handling implementation.
This should help us:
- Locate test files to check test coverage
- Find query builder related files
- Examine null value handling implementation
- Understand operator handling context
Let me search for the operator definitions and validation to understand the complete context.
This should help us understand:
- What operators are supported
- How operators are validated
- Whether there are specific tests for null handling
- The validation logic for operators
Let me search for one final piece of information about the OPERATORS constant.
This will help us understand the complete set of supported operators before making the final verification.
Based on the gathered information, I can now provide a final verification response:
Null value handling implementation is correct and well-tested
The implementation is verified to be correct for the following reasons:
- The code correctly handles null values by generating proper SQL expressions using
['op', opName, quotedColumn, ['const', null]]- The test suite includes specific test cases for both
is nullandis not nulloperators- The operator validation is properly implemented through
assertOpfunction which validates against Kysely's standardOPERATORS- The null handling is integrated with the type system through proper operator type definitions
The review comment's suggestion about verifying operator compatibility with null values is addressed by the existing implementation because:
- The code uses Kysely's standard operator set which includes proper null handling operators
- The test suite explicitly covers null value scenarios with both positive (
is null) and negative (is not null) cases- The operator validation ensures only valid SQL operators are used through the type system and runtime checks
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check all usages of SQL operators with null values in tests # This helps ensure the implementation works with all supported operators # Search for test cases involving null values echo "Searching for null value test cases..." rg -A 5 "null.*where" "*.test.ts" # Search for SQL operator usage with null echo "Searching for SQL operator usage with null..." rg -A 5 "(is|is not|=|!=|>|<|>=|<=).*null" "*.test.ts"Length of output: 380
Script:
#!/bin/bash # Search for test files in the repository echo "Finding test files..." fd "\.test\.(ts|js)$" # Search for SQL query builder related files echo -e "\nSearching for query builder files..." fd "query.*\.(ts|js)$" # Look for null value handling in the query builder implementation echo -e "\nSearching for null value handling in query builder..." rg -A 5 "value\s*===\s*null" packages/backend/src/execution/executors/PgExecutor/queryBuilder/ # Search for operator definitions and usage echo -e "\nSearching for operator definitions..." rg -B 2 -A 2 "opName|operator" packages/backend/src/execution/executors/PgExecutor/queryBuilder/Length of output: 10220
Script:
#!/bin/bash # Search for operator definitions and validations echo "Searching for operator definitions..." rg -B 5 -A 5 "OPERATORS.*=|ops.*=" packages/backend/src/execution/executors/PgExecutor/queryBuilder/ # Look for type definitions related to operators echo -e "\nSearching for operator type definitions..." rg -B 2 -A 2 "type.*Operator|interface.*Operator" packages/backend/src/execution/executors/PgExecutor/queryBuilder/ # Search for null-related test cases in PgExecutor tests echo -e "\nSearching for null-related test cases..." rg -B 3 -A 3 "null.*where|where.*null" packages/backend/src/execution/executors/PgExecutor/PgExecutor.test.ts # Look for operator validation implementations echo -e "\nSearching for operator validation..." rg -B 3 -A 3 "assertOp|validateOp|isValidOp" packages/backend/src/execution/executors/PgExecutor/queryBuilder/Length of output: 5397
Script:
#!/bin/bash # Search for OPERATORS constant definition echo "Searching for OPERATORS constant definition..." rg -B 5 -A 5 "const OPERATORS.*=" packages/backend/src/execution/executors/PgExecutor/queryBuilder/ # Search for imports of OPERATORS echo -e "\nSearching for OPERATORS imports..." rg "import.*OPERATORS" packages/backend/src/execution/executors/PgExecutor/queryBuilder/Length of output: 503
| it('single provider is null', async () => { | ||
| const q = from('film') | ||
| .columns('film_id', 'original_language_id', 'title', 'rating') | ||
| .where({ original_language_id: { is: null } }) | ||
| .take(2); | ||
|
|
||
| const result = await executor.execute(q, executeProps); | ||
|
|
||
| expect(result).toEqual([ | ||
| { | ||
| film_id: 1, | ||
| original_language_id: null, | ||
| title: 'ACADEMY DINOSAUR', | ||
| rating: 'PG', | ||
| }, | ||
| { | ||
| film_id: 2, | ||
| original_language_id: null, | ||
| title: 'ACE GOLDFINGER', | ||
| rating: 'G', | ||
| }, | ||
| ]); | ||
| }); | ||
|
|
||
| it('single provider is not null', async () => { | ||
| const q = from('customer') | ||
| .columns('customer_id', 'email', 'first_name', 'last_name') | ||
| .where({ email: { 'is not': null } }) | ||
| .take(2); | ||
|
|
||
| const result = await executor.execute(q, executeProps); | ||
|
|
||
| expect(result).toEqual([ | ||
| { | ||
| customer_id: 1, | ||
| email: '[email protected]', | ||
| first_name: 'MARY', | ||
| last_name: 'SMITH', | ||
| }, | ||
| { | ||
| customer_id: 2, | ||
| email: '[email protected]', | ||
| first_name: 'PATRICIA', | ||
| last_name: 'JOHNSON', | ||
| }, | ||
| ]); | ||
| }); |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Based on the search results from all our queries, I can now provide a final assessment of the test coverage for null handling:
The current test coverage for null conditions is sufficient but could be enhanced
The existing tests effectively cover:
- Basic
is nullcondition (film.original_language_id) - Basic
is not nullcondition (customer.email) - Real database data validation
- Different table contexts (film and customer tables)
However, no tests were found for:
- Complex conditions combining null checks with other operators
- Null checks in nested queries/includes
- Multiple null conditions in the same query
Since these are edge cases that could potentially uncover subtle issues, consider adding them for more robust coverage.
🔗 Analysis chain
Consider additional test scenarios for comprehensive coverage.
The current tests effectively verify the basic functionality of null value handling. Consider adding these scenarios:
- Complex conditions combining null checks with other operators
- Null checks in nested queries or includes
- Multiple null conditions in the same query
Let's verify if there are any existing tests covering these scenarios:
Example of a combined condition test:
const q = from('film')
.where({
original_language_id: { is: null },
rating: { in: ['PG', 'G'] }
})
.take(2);Let me search for more specific test cases related to null conditions.
Let me try a different search pattern to find test files with null conditions.
Let me search for any additional test files that might contain complex null conditions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing null-related tests
rg -i "is.*null|null.*is" --type ts
Length of output: 87281
Script:
#!/bin/bash
# Search for test files containing null-related query conditions
rg -t ts "where.*\{.*null" --context 10 "test"
Length of output: 91
Script:
#!/bin/bash
# Search for test files containing null-related query conditions with proper path handling
rg -t ts "where.*\{.*null" -g "*.test.ts" --context 10
Length of output: 4328
Script:
#!/bin/bash
# Search for test files with more complex null-related conditions
rg -t ts "where.*\{.*\{.*null.*\}.*\{" -g "*.test.ts" --context 10
# Also search for null checks in nested queries
rg -t ts "include.*where.*null" -g "*.test.ts" --context 10
Length of output: 130
SynthQL pull request template
Why
Queries with a "null" value were not executing correctly. This fixes that
What changed
Three lines added to the
expressionFromWhere()compilerImportant
Summary by CodeRabbit
PgExecutorto ensure correct handling of null and non-null conditions in queries.