Should `allowNull: false` be equivalent to `notNull: true`?

[andresakata]

Currently, allowNull: false is not equivalent to notNull: true. It doesn't add a NOT NULL constraint. I think it can lead to mistakes. Is it intended to function in that manner?

[Shinigami92]

allowNull exists in alterColumn and in alterDomain. Do you mean one specific? Or is the request equivalent for both?

[andresakata]

I was thinking of alterColumn, but it looks like it applies to both.

[brenoepics]

I agree, it can be a bit confusing, but IMO adding a breaking change like that in such a widely used function can be tricky.

We could either

• Add runtime guardrails in alterColumn:
  If allowNull === false and notNull !== true, warn: allowNull: false is a no‑op; use notNull: true to add NOT NULL.
  If both notNull and allowNull are provided, warn about the conflict.

• Tighten TypeScript types:
  Maybe using a union type allowing only { notNull: true } | { notNull: false } | { allowNull: true } | {}...
  Mark allowNull: false as deprecated.

[Shinigami92]

https://www.postgresql.org/docs/current/sql-altertable.html#SQL-ALTERTABLE-DESC-SET-DROP-NOT-NULL

ALTER [ COLUMN ] column_name { SET | DROP } NOT NULL

https://www.postgresql.org/docs/current/sql-alterdomain.html

ALTER DOMAIN name { SET | DROP } NOT NULL

My personal preference would be to align with the official documentation as near as possible.
They use SET/DROP for NOT NULL.
So if we change that and even aiming for a breaking change, we could introduce a notNull: 'SET'/notNull: 'DROP'.

But yes, for the upcoming v10 we should first introduce the mentioned warning and also add the additional new values for notNull and deprecate allowNull.

@brenoepics Please sanity-check me so I don't have missed something fundamentally 😅
Also we should try this out in a PR and see how the integration tests react.

[Shinigami92]

Self-Feedback: As I now use node-pg-migrate in one of my projects, I found my self using allowNull: true quite often when I remove a previous notNull: true constraint.

so e.g. I had

pgm.createTable('tname', {
  my_col_1: { type: PgType.TEXT, notNull: true }
})

and then later on

pgm.alterColumn("tname", "my_col_1", { allowNull: true })

Somehow I like this in terms of reading the code 🤔

If we would remove allowNull, this would become following:

pgm.createTable('tname', {
  my_col_1: { type: PgType.TEXT, notNull: 'SET' }
})
// ...
pgm.alterColumn("tname", "my_col_1", { notNull: 'DROP' })

I don't know 🤷 doesn't feel to me like a valuable change to really make that break.
Maybe we could even support both, I mean: notNull: boolean | 'SET' | 'DROP'
And maybe we should find out if anyone out there is using allowNull: false, and if so: why