Skip to content

Fields of composite types are always nullable #565

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Aug 12, 2024
Merged

Conversation

plcplc
Copy link
Contributor

@plcplc plcplc commented Aug 8, 2024

What

We used to mark fields of composite type as not-nullable in the NDC schema.

This is wrong. Nullability is a property of columns of tables, not of fields of record types.

This is also demonstrated by the following transcript:

postgres=# create table a_table (nullable_text text, not_nullable_text text not null);
CREATE TABLE
postgres=# create table derived_table (a_table a_table, other text);
CREATE TABLE

postgres=# insert into derived_table values (('nullable', 'not nullable'), 'other text');
INSERT 0 1
postgres=# insert into derived_table values ((null, 'not nullable'), 'other text');
INSERT 0 1

postgres=# select (a_table).* from derived_table;
 nullable_text | not_nullable_text
---------------+-------------------
 nullable      | not nullable
               | not nullable
(2 rows)

postgres=# insert into a_table select (a_table).* from derived_table;
INSERT 0 2
postgres=# select * from a_table;
 nullable_text | not_nullable_text
---------------+-------------------
 nullable      | not nullable
               | not nullable
(2 rows)

-- We can easily construct a record with (not_nullable_text=null) when on **the composite type** a_table:
postgres=# insert into derived_table values (('nullable', null), 'other text');
INSERT 0 1
postgres=# select * from derived_table;
          a_table          |   other
---------------------------+------------
 (nullable,"not nullable") | other text
 (,"not nullable")         | other text
 (nullable,)               | other text
(3 rows)

postgres=# select (a_table).* from derived_table;
 nullable_text | not_nullable_text
---------------+-------------------
 nullable      | not nullable
               | not nullable
 nullable      |
(3 rows)

-- ... But we cannot insert this into **the table** a_table.
postgres=# insert into a_table select (a_table).* from derived_table;
ERROR:  null value in column "not_nullable_text" of relation "a_table" violates not-null constraint
DETAIL:  Failing row contains (nullable, null).

This also means that fields of table type on a composite type cannot respect the column nullabilities specified on the table, because those are not part of the table's type! This should be documented.

How

We simply make the schema endpoint always return nullable fields of composite types.

@plcplc plcplc requested a review from soupi August 8, 2024 14:41
@plcplc plcplc enabled auto-merge August 8, 2024 14:43
@plcplc
Copy link
Contributor Author

plcplc commented Aug 8, 2024

Strictly speaking, fixing this will require a configuration version bump, since it changes the schema :-/

@plcplc plcplc disabled auto-merge August 8, 2024 14:58
@soupi
Copy link
Contributor

soupi commented Aug 12, 2024

I'm fine with approving this, I would just like to see ndc-test pass with the changes here: https://github.com/hasura/ndc-postgres/pull/564/files#r1709631720

@plcplc plcplc enabled auto-merge August 12, 2024 14:42
@plcplc plcplc added this pull request to the merge queue Aug 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 12, 2024
@plcplc plcplc added this pull request to the merge queue Aug 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 12, 2024
@plcplc plcplc added this pull request to the merge queue Aug 12, 2024
Merged via the queue into main with commit 1378805 Aug 12, 2024
30 checks passed
@plcplc plcplc deleted the plc/issues/APIPG-760 branch August 12, 2024 15:29
github-merge-queue bot pushed a commit that referenced this pull request Aug 13, 2024
### What

PR #565 fixed field nullability of composite type fields, but neglected
to treat the similar issue that exists for arrays.

In Postgres, any array type (e.g. `int4[]`, `text[]`, etc.) may contain
elements which are null. The type system does not support declaring
arrays with non-null elements.

This PR fixes that issue.

### How

Whenever we have to translate an ndc-postgres array type to an ndc-spec
array type in the schema generation we annotate the element type as
being nullable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants