New SQLite driver: capacitor-sqlite #484

gregzo · 2023-09-22T12:11:47Z

This PR adds compatibility with capacitor-community/sqlite via a new adapter and accompanying plumbing and tests.

We could envisage asking capacitor-community/sqlite's maintainer to have a quick look to ensure that there's no flagrant misuse of its APIs. As it stands, all tests pass but using a mock database.

This adapter implementation is built on my understanding of electric and capacitor-community/sqlite documentation and codebase. Much more extensive real-world testing is imo required.

clients/typescript/src/drivers/capacitor-sqlite/database.ts

kevin-dp

Thanks again for your contribution!
I reviewed your PR and mostly have minor comments to improve readability. However, i do have one major concern. The transaction<T> method of the adapter is managing the transaction manually by calling BEGIN and COMMIT/ABORT.

This is prone to concurrency issues if one would call transaction<T> before the previous transaction finished; in which case the transactions will get interleaved. Even if we assume that clients won't do that, a similar issue could still occur when clients call transaction<T> (e.g. as a result of calling db.items.create) and concurrently there is a transaction coming in via the replication stream and being applied.

We've had similar issues with the wa-sqlite driver and solved them by using a mutex as to ensure that the adapter never tries to run two transactions concurrently. I suggest you have a look at the implementation of transaction<T> in the wa-sqlite driver.

clients/typescript/src/drivers/capacitor-sqlite/adapter.ts

kevin-dp · 2023-09-26T08:18:16Z

clients/typescript/src/drivers/capacitor-sqlite/adapter.ts

