salsita / node-pg-migrate

Node.js database migration management for PostgreSQL
https://salsita.github.io/node-pg-migrate
MIT License
1.3k stars 179 forks source link

New constraint format #508

Open ab-pm opened 5 years ago

ab-pm commented 5 years ago

This is a follow-up to #467. I've had a use case now where I wanted to create a primary key constraint with a comment, and it's not easily possible. For foreign keys we now have the referencesConstraintComment, but adding more options in this scheme would be quite ugly. Also I noticed that constraint options like deferrable and deferred are placed next to the constraints, not related to the constraint itself.

I would recommend a complete redesign, with one object per constraint definition following the SQL data model. My idea:

type ConstraintOptions = {
    name: string
    comment: string
    deferrable: boolean
    deferred: boolean
}
interface ExpressionConstraint extends ConstraintOptions {
    type: 'CHECK' | 'EXCLUDE'
    expression: string
}
interface IndexConstraint extends ConstraintOptions {
    type: 'UNIQUE' | 'PRIMARY (KEY)'
    columns: string | string[]
    index: string | IndexOptions
}
interface RefConstraint extends ConstraintOptions {
    type: 'FOREIGN (KEY)'
    columns: string | string[]
    references: string | Name
    referencesColumns?: string | string[]
    match: …
    onDelete: …
    onUpdate: …
}
type Constraint = ExpressionConstraint | IndexConstraint | RefConstraint

(maybe more detailed wrt exclude constraints - but for now a simple string should suffice)

I will ignore NOT NULL, NULL, DEFAULT and IDENTITY constraints as these seem more like column options, and although technically they seem to be constraints and could be named, that seems to be ignored anyway, and they have their own ALTER COLUMN syntax.

So each column definition would simply get a constraints array property similar to what table options already have. For ease of use, we'd get the following shorthands:

For backwards compatibility (and ease of use), the column definition current format is interpreted as a shorthand as well:

What do you think about this? Does it sound reasonable? Are there any backward-compatibility concerns?

dolezel commented 5 years ago

Sounds good, but I would rather implement it after typescript rewrite ... https://github.com/salsita/node-pg-migrate/pull/502

ab-pm commented 5 years ago

Cool, good I waited before rushing to the implementation :-) I should find some time to do it this or next week.