[feat] Add support for other db clients

[marvin-wtt]

About

Add support for other DB client drivers.

• The visitor query needed some modification, as some drivers (like SQLite) don't support queries to increment JSON values. Also, the insertion of a new visit must occur in the transaction too to avoid concurrent writes.

See ##761

Breaking Change

• When migrating, all cooldowns are lost.

Tasks

• [x] Convert raw queries
• [x] Replace special column types
• [x] Do not use returning in queries

[marvin-wtt]

@poeti8 It seems like the migrations got messed up somewhere in the past. Some migrations are modified at a later stage or appied multiple times. I'd adress this in another PR to not blow this one up.

[poeti8]

@marvin-wtt Hmmm I see. Thanks.

[marvin-wtt]

@poeti8 PR should be ready for review now

[marvin-wtt]

@poeti8 I think it would be best to add some tests to verify that it actually works for different DB types, mainly mysql, mariadb, pg, and sqlite3. I offer to do it. Does vitest work for you, or do you prefer jest?

[poeti8]

Thank you, I'll run this to test it soon.

Does vitest work for you, or do you prefer jest?

vitest please.

[marvin-wtt]

Thank you, I'll run this to test it soon.

I think it's easier if you just check out the migrations PR directly. This way you can just run npm run migrate and everything is set up.

[poeti8]

Hey, any updates on this? Is it done other than my suggestions?

[marvin-wtt]

Hey, any updates on this? Is it done other than my suggestions?

I tested the branch manually and everything worked fine. I created a testsuit but adding all tests will take some time. I'm currently on vacation. Maybe it's better to push it in a separate PR.

The migration branch addresses the issues regarding the migrations but I think we disagree on how it should be run.

[poeti8]

I tested the branch manually and everything worked fine. I created a testsuit but adding all tests will take some time. I'm currently on vacation. Maybe it's better to push it in a separate PR.

Sweet, thanks. Yes, we can leave the tests for another pull request. I'll go ahead by testing and merging this one soon.

The migration branch addresses the issues regarding the migrations but I think we disagree on how it should be run.

I'll take a close look again. We'll figure something out.

[marvin-wtt]

Returning is not supported by mysql. By default the inserted id is returned. For pg, returning id should result in the same

[poeti8]

Returning is not supported by mysql. By default the inserted id is returned. For pg, returning id should result in the same

Yes I noticed. The mistake however was that knex doesn't return id as value, it returns it within the object: [{ id }]. So I updated the code to access the id correctly via createdRows[0].id.

[marvin-wtt]

I think your fix breaks MYSQL support. Can you confirm? Maybe we need to add a helper function.

return typeof res[0] === 'string' ? res[0] : res[0].id;

[poeti8]

I think your fix breaks MYSQL support. Can you confirm? Maybe we need to add a helper function.

You're right I should make sure. I'll write a helper function if that is the case. Although, I assume knex would be consistent with what it returns, otherwise people would end up handling all sort of edge cases manually.

[poeti8]

This looks good. I'll test everything manually again with the three databases and merge it after.

Really good job. Thanks!

[poeti8]

Found an issue: Dates in SQLite are saved like 2024-09-25 12:56:52. This would break any place we check or parse the dates since it's not valid for new Date(). Need to write a helper function to handle these. Maybe can only use this helper function if the client is one of SQLite's to avoid using any RegExp that would make things slower.

[marvin-wtt]

I'll take a look. If it's not possible to store it in zulu time, at least we can add a knex typecast so no helper function is needed

[poeti8]

I see the problem. Postgres stores the date with timezone but others not. When converting them to a JavaScript date I have to make sure they are created as a UTC date instead of a local-time date. Typecast is a MySQL driver feature only, so probably I have to write a util function at the end.

[marvin-wtt]

I see the problem. Postgres stores the date with timezone but others not. When converting them to a JavaScript date I have to make sure they are created as a UTC date instead of a local-time date. Typecast is a MySQL driver feature only, so probably I have to write a util function at the end.

This can be configured with knex for pg:

https://knexjs.org/guide/schema-builder.html#timestamp

[poeti8]

This can be configured with knex for pg:

https://knexjs.org/guide/schema-builder.html#timestamp

Yep, I'll make a migration to convert all current dates to UTC, and then assume UTC dates across.

Edit: I noticed the pg driver actually returns the time in UTC and as a JavaScript date, so there's no problem for Postgres after all. I have to test every database driver and see which ones do I need to convert to UTC manually in the server.

[poeti8]

Manged to make SQLite working.

MySQL driver however, likes to convert dates before sending it to the databases: I pass "2024-10-07 12:30:00" but it assumes it's not in UTC so it converts it to "2024-10-07 09:00:00". There's an option to bypass it for the incoming data, but not for when inserting or updating the data.

That's the only issue as far as I tested, I'll merge this branch for now and revisit this problem later. You can look into it if you want.

[poeti8]

^ I fixed the MySQL problem for dates by assuming app server and MySQL server are in the same timezone. Feel free to test. I'll merge the pull request now.