node-pg-migrate icon indicating copy to clipboard operation
node-pg-migrate copied to clipboard

Syntax error when adding constraint

Open shriharip opened this issue 9 months ago • 1 comments

Describe the bug

I am adding a new constraint by creating new migration. node-pg-migrate create constriantonTable

Steps to reproduce

  1. After the migration file is created. update it with following code

exports.up = (pgm) => { pgm.createConstraint("books", "books_author_id_fkey", { deferrable: true, }); };

Then run the migration with node-pg-migrate up command

Logs

MIGRATION 1715771368273_constraint (UP)

ALTER TABLE "books" ADD ; INSERT INTO "public"."pgmigrations" (name, run_on) VALUES ('1715771368273_constraint', NOW());

Error executing: ALTER TABLE "books" ADD ; ^^^^

syntax error at or near ";"

Rolling back attempted migration ... error: syntax error at or near ";" at /Users/hari/joist-sample/node_modules/pg/lib/client.js:526:17 at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at async Object.query (/Users/hari/joist-sample/node_modules/node-pg-migrate/dist/db.js:70:14) at async runner (/Users/hari/joist-sample/node_modules/node-pg-migrate/dist/runner.js:261:9) { length: 90, severity: 'ERROR', code: '42601', detail: undefined, hint: undefined, position: '27', internalPosition: undefined, internalQuery: undefined, where: undefined, schema: undefined, table: undefined, column: undefined, dataType: undefined, constraint: undefined, file: 'scan.l', line: '1241', routine: 'scanner_yyerror' }

System Info

System:
    OS: macOS 14.3.1
    CPU: (12) x64 Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz
    Memory: 14.26 MB / 16.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 21.6.2 - ~/.pkgx/nodejs.org/v21.6.2/bin/node
    Yarn: 3.5.0 - ~/.pkgx/yarnpkg.com/v4.2.2/bin/yarn
    npm: 10.4.0 - ~/.pkgx/npmjs.com/v10.4.0/bin/npm
    pnpm: 9.1.0 - ~/.pkgx/pnpm.io/v9.1.0/bin/pnpm

Used Module System

esm

shriharip avatar May 15 '24 11:05 shriharip

I'm currently in vacation mode and do not actively contribute

Please consider doing a PR and explore around here: https://github.com/salsita/node-pg-migrate/blob/main/src/operations/tables/shared.ts#L273-L274 coming from here https://github.com/salsita/node-pg-migrate/blob/main/src/operations/tables/addConstraint.ts#L33C11-L38

Also tests should be added / enhanced

Shinigami92 avatar May 15 '24 13:05 Shinigami92

@shriharip It looks like ´deferrable´ cannot be used just on its own. It needs to be used together with ´exclude´, ´unique´, ´primaryKey´ or ´foreignKeys´.

https://www.postgresql.org/docs/current/sql-createtable.html#SQL-CREATETABLE-PARMS-DEFERRABLE

I think the best we could do here is to make the TypeScript type for ´ConstraintOptions´ a discriminated union interface

Would you like to make a PR?

Shinigami92 avatar May 29 '24 13:05 Shinigami92

@Shinigami92 , thanks again for your inputs.

I think you are right. My usage of the constraint was wrong. I already had a foreign key constraint and I just wanted to update that. Looks like SQL does not have that option. I should drop the constraint and then create a new one with the deferred options.

However, I saw that the function parseConstraints the function destructures the options and misses the deferred variable https://github.com/salsita/node-pg-migrate/blob/b8cba65bba04de424420a621f2458d03ca33ccf0/src/operations/tables/shared.ts#L318

and when we do parseDeferrable https://github.com/salsita/node-pg-migrate/blob/f98e00aa983f089b6c5baf3dc4cc6e0807856871/src/operations/tables/shared.ts#L401C40-L401C55 this does not get through.

Do you think I am right?

shriharip avatar May 29 '24 19:05 shriharip

However, I saw that the function parseConstraints the function destructures the options and misses the deferred variable

https://github.com/salsita/node-pg-migrate/blob/b8cba65bba04de424420a621f2458d03ca33ccf0/src/operations/tables/shared.ts#L318

and when we do parseDeferrable f98e00a/src/operations/tables/shared.ts#L401C40-L401C55 this does not get through.

Do you think I am right?

I'm not sure if I understand you correctly but I assume you are wrong. Destructuring does not modify the original options and the whole options are passed to the parseDeferrable. Also there are some unit tests covering that, and they look correct.

https://github.com/salsita/node-pg-migrate/blob/f98e00aa983f089b6c5baf3dc4cc6e0807856871/test/operations/constraints/addConstraint.spec.ts#L31-L34

Please close this issue if nothing needs to be done anymore.

Shinigami92 avatar May 29 '24 20:05 Shinigami92