fix: filter datetime (sqlite / mysql) #8385
Conversation
|
|
WalkthroughWalkthroughThe recent updates to the Changes
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 (
|
![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
Out of diff range and nitpick comments (1)
packages/nocodb/src/db/conditionV2.ts (1)
Line range hint
1-1000: Review the overall structure and logic of theparseConditionV2function.The
parseConditionV2function is quite large and handles multiple responsibilities, which could be split into smaller, more manageable functions. This would improve readability and maintainability. Consider refactoring to separate the logic for different types of filters and their handling into distinct functions or modules.
| @@ -740,7 +759,9 @@ const parseConditionV2 = async ( | |||
| // else | |||
| qb = qb.where(knex.raw('??::date = ?', [field, val])); | |||
| } else { | |||
| qb = qb.where(knex.raw('DATE(??) = DATE(?)', [field, val])); | |||
| qb = qb.where( | |||
| knex.raw("DATE(??, 'localtime') = DATE(?)", [field, val]), | |||
There was a problem hiding this comment.
Does all databases(sqlite, mysql and mssql) take localtime as second argument ?
There was a problem hiding this comment.
MySQL was excluded from the beginning. Here, I am only testing SQLite and have not considered MSSQL. I can modify this through conditional statements.
There was a problem hiding this comment.
You can introduce an if statement to avoid any unexpected behaviour
There was a problem hiding this comment.
this only fixes the issue. I feel this part can be abstracted and optimized further.
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
packages/nocodb/src/db/conditionV2.ts (1)
Line range hint
600-684: Ensure consistent handling of timezone and date boundaries.The changes made to handle dates more accurately by considering timezone and the start or end of the day are crucial. However, the logic is quite complex and could benefit from being broken down into smaller, more manageable functions. This would improve readability and maintainability. Consider refactoring the date manipulation logic into separate functions that handle the conversion and boundary setting.
Review Details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- packages/nocodb/src/db/conditionV2.ts (4 hunks)
Additional comments not posted (1)
packages/nocodb/src/db/conditionV2.ts (1)
763-764: Address potential timezone issues for SQLite date comparisons.This change is crucial for ensuring that SQLite handles dates correctly in different timezones. Please verify that this adjustment behaves as expected across different timezone settings.
Verification successful
The search results indicate that the specific pattern
DATE(??, 'localtime') = DATE(?)is used inpackages/nocodb/src/db/conditionV2.ts. This confirms that the code is attempting to handle local time for date comparisons in SQLite.
- File:
packages/nocodb/src/db/conditionV2.ts- Line: 763-764
This usage ensures that the date comparison is done in local time, which addresses potential timezone issues for SQLite date comparisons.
Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg --type typescript $'DATE(??, \\'localtime\\') = DATE(?)'Length of output: 148
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg --type typescript 'DATE\(\?\?, \'localtime\'\) = DATE\(\?\)'Length of output: 93
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg --type typescript "DATE\(??, 'localtime'\) = DATE\(\?\)"Length of output: 102
Script:
#!/bin/bash # Description: Verify that the SQLite date comparison handles timezones correctly. # Test: Search for the function usage in SQLite context. Expect: Correct timezone handling. rg "DATE\(??, 'localtime'\) = DATE\(\?\)"Length of output: 49
Script:
#!/bin/bash # Description: Verify the usage of knex.raw for date comparisons in the codebase. # Test: Search for the usage of knex.raw in the codebase to understand how date comparisons are handled. rg "knex.raw"Length of output: 24434
Change Summary
Change type
Test/ Verification
Provide summary of changes.
Additional information / screenshots (optional)
Anything for maintainers to be made aware of