Skip to content

Commit

Permalink
Cleanup pgrx sql entity graph more (#1387)
Browse files Browse the repository at this point in the history 
A second pass at cleanup. While the first was simply moving code around,
this flattens out significant parts of the logic using more streamlined
expressions so that the rest is more readable. No functional changes.
  • Loading branch information
@workingjubilee
workingjubilee authored Nov 17, 2023
1 parent 5bf5481 commit 9e9feb4
Show file tree
Hide file tree
Showing 9 changed files with 129 additions and 235 deletions.
20 changes: 7 additions & 13 deletions pgrx-sql-entity-graph/src/extension_sql/entity.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,9 @@ impl SqlDeclaredEntity {

pub fn has_sql_declared_entity(&self, identifier: &SqlDeclared) -> bool {
match (&identifier, &self) {
(SqlDeclared::Type(identifier_name), &SqlDeclaredEntity::Type(data))
| (SqlDeclared::Enum(identifier_name), &SqlDeclaredEntity::Enum(data))
| (SqlDeclared::Function(identifier_name), &SqlDeclaredEntity::Function(data)) => {
(SqlDeclared::Type(ident_name), &SqlDeclaredEntity::Type(data))
| (SqlDeclared::Enum(ident_name), &SqlDeclaredEntity::Enum(data))
| (SqlDeclared::Function(ident_name), &SqlDeclaredEntity::Function(data)) => {
let matches = |identifier_name: &str| {
identifier_name == &data.name
|| identifier_name == &data.option
Expand All @@ -206,21 +206,15 @@ impl SqlDeclaredEntity {
|| identifier_name == &data.option_array
|| identifier_name == &data.varlena
};
if matches(identifier_name) || data.pg_box.contains(identifier_name) {
if matches(ident_name) || data.pg_box.contains(ident_name) {
return true;
}
// there are cases where the identifier is
// `core::option::Option<Foo>` while the data stores
// `Option<Foo>` check again for this
let generics_start = match identifier_name.find('<') {
None => return false,
Some(idx) => idx,
};
let qualification_end = match identifier_name[..generics_start].rfind("::") {
None => return false,
Some(idx) => idx,
};
matches(&identifier_name[qualification_end + 2..])
let Some(generics_start) = ident_name.find('<') else { return false };
let Some(qual_end) = ident_name[..generics_start].rfind("::") else { return false };
matches(&ident_name[qual_end + 2..])
}
_ => false,
}
Expand Down
17 changes: 7 additions & 10 deletions pgrx-sql-entity-graph/src/lib.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ pub enum SqlGraphEntity {
impl SqlGraphEntity {
pub fn sql_anchor_comment(&self) -> String {
let maybe_file_and_line = if let (Some(file), Some(line)) = (self.file(), self.line()) {
format!("-- {file}:{line}\n", file = file, line = line)
format!("-- {file}:{line}\n")
} else {
String::default()
};
Expand All @@ -119,7 +119,6 @@ impl SqlGraphEntity {
{maybe_file_and_line}\
-- {rust_identifier}\
",
maybe_file_and_line = maybe_file_and_line,
rust_identifier = self.rust_identifier(),
)
}
Expand All @@ -132,7 +131,7 @@ impl SqlGraphIdentifier for SqlGraphEntity {
SqlGraphEntity::CustomSql(item) => item.dot_identifier(),
SqlGraphEntity::Function(item) => item.dot_identifier(),
SqlGraphEntity::Type(item) => item.dot_identifier(),
SqlGraphEntity::BuiltinType(item) => format!("preexisting type {}", item),
SqlGraphEntity::BuiltinType(item) => format!("preexisting type {item}"),
SqlGraphEntity::Enum(item) => item.dot_identifier(),
SqlGraphEntity::Ord(item) => item.dot_identifier(),
SqlGraphEntity::Hash(item) => item.dot_identifier(),
Expand Down Expand Up @@ -268,18 +267,16 @@ impl ToSql for SqlGraphEntity {
pub fn ident_is_acceptable_to_postgres(ident: &syn::Ident) -> Result<(), syn::Error> {
// Roughly `pgrx::pg_sys::NAMEDATALEN`
//
// Technically it **should** be that exactly, however this is `pgrx-utils` and a this data is used at macro time.
// Technically it **should** be that, but we need to guess at build time
const POSTGRES_IDENTIFIER_MAX_LEN: usize = 64;

let ident_string = ident.to_string();
if ident_string.len() >= POSTGRES_IDENTIFIER_MAX_LEN {
let len = ident.to_string().len();
if len >= POSTGRES_IDENTIFIER_MAX_LEN {
return Err(syn::Error::new(
ident.span(),
format!(
"Identifier `{}` was {} characters long, PostgreSQL will truncate identifiers with less than \
{POSTGRES_IDENTIFIER_MAX_LEN} characters, opt for an identifier which Postgres won't truncate",
ident,
ident_string.len(),
"Identifier `{ident}` was {len} characters long, PostgreSQL will truncate identifiers with less than \
{POSTGRES_IDENTIFIER_MAX_LEN} characters, opt for an identifier which Postgres won't truncate"
)
));
}
Expand Down
47 changes: 22 additions & 25 deletions pgrx-sql-entity-graph/src/lifetimes.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -117,38 +117,35 @@ pub fn anonymize_lifetimes(value: &mut syn::Type) {
match value {
syn::Type::Path(type_path) => {
for segment in &mut type_path.path.segments {
match &mut segment.arguments {
syn::PathArguments::AngleBracketed(bracketed) => {
for arg in &mut bracketed.args {
match arg {
// rename lifetimes to the anonymous lifetime
syn::GenericArgument::Lifetime(lifetime) => {
lifetime.ident = syn::Ident::new("_", lifetime.ident.span());
}
if let syn::PathArguments::AngleBracketed(bracketed) = &mut segment.arguments {
for arg in &mut bracketed.args {
match arg {
// rename lifetimes to the anonymous lifetime
syn::GenericArgument::Lifetime(lifetime) => {
lifetime.ident = syn::Ident::new("_", lifetime.ident.span());
}

// recurse
syn::GenericArgument::Type(ty) => anonymize_lifetimes(ty),
syn::GenericArgument::Binding(binding) => {
anonymize_lifetimes(&mut binding.ty)
}
syn::GenericArgument::Constraint(constraint) => {
for bound in constraint.bounds.iter_mut() {
match bound {
syn::TypeParamBound::Lifetime(lifetime) => {
lifetime.ident =
syn::Ident::new("_", lifetime.ident.span())
}
_ => {}
// recurse
syn::GenericArgument::Type(ty) => anonymize_lifetimes(ty),
syn::GenericArgument::Binding(binding) => {
anonymize_lifetimes(&mut binding.ty)
}
syn::GenericArgument::Constraint(constraint) => {
for bound in constraint.bounds.iter_mut() {
match bound {
syn::TypeParamBound::Lifetime(lifetime) => {
lifetime.ident =
syn::Ident::new("_", lifetime.ident.span())
}
_ => {}
}
}

// nothing to do otherwise
_ => {}
}

// nothing to do otherwise
_ => {}
}
}
_ => {}
}
}
}
Expand Down
21 changes: 9 additions & 12 deletions pgrx-sql-entity-graph/src/pg_extern/entity/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -273,11 +273,10 @@ impl ToSql for PgExternEntity {
PgExternReturnEntity::Iterated { tys: table_items, optional: _, result: _ } => {
let mut items = String::new();
let metadata_retval = self.metadata.retval.clone().ok_or_else(|| eyre!("Macro expansion time and SQL resolution time had differing opinions about the return value existing"))?;
let metadata_retval_sqls = match metadata_retval.return_sql {
let metadata_retval_sqls: Vec<String> = match metadata_retval.return_sql {
Ok(Returns::Table(variants)) => {
let mut retval_sqls = vec![];
for (idx, variant) in variants.iter().enumerate() {
let sql = match variant {
variants.iter().enumerate().map(|(idx, variant)| {
match variant {
SqlMapping::As(sql) => sql.clone(),
SqlMapping::Composite { array_brackets } => {
let composite = table_items[idx].ty.composite_type.unwrap();
Expand All @@ -287,10 +286,8 @@ impl ToSql for PgExternEntity {
fmt::with_array_brackets(context.source_only_to_sql_type(table_items[idx].ty.ty_source).unwrap(), *array_brackets)
}
SqlMapping::Skip => todo!(),
};
retval_sqls.push(sql)
}
retval_sqls
}
}).collect()
},
Ok(_other) => return Err(eyre!("Got non-table return variant SQL in what macro-expansion thought was a table")),
Err(err) => return Err(err).wrap_err("Error mapping return SQL"),
Expand All @@ -302,11 +299,11 @@ impl ToSql for PgExternEntity {
let graph_index =
context.graph.neighbors_undirected(self_index).find(|neighbor| {
match &context.graph[*neighbor] {
SqlGraphEntity::Type(neightbor_ty) => {
neightbor_ty.id_matches(&ty.ty_id)
SqlGraphEntity::Type(neighbor_ty) => {
neighbor_ty.id_matches(&ty.ty_id)
}
SqlGraphEntity::Enum(neightbor_en) => {
neightbor_en.id_matches(&ty.ty_id)
SqlGraphEntity::Enum(neighbor_en) => {
neighbor_en.id_matches(&ty.ty_id)
}
SqlGraphEntity::BuiltinType(defined) => defined == ty.ty_source,
_ => false,
Expand Down
3 changes: 1 addition & 2 deletions pgrx-sql-entity-graph/src/pg_extern/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -190,12 +190,11 @@ impl PgExtern {
} else if in_commented_sql_block && inner.value().trim() == "```" {
in_commented_sql_block = false;
} else if in_commented_sql_block {
let sql = retval.get_or_insert_with(String::default);
let line = inner.value().trim_start().replace(
"@FUNCTION_NAME@",
&(self.func.sig.ident.to_string() + "_wrapper"),
) + "\n";
sql.push_str(&line);
retval.get_or_insert_with(String::default).push_str(&line);
}
}
}
Expand Down
6 changes: 1 addition & 5 deletions pgrx-sql-entity-graph/src/pg_trigger/entity.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ impl ToSql for PgTriggerEntity {
let self_index = context.triggers[self];
let schema = context.schema_prefix_for(&self_index);

let PgTriggerEntity { file, line, full_path, function_name, .. } = self;
let sql = format!(
"\n\
-- {file}:{line}\n\
Expand All @@ -52,11 +53,6 @@ impl ToSql for PgTriggerEntity {
\tRETURNS TRIGGER\n\
\tLANGUAGE c\n\
\tAS 'MODULE_PATHNAME', '{wrapper_function_name}';",
schema = schema,
file = self.file,
line = self.line,
full_path = self.full_path,
function_name = self.function_name,
wrapper_function_name = self.wrapper_function_name(),
);
Ok(sql)
Expand Down
Loading

0 comments on commit 9e9feb4

Please sign in to comment.