Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cast datetime columns in sqlite before comparing #26540

Merged
merged 2 commits into from
Dec 29, 2023

Conversation

juliusknorr
Copy link
Member

When debugging failing integration tests in nextcloud/deck#2934 i noticed that sqlite does strange things when trying to compare a datetime field with a DateTime value through the expression builder. It turns out sqlite doesn't store them as real dates and therefore operates a string comparison through our database abstraction:

https://www.sqlite.org/datatype3.html

2.2. Date and Time Datatype

SQLite does not have a storage class set aside for storing dates and/or times. Instead, the built-in Date And Time Functions of SQLite are capable of storing dates and times as TEXT, REAL, or INTEGER values:

TEXT as ISO8601 strings ("YYYY-MM-DD HH:MM:SS.SSS").
REAL as Julian day numbers, the number of days since noon in Greenwich on November 24, 4714 B.C. according to the proleptic Gregorian calendar.
INTEGER as Unix Time, the number of seconds since 1970-01-01 00:00:00 UTC. 

Applications can chose to store dates and times in any of these formats and freely convert between formats using the built-in date and time functions.

In order to resolve this issue globally this PR wraps the required parameters when they are passed to the query builder with using the DATETIME() function so both values are compared as real datetimes.

Might make sense to also look into covering this with some tests, but I didn't have time to look into yet. I can have a look if the general approach to this is fine.

@nickvergessen
Copy link
Member

Looking at https://github.com/nextcloud/server/blob/master/lib/private/DB/QueryBuilder/ExpressionBuilder/OCIExpressionBuilder.php thats a lot of duplication. Xan we abstract the prepareColumn calls to another level?

@juliusknorr juliusknorr force-pushed the bugfix/noid/sqlite-datetime branch from cfc381e to f2d744b Compare October 14, 2022 12:07
@juliusknorr
Copy link
Member Author

@nickvergessen I moved the prepareColum to the regular ExpressionBuilder so that sqlite and OCI now just extend that.

@juliusknorr juliusknorr added enhancement 3. to review Waiting for reviews labels Oct 14, 2022
@juliusknorr juliusknorr added this to the Nextcloud 26 milestone Oct 14, 2022
@juliusknorr juliusknorr marked this pull request as ready for review October 14, 2022 12:10
@juliusknorr juliusknorr force-pushed the bugfix/noid/sqlite-datetime branch from f2d744b to 43608b8 Compare October 14, 2022 22:08
@nickvergessen
Copy link
Member

Can we add a test? :)

@juliusknorr
Copy link
Member Author

Definitely. I'd probably add a case for this in https://github.com/nextcloud/server/blob/6312c0df6949955d1cd59c3dd444268e0773293c/tests/lib/DB/QueryBuilder/ExpressionBuilderDBTest.php but we'd need a new table in the testing app to properly test this against real databases. Or would you have any other idea to cover that?

@blizzz blizzz mentioned this pull request Feb 1, 2023
This was referenced May 3, 2023
@blizzz blizzz mentioned this pull request May 17, 2023
@blizzz blizzz modified the milestones: Nextcloud 27, Nextcloud 28 May 23, 2023
@juliusknorr juliusknorr force-pushed the bugfix/noid/sqlite-datetime branch from 43608b8 to ab3cdcc Compare July 10, 2023 17:42
@juliusknorr juliusknorr requested review from a team and ArtificialOwl and removed request for rullzer, PVince81, CarlSchwan and a team July 10, 2023 17:42
@juliusknorr juliusknorr requested review from icewind1991 and nfebe July 10, 2023 17:42
@juliusknorr
Copy link
Member Author

Rebased and pushed a test case for this. I added a new table to the testing app to have a datetime column around during tests.

@juliusknorr juliusknorr force-pushed the bugfix/noid/sqlite-datetime branch from 197d052 to c829472 Compare July 13, 2023 21:28
@skjnldsv skjnldsv mentioned this pull request Nov 1, 2023
This was referenced Nov 6, 2023
@blizzz blizzz mentioned this pull request Nov 14, 2023
@juliusknorr juliusknorr force-pushed the bugfix/noid/sqlite-datetime branch from c829472 to 4d1edf2 Compare November 20, 2023 09:17
@juliusknorr juliusknorr force-pushed the bugfix/noid/sqlite-datetime branch from 4d1edf2 to 58c4c1e Compare December 19, 2023 20:39
Move the logic to prepare a column to the parent ExpressionBuilder so
that it can be reused for OCI and sqlite

Signed-off-by: Julius Härtl <jus@bitgrid.net>
Signed-off-by: Julius Härtl <jus@bitgrid.net>
@juliusknorr juliusknorr force-pushed the bugfix/noid/sqlite-datetime branch from 58c4c1e to e50cb0a Compare December 29, 2023 09:37
@juliusknorr juliusknorr merged commit 6a63974 into master Dec 29, 2023
50 checks passed
@juliusknorr juliusknorr deleted the bugfix/noid/sqlite-datetime branch December 29, 2023 10:52
@blizzz blizzz mentioned this pull request Mar 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants