Merge pull request #441 from supabase/fix/439

fix a bug in which a UDF call returned null
supabase · Oct 24, 2023 · cf50260 · cf50260
2 parents 4d475f9 + 80e5e3e
commit cf50260
Show file tree

Hide file tree

Showing 5 changed files with 245 additions and 3 deletions.
diff --git a/docs/functions.md b/docs/functions.md
@@ -88,7 +88,7 @@ Functions marked `immutable` or `stable` are available on the query type. Functi
 ## Supported Return Types
 
 
-Built-in GraphQL scalar types `Int`, `Float`, `String`, `Boolean` and [custom scalar types](api.md#custom-scalars) are supported as function arguments and return types. Function types returning a table or view are supported as well:
+Built-in GraphQL scalar types `Int`, `Float`, `String`, `Boolean` and [custom scalar types](api.md#custom-scalars) are supported as function arguments and return types. Function types returning a table or view are supported as well. Such functions implement the [Node interface](api.md#node):
 
 === "Function"
 
@@ -125,6 +125,7 @@ Built-in GraphQL scalar types `Int`, `Float`, `String`, `Boolean` and [custom sc
       accountById(accountId: 1) {
           id
           email
+          nodeId
       }
     }
     ```
@@ -137,11 +138,57 @@ Built-in GraphQL scalar types `Int`, `Float`, `String`, `Boolean` and [custom sc
         "accountById": {
           "id": 1,
           "email": "[email protected]"
+          "nodeId": "WyJwdWJsaWMiLCAiYWNjb3VudCIsIDFd"
         }
       }
     }
     ```
 
+Since Postgres considers a row/composite type containing only null values to be null, the result can be a little surprising in this case. Instead of an object with all columns null, the top-level field is null:
+
+=== "Function"
+
+    ```sql
+    create table account(
+        id int,
+        email varchar(255),
+        name text null
+    );
+
+    insert into account(id, email, name)
+    values
+        (1, '[email protected]', 'aardvark'),
+        (2, '[email protected]', null),
+        (null, null, null);
+
+    create function returns_account_with_all_null_columns()
+        returns account language sql stable
+    as $$ select id, email, name from account where id is null; $$;
+    ```
+
+=== "Query"
+
+    ```graphql
+    query {
+      returnsAccountWithAllNullColumns {
+        id
+        email
+        name
+        __typename
+      }
+    }
+    ```
+
+=== "Response"
+
+    ```json
+    {
+      "data": {
+        "returnsAccountWithAllNullColumns": null
+      }
+    }
+    ```
+
 Functions returning multiple rows of a table or view are exposed as [collections](api.md#collections).
 
 === "Function"

diff --git a/src/sql_types.rs b/src/sql_types.rs
@@ -491,7 +491,7 @@ impl Table {
     pub fn primary_key_columns(&self) -> Vec<&Arc<Column>> {
         self.primary_key()
             .map(|x| x.column_names)
-            .unwrap_or(vec![])
+            .unwrap_or_default()
             .iter()
             .map(|col_name| {
                 self.columns

diff --git a/src/transpile.rs b/src/transpile.rs
@@ -571,7 +571,7 @@ impl FunctionCallBuilder {
                 } else {
                     select_clause
                 };
-                format!("select coalesce((select {select_clause} from {func_schema}.{func_name}{args_clause} {block_name} where {block_name} is not null), null::jsonb);")
+                format!("select coalesce((select {select_clause} from {func_schema}.{func_name}{args_clause} {block_name} where not ({block_name} is null)), null::jsonb);")
             }
             FuncCallReturnTypeBuilder::Connection(connection_builder) => {
                 let from_clause = format!("{func_schema}.{func_name}{args_clause}");

diff --git a/test/expected/function_calls.out b/test/expected/function_calls.out
@@ -2195,4 +2195,118 @@ begin;
  }
 (1 row)
 
+    rollback to savepoint a;
+    create table account(
+        id int,
+        email varchar(255),
+        name text null
+    );
+    create function returns_all_columns_non_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id = 1; $$;
+    create function returns_one_column_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id = 2; $$;
+    create function returns_all_columns_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id is null; $$;
+    create function returns_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id = 9; $$;
+    insert into account(id, email, name)
+    values
+        (1, '[email protected]', 'aardvark'),--all columns non-null
+        (2, '[email protected]', null),--mixed: some null, some non-null
+        (null, null, null);--all columns null
+    -- comment on table account is e'@graphql({"totalCount": {"enabled": true}})';
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsAllColumnsNonNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+                 jsonb_pretty                 
+----------------------------------------------
+ {                                           +
+     "data": {                               +
+         "returnsAllColumnsNonNullAccount": {+
+             "id": 1,                        +
+             "name": "aardvark",             +
+             "email": "[email protected]",      +
+             "__typename": "Account"         +
+         }                                   +
+     }                                       +
+ }
+(1 row)
+
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsOneColumnNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+               jsonb_pretty               
+------------------------------------------
+ {                                       +
+     "data": {                           +
+         "returnsOneColumnNullAccount": {+
+             "id": 2,                    +
+             "name": null,               +
+             "email": "[email protected]",       +
+             "__typename": "Account"     +
+         }                               +
+     }                                   +
+ }
+(1 row)
+
+    -- With current implementation we can't distinguish between
+    -- when all columns of a composite type are null and when
+    -- the composite type itself is null. In both these cases
+    -- the result will be null for the top-level field.
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsAllColumnsNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+                 jsonb_pretty                 
+----------------------------------------------
+ {                                           +
+     "data": {                               +
+         "returnsAllColumnsNullAccount": null+
+     }                                       +
+ }
+(1 row)
+
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+            jsonb_pretty            
+------------------------------------
+ {                                 +
+     "data": {                     +
+         "returnsNullAccount": null+
+     }                             +
+ }
+(1 row)
+
 rollback;
diff --git a/test/sql/function_calls.sql b/test/sql/function_calls.sql
@@ -782,4 +782,85 @@ begin;
             returnsEventTrigger
         }
     $$));
+
+    rollback to savepoint a;
+
+    create table account(
+        id int,
+        email varchar(255),
+        name text null
+    );
+
+    create function returns_all_columns_non_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id = 1; $$;
+
+    create function returns_one_column_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id = 2; $$;
+
+    create function returns_all_columns_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id is null; $$;
+
+    create function returns_null_account()
+        returns account language sql stable
+    as $$ select id, email, name from account where id = 9; $$;
+
+    insert into account(id, email, name)
+    values
+        (1, '[email protected]', 'aardvark'),--all columns non-null
+        (2, '[email protected]', null),--mixed: some null, some non-null
+        (null, null, null);--all columns null
+
+    -- comment on table account is e'@graphql({"totalCount": {"enabled": true}})';
+
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsAllColumnsNonNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsOneColumnNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+
+    -- With current implementation we can't distinguish between
+    -- when all columns of a composite type are null and when
+    -- the composite type itself is null. In both these cases
+    -- the result will be null for the top-level field.
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsAllColumnsNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+
+    select jsonb_pretty(graphql.resolve($$
+        query {
+            returnsNullAccount {
+                id
+                email
+                name
+                __typename
+            }
+        }
+    $$));
+
 rollback;