Skip to content

Commit

Permalink
fix a bug in which Node returning function didn't handle nulls properly
Browse files Browse the repository at this point in the history 
If a function returning Node returned null, it output a row with every
field as null rather than a null top level object.
  • Loading branch information
@imor
imor committed Sep 14, 2023
1 parent 07afef4 commit 3ff3598
Show file tree
Hide file tree
Showing 3 changed files with 110 additions and 24 deletions.
106 changes: 82 additions & 24 deletions src/transpile.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,8 @@ pub trait QueryEntrypoint {
}
};

// notice!("SQL: {sql}");

let spi_result: Result<Option<pgrx::JsonB>, spi::Error> = Spi::connect(|c| {
let val = c.select(sql, Some(1), Some(param_context.params))?;
// Get a value from the query
Expand Down Expand Up @@ -334,7 +336,7 @@ impl InsertSelection {
format!(
"{}, coalesce(jsonb_agg({}), jsonb_build_array())",
quote_literal(&x.alias),
x.to_sql(block_name, param_context)?
x.to_sql(block_name, param_context, false)?
)
}
Self::Typename { alias, typename } => {
Expand All @@ -359,7 +361,7 @@ impl UpdateSelection {
format!(
"{}, coalesce(jsonb_agg({}), jsonb_build_array())",
quote_literal(&x.alias),
x.to_sql(block_name, param_context)?
x.to_sql(block_name, param_context, false)?
)
}
Self::Typename { alias, typename } => {
Expand All @@ -384,7 +386,7 @@ impl DeleteSelection {
format!(
"{}, coalesce(jsonb_agg({}), jsonb_build_array())",
quote_literal(&x.alias),
x.to_sql(block_name, param_context)?
x.to_sql(block_name, param_context, false)?
)
}
Self::Typename { alias, typename } => {
Expand Down Expand Up @@ -564,13 +566,14 @@ impl FunctionCallBuilder {
format!("select to_jsonb({func_schema}.{func_name}{args_clause}{type_adjustment_clause}) {block_name};")
}
FuncCallReturnTypeBuilder::Node(node_builder) => {
let select_clause = node_builder.to_sql(block_name, param_context)?;
let select_clause = node_builder.to_sql(block_name, param_context, true)?;
let select_clause = if select_clause.is_empty() {
"jsonb_build_object()".to_string()
} else {
select_clause
};
format!("select {select_clause} from {func_schema}.{func_name}{args_clause} {block_name};")
let cte_name = &rand_block_name();
format!("with {cte_name} as (select {func_schema}.{func_name}{args_clause} {block_name}) select {select_clause} from {cte_name} where {cte_name} is not null;")
}
FuncCallReturnTypeBuilder::Connection(connection_builder) => {
let from_clause = format!("{func_schema}.{func_name}{args_clause}");
Expand All @@ -579,6 +582,7 @@ impl FunctionCallBuilder {
param_context,
None,
Some(from_clause),
false,
)?;
format!("{select_clause}")
}
Expand Down Expand Up @@ -858,6 +862,7 @@ impl ConnectionBuilder {
&self,
quoted_block_name: &str,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
let frags: Vec<String> = self
.selections
Expand All @@ -868,6 +873,7 @@ impl ConnectionBuilder {
&self.order_by,
&self.source.table,
param_context,
parenthesize_block_name,
)
})
.collect::<Result<Vec<_>, _>>()?;
Expand Down Expand Up @@ -910,6 +916,7 @@ impl ConnectionBuilder {
param_context: &mut ParamContext,
from_func: Option<FromFunction>,
from_clause: Option<String>,
parenthesize_block_name: bool,
) -> Result<String, String> {
let quoted_block_name = rand_block_name();

Expand Down Expand Up @@ -941,7 +948,8 @@ impl ConnectionBuilder {

let cursor = &self.before.clone().or_else(|| self.after.clone());

let object_clause = self.object_clause(&quoted_block_name, param_context)?;
let object_clause =
self.object_clause(&quoted_block_name, param_context, parenthesize_block_name)?;

let selectable_columns_clause = self.source.table.to_selectable_columns_clause();

Expand Down Expand Up @@ -1069,7 +1077,7 @@ impl ConnectionBuilder {

impl QueryEntrypoint for ConnectionBuilder {
fn to_sql_entrypoint(&self, param_context: &mut ParamContext) -> Result<String, String> {
self.to_sql(None, param_context, None, None)
self.to_sql(None, param_context, None, None, false)
}
}

Expand Down Expand Up @@ -1143,13 +1151,20 @@ impl ConnectionSelection {
order_by: &OrderByBuilder,
table: &Table,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
Ok(match self {
Self::Edge(x) => {
format!(
"{}, {}",
quote_literal(&x.alias),
x.to_sql(block_name, order_by, table, param_context)?
x.to_sql(
block_name,
order_by,
table,
param_context,
parenthesize_block_name
)?
)
}
Self::PageInfo(x) => {
Expand Down Expand Up @@ -1179,11 +1194,20 @@ impl EdgeBuilder {
order_by: &OrderByBuilder,
table: &Table,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
let frags: Vec<String> = self
.selections
.iter()
.map(|x| x.to_sql(block_name, order_by, table, param_context))
.map(|x| {
x.to_sql(
block_name,
order_by,
table,
param_context,
parenthesize_block_name,
)
})
.collect::<Result<Vec<_>, _>>()?;

let x = frags.join(", ");
Expand All @@ -1208,6 +1232,7 @@ impl EdgeSelection {
order_by: &OrderByBuilder,
table: &Table,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
Ok(match self {
Self::Cursor { alias } => {
Expand All @@ -1217,7 +1242,7 @@ impl EdgeSelection {
Self::Node(builder) => format!(
"{}, {}",
quote_literal(&builder.alias),
builder.to_sql(block_name, param_context)?
builder.to_sql(block_name, param_context, parenthesize_block_name)?
),
Self::Typename { alias, typename } => {
format!("{}, {}", quote_literal(alias), quote_literal(typename))
Expand All @@ -1231,15 +1256,16 @@ impl NodeBuilder {
&self,
block_name: &str,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
let frags: Vec<String> = self
.selections
.iter()
.map(|x| x.to_sql(block_name, param_context))
.map(|x| x.to_sql(block_name, param_context, parenthesize_block_name))
.collect::<Result<Vec<_>, _>>()?;

const MAX_ARGS_IN_JSONB_BUILD_OBJECT: usize = 100; //jsonb_build_object has a limit of 100 arguments
const ARGS_PER_FRAG: usize = 2; // each x.to_sql(...) function above return a pair of args
const ARGS_PER_FRAG: usize = 2; // each x.to_sql(...) function above returns a pair of args
const CHUNK_SIZE: usize = MAX_ARGS_IN_JSONB_BUILD_OBJECT / ARGS_PER_FRAG;

let frags: Vec<String> = frags
Expand All @@ -1254,6 +1280,7 @@ impl NodeBuilder {
&self,
parent_block_name: &str,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
let quoted_block_name = rand_block_name();
let quoted_schema = quote_ident(&self.table.schema);
Expand All @@ -1267,7 +1294,7 @@ impl NodeBuilder {
let frags: Vec<String> = self
.selections
.iter()
.map(|x| x.to_sql(&quoted_block_name, param_context))
.map(|x| x.to_sql(&quoted_block_name, param_context, parenthesize_block_name))
.collect::<Result<Vec<_>, _>>()?;

let object_clause = frags.join(", ");
Expand Down Expand Up @@ -1298,7 +1325,7 @@ impl QueryEntrypoint for NodeBuilder {
let quoted_block_name = rand_block_name();
let quoted_schema = quote_ident(&self.table.schema);
let quoted_table = quote_ident(&self.table.name);
let object_clause = self.to_sql(&quoted_block_name, param_context)?;
let object_clause = self.to_sql(&quoted_block_name, param_context, false)?;

if self.node_id.is_none() {
return Err("Expected nodeId argument missing".to_string());
Expand Down Expand Up @@ -1362,26 +1389,33 @@ impl NodeSelection {
&self,
block_name: &str,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
Ok(match self {
// TODO need to provide alias when called from node builder.
Self::Connection(builder) => format!(
"{}, {}",
quote_literal(&builder.alias),
builder.to_sql(Some(block_name), param_context, None, None)?
builder.to_sql(
Some(block_name),
param_context,
None,
None,
parenthesize_block_name
)?
),
Self::Node(builder) => format!(
"{}, {}",
quote_literal(&builder.alias),
builder.to_relation_sql(block_name, param_context)?
builder.to_relation_sql(block_name, param_context, parenthesize_block_name)?
),
Self::Column(builder) => {
let type_adjustment_clause = apply_suffix_casts(builder.column.type_oid);

format!(
"{}, {}{}",
quote_literal(&builder.alias),
builder.to_sql(block_name)?,
builder.to_sql(block_name, parenthesize_block_name)?,
type_adjustment_clause
)
}
Expand All @@ -1390,14 +1424,14 @@ impl NodeSelection {
format!(
"{}, {}{}",
quote_literal(&builder.alias),
builder.to_sql(block_name, param_context)?,
builder.to_sql(block_name, param_context, parenthesize_block_name)?,
type_adjustment_clause
)
}
Self::NodeId(builder) => format!(
"{}, {}",
quote_literal(&builder.alias),
builder.to_sql(block_name)?
builder.to_sql(block_name, parenthesize_block_name)?
),
Self::Typename { alias, typename } => {
format!("{}, {}", quote_literal(alias), quote_literal(typename))
Expand All @@ -1407,8 +1441,16 @@ impl NodeSelection {
}

impl ColumnBuilder {
pub fn to_sql(&self, block_name: &str) -> Result<String, String> {
let col = format!("{}.{}", &block_name, quote_ident(&self.column.name));
pub fn to_sql(
&self,
block_name: &str,
parenthesize_block_name: bool,
) -> Result<String, String> {
let col = if parenthesize_block_name {
format!("({}).{}", &block_name, quote_ident(&self.column.name))
} else {
format!("{}.{}", &block_name, quote_ident(&self.column.name))
};
let maybe_enum = self.column.type_.as_ref().and_then(|t| match t.details {
Some(TypeDetails::Enum(ref enum_)) => Some(enum_),
_ => None,
Expand Down Expand Up @@ -1437,11 +1479,21 @@ impl ColumnBuilder {
}

impl NodeIdBuilder {
pub fn to_sql(&self, block_name: &str) -> Result<String, String> {
pub fn to_sql(
&self,
block_name: &str,
parenthesize_block_name: bool,
) -> Result<String, String> {
let column_selects: Vec<String> = self
.columns
.iter()
.map(|col| format!("{}.{}", block_name, col.name))
.map(|col| {
if parenthesize_block_name {
format!("({}).{}", block_name, col.name)
} else {
format!("{}.{}", block_name, col.name)
}
})
.collect();
let column_clause = column_selects.join(", ");
let schema_name = quote_literal(&self.schema_name);
Expand All @@ -1457,6 +1509,7 @@ impl FunctionBuilder {
&self,
block_name: &str,
param_context: &mut ParamContext,
parenthesize_block_name: bool,
) -> Result<String, String> {
let schema_name = quote_ident(&self.function.schema_name);
let function_name = quote_ident(&self.function.name);
Expand All @@ -1469,7 +1522,11 @@ impl FunctionBuilder {
),
FunctionSelection::Node(node_builder) => {
let func_block_name = rand_block_name();
let object_clause = node_builder.to_sql(&func_block_name, param_context)?;
let object_clause = node_builder.to_sql(
&func_block_name,
param_context,
parenthesize_block_name,
)?;

let from_clause = format!(
"{schema_name}.{function_name}({block_name}::{}.{})",
Expand Down Expand Up @@ -1498,6 +1555,7 @@ impl FunctionBuilder {
input_block_name: block_name.to_string(),
}),
None,
parenthesize_block_name,
)?,
};
Ok(sql_frag)
Expand Down
18 changes: 18 additions & 0 deletions test/expected/function_calls.out
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1470,6 +1470,24 @@ begin;
}
(1 row)

select jsonb_pretty(graphql.resolve($$
query {
returns_account_with_id(id_to_search: 42) { # search a non-existent id
id
email
nodeId
}
}
$$));
jsonb_pretty
-----------------------------------------
{ +
"data": { +
"returns_account_with_id": null+
} +
}
(1 row)

comment on schema public is e'@graphql({"inflect_names": true})';
select jsonb_pretty(graphql.resolve($$
query {
Expand Down
10 changes: 10 additions & 0 deletions test/sql/function_calls.sql
View file
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,16 @@ begin;
}
$$));

select jsonb_pretty(graphql.resolve($$
query {
returns_account_with_id(id_to_search: 42) { # search a non-existent id
id
email
nodeId
}
}
$$));

comment on schema public is e'@graphql({"inflect_names": true})';

select jsonb_pretty(graphql.resolve($$
Expand Down

0 comments on commit 3ff3598

Please sign in to comment.