Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pretty print auto-migration plans #2065

Draft
wants to merge 5 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions crates/cli/src/subcommands/publish.rs
Original file line number Diff line number Diff line change
Expand Up @@ -170,6 +170,7 @@ pub async fn exec(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error
domain,
database_identity,
op,
update_summary,
} => {
let op = match op {
PublishOp::Created => "Created new",
Expand All @@ -180,6 +181,9 @@ pub async fn exec(config: Config, args: &ArgMatches) -> Result<(), anyhow::Error
} else {
println!("{} database with identity: {}", op, database_identity);
}
if let Some(update_summary) = update_summary {
println!("{}", update_summary);
}
}
PublishResult::TldNotRegistered { domain } => {
return Err(anyhow::anyhow!(
Expand Down
11 changes: 11 additions & 0 deletions crates/client-api-messages/src/name.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,17 @@ pub enum PublishResult {
/// or not.
database_identity: Identity,
op: PublishOp,

/// If the database was updated, may contain a string describing the update.
/// Contains ANSI escape codes for color. You can use
/// `spacetimedb_schema::auto_migrate::pretty_print::strip_ansi_escape_codes`
/// to remove these if needed.
///
/// TODO: it would be much better to put a more structured data type here.
/// This could then be formatted on client, possibly in a machine-readable form;
/// it could also allow, say, a GUI that shows migration histories on the website.
/// However, this requires reworking the `MigrationPlan` type to be serializable.
update_summary: Option<String>,
},

// TODO: below variants are obsolete with control db module
Expand Down
10 changes: 7 additions & 3 deletions crates/client-api/src/routes/database.rs
Original file line number Diff line number Diff line change
Expand Up @@ -658,7 +658,7 @@ pub async fn publish<S: NodeDelegate + ControlStateDelegate>(
.await
.map_err(log_and_500)?;

if let Some(updated) = maybe_updated {
let update_summary = if let Some(updated) = maybe_updated {
match updated {
UpdateDatabaseResult::AutoMigrateError(errs) => {
return Err((StatusCode::BAD_REQUEST, format!("Database update rejected: {errs}")).into());
Expand All @@ -670,14 +670,18 @@ pub async fn publish<S: NodeDelegate + ControlStateDelegate>(
)
.into());
}
UpdateDatabaseResult::NoUpdateNeeded | UpdateDatabaseResult::UpdatePerformed => {}
UpdateDatabaseResult::NoUpdateNeeded => None,
UpdateDatabaseResult::UpdatePerformed(summary) => Some(summary),
}
}
} else {
None
};

Ok(axum::Json(PublishResult::Success {
domain: db_name.as_ref().map(ToString::to_string),
database_identity,
op,
update_summary,
}))
}

Expand Down
14 changes: 1 addition & 13 deletions crates/core/src/db/update.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use spacetimedb_lib::db::auth::StTableType;
use spacetimedb_lib::identity::AuthCtx;
use spacetimedb_lib::AlgebraicValue;
use spacetimedb_primitives::ColSet;
use spacetimedb_schema::auto_migrate::{AutoMigratePlan, ManualMigratePlan, MigratePlan};
use spacetimedb_schema::auto_migrate::{AutoMigratePlan, MigratePlan};
use spacetimedb_schema::def::TableDef;
use spacetimedb_schema::schema::{IndexSchema, Schema, SequenceSchema, TableSchema};
use std::sync::Arc;
Expand Down Expand Up @@ -45,22 +45,10 @@ pub fn update_database(
}

match plan {
MigratePlan::Manual(plan) => manual_migrate_database(stdb, tx, plan, system_logger, existing_tables),
MigratePlan::Auto(plan) => auto_migrate_database(stdb, tx, auth_ctx, plan, system_logger, existing_tables),
}
}

/// Manually migrate a database.
fn manual_migrate_database(
_stdb: &RelationalDB,
_tx: &mut MutTxId,
_plan: ManualMigratePlan,
_system_logger: &SystemLogger,
_existing_tables: Vec<Arc<TableSchema>>,
) -> anyhow::Result<()> {
unimplemented!("Manual database migrations are not yet implemented")
}

/// Automatically migrate a database.
fn auto_migrate_database(
stdb: &RelationalDB,
Expand Down
2 changes: 1 addition & 1 deletion crates/core/src/host/host_controller.rs
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ impl HostController {
)
.await?;
match update_result {
UpdateDatabaseResult::NoUpdateNeeded | UpdateDatabaseResult::UpdatePerformed => {
UpdateDatabaseResult::NoUpdateNeeded | UpdateDatabaseResult::UpdatePerformed(_) => {
*guard = Some(host);
}
UpdateDatabaseResult::AutoMigrateError(e) => {
Expand Down
6 changes: 4 additions & 2 deletions crates/core/src/host/module_host.rs
Original file line number Diff line number Diff line change
Expand Up @@ -448,7 +448,9 @@ pub struct WeakModuleHost {
#[derive(Debug)]
pub enum UpdateDatabaseResult {
NoUpdateNeeded,
UpdatePerformed,
/// The string is a printable summary of the update that happened.
/// Contains ANSI escape sequences for color.
UpdatePerformed(String),
AutoMigrateError(ErrorStream<AutoMigrateError>),
ErrorExecutingMigration(anyhow::Error),
}
Expand All @@ -457,7 +459,7 @@ impl UpdateDatabaseResult {
pub fn was_successful(&self) -> bool {
matches!(
self,
UpdateDatabaseResult::UpdatePerformed | UpdateDatabaseResult::NoUpdateNeeded
UpdateDatabaseResult::UpdatePerformed(_) | UpdateDatabaseResult::NoUpdateNeeded
)
}
}
Expand Down
7 changes: 6 additions & 1 deletion crates/core/src/host/wasm_common/module_host_actor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use bytes::Bytes;
use spacetimedb_client_api_messages::timestamp::Timestamp;
use spacetimedb_primitives::TableId;
use spacetimedb_schema::auto_migrate::ponder_migrate;
use spacetimedb_schema::auto_migrate::pretty_print::pretty_print;
use spacetimedb_schema::def::ModuleDef;
use spacetimedb_schema::schema::{Schema, TableSchema};
use std::sync::Arc;
Expand Down Expand Up @@ -340,6 +341,10 @@ impl<T: WasmInstance> ModuleInstance for WasmModuleInstance<T> {
return Ok(UpdateDatabaseResult::AutoMigrateError(errs));
}
};
let summary = pretty_print(&plan).unwrap_or_else(|_| {
log::warn!("Failed to pretty-print migration plan: {plan:#?}");
"(plan not rendered, but succeeded)".to_string()
});
let stdb = &*self.replica_context().relational_db;

let program_hash = program.hash;
Expand All @@ -361,7 +366,7 @@ impl<T: WasmInstance> ModuleInstance for WasmModuleInstance<T> {
stdb.commit_tx(tx)?;
self.system_logger().info("Database updated");
log::info!("Database updated, {}", stdb.database_identity());
Ok(UpdateDatabaseResult::UpdatePerformed)
Ok(UpdateDatabaseResult::UpdatePerformed(summary))
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions crates/schema/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ spacetimedb-sats.workspace = true
spacetimedb-data-structures.workspace = true
spacetimedb-sql-parser.workspace = true

regex.workspace = true
anyhow.workspace = true
colored.workspace = true
indexmap.workspace = true
itertools.workspace = true
lazy_static.workspace = true
Expand Down
72 changes: 44 additions & 28 deletions crates/schema/src/auto_migrate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,41 +6,32 @@ use spacetimedb_data_structures::{
use spacetimedb_lib::db::raw_def::v9::{RawRowLevelSecurityDefV9, TableType};
use spacetimedb_sats::WithTypespace;

pub mod pretty_print;

pub type Result<T> = std::result::Result<T, ErrorStream<AutoMigrateError>>;

/// A plan for a migration.
#[derive(Debug)]
pub enum MigratePlan<'def> {
Manual(ManualMigratePlan<'def>),
Auto(AutoMigratePlan<'def>),
}

impl<'def> MigratePlan<'def> {
/// Get the old `ModuleDef` for this migration plan.
pub fn old_def(&self) -> &'def ModuleDef {
match self {
MigratePlan::Manual(plan) => plan.old,
MigratePlan::Auto(plan) => plan.old,
}
}

/// Get the new `ModuleDef` for this migration plan.
pub fn new_def(&self) -> &'def ModuleDef {
match self {
MigratePlan::Manual(plan) => plan.new,
MigratePlan::Auto(plan) => plan.new,
}
}
}

/// A plan for a manual migration.
/// `new` must have a reducer marked with `Lifecycle::Update`.
#[derive(Debug)]
pub struct ManualMigratePlan<'def> {
pub old: &'def ModuleDef,
pub new: &'def ModuleDef,
}

/// A plan for an automatic migration.
#[derive(Debug)]
pub struct AutoMigratePlan<'def> {
Expand All @@ -52,45 +43,62 @@ pub struct AutoMigratePlan<'def> {
/// There is also an implied check: that the schema in the database is compatible with the old ModuleDef.
pub prechecks: Vec<AutoMigratePrecheck<'def>>,
/// The migration steps to perform.
/// Order should not matter, as the steps are independent.
/// Order matters: `Remove`s of a particular `Def` must be ordered before `Add`s.
pub steps: Vec<AutoMigrateStep<'def>>,
}

/// Checks that must be performed before performing an automatic migration.
/// These checks can access table contents and other database state.
#[derive(PartialEq, Eq, Debug)]
#[derive(PartialEq, Eq, Debug, PartialOrd, Ord)]
pub enum AutoMigratePrecheck<'def> {
/// Perform a check that adding a sequence is valid (the relevant column contains no values
/// greater than the sequence's start value).
CheckAddSequenceRangeValid(<SequenceDef as ModuleDefLookup>::Key<'def>),
}

/// A step in an automatic migration.
#[derive(PartialEq, Eq, Debug)]
#[derive(PartialEq, Eq, Debug, PartialOrd, Ord)]
pub enum AutoMigrateStep<'def> {
// It is important FOR CORRECTNESS that `Remove` variants are declared before `Add` variants in this enum!
//
// The ordering is used to sort the steps of an auto-migration.
// If adds go before removes, the following can occur:
//
// 1. `AddIndex("my_special_boy)`
// 2. `RemoveIndex("my_special_boy)`
//
// You see the problem.
//
// For now, we just ensure that we declare all `Remove` variants before `Add` variants
// and let `#[derive(PartialOrd)]` take care of the rest.
// TODO: when this enum is made serializable, a more durable fix will be needed here.
// Probably we will want to have separate arrays of add and remove steps.
//
/// Remove an index.
RemoveIndex(<IndexDef as ModuleDefLookup>::Key<'def>),
/// Remove a constraint.
RemoveConstraint(<ConstraintDef as ModuleDefLookup>::Key<'def>),
/// Remove a sequence.
RemoveSequence(<SequenceDef as ModuleDefLookup>::Key<'def>),
/// Remove a schedule annotation from a table.
RemoveSchedule(<ScheduleDef as ModuleDefLookup>::Key<'def>),
/// Remove a row-level security query.
RemoveRowLevelSecurity(<RawRowLevelSecurityDefV9 as ModuleDefLookup>::Key<'def>),

/// Add a table, including all indexes, constraints, and sequences.
/// There will NOT be separate steps in the plan for adding indexes, constraints, and sequences.
AddTable(<TableDef as ModuleDefLookup>::Key<'def>),
/// Add an index.
AddIndex(<IndexDef as ModuleDefLookup>::Key<'def>),
/// Remove an index.
RemoveIndex(<IndexDef as ModuleDefLookup>::Key<'def>),
/// Remove a constraint.
RemoveConstraint(<ConstraintDef as ModuleDefLookup>::Key<'def>),
/// Add a sequence.
AddSequence(<SequenceDef as ModuleDefLookup>::Key<'def>),
/// Remove a sequence.
RemoveSequence(<SequenceDef as ModuleDefLookup>::Key<'def>),
/// Change the access of a table.
ChangeAccess(<TableDef as ModuleDefLookup>::Key<'def>),
/// Add a schedule annotation to a table.
AddSchedule(<ScheduleDef as ModuleDefLookup>::Key<'def>),
/// Remove a schedule annotation from a table.
RemoveSchedule(<ScheduleDef as ModuleDefLookup>::Key<'def>),
/// Add a row-level security query.
AddRowLevelSecurity(<RawRowLevelSecurityDefV9 as ModuleDefLookup>::Key<'def>),
/// Remove a row-level security query.
RemoveRowLevelSecurity(<RawRowLevelSecurityDefV9 as ModuleDefLookup>::Key<'def>),

/// Change the access of a table.
ChangeAccess(<TableDef as ModuleDefLookup>::Key<'def>),
}

/// Something that might prevent an automatic migration.
Expand Down Expand Up @@ -181,6 +189,9 @@ pub fn ponder_auto_migrate<'def>(old: &'def ModuleDef, new: &'def ModuleDef) ->

let ((), (), (), (), ()) = (tables_ok, indexes_ok, sequences_ok, constraints_ok, rls_ok).combine_errors()?;

plan.steps.sort();
plan.prechecks.sort();

Ok(plan)
}

Expand Down Expand Up @@ -418,9 +429,14 @@ fn auto_migrate_constraints(plan: &mut AutoMigratePlan, new_tables: &HashSet<&Id
.collect_all_errors()
}

// Because we can refer to many tables and fields on the row level-security query, we need to remove all of them,
// then add the new ones, instead of trying to track the graph of dependencies.
fn auto_migrate_row_level_security(plan: &mut AutoMigratePlan) -> Result<()> {
// Because we can refer to many tables and fields on the row level-security query, we need to remove all of them,
// then add the new ones, instead of trying to track the graph of dependencies.
// When pretty-printing, steps to remove and re-add a row-level-security policy are not shown,
// since they are an implementation detail that will be surprising to users.
// TODO: change this to not add-and-remove unchanged policies, and hand the responsibility for reinitializing
// queries to the `core` crate instead.
// When you do this, you will need to update `pretty_print.rs`.
for rls in plan.old.row_level_security() {
plan.steps.push(AutoMigrateStep::RemoveRowLevelSecurity(rls.key()));
}
Expand Down
Loading
Loading