fix: correctly pass table with schema when using comment #1139
Conversation
Shinigami92
changed the title
Shinigami92
changed the title
|
@osdiab Would you like to do a first review with |
| @@ -383,7 +383,7 @@ export function parseConstraints( | |||
| comments.push( | |||
| makeComment( | |||
| `CONSTRAINT ${name} ON`, | |||
| literal(tableName), | |||
| literal(table), | |||
There was a problem hiding this comment.
Looking purely at the code, this looks sound, given that the literal function passed in should be a function that resolves the fully qualified name this looks like it would do the trick. Reviewing the actual issue, since it's just lacking the table schema, this would do the job. Thanks!
There was a problem hiding this comment.
Yes, an alternative (I did not wanted to go in this PR) would be to check for potential quotes and a dot . and if there is some, then normalize it.
e.g. literal "just" takes a string, and so if e.g. there was already a literal-processed value (literal(literal({ schema: "myschema", name: "mytable" })) => literal("myschema"."mytable")) would currently result in double-quoted ""myschema"."mytable"", which is obviously an error.
So instead the literal function itself could e.g. remove all quotes, and if there is a dot, then use schema + name.
| is_remark: 'boolean', | ||
| } | ||
| ); | ||
| pgm.addConstraint( |
There was a problem hiding this comment.
do these tests just make sure that the migrations themselves don't error? or is there some notion of checking that the resulting schema comes out how was intended? I see that the integration tests invoked in the postgres-test.yml action runs these migrations, but a little surprised there's no assertions/expectations in the tests themselves.
There was a problem hiding this comment.
PostgreSQL itself would fail the migration. Look here for an example of a previous commit that failed: https://github.com/salsita/node-pg-migrate/actions/runs/8897624981/job/24432928126?pr=1139#step:8:321
Example failed action
### MIGRATION 014_add_constraint (UP) ###
ALTER TABLE "t1"
ADD CONSTRAINT "chck_nmbr" CHECK (nmbr < 30);
COMMENT ON CONSTRAINT "chck_nmbr" ON "t1" IS $pga$nmbr must be less than 30$pga$;
CREATE SCHEMA "payroll_reports";
CREATE TABLE "payroll_reports"."upload_headers" (
"id" serial PRIMARY KEY,
"paycode_type" text,
"aggregate_paycode_type" text,
"meta_field_type" text,
"is_remark" boolean
);
ALTER TABLE "payroll_reports"."upload_headers"
ADD CONSTRAINT "chk_only_one_header_type" CHECK (( CASE WHEN paycode_type IS NOT NULL THEN 1 ELSE 0 END + CASE WHEN aggregate_paycode_type IS NOT NULL THEN 1 ELSE 0 END + CASE WHEN meta_field_type IS NOT NULL THEN 1 ELSE 0 END + CASE WHEN is_remark THEN 1 ELSE 0 END ) <= 1);
COMMENT ON CONSTRAINT "chk_only_one_header_type" ON "upload_headers" IS $pga$If no type present/truthy, it's ignored; cannot have more than one active at once$pga$;
INSERT INTO "public"."pgmigrations" (name, run_on) VALUES ('014_add_constraint', NOW());
Error executing:
ALTER TABLE "payroll_reports"."upload_headers"
ADD CONSTRAINT "chk_only_one_header_type" CHECK (( CASE WHEN paycode_type IS NOT NULL THEN 1 ELSE 0 END + CASE WHEN aggregate_paycode_type IS NOT NULL THEN 1 ELSE 0 END + CASE WHEN meta_field_type IS NOT NULL THEN 1 ELSE 0 END + CASE WHEN is_remark THEN 1 ELSE 0 END ) <= 1);
COMMENT ON CONSTRAINT "chk_only_one_header_type" ON "upload_headers" IS $pga$If no type present/truthy, it's ignored; cannot have more than one active at once$pga$;
error: relation "upload_headers" does not exist
> Rolling back attempted migration ...The integration tests are running against real PostgreSQL docker containers in all currently PostgreSQL supported versions => https://www.postgresql.org/
Shinigami92
changed the title
|
That makes sense to me, to have `literal` basically behave idempotently,
though the mechanism would need to be constructed with a little care.
It seems that Postgres schema and table names can in fact have periods in
them. So if you had `"schema.with.dots"."table.with.dots"` then if you
stripped the quotes first, you'd end up with
`"schema"."with.dots.table.with.dots"` which isn't right. So if the
`literal` function is made to be aware of this aspect of Postgres, it
probably should handle detecting a schema first.
But that would also mean that if you actually passed in as a user, the
name `'my.table'` and you didn't want `my` to be interpreted as a schema
but rather that this would be the table name, then the correct behavior
would be ambiguous. That said, this seems rare and it doesn't seem like it
would be a problem just to put in the docs that if you want to have periods
in your table name, you need to use the object syntax.
Sound good?
Omar
…On Wed, May 01, 2024 at 5:59 PM, Shinigami ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/operations/tables/shared.ts
<#1139 (comment)>
:
> @@ -383,7 +383,7 @@ export function parseConstraints(
comments.push(
makeComment(
`CONSTRAINT ${name} ON`,
- literal(tableName),
+ literal(table),
Yes, an alternative (I did not wanted to go in this PR) would be to check
for potential quotes and a dot . and if there is some, then normalize it.
e.g. literal "just" takes a string, and so if e.g. there was already a
literal-processed value (literal(literal({ schema: "myschema", name:
"mytable" })) => literal("myschema"."mytable")) would currently result in
double-quoted ""myschema"."mytable"", which is obviously an error.
So instead the literal function itself could e.g. remove all quotes, and
if there is a dot, then use schema + name.
—
Reply to this email directly, view it on GitHub
<#1139 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAONU3Q5CDO2W7WDYAD4FYDZACVGHAVCNFSM6AAAAABHAQCOF2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDAMZTGI3DEMJWHE>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
😲 I did not know that dots |
fixes #939