Merge pull request #16 from hasura/fix/only-write-config-if-changed

Don't write config file unless config has changed

CHANGELOG.md

@@ -7,6 +7,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 ## [Unreleased]
 
+- Implement validate cli command
+- Do not write out config file unless it has changed. This avoids issues with the ddn cli which watches for filechange events
 - Cast variables to UUID before comparison to UUID columns. This enables remote relationships to UUID columns
 
 ## [0.2.4]

crates/common/src/config_file.rs

@@ -3,7 +3,7 @@ use std::collections::BTreeMap;
 use schemars::JsonSchema;
 use serde::{Deserialize, Serialize};
 
-#[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema)]
+#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
 /// the main configuration file
 pub struct ServerConfigFile {
     #[serde(rename = "$schema")]
@@ -22,7 +22,7 @@ pub struct ServerConfigFile {
     pub queries: BTreeMap<String, ParameterizedQueryConfigFile>,
 }
 
-#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
 pub struct TableConfigFile {
     /// The table name
     pub name: String,
@@ -41,14 +41,14 @@ pub struct TableConfigFile {
     pub return_type: ReturnType,
 }
 
-#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
 pub struct PrimaryKey {
     pub name: String,
     /// The names of columns in this primary key
     pub columns: Vec<String>,
 }
 
-#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)]
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
 #[serde(tag = "kind", rename_all = "snake_case")]
 pub enum ReturnType {
     /// A custom return type definition
@@ -76,7 +76,7 @@ impl Default for ReturnType {
     }
 }
 
-#[derive(Debug, Default, Clone, Serialize, Deserialize, JsonSchema)]
+#[derive(Debug, Default, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
 pub struct ParameterizedQueryConfigFile {
     /// Whether this query should be exposed as a procedure (mutating) or collection (non-mutating)
     pub exposed_as: ParameterizedQueryExposedAs,

crates/ndc-clickhouse-cli/src/main.rs

@@ -14,8 +14,8 @@ use common::{
     },
     config::ConnectionConfig,
     config_file::{
-        self, ParameterizedQueryConfigFile, PrimaryKey, ReturnType, ServerConfigFile,
-        TableConfigFile, CONFIG_FILE_NAME, CONFIG_SCHEMA_FILE_NAME,
+        ParameterizedQueryConfigFile, PrimaryKey, ReturnType, ServerConfigFile, TableConfigFile,
+        CONFIG_FILE_NAME, CONFIG_SCHEMA_FILE_NAME,
     },
 };
 use database_introspection::{introspect_database, TableInfo};
@@ -120,13 +120,21 @@ async fn main() -> Result<(), Box<dyn Error>> {
 
     match args.command {
         Command::Init {} => {
-            update_tables_config(&context.context_path, &context.connection).await?;
+            let introspection = introspect_database(&context.connection).await?;
+            let config = update_tables_config(&context.context_path, &introspection).await?;
+            validate_table_config(&context.context_path, &config).await?;
         }
         Command::Update {} => {
-            update_tables_config(&context.context_path, &context.connection).await?;
+            let introspection = introspect_database(&context.connection).await?;
+            let config = update_tables_config(&context.context_path, &introspection).await?;
+            validate_table_config(&context.context_path, &config).await?;
         }
         Command::Validate {} => {
-            todo!("implement validate command")
+            let file_path = context.context_path.join(CONFIG_FILE_NAME);
+            let config = read_config_file(&file_path).await?;
+            if let Some(config) = config {
+                validate_table_config(&context.context_path, &config).await?;
+            }
         }
         Command::Watch {} => {
             todo!("implement watch command")
@@ -136,26 +144,30 @@ async fn main() -> Result<(), Box<dyn Error>> {
     Ok(())
 }
 
-pub async fn update_tables_config(
-    configuration_dir: impl AsRef<Path> + Send,
-    connection_config: &ConnectionConfig,
-) -> Result<(), Box<dyn Error>> {
-    let table_infos = introspect_database(connection_config).await?;
+async fn read_config_file(file_path: &PathBuf) -> Result<Option<ServerConfigFile>, Box<dyn Error>> {
+    let config: Option<ServerConfigFile> = match fs::read_to_string(file_path).await {
+        Ok(file) => Some(serde_json::from_str(&file)
+            .map_err(|err| format!("Error parsing {CONFIG_FILE_NAME}: {err}\n\nDelete {CONFIG_FILE_NAME} to create a fresh file"))),
+        Err(err) if err.kind() == std::io::ErrorKind::NotFound => None,
+        Err(_) => Some(Err(format!("Error reading {CONFIG_FILE_NAME}"))),
+    }.transpose()?;
 
+    Ok(config)
+}
+
+async fn update_tables_config(
+    configuration_dir: impl AsRef<Path> + Send,
+    introspection: &Vec<TableInfo>,
+) -> Result<ServerConfigFile, Box<dyn Error>> {
     let file_path = configuration_dir.as_ref().join(CONFIG_FILE_NAME);
     let schema_file_path = configuration_dir.as_ref().join(CONFIG_SCHEMA_FILE_NAME);
 
-    let old_config = match fs::read_to_string(&file_path).await {
-        Ok(file) => serde_json::from_str(&file)
-            .map_err(|err| format!("Error parsing {CONFIG_FILE_NAME}: {err}\n\nDelete {CONFIG_FILE_NAME} to create a fresh file")),
-        Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(config_file::ServerConfigFile::default()),
-        Err(_) => Err(format!("Error reading {CONFIG_FILE_NAME}")),
-    }?;
+    let old_config = read_config_file(&file_path).await?;
 
-    let tables = table_infos
+    let tables = introspection
         .iter()
         .map(|table| {
-            let old_table_config = get_old_table_config(table, &old_config.tables);
+            let old_table_config = get_old_table_config(table, &old_config);
             let table_alias = get_table_alias(table, &old_table_config);
 
             let arguments = ParameterizedQuery::from_str(&table.view_definition)
@@ -194,7 +206,7 @@ pub async fn update_tables_config(
                     table,
                     &old_table_config,
                     &old_config,
-                    &table_infos,
+                    &introspection,
                 ),
             };
 
@@ -205,17 +217,29 @@ pub async fn update_tables_config(
     let config = ServerConfigFile {
         schema: CONFIG_SCHEMA_FILE_NAME.to_owned(),
         tables: tables,
-        queries: old_config.queries.to_owned(),
+        queries: old_config
+            .as_ref()
+            .map(|old_config| old_config.queries.to_owned())
+            .unwrap_or_default(),
     };
     let config_schema = schema_for!(ServerConfigFile);
 
-    fs::write(&file_path, serde_json::to_string_pretty(&config)?).await?;
-    fs::write(
-        &schema_file_path,
-        serde_json::to_string_pretty(&config_schema)?,
-    )
-    .await?;
+    if old_config.is_none() || old_config.is_some_and(|old_config| old_config != config) {
+        fs::write(&file_path, serde_json::to_string_pretty(&config)?).await?;
+        fs::write(
+            &schema_file_path,
+            serde_json::to_string_pretty(&config_schema)?,
+        )
+        .await?;
+    }
+
+    Ok(config)
+}
 
+async fn validate_table_config(
+    configuration_dir: impl AsRef<Path> + Send,
+    config: &ServerConfigFile,
+) -> Result<(), Box<dyn Error>> {
     // validate after writing out the updated metadata. This should help users understand what the problem is
     // check if some column types can't be parsed
     for (table_alias, table_config) in &config.tables {
@@ -379,10 +403,12 @@ pub async fn update_tables_config(
 /// This allows custom aliases to be preserved
 fn get_old_table_config<'a>(
     table: &TableInfo,
-    old_tables: &'a BTreeMap<String, TableConfigFile>,
+    old_config: &'a Option<ServerConfigFile>,
 ) -> Option<(&'a String, &'a TableConfigFile)> {
-    old_tables.iter().find(|(_, old_table)| {
-        old_table.name == table.table_name && old_table.schema == table.table_schema
+    old_config.as_ref().and_then(|old_config| {
+        old_config.tables.iter().find(|(_, old_table)| {
+            old_table.name == table.table_name && old_table.schema == table.table_schema
+        })
     })
 }
 
@@ -408,7 +434,7 @@ fn get_table_alias(table: &TableInfo, old_table: &Option<(&String, &TableConfigF
 fn get_table_return_type(
     table: &TableInfo,
     old_table: &Option<(&String, &TableConfigFile)>,
-    old_config: &ServerConfigFile,
+    old_config: &Option<ServerConfigFile>,
     introspection: &Vec<TableInfo>,
 ) -> ReturnType {
     let new_columns = get_return_type_columns(table);
@@ -419,7 +445,9 @@ fn get_table_return_type(
                 ReturnType::Definition { .. } => None,
                 ReturnType::TableReference { table_name } => {
                     // get the old table config for the referenced table
-                    let referenced_table_config = old_config.tables.get(table_name);
+                    let referenced_table_config = old_config
+                        .as_ref()
+                        .and_then(|old_config| old_config.tables.get(table_name));
                     // get the new table info for the referenced table, if the referenced table's return type is a definition
                     let referenced_table_info =
                         referenced_table_config.and_then(|old_table| match old_table.return_type {
@@ -448,8 +476,8 @@ fn get_table_return_type(
                 }
                 // if the old config references a query, keep the it if it points to a query that returns the same type as we just introspected
                 ReturnType::QueryReference { query_name } => old_config
-                    .queries
-                    .get(query_name)
+                    .as_ref()
+                    .and_then(|old_config| old_config.queries.get(query_name))
                     .and_then(|query| match &query.return_type {
                         ReturnType::TableReference { .. } | ReturnType::QueryReference { .. } => {
                             None