SkillDevs / electric_dart

A Dart implementation for Electric (electric-sql.com).
Apache License 2.0
104 stars 9 forks source link

Support postgres, add simple backend to example #8

Closed simolus3 closed 9 months ago

simolus3 commented 9 months ago

Hi :wave: Since drift also supports accessing postgres databases, I wanted to see what it takes to connect to postgres databases via the drift database generated by this package. As it turns out, not many changes are needed at all. I think the biggest change is that the custom types don't need to do any mappings when talking to a postgres database (at least that seems to make them work for the timezone column in the example).

As a proof-of-concept, I've added a very simple backend to the todo app example. It serves all todos by connecting to the postgres database through the electric proxy. At the moment this is all quite hacky, I've just done the minimal work required for that not to crash :D I also had to move some files around to avoid importing dart:ui in the backend.

If you think postgres support looks promising / is something worth doing, I'm happy to get this into a merge-able state. I want to highlight some synchronization solutions on drift's documentation website, and being able to point to a (sort-of) fullstack example would be pretty neat.

davidmartos96 commented 9 months ago

Hello Simon! That's a great idea. Electric supports bi-directional syncing, so having a simple demo showcasing it, it's great. One thing that would be even better would be to add an additional shelf route which inserts a random todo, to show that updates made on the backend sync back to the clients.

Regarding type mappings, there is a test suite in the client with all the supported data types. Supported types are listed here: https://electric-sql.com/docs/usage/data-modelling/types#supported-data-types

I guess most of them don't need to be encoded with the postgres interface. Maybe it will be necessary to add some conditionals based on the context the drift schema is running (sqlite or postgres). The test suite could also be extended/adapted to run for postgres or for sqlite "backends". Sharing the same drift schema in a fullstack example would be very ideal.

Thank you for contributing! I take the chance to thank you for the work you do with drift. It's very well thought out and extensible.

simolus3 commented 9 months ago

Alright, I've added very simple tests for the postgres-specific types to make sure nothing is completely wrong. So I think this is ready for review now.

davidmartos96 commented 9 months ago

@simolus3 Thank you! Very nice and simple implementation for the tests, I thought a Postgres connection would be needed.

I will try to test the changes tomorrow. Some of my worries were regarding the special mapping for float (NaN/Infinite) and the json types, but I guess with the pg.TypedValue mapping, that is covered automatically by the postgres package 😄

davidmartos96 commented 9 months ago

@simolus3 I'm looking into also adding support for enum types in the new postgres mapping. Do you know what would be the corresponding pg.TypedValue and pg.Type for a Postgres enum? Is it pg.Type.name ?

That one appears to work in a simple enum test. The type mapping test file was missing tests for enum types. Electric generates a codec that encodes a Dart enum into a String, so I think the String could be used in the pg.Type.name. What do you think?

I've just pushed the changes

davidmartos96 commented 9 months ago

@simolus3 Unfortunately it doesn't seem to work with Type.name. I'm currently trying with a real postgres connection through the backend.dart script with the following Postgres schema:

CREATE TYPE color AS ENUM ('RED', 'GREEN', 'BLUE');

CREATE TABLE public.enums (
    id TEXT PRIMARY KEY,
    c color
);

ALTER TABLE
    public.enums ENABLE ELECTRIC;
// Dart enum and codecs are automatically generated with `dart run electricsql_cli generate`
await database.into(database.enums).insert(
    EnumsCompanion.insert(
        id: genUUID(),
        c: Value(DbColor.red),
    ),
);
simolus3 commented 9 months ago

In the protocol, we'd have to bind enum values to their correct type oids and binary representation if we wanted to send them directly. For inserts, it looks like sending them as text would be accepted by postgres, but it might be better to explicitly generate SQL like $1::color and then bind a string. Since the oid is unknown, the postgres package will return enums as an UndecodedBytes representation, but calling asString on that seems to return the name of the enum which might be useful.

https://github.com/simolus3/drift/issues/2841 is an issue to track transforming SQL when selecting from values of a certain type, or to change the SQL when binding variables of a certain type. If that were supported in drift, we could explicitly cast to strings whenever enums are involved and not worry about it. But it looks like the workaround of sending them as strings and interpreting the bytes manually to parse them should work.

davidmartos96 commented 9 months ago

@simolus3 Thanks! I'm now trying with pg.Type.text, but it seems to be rejected by the driver. I'm not sure how to insert an enum as text with drift.

It works with a direct connection using the postgres package:

await conn.execute(
  r'INSERT INTO enums(id, c) VALUES($1, $2)',
  parameters: [genUUID(), 'RED'],
);

Using drift these don't work:

await database.customInsert(
  r'INSERT INTO enums(id, c) VALUES($1, $2)',
  variables: [Variable(genUUID()), Variable('RED')],
);

// Using `pg.Type.text` in Electric
await database.into(database.enums).insert(
  EnumsCompanion.insert(
    id: genUUID(),
    c: Value(DbColor.red),
  ),
);
Severity.error 42804: column "c" is of type color but expression is of type text hint: You will need to rewrite or cast the expression.
package:postgres/src/v3/connection.dart 81:30                   _PgSessionBase._sendAndWaitForQuery
package:postgres/src/v3/connection.dart 182:11                  _PgSessionBase._prepare
package:postgres/src/v3/connection.dart 159:30                  _PgSessionBase.execute
package:drift_postgres/src/pg_database.dart 124:34              _PgDelegate.runInsert
package:drift/src/runtime/executor/helpers/engines.dart 105:19  _BaseExecutor.runInsert.<fn>
package:drift/src/runtime/executor/helpers/engines.dart 57:22   _BaseExecutor._synchronized.<fn>
dart:async                                                      new Future.sync
package:drift/src/utils/synchronized.dart 17:21                 Lock.synchronized.callBlockAndComplete
package:drift/src/utils/synchronized.dart 23:14                 Lock.synchronized
package:drift/src/runtime/executor/helpers/engines.dart 55:20   _BaseExecutor._synchronized
package:drift/src/runtime/executor/helpers/engines.dart 102:12  _BaseExecutor.runInsert
package:drift/src/runtime/api/connection_user.dart 292:25       DatabaseConnectionUser.customInsert.<fn>
package:drift/src/runtime/api/connection_user.dart 334:48       DatabaseConnectionUser._customWrite.<fn>
package:drift/src/runtime/api/connection_user.dart 162:64       DatabaseConnectionUser.doWhenOpened.<fn>
davidmartos96 commented 9 months ago

@simolus3 I managed to make it work using Type.unspecified, and then bind the String value. To read decode the UndecodedBytes as you suggested. Would you say this is the correct approach because of the lack of an oid? Changes in cb9038382fe21369644bccb294bc7a99f905b683

simolus3 commented 9 months ago

Yeah that looks correct to me, it's probably the best we can do here at the moment without better support for that in drift itself.

davidmartos96 commented 9 months ago

@simolus3 Thank you for the contribution! 😄