fix: Mark SQLite backend as "returning"

[andoriyu]

The RETURNING syntax has been supported by SQLite since version 3.35.0 (2021-03-12) and is modeled after PostgreSQL.

PR Info

• Closes SQLite backend isn't using returning  #653

Adds

Makes it possible to use returning branch in updates in inserts. The same way it's working in PostgreSQL.

Fixes

Breaking Changes

No breaking changes.

Changes

Changes how update and insert behaves internally.

[tyt2y3]

Tests are failing, there must be something more we need to do to support returning in SQLite

[andoriyu]

@tyt2y3 weird, I remember tests passing locally. I will work some more on this.

[Sytten]

This is only for more recent versions of sqlite, possible that the version of sqlite used by the runner is too old.

[andoriyu]

This is only for more recent versions of sqlite, possible that the version of sqlite used by the runner is too old.

@Sytten Well, new as in since version 3.35.0 (2021-03-12). Should I put it behind a feature gate? I'm currently using RETURNING * in my project without any issues

Unsure if it's possible to get SQLite version at build time and conditionally enable that feature.

[Sytten]

Diesel uses a feature flag, so it might be a good idea since sea-orm is on a similar stack level (it would not make sense in sea-query). I checked the runner and it is using ubuntu-20.04 which ships with sqlite 3.31 by default. Add this in the dev-dependencies and it should work (it will use a static sqlite instead of the system one):

libsqlite3-sys = { version = "0.24.2", features = ["bundled"] }

[billy1624]

Yep, GitHub Actions on Ubuntu 20.04 ships with SQLite 3.31.
https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2004-Readme.md#databases

[billy1624]

As @Sytten suggested, yes, we can introduce a flag just like what diesel did: feature returning_clauses_for_sqlite_3_35 flag. But the problem is does this fit SeaORM design paradigm? Personally, I think adding the feature flag for SQLite returning support is okay.

Thoughts? @tyt2y3

[tyt2y3]

I think we want to always look forward. If support for that is universal (enough), we'd simply enable it without the possibility to turn it off.

[tyt2y3]

So the question is how universal is this support across major linux distros (for reference)?

[andoriyu]

Quick glance: Ubuntu 22.04 LTS, Ubuntu 21.10, Debían testing, Fedora 35/36, Alpine, nix stable, homebrew (macos) support it. Couldn't find a CentOS version that does, but that's just CentOS-life. I think this mostly effects CI builders since a lot of them use 20.04 release as base. Amazon Linux 2 doesn't, but that should affect no one. Not a jab at Amazon Linux, it just not used anywhere to build rust projects to my knowledge, it could be used to host containers that do, but not by itself.

…

On Sun, Jul 10, 2022, 1:23 AM Chris Tsang ***@***.***> wrote: So the question is how universal is this support across major linux distros (for reference)? — Reply to this email directly, view it on GitHub <#654 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABEFG574HAU5ZE5VG27T4DVTKCB3ANCNFSM5SG65HWQ> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[andoriyu]

This been assigned to me, but what's the consensus on this, so I know what to do next?

[tyt2y3]

I think that since we upgraded sqlx to 0.6 now, we if mark support_returning as true and fixed all tests, we can release it in 0.10.0

[andoriyu]

I think that since we upgraded sqlx to 0.6 now, we if mark support_returning as true and fixed all tests, we can release it in 0.10.0

Well, the tests are failing because CI runner has old version of SQLite. I'll double check on my fork that embedded SQLite version works.

I think if we don't hide behind feature flag, then using older version of SQLite should be a compile time error to prevent surprises at runtime.

[billy1624]

Hey @andoriyu, I think we can bump the OS version on GitHub Action to ubuntu-22.04. As its sqlite3 version is 3.37.2.
https://github.com/actions/virtual-environments/blob/main/images/linux/Ubuntu2204-Readme.md

[tyt2y3]

Sounds like a good plan

[andoriyu]

@tyt2y3 @billy1624 I've updated workflows that use SQLite to ubuntu-22.04

[billy1624]

Hmmmm... the behaviour of SQLite changed in newer version?

thread 'main' panicked at 'could not update cake: Query("error occurred while decoding column "price": mismatched types; Rust type core::option::Option<f64> (as SQL type REAL) is not compatible with SQL type INTEGER")', tests/crud/updates.rs:46:66

https://github.com/SeaQL/sea-orm/runs/7606184783?check_suite_focus=true#step:5:671

[andoriyu]

@billy1624 related sqlx issue and SQLite forum post.

It's a bug in SQLite that is fixed, but not in the version installed on runners 🤦‍♂️. I think it's passing on sqlx-vendored SQLite, that's what I used when I first opened this PR.

[billy1624]

Hey @andoriyu, can you try something on your machine?

I can run issues/654/src/main.rs on my machine

• billy1624@5d16fcf

But SeaORM test cases failed. Both are essentially the same: inserting a floating point number w/o decimal e.g. 5.0 with returning clause.

Why it can be run w/o error on SQLx but not SeaORM?

[billy1624]

@billy1624 related sqlx issue and SQLite forum post.

It's a bug in SQLite that is fixed, but not in the version installed on runners 🤦‍♂️. I think it's passing on sqlx-vendored SQLite, that's what I used when I first opened this PR.

I think it has been patched on SQLite long time ago and the patch should be included in SQLite version 3.37.2