Support setting table and column comments to `NULL`

andrew-farries commented 4 months ago

Build on #344 to allow removing column comments by setting them to null.

Make use of https://github.com/omissis/go-jsonschema/pull/220 and use the nullable package so that it's possible to distingush between a missing comment field and one that is explicitly set to null.

With https://github.com/omissis/go-jsonschema/pull/220 not being part of a release yet, use a custom build of go-jsonschema. It should be possible to switch back to the official release images once https://github.com/omissis/go-jsonschema/pull/220 is part of a release.

Without this change it becomes impossible to remove a comment from a column using the 'set comment' 'alter column' sub-operation (https://github.com/xataio/pgroll/pull/344).

exekias commented 4 months ago

uhm, I think the need for quotes is quite unexpected as a user. Wouldn't be enough to pass a "comment": null JSON value?

andrew-farries commented 4 months ago

Wouldn't be enough to pass a "comment": null JSON value?

Passing "comment": null would mean that we can't easily distinguish between a comment field that is unset vs one that is explicitly set to NULL. In an 'alter column' operation therefore, we wouldn't know whether to set the comment to NULL or leave it unchanged. This is a common problem with Go JSON unmarshalling (see eg here).

pgroll already requires quoted literal strings when setting column DEFAULT values; here the user may set the column DEFAULT to some function call (eg now() for timestamp columns) so pgroll can't assume that all DEFAULTs should be quoted so it is left to the user to do so.

In the case of comments, the only two values allowed are literal strings or NULL. So arguably it's more surprising that non-NULL comments have to be single quoted strings than it is when setting a DEFAULT where arbitrary expressions are also allowed.

We have some options:

Accept the changes in this PR and require comment fields to be single-quoted strings or a bare NULL.
Close this PR and accept that comments can't be removed by alter column statements, only set to the empty string (removal of a comment could still be done with a raw SQL migration)
Invest more time in coming up with a solution to allow distinguishing explicitly set to NULL comments vs missing fields (eg using the nulllable package). This will be complicated by the fact that our operation types are generated from the JSON schema so doesn't allow us much control over the type of the comment field in an OpAlterColumn.

andrew-farries commented 4 months ago

After some more experimentation, allowing "comment": null to remove a comment field will work as long as we can specify the type of the OpAlterColumn.Comment field as Nullable[string] (using the nullable package).

It requires an upstream change to go-jsonschema in order to emit the nullable.Nullable type correctly from the schema.json file.

This would mean users could specify either:

comment: null

or

comment: "some string"

without having to enclose "some string" in single quotes. This makes it inconsistent with how string literals are specified with DEFAULT values, but perhaps less surprising.

exekias commented 4 months ago

I like this one! Wondering if we could do the same for others. roping @eemmiillyy @SferaDev for thoughts on the json schema

andrew-farries commented 4 months ago

I think this is ready now. The upstream change to go-jsonschema has landed: https://github.com/omissis/go-jsonschema/pull/220.

I've updated the PR description:

Build on #344 to allow removing column comments by setting them to null.

Make use of https://github.com/omissis/go-jsonschema/pull/220 and use the nullable package so that it's possible to distingush between a missing comment field and one that is explicitly set to null.

With https://github.com/omissis/go-jsonschema/pull/220 not being part of a release yet, use a custom build of go-jsonschema. It should be possible to switch back to the official release images once that PR is part of a release.

Without this change it becomes impossible to remove a comment from a column using the 'set comment' 'alter column' sub-operation (https://github.com/xataio/pgroll/pull/344).

andrew-farries commented 4 months ago

Wondering if we could do the same for others

What do you mean by this @exekias? I don't think the same approach makes sense for setting default values for the reasons described in https://github.com/xataio/pgroll/pull/345#issuecomment-2071930676.

It makes sense for comment because there are only two allowed values: string literals or null.

andrew-farries commented 4 months ago

We also need to approve the PR on which this one is based: https://github.com/xataio/pgroll/pull/344

xataio / pgroll

Support setting table and column comments to `NULL` #345