Test custom pg schema #457
Conversation
DZakh
force-pushed
the
integration-testing-add-custom-schema
branch
from
be58e4a to
17acae6
Compare
|
I think we need to add the schema name to the persisted state to ensure envio dev reruns codegen if it changes |
| //TODO: put the start script in the generated package.json | ||
| //and run from there. | ||
| if should_use_raw_events_worker { | ||
| args.push("--"); | ||
| args.push("--sync-from-raw-events"); | ||
| } |
| let sql = Db.sql | ||
| let needsRunUpMigrations = await sql->Migrations.needsRunUpMigrations | ||
| if needsRunUpMigrations { | ||
| let _ = await Migrations.runUpMigrations(~shouldExit=false) | ||
| } |
There was a problem hiding this comment.
Added a run up migration here when a db schema doesn't exist
| | Custom(name) => `${Env.Db.publicSchema}.${name}` | ||
| | _ => fieldType :> string | ||
| }}${isArray ? "[]" : ""}${switch defaultValue { |
There was a problem hiding this comment.
Had to add schema name here as well
| } | ||
| "configuration": { | ||
| // Otherwise the entity in gql will be prefixed with the schema name (when it's not public) | ||
| "custom_name": tableName |
There was a problem hiding this comment.
Remove the prefix schema and optimise multiple requests into one.
| @@ -11,7 +11,10 @@ let creatTableIfNotExists = (sql, table) => { | |||
| let fieldName = field->Table.getDbFieldName | |||
|
|
|||
| { | |||
| `"${fieldName}" ${(fieldType :> string)}${isArray ? "[]" : ""}${switch defaultValue { | |||
| `"${fieldName}" ${switch fieldType { | |||
| | Custom(name) if !(name->Js.String2.startsWith("NUMERIC(")) => `"${Env.Db.publicSchema}".${name}` | |||
There was a problem hiding this comment.
This seems a bit fragile and I'm not following. When does custom start with "NUMERIC(" ?
There was a problem hiding this comment.
I kind of think we should rather separate these into two variant constructors. Maybe Enum and Custom for eg.
There was a problem hiding this comment.
Numeric is for precision. This is actually should be a separate constructor. I just don't want to spend time on this now. And besides the Numeric custom is used only for enums
There was a problem hiding this comment.
Ok cool 👍🏼 maybe just add a comment explaining then.
| `DO $$ | ||
| BEGIN | ||
| ${reset ? `DROP SCHEMA IF EXISTS ${Env.Db.publicSchema} CASCADE;` : ``} | ||
| IF NOT EXISTS(SELECT 1 FROM information_schema.schemata WHERE schema_name = '${Env.Db.publicSchema}') THEN | ||
| CREATE SCHEMA ${Env.Db.publicSchema}; | ||
| GRANT ALL ON SCHEMA ${Env.Db.publicSchema} TO postgres; | ||
| GRANT ALL ON SCHEMA ${Env.Db.publicSchema} TO public; | ||
| END IF; | ||
| END $$;`, | ||
| ) |
There was a problem hiding this comment.
Why does any of this run on the non reset case? I see it only drops in this case but what about the rest?
There was a problem hiding this comment.
For example, you run pnpm start with a different schema. In this case it won't trigger the down migration (since pnpm start doesn't have the logic), but it still should create the schema if it's missing, so it's able to work correctly.
| | _ => Failure | ||
| } | ||
|
|
||
| process->exit(exitCode) |
There was a problem hiding this comment.
It looks like we no longer call process.exit with exit code. This could be a problem for the hosted service
There was a problem hiding this comment.
We call it in runUpMigrations, but the first await sql->unsafe( actually don't have it. Should I add it back? Won't it exit with 1 automatically if the query fails?
There was a problem hiding this comment.
All of the other potential failures are caught and only returned in the exit code. We definitely need the exit code to work accurately for the hosted service so I would suggest we keep the same logic.
| let tableNames = | ||
| [Db.allStaticTables, Db.allEntityTables] | ||
| ->Belt.Array.concatMany | ||
| ->Js.Array2.map(({tableName}) => tableName) | ||
| await trackTables(~tableNames) |
There was a problem hiding this comment.
I guess once the tables are all tracked the relationships and permisions can also be setup concurrently as well?
| @@ -37,7 +37,6 @@ let allStaticTables: array<Table.table> = [ | |||
| TablesStatic.PersistedState.table, | |||
| TablesStatic.EndOfBlockRangeScannedData.table, | |||
| TablesStatic.RawEvents.table, | |||
| TablesStatic.DynamicContractRegistry.table, | |||
There was a problem hiding this comment.
Is this only affecting hasura visibility? Even so, Could be a breaking change unfortunately
There was a problem hiding this comment.
It's not, since the table is a part of Entity tables. So this was duplicated here.
| SELECT 1 | ||
| FROM pg_type | ||
| WHERE typname = '${name->Js.String2.toLowerCase}' | ||
| AND typnamespace = (SELECT oid FROM pg_namespace WHERE nspname = '${Env.Db.publicSchema}') |
There was a problem hiding this comment.
Wow, devils here hey, was this caught by a test? :)
| IF EXISTS(SELECT 1 FROM information_schema.schemata WHERE schema_name = '${Env.Db.publicSchema}') THEN | ||
| DROP SCHEMA ${Env.Db.publicSchema} CASCADE; | ||
| END IF; | ||
| DROP SCHEMA IF EXISTS ${Env.Db.publicSchema} CASCADE; |
There was a problem hiding this comment.
Does this work if it doesn't previously exist?
There was a problem hiding this comment.
Yes, it doesn't do anything in this case.
No description provided.