From 174057f07cb6d5d85e1fc0021fb75fd64d90e5df Mon Sep 17 00:00:00 2001 From: Michael Cuffaro Date: Wed, 18 Sep 2024 10:17:09 -0400 Subject: [PATCH] small fixes and tweaks --- README.md | 6 +-- scripts/export_messages.py | 2 +- src/guess.rs | 9 +---- src/toolkit.rs | 76 ++++++++++++++++---------------------- src/validate.rs | 20 +++++----- src/valve.rs | 8 ++-- 6 files changed, 51 insertions(+), 70 deletions(-) diff --git a/README.md b/README.md index cb19b9b..ff754f8 100644 --- a/README.md +++ b/README.md @@ -4,7 +4,7 @@ Valve - A lightweight validation engine written in rust. ## Table of contents -_Generated using [markdown-toc](https://github.com/jonschlinkert/markdown-toc)_ +_ToC Generated using [markdown-toc](https://github.com/jonschlinkert/markdown-toc)_ * [Design and concepts](#design-and-concepts) + [The Valve database](#the-valve-database) @@ -723,11 +723,11 @@ The columns of the rule table have the following significance: In some cases it is useful to be able to try and guess what the table table and column table configuration should be, using information about the current state of the Valve instance, for a given data table not currently managed by Valve. To do this one may use Valve's command line interface to run the **guess** subcommand as follows: - ontodev_valve guess [OPTIONS] SOURCE DESTINATION TABLE_TSV + ontodev_valve guess [OPTIONS] SOURCE DATABASE TABLE_TSV where: - `SOURCE` is the location of the '.tsv' representing the table table. -- `DESTINATION` is the path to a PostgreSQL or SQLite database. +- `DATABASE` is the path to a PostgreSQL or SQLite database. - `TABLE_TSV` is the '.tsv' file representing the data table whose column configuration is to be guessed. For the list of possible options, and for general information on Valve's command line interface, see [command line usage](#command-line-usage). diff --git a/scripts/export_messages.py b/scripts/export_messages.py index c135b4f..dc8bd64 100755 --- a/scripts/export_messages.py +++ b/scripts/export_messages.py @@ -88,7 +88,7 @@ def get_column_order_and_info_for_sqlite(cursor, table): for row in pragma_rows: if row["pk"] != 0: primary_keys[row["pk"]] = row["name"] - elif not row["name"] not in ("row_number", "row_order"): + else: non_pk_columns.append(row["name"]) primary_keys = dict(sorted(primary_keys.items())) sorted_columns = [primary_keys[key] for key in primary_keys] + non_pk_columns diff --git a/src/guess.rs b/src/guess.rs index 2834dab..c038262 100644 --- a/src/guess.rs +++ b/src/guess.rs @@ -1,7 +1,7 @@ //! Implementation of the column configuration guesser use crate::{ - toolkit::{get_query_params, local_sql_syntax, QueryParam}, + toolkit::{get_query_param, local_sql_syntax, QueryParam}, valve::{Valve, ValveConfig, ValveDatatypeConfig}, SQL_PARAM, }; @@ -668,12 +668,7 @@ pub fn annotate( foreign.table, foreign.column, SQL_PARAM ), ); - let param = get_query_params(&vec![json!(value)], &foreign.sql_type) - .pop() - .expect(&format!( - "Could not determine query parameter for '{}'", - value - )); + let param = get_query_param(&json!(value), &foreign.sql_type); let mut query = sqlx_query(&sql); match param { QueryParam::Integer(p) => query = query.bind(p), diff --git a/src/toolkit.rs b/src/toolkit.rs index b180de6..8c1efcd 100644 --- a/src/toolkit.rs +++ b/src/toolkit.rs @@ -211,7 +211,9 @@ pub fn get_column_for_label( .into()) } else { // If we cannot find the column corresponding to the label, then we conclude that the label - // name we received was fact the column name, for a column that has no label: + // name we received was the column name of a column with an empty label. This is because an + // an empty label signifies, not that the column has no label, but that the column's label + // is the same as it's column name. Ok(label.to_string()) } } @@ -1185,9 +1187,7 @@ pub fn read_config_files( for label_name in &defined_labels { let column_name = get_column_for_label(&this_column_config, label_name, &table_name)?; - if !column_name.is_empty() { - column_order.push(column_name); - } + column_order.push(column_name); } } else { return Err(ValveError::ConfigError(format!("'{}' is empty", path)).into()); @@ -1661,7 +1661,7 @@ pub fn get_sql_for_text_view( /// reformatted (for instance using the function [local_sql_syntax()]) in accordance with the syntax /// accepted by the underlying database, and then bound before being executed by sqlx. /// These placeholders represent: (1) the column name and (2) the table name. -pub fn query_column_with_message_value(column: &str, pool: &AnyPool) -> String { +pub fn generic_select_with_message_value(column: &str, pool: &AnyPool) -> String { let is_clause = if pool.any_kind() == AnyKind::Sqlite { "IS" } else { @@ -1715,7 +1715,7 @@ pub fn generic_select_with_message_values( .iter() .map(|column| { sql_params.append(&mut vec![column.to_string(), table.to_string()]); - query_column_with_message_value(column, pool) + generic_select_with_message_value(column, pool) }) .collect::>(); @@ -1781,11 +1781,11 @@ pub async fn get_affected_rows( ), ); sql_params.push(value.to_string()); + let mut query = sqlx_query(&sql); for param in &sql_params { query = query.bind(param); } - let mut valve_rows = vec![]; for row in query.fetch_all(tx.acquire().await?).await? { let mut contents = IndexMap::new(); @@ -1934,9 +1934,6 @@ pub async fn get_db_value( END AS "{column}" FROM "{table}_view" WHERE "row_number" = {row_number} "#, - column = column, - is_clause = is_clause, - row_number = row_number, casted_column = if pool.any_kind() == AnyKind::Sqlite { cast_column_sql_to_text(column, "non-text") } else { @@ -2353,17 +2350,10 @@ pub async fn record_row_change( .into()); } - fn to_text(row: Option<&SerdeMap>, quoted: bool) -> String { + fn to_text(row: Option<&SerdeMap>) -> String { match row { None => "NULL".to_string(), - Some(r) => { - let inner = format!("{}", json!(r)); - if !quoted { - inner - } else { - format!("'{}'", inner) - } - } + Some(r) => format!("{}", json!(r)), } } @@ -2460,7 +2450,7 @@ pub async fn record_row_change( format_value(&new_value.to_string(), &numeric_re), )), ); - let column_summary = to_text(Some(&column_summary), false); + let column_summary = to_text(Some(&column_summary)); summary.push(column_summary); } } @@ -2470,11 +2460,11 @@ pub async fn record_row_change( } let summary = summarize(from, to)?; - let (from, to) = (to_text(from, false), to_text(to, false)); + let (from, to) = (to_text(from), to_text(to)); let mut params = vec![table]; let from_param = { if from == "NULL" { - from.to_string() + from } else { params.push(&from); SQL_PARAM.to_string() @@ -2482,7 +2472,7 @@ pub async fn record_row_change( }; let to_param = { if to == "NULL" { - to.to_string() + to } else { params.push(&to); SQL_PARAM.to_string() @@ -2490,7 +2480,7 @@ pub async fn record_row_change( }; let summary_param = { if summary == "NULL" { - summary.to_string() + summary } else { params.push(&summary); SQL_PARAM.to_string() @@ -3224,7 +3214,7 @@ pub async fn insert_new_row_tx( ), ); let mut query = sqlx_query(&message_sql); - for param in vec![table, column, &value, level, rule, &message] { + for param in [table, column, &value, level, rule, &message] { query = query.bind(param); } query.execute(tx.acquire().await?).await?; @@ -3751,36 +3741,34 @@ pub fn compile_condition( } } -/// Given a list of [SerdeValue]s and the SQL type that they should all conform to, return -/// a list of [QueryParam]s corresponding to each value, which can then be bound to an SQL string. -pub fn get_query_params(values: &Vec, sql_type: &str) -> Vec { - let mut param_values = vec![]; - - for value in values { - let param_value = value - .as_str() - .expect(&format!("'{}' is not a string", value)); - if sql_type == "numeric" { +/// Given a [SerdeValue] of the type [SerdeValue::String], as well as the SQL type that it will +/// be converted to when it is inserted to the database (which can be a non-string), return a +/// [QueryParam] corresponding to the value. +pub fn get_query_param(value: &SerdeValue, sql_type: &str) -> QueryParam { + let param_value = value + .as_str() + .expect(&format!("'{}' is not a string", value)); + match sql_type { + "numeric" => { let numeric_value: f64 = param_value .parse() .expect(&format!("{param_value} is not numeric")); - param_values.push(QueryParam::Numeric(numeric_value)); - } else if sql_type == "integer" { + QueryParam::Numeric(numeric_value) + } + "integer" => { let integer_value: i32 = param_value .parse() .expect(&format!("{param_value} is not an integer")); - param_values.push(QueryParam::Integer(integer_value)); - } else if sql_type == "real" { + QueryParam::Integer(integer_value) + } + "real" => { let real_value: f64 = param_value .parse() .expect(&format!("{param_value} is not a real")); - param_values.push(QueryParam::Real(real_value)); - } else { - param_values.push(QueryParam::String(param_value.to_string())); + QueryParam::Real(real_value) } + _ => QueryParam::String(param_value.to_string()), } - - param_values } /// Given the config map, the name of a datatype, and a database connection pool used to determine diff --git a/src/validate.rs b/src/validate.rs index a61a8b0..2f00996 100644 --- a/src/validate.rs +++ b/src/validate.rs @@ -4,7 +4,7 @@ use crate::{ ast::Expression, toolkit::{ cast_sql_param_from_text, get_column_value, get_column_value_as_string, - get_datatype_ancestors, get_query_params, get_sql_type_from_global_config, + get_datatype_ancestors, get_query_param, get_sql_type_from_global_config, get_table_options_from_config, get_value_type, is_sql_type_error, local_sql_syntax, ColumnRule, CompiledCondition, QueryAsIf, QueryAsIfKind, QueryParam, ValueType, }, @@ -328,7 +328,10 @@ pub async fn validate_rows_constraints( .map(|_| SQL_PARAM.to_string()) .collect::>() .join(", "); - let param_values = get_query_params(&values, &sql_type); + let param_values = values + .iter() + .map(|value| get_query_param(value, &sql_type)) + .collect::>(); // Foreign keys always correspond to columns with unique constraints so we do not // need to use the keyword 'DISTINCT' when querying the normal version of the table: @@ -457,7 +460,10 @@ pub async fn validate_rows_constraints( .map(|_| SQL_PARAM.to_string()) .collect::>() .join(", "); - let param_values = get_query_params(&values, &sql_type); + let param_values = values + .iter() + .map(|value| get_query_param(value, &sql_type)) + .collect::>(); let sql = local_sql_syntax( pool, @@ -1284,15 +1290,9 @@ pub async fn validate_cell_foreign_constraints( as_if_clause, table, column, SQL_PARAM, ), ); - let param = get_query_params(&vec![value.into()], &sql_type) - .pop() - .expect(&format!( - "Could not determine query parameter for '{}'", - value - )); let frows = { let mut query = sqlx_query(&fsql); - match param { + match get_query_param(&json!(value), &sql_type) { QueryParam::Integer(p) => query = query.bind(p), QueryParam::Numeric(p) => query = query.bind(p), QueryParam::Real(p) => query = query.bind(p), diff --git a/src/valve.rs b/src/valve.rs index b30eb7d..3d5d1c3 100644 --- a/src/valve.rs +++ b/src/valve.rs @@ -1955,11 +1955,9 @@ impl Valve { if self.verbose { println!("Saving tables: {} ...", tables.join(", ")); } - // Collect the paths and possibly the options of all of the tables that were requested to be - // saved: + // Build the query to get the path and possibly the options info from the table table for + // all of the tables that were requested to be saved: let options_enabled = self.column_enabled_in_db("table", "options").await?; - - // Build the query to get the path and options info from the table table: let mut params = vec![]; let sql_param_str = tables .iter() @@ -2008,7 +2006,7 @@ impl Valve { let path = row.try_get::<&str, &str>("path").ok().unwrap_or_default(); let path = match save_dir { Some(save_dir) => { - // If the table is not saveable it can still be saved to the saved_dir if it + // If the table is not saveable it can still be saved to the saved_dir if one // has been specified and if it is not identical to the table's configured path: if !options.contains("save") { let path_dir = Path::new(path)