Add support for ?dialect=postgresql to /api/migrations #956
Conversation
alco
changed the title
alco
force-pushed
the
alco/vax-1659-postgresql-dialect
branch
2 times, most recently
from
5d932fd
to
f573c5f
Compare
There was a problem hiding this comment.
I think this is exactly the way to do it. maybe we should sort out that type field in the migration message now though. what is it for? i'm guessing the intention was to make the schema info available to the clients without them having to parse the migration sql, In which case what's the best way to pass that? we could have a generic structure that gave the base type as a string and any modifiers (like length, precision etc) as a map or something? if we knew we were only ever going to support sqlite and pg, then what we have works but that makes me wary.
| } | ||
| end | ||
|
|
||
| defp replication_msg_table_col(%Proto.Column{} = column, dialect) do | ||
| defp replication_msg_table_col(%Proto.Column{} = column) do | ||
| %SatOpMigrate.Column{ | ||
| name: column.name, | ||
| pg_type: replication_msg_table_col_type(column.type), |
There was a problem hiding this comment.
my original {sqlite,pg}_type thing makes no sense. why don't we just have type here?
There was a problem hiding this comment.
I believe I left both sqlite_type and pg_type here to avoid breaking existing code, as this was a POC. I did mark both as deprecated in the protocol definition, so ideally only type_info should remain.
But as of #1054, Kevin and I encountered some TypeScript typing issues with type_info. Long story short, the latest iteration of this changes now lives in #1065 where sqlite_type is deprecated (can be removed today, just needs some client-side tests for /api/migrations updated) and only pg_type remain as the source of truth for the column type.
| columns: [ | ||
| %SatOpMigrate.Column{ | ||
| name: "id", | ||
| sqlite_type: "TEXT", |
There was a problem hiding this comment.
so the pg_type bit carries the original type info dropped by the conversion to sqlite's type. hmm. perhaps just dropping the sqlite_ prefix and having a dialect-defined type which is just a string?
We have the From looking at the client code, it doesn't need So we can drop |
sqlite_type is not needed and pg_type is replaced by type_info.
alco
force-pushed
the
alco/vax-1659-postgresql-dialect
branch
from
f2579d6
to
12e769b
Compare
Closes VAX-1659.
Passing
?dialect=postgresqlto/api/migrationswill now result in the returned migrations to have the original Postgres DDL.