Fix parse nullable column constraint and clearing of Record API form dirty state. #117

ignatz · ignatz · commit 03fafe74dbbe · 2025-08-02T21:39:04.000+02:00
diff --git a/trailbase-assets/js/admin/src/components/tables/CreateAlterTable.tsx b/trailbase-assets/js/admin/src/components/tables/CreateAlterTable.tsx
@@ -64,17 +64,19 @@ export function CreateAlterTableForm(props: {
     try {
       const o = original();
       if (o !== undefined) {
+        // Alter table
         const response = await alterTable({
           source_schema: o,
           target_schema: value,
           dry_run: dryRun,
         });
-        console.debug("AlterTableResponse:", response);
+        console.debug(`AlterTableResponse [dry: ${dryRun}]:`, response);
 
         if (dryRun) {
           setSql(response.sql);
         }
       } else {
+        // Create table
         const response = await createTable({ schema: value, dry_run: dryRun });
         console.debug(`CreateTableResponse [dry: ${dryRun}]:`, response);
 
@@ -105,6 +107,7 @@ export function CreateAlterTableForm(props: {
   };
 
   const form = createForm(() => ({
+    onSubmit: async ({ value }) => await onSubmit(value, /*dryRun=*/ false),
     defaultValues:
       props.schema ??
       ({
@@ -128,7 +131,6 @@ export function CreateAlterTableForm(props: {
         virtual_table: false,
         temporary: false,
       } as Table),
-    onSubmit: async ({ value }) => await onSubmit(value, false),
   }));
 
   form.useStore((state) => {
@@ -244,7 +246,7 @@ export function CreateAlterTableForm(props: {
                         disabled={!state().canSubmit}
                         variant="outline"
                         onClick={() => {
-                          onSubmit(form.state.values, true).catch(
+                          onSubmit(form.state.values, /*dryRun=*/ true).catch(
                             console.error,
                           );
                         }}
diff --git a/trailbase-assets/js/admin/src/components/tables/RecordApiSettings.tsx b/trailbase-assets/js/admin/src/components/tables/RecordApiSettings.tsx
@@ -476,7 +476,7 @@ function AddApiDialog(props: {
 
 export function RecordApiSettingsForm(props: {
   close: () => void;
-  markDirty: () => void;
+  markDirty: (dirty?: boolean) => void;
   schema: Table | View;
 }) {
   const config = createConfigQuery();
@@ -595,7 +595,7 @@ enum Mode {
 
 function IndividualRecordApiSettingsForm(props: {
   reset: (api?: RecordApiConfig) => void;
-  markDirty: () => void;
+  markDirty: (dirty?: boolean) => void;
   schema: Table | View;
   api: RecordApiConfig;
   mode: Mode;
@@ -618,10 +618,6 @@ function IndividualRecordApiSettingsForm(props: {
           return;
         }
 
-        // FIXME: On submit we either have to close the Sheet or unset dirty.
-        // Otherwise the user will always be prompted for dirty state even
-        // after submitting.
-
         const newConfig = updateRecordApiConfig(c, value);
         try {
           await setConfig(queryClient, newConfig);
@@ -633,6 +629,7 @@ function IndividualRecordApiSettingsForm(props: {
             variant: "success",
           });
           props.reset(value);
+          props.markDirty(false);
         } catch (err) {
           showToast({
             title: "Uncaught Error",
@@ -646,7 +643,7 @@ function IndividualRecordApiSettingsForm(props: {
 
   form.useStore((state) => {
     if (state.isDirty && !state.isSubmitted) {
-      props.markDirty();
+      props.markDirty(true);
     }
   });
 
diff --git a/trailbase-assets/js/bindings/AlterTableRequest.ts b/trailbase-assets/js/bindings/AlterTableRequest.ts
@@ -1,4 +1,12 @@
 // This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
 import type { Table } from "./Table";
 
+/**
+ * Request to alter `TABLE` schema.
+ *
+ * NOTE: The current approach of (source schema) -> (target schema) is not great. It doesn't allow
+ * us to easily rename columns, e.g. is a column deleted, new or renamed?
+ * We should probably switch to (source schema + operations).
+ * Operations would be: add column, remove column, alter column.
+ */
 export type AlterTableRequest = { source_schema: Table, target_schema: Table, dry_run: boolean | null, };
diff --git a/trailbase-core/src/admin/table/alter_table.rs b/trailbase-core/src/admin/table/alter_table.rs
@@ -11,6 +11,12 @@ use crate::app_state::AppState;
 use crate::config::proto::hash_config;
 use crate::transaction::{TransactionLog, TransactionRecorder};
 
+/// Request to alter `TABLE` schema.
+///
+/// NOTE: The current approach of (source schema) -> (target schema) is not great. It doesn't allow
+/// us to easily rename columns, e.g. is a column deleted, new or renamed?
+/// We should probably switch to (source schema + operations).
+/// Operations would be: add column, remove column, alter column.
 #[derive(Clone, Debug, Deserialize, TS)]
 #[ts(export)]
 pub struct AlterTableRequest {
diff --git a/trailbase-core/src/schema_metadata.rs b/trailbase-core/src/schema_metadata.rs
@@ -295,13 +295,72 @@ mod tests {
   use serde_json::json;
   use trailbase_schema::QualifiedName;
   use trailbase_schema::json_schema::{Expand, JsonSchemaMode, build_json_schema_expanded};
+  use trailbase_schema::sqlite::{Column, ColumnDataType, ColumnOption};
 
   use crate::app_state::*;
   use crate::config::proto::{PermissionFlag, RecordApiConfig};
   use crate::records::list_records::list_records_handler;
   use crate::records::read_record::{ReadRecordQuery, read_record_handler};
   use crate::records::test_utils::add_record_api_config;
 
+  #[tokio::test]
+  async fn test_column_nullability() {
+    let state = test_state(None).await.unwrap();
+
+    state
+      .conn()
+      .execute_batch(
+        "
+            CREATE TABLE test (
+                id  INTEGER PRIMARY KEY,
+                a   INT NOT NULL,
+                b   INT NULL,
+                c   INT
+            ) STRICT;
+
+            INSERT INTO test (a, b, c) VALUES (5, NULL, NULL), (6, 1, 2);
+        ",
+      )
+      .await
+      .unwrap();
+
+    state.schema_metadata().invalidate_all().await.unwrap();
+
+    let test_table = state
+      .schema_metadata()
+      .get_table(&QualifiedName {
+        name: "test".to_string(),
+        database_schema: None,
+      })
+      .unwrap();
+
+    assert_eq!(4, test_table.schema.columns.len());
+    assert_eq!(
+      test_table.schema.columns[1],
+      Column {
+        name: "a".to_string(),
+        data_type: ColumnDataType::Int,
+        options: vec![ColumnOption::NotNull,],
+      }
+    );
+    assert_eq!(
+      test_table.schema.columns[2],
+      Column {
+        name: "b".to_string(),
+        data_type: ColumnDataType::Int,
+        options: vec![ColumnOption::Null,],
+      }
+    );
+    assert_eq!(
+      test_table.schema.columns[3],
+      Column {
+        name: "c".to_string(),
+        data_type: ColumnDataType::Int,
+        options: vec![],
+      }
+    );
+  }
+
   #[tokio::test]
   async fn test_expanded_foreign_key() {
     let state = test_state(None).await.unwrap();
@@ -527,33 +586,29 @@ mod tests {
   async fn test_expanded_with_multiple_foreign_keys() {
     let state = test_state(None).await.unwrap();
 
-    let exec = {
-      let conn = state.conn();
-      move |sql: &str| {
-        let conn = conn.clone();
-        let owned = sql.to_string();
-        return async move { conn.execute(owned, ()).await };
-      }
-    };
+    let table_name = "test_table";
+    state
+      .conn()
+      .execute_batch(format!(
+        r#"
+        CREATE TABLE foreign_table0 (id INTEGER PRIMARY KEY) STRICT;
+        INSERT INTO foreign_table0 (id) VALUES (1);
 
-    exec("CREATE TABLE foreign_table0 (id INTEGER PRIMARY KEY) STRICT")
-      .await
-      .unwrap();
-    exec("CREATE TABLE foreign_table1 (id INTEGER PRIMARY KEY) STRICT")
-      .await
-      .unwrap();
+        CREATE TABLE foreign_table1 (id INTEGER PRIMARY KEY) STRICT;
+        INSERT INTO foreign_table1 (id) VALUES (1);
 
-    let table_name = "test_table";
-    exec(&format!(
-      r#"CREATE TABLE {table_name} (
+        CREATE TABLE {table_name} (
           id        INTEGER PRIMARY KEY,
           fk0       INTEGER REFERENCES foreign_table0(id),
           fk0_null  INTEGER REFERENCES foreign_table0(id),
           fk1       INTEGER REFERENCES foreign_table1(id)
-        ) STRICT"#
-    ))
-    .await
-    .unwrap();
+        ) STRICT;
+
+        INSERT INTO {table_name} (id, fk0, fk0_null, fk1) VALUES (1, 1, NULL, 1);
+        "#
+      ))
+      .await
+      .unwrap();
 
     state.schema_metadata().invalidate_all().await.unwrap();
 
@@ -570,19 +625,6 @@ mod tests {
     .await
     .unwrap();
 
-    exec("INSERT INTO foreign_table0 (id) VALUES (1);")
-      .await
-      .unwrap();
-    exec("INSERT INTO foreign_table1 (id) VALUES (1);")
-      .await
-      .unwrap();
-
-    exec(&format!(
-      "INSERT INTO {table_name} (id, fk0, fk0_null, fk1) VALUES (1, 1, NULL, 1);"
-    ))
-    .await
-    .unwrap();
-
     // Expand none
     {
       let Json(value) = read_record_handler(
@@ -686,7 +728,9 @@ mod tests {
 
       assert_eq!(expected, value);
 
-      exec(&format!("INSERT INTO {table_name} (id) VALUES (2);"))
+      state
+        .conn()
+        .execute(format!("INSERT INTO {table_name} (id) VALUES (2)"), ())
         .await
         .unwrap();
 
diff --git a/trailbase-schema/src/sqlite.rs b/trailbase-schema/src/sqlite.rs
@@ -321,7 +321,13 @@ impl From<sqlite3_parser::ast::ColumnConstraint> for ColumnOption {
           on_update: fk.on_update,
         }
       }
-      Constraint::NotNull { .. } => ColumnOption::NotNull,
+      Constraint::NotNull {
+        nullable,
+        conflict_clause: _,
+      } => match nullable {
+        true => ColumnOption::Null,
+        false => ColumnOption::NotNull,
+      },
       Constraint::Default(expr) => {
         // NOTE: This is not using unquote on purpose to avoid turning "DEFAULT ''" into "DEFAULT".
         ColumnOption::Default(expr.to_string())
