Skip to content
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.

Sign up for GitHub

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

Functions returning view type must have pkey (et.al.) #483

Merged
merged 2 commits into from
Jan 4, 2024
Merged

Functions returning view type must have pkey (et.al.) #483

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7d3b205
plug type system hole where functions returning views can cause inval…
olirice Jan 3, 2024
cab94f2
update tests
olirice Jan 3, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions src/graphql.rs
View file
Open in desktop
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
Open in desktop
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
Open in desktop
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
Open in desktop
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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to add this to make the test valid since we require primary keys

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
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after adding the pkey constraint, this record is no longer valid.

Since require a pkey I don't think it's necessary/possible to test for this case because can't happen. right?

(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
Open in desktop
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;
Loading