Skip to content

Commit

Permalink
Merge pull request #483 from supabase/or/plug-view-function-type-hole
Browse files Browse the repository at this point in the history 
Functions returning view type must have pkey (et.al.)
  • Loading branch information
@imor
imor authored Jan 4, 2024
2 parents 2bb35d5 + cab94f2 commit fee0c26
Show file tree
Hide file tree
Showing 5 changed files with 202 additions and 55 deletions.
6 changes: 4 additions & 2 deletions src/graphql.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1289,9 +1289,11 @@ fn function_fields(schema: &Arc<__Schema>, volatilities: &[FunctionVolatility])

// If the return type is a table type, it must be selectable
if !match &return_type {
__Type::Node(table_type) => table_type.table.permissions.is_selectable,
__Type::Node(table_type) => {
schema.graphql_table_select_types_are_valid(&table_type.table)
}
__Type::Connection(table_type) => {
table_type.table.permissions.is_selectable
schema.graphql_table_select_types_are_valid(&table_type.table)
}
_ => true,
} {
Expand Down
33 changes: 3 additions & 30 deletions test/expected/function_calls.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -2601,7 +2601,7 @@ begin;

rollback to savepoint a;
create table account(
id int,
id int primary key,
email varchar(255),
name text null
);
Expand All @@ -2611,18 +2611,13 @@ begin;
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}})';
(2, '[email protected]', null);--mixed: some null, some non-null
select jsonb_pretty(graphql.resolve($$
query {
returnsAllColumnsNonNullAccount {
Expand Down Expand Up @@ -2671,29 +2666,7 @@ begin;
}
(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)

-- When no record is found, the top level field becomes null
select jsonb_pretty(graphql.resolve($$
query {
returnsNullAccount {
Expand Down
121 changes: 121 additions & 0 deletions test/expected/function_return_view_has_pkey.out
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,121 @@
begin;
create view account as
select
1 as foo,
2 as bar;
create function returns_account()
returns account language sql stable
as $$ select foo, bar from account; $$;
-- Account should not be visible because the view has no primary key
select jsonb_pretty(
graphql.resolve($$
{
__type(name: "Account") {
__typename
}
}
$$)
);
jsonb_pretty
------------------------
{ +
"data": { +
"__type": null+
} +
}
(1 row)

-- returnsAccount should also not be visible because account has no primary key
select jsonb_pretty(
graphql.resolve($$
query IntrospectionQuery {
__schema {
queryType {
fields {
name
}
}
}
}
$$)
);
jsonb_pretty
----------------------------------------
{ +
"data": { +
"__schema": { +
"queryType": { +
"fields": [ +
{ +
"name": "node"+
} +
] +
} +
} +
} +
}
(1 row)

comment on view account is e'
@graphql({
"primary_key_columns": ["foo"]
})';
-- Account should be visible because the view is selectable and has a primary key
select jsonb_pretty(
graphql.resolve($$
{
__type(name: "Account") {
__typename
}
}
$$)
);
jsonb_pretty
-------------------------------------
{ +
"data": { +
"__type": { +
"__typename": "Account"+
} +
} +
}
(1 row)

-- returnsAccount should also be visible because account has a primary key and is selectable
select jsonb_pretty(
graphql.resolve($$
query IntrospectionQuery {
__schema {
queryType {
fields {
name
}
}
}
}
$$)
);
jsonb_pretty
-----------------------------------------------------
{ +
"data": { +
"__schema": { +
"queryType": { +
"fields": [ +
{ +
"name": "accountCollection"+
}, +
{ +
"name": "node" +
}, +
{ +
"name": "returnsAccount" +
} +
] +
} +
} +
} +
}
(1 row)

rollback;
26 changes: 3 additions & 23 deletions test/sql/function_calls.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -807,7 +807,7 @@ begin;
rollback to savepoint a;

create table account(
id int,
id int primary key,
email varchar(255),
name text null
);
Expand All @@ -820,21 +820,15 @@ begin;
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
(2, '[email protected]', null);--mixed: some null, some non-null

-- comment on table account is e'@graphql({"totalCount": {"enabled": true}})';

select jsonb_pretty(graphql.resolve($$
query {
Expand All @@ -858,21 +852,7 @@ begin;
}
$$));

-- 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
}
}
$$));

-- When no record is found, the top level field becomes null
select jsonb_pretty(graphql.resolve($$
query {
returnsNullAccount {
Expand Down
71 changes: 71 additions & 0 deletions test/sql/function_return_view_has_pkey.sql
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,71 @@
begin;

create view account as
select
1 as foo,
2 as bar;

create function returns_account()
returns account language sql stable
as $$ select foo, bar from account; $$;

-- Account should not be visible because the view has no primary key
select jsonb_pretty(
graphql.resolve($$
{
__type(name: "Account") {
__typename
}
}
$$)
);

-- returnsAccount should also not be visible because account has no primary key
select jsonb_pretty(
graphql.resolve($$
query IntrospectionQuery {
__schema {
queryType {
fields {
name
}
}
}
}
$$)
);

comment on view account is e'
@graphql({
"primary_key_columns": ["foo"]
})';

-- Account should be visible because the view is selectable and has a primary key
select jsonb_pretty(
graphql.resolve($$
{
__type(name: "Account") {
__typename
}
}
$$)
);

-- returnsAccount should also be visible because account has a primary key and is selectable
select jsonb_pretty(
graphql.resolve($$
query IntrospectionQuery {
__schema {
queryType {
fields {
name
}
}
}
}
$$)
);



rollback;

0 comments on commit fee0c26

Please sign in to comment.