+    const wrapInTransaction = true;
+
+    return this.db.executeSet(set, wrapInTransaction).then( (result: capSQLiteChanges) => {
+      // TODO: unsure how capacitor-sqlite populates the changes value (additive?), and what is expected of electric here.


We expect the changes to be additive, i.e. the total amount of changes accumulated over all the statements in this transaction. For example, in the better-sqlite3 adapter we have to accumulate those changes manually. Could you write a test for this?

I'd need to write a test against a real db, not the mock. What I can do for now is ask @samwillis to double check this in his example project.

clients/typescript/src/drivers/capacitor-sqlite/adapter.ts

clients/typescript/src/drivers/capacitor-sqlite/mock.ts

gregzo · 2023-09-29T08:54:45Z

All requested changes implemented, all tests still pass. Thank you for the review! The only change that is more than a stylistic or readability refactor is the mutex implementation. I've left that conversation unresolved.

clients/typescript/src/drivers/capacitor-sqlite/adapter.ts

samwillis · 2023-10-02T20:03:54Z

Hey @gregzo, good news, with the two small changes I've comment on this worked with my demo app (I've tested iOS but not android yet, but I don't think that will change anything) 😁

It looks like you also have an issue with pnpm-lock.yaml not being top to date (see the failed CI tests), I think all you need to do is run pnpm install to update it.

gregzo · 2023-10-03T09:03:58Z

@samwillis pnpm-lock now up to date. FYI ran into this known pnpm issue: pnpm/pnpm#1801
Manually created missing folders and js files to work around it.

kevin-dp

Good job @gregzo!
Driver looks great, i'm waiting for the tests to finish and will merge afterwards.
Thanks again for your amazing contribution 💯

kevin-dp · 2023-10-03T12:33:51Z

@gregzo Could you run yarn format in clients/typescript please?

alco

I see you merged main into your PR branch. We don't do that normally and instead rebase PR branches on top of main.

Please rebase your branch on top of the latest main and remove the merge commit and the Version Packages one. Let me know if you need a hand with that.

clients/typescript/CHANGELOG.md

clients/typescript/package.json

clients/typescript/src/version/index.ts

alco · 2023-10-04T09:33:39Z

The E2E test failures seem to be caused by the fact that your branch is based on an older version of the server. When you rebase it on top of the latest main, the E2E problems should be resolved.

…base.ts

…at this stage.

…st.ts

…tches native calls.

…d rollbacks

…mplementations.

…qlite tests.

alco · 2023-10-05T09:35:07Z

@gregzo Rebase looks great. Thanks!

I was just about to comment about the pnpm lockfile update. I've pushed a commit for it here. But you beat me to it.

Did you also get these warnings when you ran pnpm install?

$ pnpm install
Scope: all 6 workspace projects
clients/typescript                       |  WARN  deprecated @types/react-native-uuid@2.0.0
clients/typescript                       |  WARN  deprecated squel@5.13.0
e2e/satellite_client                     |  WARN  deprecated tslint@6.1.3
e2e/satellite_client                     |  WARN  deprecated @tsmodule/tsmodule@40.19.0
Downloading registry.npmjs.org/typescript/5.1.3: 7.15 MB/7.15 MB, done
Downloading registry.npmjs.org/typescript/4.9.5: 11.62 MB/11.62 MB, done
Downloading registry.npmjs.org/react-native/0.68.0: 21.14 MB/21.14 MB, done
Downloading registry.npmjs.org/hermes-engine/0.11.0: 15.39 MB/15.39 MB, done
Downloading registry.npmjs.org/sql.js/1.8.0: 7.45 MB/7.45 MB, done
Downloading registry.npmjs.org/@stencil/core/4.4.0: 8.75 MB/8.75 MB, done
Downloading registry.npmjs.org/jsc-android/250230.2.1: 36.09 MB/36.09 MB, done
Downloading registry.npmjs.org/dprint-node/1.0.7: 10.55 MB/10.55 MB, done
 WARN  14 deprecated subdependencies found: @babel/plugin-proposal-async-generator-functions@7.20.7, @babel/plugin-proposal-private-property-in-object@7.21.0, @babel/plugin-proposal-unicode-property-regex@7.18.6, @npmcli/move-file@1.1.2, har-validator@5.1.5, request-promise-native@1.0.9, request@2.88.2, resolve-url@0.2.1, source-map-resolve@0.5.3, source-map-url@0.4.1, uglify-es@3.3.9, urix@0.1.0, uuid@3.4.0, w3c-hr-time@1.0.2
Packages: +12
++++++++++++
Progress: resolved 1844, reused 0, downloaded 1759, added 12, done
 WARN  Issues with peer dependencies found
clients/typescript
└─┬ react-native 0.68.0
  ├── ✕ unmet peer react@17.0.2: found 18.2.0
  └─┬ react-shallow-renderer 16.14.1
    └── ✕ unmet peer react@"^16.0.0 || ^17.0.0": found 18.2.0

Done in 34.6s

I'm wondering if there are any fixes we need to apply to our deps. And it's curious that the lockfile version dropped to 6.0 from 6.1, even though I had just updated my pnpm to the latest version yesterday.

@balegas @kevin-dp @samwillis

.changeset/odd-falcons-float.md

gregzo · 2023-10-05T09:41:52Z

The E2E test failures seem to be caused by the fact that your branch is based on an older version of the server. When you rebase it on top of the latest main, the E2E problems should be resolved.

Rebased on electric/main and added changeset. E2E tests still fail though - happy to follow your lead if there's anything else I can do.

Edit: all tests pass. Got 3 failure notifications in my inbox after pushing, among them E2E. All seems well now.

alco

🏅

gregzo · 2023-10-05T10:13:02Z

@alco Yes, I encountered the same warnings.

balegas requested a review from kevin-dp September 25, 2023 10:01

kevin-dp reviewed Sep 26, 2023

View reviewed changes

clients/typescript/src/drivers/capacitor-sqlite/database.ts Outdated Show resolved Hide resolved

kevin-dp requested changes Sep 26, 2023

View reviewed changes

samwillis reviewed Oct 2, 2023

View reviewed changes

clients/typescript/src/drivers/capacitor-sqlite/adapter.ts Outdated Show resolved Hide resolved

kevin-dp approved these changes Oct 3, 2023

View reviewed changes

alco reviewed Oct 4, 2023

View reviewed changes

clients/typescript/CHANGELOG.md Outdated Show resolved Hide resolved

clients/typescript/package.json Outdated Show resolved Hide resolved

clients/typescript/src/version/index.ts Outdated Show resolved Hide resolved

gregzo force-pushed the main branch 3 times, most recently from 4637899 to 8a301ac Compare October 4, 2023 20:19

gregzo added 16 commits October 5, 2023 11:02

Stubbed capacitor-sqlite driver and Ionic app example

993bd16

package.json added capacitor-sqlite deps

0e46ffc

Expose only relevant functions of the Capacitor SQLite plugin in data…

0c2713b

…base.ts

adapter WIP

e089f83

adapter WIP

43329b6

run and runInTransaction first implementations

442d5fb

Implementation done. transaction<T> implementation most likely wrong …

562b469

…at this stage.

Still WIP but basic adapter implementation done. Next: mock.ts and te…

f4f7fe0

…st.ts

Removed erroneous @types declaration in package.json

cc5b7ed

Removed ionic-react example stub

9c9da4a

Cleanup

9913eeb

Disable automatic transactions in run method implementation

4df4fac

Swapped executeInTransaction for executeSet which more efficiently ba…

855cb46

…tches native calls.

New implementation of transaction<T> with handling of transactions an…

8ef338a

…d rollbacks

Added mock.ts and test.ts, based on existing cordova-sqlite-storage i…

3fe0e09

…mplementations.

MockDatabase updated to mock errors. Tests added, modelled after wa-s…

5392304

…qlite tests.

gregzo added 10 commits October 5, 2023 11:02

Test file added.

aa66d49

All tests now pass.

523fe6d

Prettier

d874df7

async/await style in adapter.ts

003df38

Typo fix. Readability refactors according to reviewer suggestions.

ba46688

Implemented synchronized transaction with mutex as requested by reviewer

ea54919

Prettier

e72445a

All tests pass. Cleanup and comments.

6729b34

Coalesce args to empty array when building capSQLiteSet

ac33bd0

Rebased on electric/main. Prettier. Changeset.

db83192

gregzo force-pushed the main branch from 8a301ac to db83192 Compare October 5, 2023 09:26

kevin-dp reviewed Oct 5, 2023

View reviewed changes

.changeset/odd-falcons-float.md Outdated Show resolved Hide resolved

Fixed changeset file as per reviewer request.

e125d5a

alco approved these changes Oct 5, 2023

View reviewed changes

samwillis merged commit 3d98c1f into electric-sql:main Oct 5, 2023
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New SQLite driver: capacitor-sqlite #484

New SQLite driver: capacitor-sqlite #484

gregzo commented Sep 22, 2023

kevin-dp left a comment

kevin-dp Sep 26, 2023

gregzo Sep 29, 2023

gregzo commented Sep 29, 2023

samwillis commented Oct 2, 2023

gregzo commented Oct 3, 2023

kevin-dp left a comment

kevin-dp commented Oct 3, 2023

alco left a comment

alco commented Oct 4, 2023

alco commented Oct 5, 2023 •

edited

gregzo commented Oct 5, 2023 •

edited

alco left a comment

gregzo commented Oct 5, 2023

New SQLite driver: capacitor-sqlite #484

New SQLite driver: capacitor-sqlite #484

Conversation

gregzo commented Sep 22, 2023

kevin-dp left a comment

Choose a reason for hiding this comment

kevin-dp Sep 26, 2023

Choose a reason for hiding this comment

gregzo Sep 29, 2023

Choose a reason for hiding this comment

gregzo commented Sep 29, 2023

samwillis commented Oct 2, 2023

gregzo commented Oct 3, 2023

kevin-dp left a comment

Choose a reason for hiding this comment

kevin-dp commented Oct 3, 2023

alco left a comment

Choose a reason for hiding this comment

alco commented Oct 4, 2023

alco commented Oct 5, 2023 • edited

gregzo commented Oct 5, 2023 • edited

alco left a comment

Choose a reason for hiding this comment

gregzo commented Oct 5, 2023

alco commented Oct 5, 2023 •

edited

gregzo commented Oct 5, 2023 •

edited