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

andrew-farries · 2024-04-23T08:45:27Z

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

Make use of omissis/go-jsonschema#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 omissis/go-jsonschema#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 omissis/go-jsonschema#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 (#344).

exekias · 2024-04-23T09:12:06Z

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 · 2024-04-23T10:13:47Z

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 · 2024-04-23T15:58:19Z

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 · 2024-04-26T11:25:33Z

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

andrew-farries · 2024-04-26T12:05:59Z

schema.json

+          "type": "string",
+          "goJSONSchema": {
+            "imports": ["github.com/oapi-codegen/nullable"],
+            "nillable": true,
+            "type": "nullable.Nullable[string]"
+          }


The problem with using type: ["string", "null"] is that it generates the Go type:

Comment *string `json:"name"

which doesn't allow us to distinguish between the case where the 'alter column' comment field is specified and set to null, vs left unspecified.

See #345 (comment)

Using nullable.Nullable allows us to distinguish the 'set to null' and the 'unspecified' cases.

SferaDev · 2024-04-26T14:10:41Z

schema.json

@@ -159,7 +159,12 @@
        },
        "comment": {
          "description": "New comment on the column",
-          "type": "string"
+          "type": "string",


Suggested change

"type": "string",

"type": ["string", "null"],

andrew-farries · 2024-04-28T16:56:51Z

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

I've updated the PR description:

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

Make use of omissis/go-jsonschema#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 omissis/go-jsonschema#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 (#344).

andrew-farries · 2024-04-28T16:58:35Z

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 #345 (comment).

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

andrew-farries · 2024-04-29T08:27:25Z

We also need to approve the PR on which this one is based: #344

Ensure that leaving the comment unspecified in an alter column statement does not change the comment.

Allow unrecognized properties in the schema definition. This allows the `goJSONSchema` extension keyword. See: https://ajv.js.org/strict-mode.html#prohibit-ignored-keywords

andrew-farries requested a review from exekias April 23, 2024 08:45

andrew-farries force-pushed the allow-setting-comment-to-null branch from a86846d to 068252d Compare April 26, 2024 11:04

andrew-farries changed the base branch from main to add-alter-column-comment-sub-op April 26, 2024 11:04

andrew-farries marked this pull request as draft April 26, 2024 11:12

andrew-farries removed the request for review from exekias April 26, 2024 11:12

andrew-farries commented Apr 26, 2024

View reviewed changes

SferaDev reviewed Apr 26, 2024

View reviewed changes

andrew-farries force-pushed the allow-setting-comment-to-null branch 4 times, most recently from 0dfcd2a to 9f74b91 Compare April 28, 2024 16:41

andrew-farries marked this pull request as ready for review April 28, 2024 16:58

andrew-farries requested a review from exekias April 28, 2024 16:58

exekias approved these changes Apr 29, 2024

View reviewed changes

Base automatically changed from add-alter-column-comment-sub-op to main April 29, 2024 09:40

andrew-farries added 8 commits April 29, 2024 10:40

Update go-jsonschema to 0.16.1

29e96ce

go get github.com/oapi-codegen/nullable

324af43

Use Nullable[string] as type of Comment field

413e10c

Allow setting comments to NULL

936c573

Update tests to use Nullable for comments

3d085a0

Add ColumnMustNotHaveComment test assertion

83469d0

Add test for setting a column comment to NULL

357d774

Add example migration for removing a comment

7932935

andrew-farries added 3 commits April 29, 2024 10:40

Add test for unspecified comments

5efae0b

Ensure that leaving the comment unspecified in an alter column statement does not change the comment.

Update docs

1ea9d9d

Validate schema in non-strict mode

b16c7c6

Allow unrecognized properties in the schema definition. This allows the `goJSONSchema` extension keyword. See: https://ajv.js.org/strict-mode.html#prohibit-ignored-keywords

andrew-farries force-pushed the allow-setting-comment-to-null branch from 9f74b91 to b16c7c6 Compare April 29, 2024 09:40

andrew-farries merged commit 4f0a715 into main Apr 29, 2024
44 checks passed

andrew-farries deleted the allow-setting-comment-to-null branch April 29, 2024 12:23

andrew-farries mentioned this pull request Nov 20, 2024

Support for drop default on a column #450

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

andrew-farries commented Apr 23, 2024 •

edited

Loading

exekias commented Apr 23, 2024

andrew-farries commented Apr 23, 2024 •

edited

Loading

andrew-farries commented Apr 23, 2024

exekias commented Apr 26, 2024

andrew-farries Apr 26, 2024 •

edited

Loading

SferaDev Apr 26, 2024

andrew-farries commented Apr 28, 2024 •

edited

Loading

andrew-farries commented Apr 28, 2024

andrew-farries commented Apr 29, 2024

Support setting table and column comments to NULL #345

Support setting table and column comments to NULL #345

Conversation

andrew-farries commented Apr 23, 2024 • edited Loading

exekias commented Apr 23, 2024

andrew-farries commented Apr 23, 2024 • edited Loading

andrew-farries commented Apr 23, 2024

exekias commented Apr 26, 2024

andrew-farries Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

SferaDev Apr 26, 2024

Choose a reason for hiding this comment

andrew-farries commented Apr 28, 2024 • edited Loading

andrew-farries commented Apr 28, 2024

andrew-farries commented Apr 29, 2024

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

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

andrew-farries commented Apr 23, 2024 •

edited

Loading

andrew-farries commented Apr 23, 2024 •

edited

Loading

andrew-farries Apr 26, 2024 •

edited

Loading

andrew-farries commented Apr 28, 2024 •

edited

Loading