diff --git a/lib/dal/src/change_set.rs b/lib/dal/src/change_set.rs index bc27dda0e4..6fba6b1cb3 100644 --- a/lib/dal/src/change_set.rs +++ b/lib/dal/src/change_set.rs @@ -26,6 +26,7 @@ use crate::{ WorkspaceError, }; +pub mod approval; pub mod event; pub mod status; pub mod view; diff --git a/lib/dal/src/change_set/approval.rs b/lib/dal/src/change_set/approval.rs new file mode 100644 index 0000000000..85a9039eae --- /dev/null +++ b/lib/dal/src/change_set/approval.rs @@ -0,0 +1,170 @@ +//! Provides the ability to approve change sets and calculate their approval status. + +#![warn( + bad_style, + clippy::missing_panics_doc, + clippy::panic, + clippy::panic_in_result_fn, + clippy::unwrap_in_result, + clippy::unwrap_used, + dead_code, + improper_ctypes, + missing_debug_implementations, + missing_docs, + no_mangle_generic_items, + non_shorthand_field_patterns, + overflowing_literals, + path_statements, + patterns_in_fns_without_body, + unconditional_recursion, + unreachable_pub, + unused, + unused_allocation, + unused_comparisons, + unused_parens, + while_true +)] + +use chrono::{DateTime, Utc}; +use serde::{Deserialize, Serialize}; +use si_data_pg::{PgError, PgRow}; +use si_events::ChangesChecksum; +use si_id::{ChangeSetApprovalId, ChangeSetId, UserPk}; +use telemetry::prelude::*; +use thiserror::Error; + +pub use si_events::ChangeSetApprovalStatus; + +use crate::{DalContext, HistoryActor, TransactionsError, WorkspaceSnapshotError}; + +#[allow(missing_docs)] +#[derive(Debug, Error)] +pub enum ChangeSetApprovalError { + #[error("invalid user for creating a change set approval")] + InvalidUserForCreation, + #[error("pg error: {0}")] + Pg(#[from] PgError), + #[error("strum parse error: {0}")] + StrumParse(#[from] strum::ParseError), + #[error("transactions error: {0}")] + Transactions(#[from] TransactionsError), + #[error("workspace snapshot error: {0}")] + WorkspaceSnapshot(#[from] WorkspaceSnapshotError), +} + +type Result = std::result::Result; + +/// An individual approval for applying a [`ChangeSet`](crate::ChangeSet). +#[derive(Debug, Serialize, Deserialize)] +pub struct ChangeSetApproval { + id: ChangeSetApprovalId, + created_at: DateTime, + updated_at: DateTime, + change_set_id: ChangeSetId, + status: ChangeSetApprovalStatus, + user_id: UserPk, + checksum: String, +} + +impl TryFrom for ChangeSetApproval { + type Error = ChangeSetApprovalError; + + fn try_from(value: PgRow) -> std::result::Result { + let status_string: String = value.try_get("status")?; + let status = ChangeSetApprovalStatus::try_from(status_string.as_str())?; + Ok(Self { + id: value.try_get("id")?, + created_at: value.try_get("created_at")?, + updated_at: value.try_get("updated_at")?, + change_set_id: value.try_get("change_set_id")?, + status, + user_id: value.try_get("user_id")?, + checksum: value.try_get("checksum")?, + }) + } +} + +impl ChangeSetApproval { + /// Creates a new approval. + #[instrument(name = "change_set.approval.new", level = "info", skip_all)] + pub async fn new( + ctx: &DalContext, + status: ChangeSetApprovalStatus, + checksum: String, + ) -> Result { + let change_set_id = ctx.change_set_id(); + let user_id = match ctx.history_actor() { + HistoryActor::User(user_id) => user_id, + HistoryActor::SystemInit => return Err(ChangeSetApprovalError::InvalidUserForCreation), + }; + let row = ctx + .txns() + .await? + .pg() + .query_one( + "INSERT INTO change_set_approvals (change_set_id, status, user_id, checksum) VALUES ($1, $2, $3, $4) RETURNING *", + &[&change_set_id, &status.to_string(), &user_id, &checksum] + ) + .await?; + Self::try_from(row) + } + + /// Returns the ID of the approval. + pub fn id(&self) -> ChangeSetApprovalId { + self.id + } + + /// Returns the status of the approval. + pub fn status(&self) -> ChangeSetApprovalStatus { + self.status + } + + /// Returns the ID of the approver. + pub fn user_id(&self) -> UserPk { + self.user_id + } + + /// Returns the checksum based on the changes compared to HEAD when the approval was performed. + pub fn checksum(&self) -> &str { + self.checksum.as_str() + } + + /// Lists all approvals in the [`ChangeSet`](crate::ChangeSet). + #[instrument(name = "change_set.approval.list", level = "info", skip_all)] + pub async fn list(ctx: &DalContext) -> Result> { + let change_set_id = ctx.change_set_id(); + let rows = ctx + .txns() + .await? + .pg() + .query( + "SELECT * from change_set_approvals WHERE change_set_id = $1 ORDER BY id ASC", + &[&change_set_id], + ) + .await?; + let mut approvals = Vec::with_capacity(rows.len()); + for row in rows { + approvals.push(Self::try_from(row)?); + } + Ok(approvals) + } + + /// Generates a checksum for changes in the current [`ChangeSet`](crate::ChangeSet). + #[instrument( + name = "change_set.approval.generate_checksum", + level = "debug", + skip_all + )] + pub async fn generate_checksum(ctx: &DalContext) -> Result { + let mut changes = ctx + .workspace_snapshot()? + .detect_changes_from_head(ctx) + .await?; + changes.sort_by_key(|c| c.id); + let mut hasher = ChangesChecksum::hasher(); + for change in changes { + hasher.update(change.merkle_tree_hash.as_bytes()); + } + Ok(hasher.finalize()) + } +} diff --git a/lib/dal/src/migrations/U3500__change_set_approvals.sql b/lib/dal/src/migrations/U3500__change_set_approvals.sql new file mode 100644 index 0000000000..841db02953 --- /dev/null +++ b/lib/dal/src/migrations/U3500__change_set_approvals.sql @@ -0,0 +1,10 @@ +CREATE TABLE change_set_approvals +( + id ident primary key NOT NULL DEFAULT ident_create_v1(), + created_at timestamp with time zone NOT NULL DEFAULT CLOCK_TIMESTAMP(), + updated_at timestamp with time zone NOT NULL DEFAULT CLOCK_TIMESTAMP(), + change_set_id ident NOT NULL, + status text NOT NULL, + user_id ident NOT NULL, + checksum text NOT NULL +); diff --git a/lib/dal/src/workspace_snapshot.rs b/lib/dal/src/workspace_snapshot.rs index f1f5376cc4..85e2c9ffcd 100644 --- a/lib/dal/src/workspace_snapshot.rs +++ b/lib/dal/src/workspace_snapshot.rs @@ -37,7 +37,7 @@ pub mod vector_clock; pub use traits::{schema::variant::SchemaVariantExt, socket::input::InputSocketExt}; use graph::correct_transforms::correct_transforms; -use graph::detect_updates::Update; +use graph::detector::{Change, Update}; use graph::{RebaseBatch, WorkspaceSnapshotGraph}; use node_weight::traits::CorrectTransformsError; use std::collections::{HashMap, HashSet}; @@ -713,12 +713,7 @@ impl WorkspaceSnapshot { Ok(()) } - #[instrument( - name = "workspace_snapshot.detect_updates", - level = "debug", - skip_all, - fields() - )] + #[instrument(name = "workspace_snapshot.detect_updates", level = "debug", skip_all)] pub async fn detect_updates( &self, onto_workspace_snapshot: &WorkspaceSnapshot, @@ -735,6 +730,41 @@ impl WorkspaceSnapshot { .await?) } + #[instrument(name = "workspace_snapshot.detect_changes", level = "debug", skip_all)] + pub async fn detect_changes( + &self, + onto_workspace_snapshot: &WorkspaceSnapshot, + ) -> WorkspaceSnapshotResult> { + let self_clone = self.clone(); + let onto_clone = onto_workspace_snapshot.clone(); + + Ok(slow_rt::spawn(async move { + self_clone + .working_copy() + .await + .detect_changes(&*onto_clone.working_copy().await) + })? + .await?) + } + + /// A wrapper around [`Self::detect_changes`](Self::detect_changes) where the "onto" snapshot is derived from the + /// workspace's default [`ChangeSet`](crate::ChangeSet). + #[instrument( + name = "workspace_snapshot.detect_changes_from_head", + level = "debug", + skip_all + )] + pub async fn detect_changes_from_head( + &self, + ctx: &DalContext, + ) -> WorkspaceSnapshotResult> { + let head_change_set_id = ctx.get_workspace_default_change_set_id().await?; + let head_snapshot = Self::find_for_change_set(&ctx, head_change_set_id).await?; + Ok(head_snapshot + .detect_changes(&ctx.workspace_snapshot()?.clone()) + .await?) + } + /// Gives the exact node index endpoints of an edge. pub async fn edge_endpoints( &self, diff --git a/lib/dal/src/workspace_snapshot/graph.rs b/lib/dal/src/workspace_snapshot/graph.rs index 59050b9047..ef864419be 100644 --- a/lib/dal/src/workspace_snapshot/graph.rs +++ b/lib/dal/src/workspace_snapshot/graph.rs @@ -1,10 +1,8 @@ use std::{fs::File, io::Write}; use deprecated::DeprecatedWorkspaceSnapshotGraphV1; -use detect_updates::Update; +use detector::Update; use petgraph::prelude::*; -/// Ensure [`NodeIndex`], and [`Direction`] are usable externally. -pub use petgraph::{graph::NodeIndex, Direction}; use serde::{Deserialize, Serialize}; use si_events::{merkle_tree_hash::MerkleTreeHash, ulid::Ulid}; use si_layer_cache::db::serialize; @@ -13,6 +11,9 @@ use strum::{EnumDiscriminants, EnumIter, EnumString, IntoEnumIterator}; use telemetry::prelude::*; use thiserror::Error; +/// Ensure [`NodeIndex`], and [`Direction`] are usable externally. +pub use petgraph::{graph::NodeIndex, Direction}; + use crate::{ socket::input::InputSocketError, workspace_snapshot::node_weight::{category_node_weight::CategoryNodeKind, NodeWeightError}, @@ -21,7 +22,7 @@ use crate::{ pub mod correct_transforms; pub mod deprecated; -pub mod detect_updates; +pub mod detector; mod tests; pub mod traits; pub mod v2; @@ -44,8 +45,6 @@ pub enum WorkspaceSnapshotGraphError { CannotCompareOrderedAndUnorderedContainers(NodeIndex, NodeIndex), #[error("could not find category node of kind: {0:?}")] CategoryNodeNotFound(CategoryNodeKind), - // #[error("ChangeSet error: {0}")] - // ChangeSet(#[from] ChangeSetError), #[error("Component error: {0}")] Component(#[from] Box), #[error("Unable to retrieve content for ContentHash")] diff --git a/lib/dal/src/workspace_snapshot/graph/correct_transforms.rs b/lib/dal/src/workspace_snapshot/graph/correct_transforms.rs index 13ab42ab22..2e6a4a69e3 100644 --- a/lib/dal/src/workspace_snapshot/graph/correct_transforms.rs +++ b/lib/dal/src/workspace_snapshot/graph/correct_transforms.rs @@ -9,7 +9,7 @@ use crate::workspace_snapshot::node_weight::NodeWeight; use crate::workspace_snapshot::NodeInformation; use crate::{EdgeWeight, EdgeWeightKind, NodeWeightDiscriminants}; -use super::{detect_updates::Update, WorkspaceSnapshotGraphVCurrent}; +use super::{detector::Update, WorkspaceSnapshotGraphVCurrent}; pub fn correct_transforms( graph: &WorkspaceSnapshotGraphVCurrent, diff --git a/lib/dal/src/workspace_snapshot/graph/detect_updates.rs b/lib/dal/src/workspace_snapshot/graph/detector.rs similarity index 88% rename from lib/dal/src/workspace_snapshot/graph/detect_updates.rs rename to lib/dal/src/workspace_snapshot/graph/detector.rs index 1b39963bbf..255096c494 100644 --- a/lib/dal/src/workspace_snapshot/graph/detect_updates.rs +++ b/lib/dal/src/workspace_snapshot/graph/detector.rs @@ -5,8 +5,9 @@ use petgraph::{ visit::{Control, DfsEvent}, }; use serde::{Deserialize, Serialize}; -use si_events::ulid::Ulid; +use si_events::{merkle_tree_hash::MerkleTreeHash, ulid::Ulid}; use strum::EnumDiscriminants; +use telemetry::prelude::*; use crate::{ workspace_snapshot::{node_weight::NodeWeight, NodeInformation}, @@ -35,6 +36,12 @@ pub enum Update { }, } +#[derive(Debug)] +pub struct Change { + pub id: Ulid, + pub merkle_tree_hash: MerkleTreeHash, +} + #[derive(Clone, Debug)] enum NodeDifference { NewNode, @@ -57,6 +64,42 @@ impl<'a, 'b> Detector<'a, 'b> { } } + /// Performs a post order walk of the updated graph, finding the updates + /// made to it when compared to the base graph, using the Merkle tree hash + /// to detect changes and ignore unchanged branches. + /// + /// This assumes that all graphs involved to not have any "garbage" laying around. If in doubt, + /// run [`cleanup`][WorkspaceSnapshotGraph::cleanup] on all involved graphs, before handing + /// them over to the [`Detector`]. + pub fn detect_updates(&self) -> Vec { + let mut updates = vec![]; + let mut difference_cache = HashMap::new(); + + petgraph::visit::depth_first_search( + self.updated_graph.graph(), + Some(self.updated_graph.root()), + |event| self.calculate_updates_dfs_event(event, &mut updates, &mut difference_cache), + ); + + updates + } + + /// Performs a post order walk of an updated graph, finding the nodes that have been added, removed or modified. + /// + /// This assumes that all graphs involved to not have any "garbage" laying around. If in doubt, perform "cleanup" + /// on both graphs before creating the [`Detector`]. + pub fn detect_changes(&self) -> Vec { + let mut changes = Vec::new(); + + petgraph::visit::depth_first_search( + self.updated_graph.graph(), + Some(self.updated_graph.root()), + |event| self.calculate_changes_dfs_event(event, &mut changes), + ); + + changes + } + fn node_diff_from_base_graph( &self, updated_graph_node_index: NodeIndex, @@ -328,23 +371,30 @@ impl<'a, 'b> Detector<'a, 'b> { } } - /// Performs a post order walk of the updated graph, finding the updates - /// made to it when compared to the base graph, using the Merkle tree hash - /// to detect changes and ignore unchanged branches. - /// - /// This assumes that all graphs involved to not have any "garbage" laying around. If in doubt, - /// run [`cleanup`][WorkspaceSnapshotGraph::cleanup] on all involved graphs, before handing - /// them over to the [`Detector`]. - pub fn detect_updates(&self) -> Vec { - let mut updates = vec![]; - let mut difference_cache = HashMap::new(); - - petgraph::visit::depth_first_search( - self.updated_graph.graph(), - Some(self.updated_graph.root()), - |event| self.calculate_updates_dfs_event(event, &mut updates, &mut difference_cache), - ); + fn calculate_changes_dfs_event( + &self, + event: DfsEvent, + changes: &mut Vec, + ) -> Control<()> { + if let DfsEvent::Discover(updated_graph_index, _) = event { + match self.updated_graph.get_node_weight(updated_graph_index) { + Ok(updated_node_weight) => { + if let Some(original_node_weight) = self.base_graph.get_node_weight_by_id_opt(updated_node_weight.id()) { + if original_node_weight.merkle_tree_hash() == updated_node_weight.merkle_tree_hash() { + return Control::Prune; + } + } - updates + // If either the original node weight was not found or it was found the merkle tree hashes differ, + // then we have information that needs to be collected! + changes.push(Change { + id: updated_node_weight.id(), + merkle_tree_hash: updated_node_weight.merkle_tree_hash(), + }); + } + Err(err) => error!(?err, "heat death of the universe error: updated node weight not found by updated node index from the same graph"), + } + } + Control::Continue } } diff --git a/lib/dal/src/workspace_snapshot/graph/tests.rs b/lib/dal/src/workspace_snapshot/graph/tests.rs index 6ac1bf2a05..65cb163ff7 100644 --- a/lib/dal/src/workspace_snapshot/graph/tests.rs +++ b/lib/dal/src/workspace_snapshot/graph/tests.rs @@ -7,6 +7,7 @@ use crate::{ EdgeWeight, EdgeWeightKind, PropKind, }; +mod detect_changes; mod detect_updates; mod exclusive_outgoing_edges; mod rebase; @@ -91,7 +92,7 @@ mod test { use crate::workspace_snapshot::{ content_address::ContentAddress, edge_weight::{EdgeWeight, EdgeWeightKind, EdgeWeightKindDiscriminants}, - graph::{detect_updates::Update, WorkspaceSnapshotGraphVCurrent}, + graph::{detector::Update, WorkspaceSnapshotGraphVCurrent}, node_weight::NodeWeight, }; use crate::{ComponentId, FuncId, PropId, SchemaId, SchemaVariantId}; diff --git a/lib/dal/src/workspace_snapshot/graph/tests/detect_changes.rs b/lib/dal/src/workspace_snapshot/graph/tests/detect_changes.rs new file mode 100644 index 0000000000..801c3d19ab --- /dev/null +++ b/lib/dal/src/workspace_snapshot/graph/tests/detect_changes.rs @@ -0,0 +1,19 @@ +#[cfg(test)] +mod test { + use crate::WorkspaceSnapshotGraphVCurrent; + + type Result = std::result::Result>; + + #[test] + fn identical_graphs() -> Result<()> { + let base_graph = WorkspaceSnapshotGraphVCurrent::new_for_unit_tests()?; + let updated_graph = base_graph.clone(); + assert!(base_graph.is_acyclic_directed()); + assert!(updated_graph.is_acyclic_directed()); + + let changes = base_graph.detect_changes(&updated_graph); + assert!(changes.is_empty()); + + Ok(()) + } +} diff --git a/lib/dal/src/workspace_snapshot/graph/tests/detect_updates.rs b/lib/dal/src/workspace_snapshot/graph/tests/detect_updates.rs index 705719ff7b..21cca73361 100644 --- a/lib/dal/src/workspace_snapshot/graph/tests/detect_updates.rs +++ b/lib/dal/src/workspace_snapshot/graph/tests/detect_updates.rs @@ -10,7 +10,7 @@ mod test { workspace_snapshot::{ content_address::ContentAddress, edge_weight::{EdgeWeight, EdgeWeightKind, EdgeWeightKindDiscriminants}, - graph::detect_updates::Update, + graph::detector::Update, node_weight::NodeWeight, NodeInformation, }, diff --git a/lib/dal/src/workspace_snapshot/graph/tests/exclusive_outgoing_edges.rs b/lib/dal/src/workspace_snapshot/graph/tests/exclusive_outgoing_edges.rs index d8555e193f..54895a0a37 100644 --- a/lib/dal/src/workspace_snapshot/graph/tests/exclusive_outgoing_edges.rs +++ b/lib/dal/src/workspace_snapshot/graph/tests/exclusive_outgoing_edges.rs @@ -8,7 +8,7 @@ mod test { action::prototype::ActionKind, workspace_snapshot::{ content_address::ContentAddress, - graph::{detect_updates::Update, WorkspaceSnapshotGraphResult}, + graph::{detector::Update, WorkspaceSnapshotGraphResult}, node_weight::{ traits::{CorrectExclusiveOutgoingEdge, CorrectTransforms}, NodeWeight, diff --git a/lib/dal/src/workspace_snapshot/graph/v2.rs b/lib/dal/src/workspace_snapshot/graph/v2.rs index b2a271d5ec..25eef8dfa0 100644 --- a/lib/dal/src/workspace_snapshot/graph/v2.rs +++ b/lib/dal/src/workspace_snapshot/graph/v2.rs @@ -16,7 +16,7 @@ use crate::{ workspace_snapshot::{ content_address::ContentAddress, graph::{ - detect_updates::Update, MerkleTreeHash, WorkspaceSnapshotGraphError, + detector::Update, MerkleTreeHash, WorkspaceSnapshotGraphError, WorkspaceSnapshotGraphResult, }, node_weight::{CategoryNodeWeight, NodeWeight}, diff --git a/lib/dal/src/workspace_snapshot/graph/v3.rs b/lib/dal/src/workspace_snapshot/graph/v3.rs index da9ee3ff3c..f3d2827e25 100644 --- a/lib/dal/src/workspace_snapshot/graph/v3.rs +++ b/lib/dal/src/workspace_snapshot/graph/v3.rs @@ -21,7 +21,7 @@ use crate::{ workspace_snapshot::{ content_address::ContentAddress, graph::{ - detect_updates::Update, MerkleTreeHash, WorkspaceSnapshotGraphError, + detector::Update, MerkleTreeHash, WorkspaceSnapshotGraphError, WorkspaceSnapshotGraphResult, }, node_weight::{CategoryNodeWeight, NodeWeight}, diff --git a/lib/dal/src/workspace_snapshot/graph/v4.rs b/lib/dal/src/workspace_snapshot/graph/v4.rs index c793b66aee..661fb26e36 100644 --- a/lib/dal/src/workspace_snapshot/graph/v4.rs +++ b/lib/dal/src/workspace_snapshot/graph/v4.rs @@ -23,7 +23,7 @@ use crate::{ workspace_snapshot::{ content_address::ContentAddress, graph::{ - detect_updates::{Detector, Update}, + detector::{Detector, Update}, MerkleTreeHash, WorkspaceSnapshotGraphError, WorkspaceSnapshotGraphResult, }, node_weight::{CategoryNodeWeight, NodeWeight}, @@ -33,6 +33,8 @@ use crate::{ Timestamp, }; +use super::detector::Change; + pub mod component; pub mod diagram; pub mod schema; @@ -689,6 +691,10 @@ impl WorkspaceSnapshotGraphV4 { Detector::new(self, updated_graph).detect_updates() } + pub fn detect_changes(&self, updated_graph: &Self) -> Vec { + Detector::new(self, updated_graph).detect_changes() + } + #[allow(dead_code)] pub fn dot(&self) { // NOTE(nick): copy the output and execute this on macOS. It will create a file in the diff --git a/lib/dal/src/workspace_snapshot/node_weight.rs b/lib/dal/src/workspace_snapshot/node_weight.rs index 2589d5d395..defb4cfa8e 100644 --- a/lib/dal/src/workspace_snapshot/node_weight.rs +++ b/lib/dal/src/workspace_snapshot/node_weight.rs @@ -26,7 +26,7 @@ use self::{ schema_variant_node_weight::SchemaVariantNodeWeightError, }; use super::graph::{ - deprecated::v1::DeprecatedNodeWeightV1, detect_updates::Update, WorkspaceSnapshotGraphError, + deprecated::v1::DeprecatedNodeWeightV1, detector::Update, WorkspaceSnapshotGraphError, }; use crate::layer_db_types::ComponentContentDiscriminants; use crate::workspace_snapshot::node_weight::diagram_object_node_weight::{ diff --git a/lib/dal/src/workspace_snapshot/node_weight/action_node_weight.rs b/lib/dal/src/workspace_snapshot/node_weight/action_node_weight.rs index 14b28b4f7b..6763815f1c 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/action_node_weight.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/action_node_weight.rs @@ -7,7 +7,7 @@ use si_events::{merkle_tree_hash::MerkleTreeHash, ulid::Ulid, ContentHash}; use crate::{ action::ActionState, workspace_snapshot::{ - graph::{deprecated::v1::DeprecatedActionNodeWeightV1, detect_updates::Update, LineageId}, + graph::{deprecated::v1::DeprecatedActionNodeWeightV1, detector::Update, LineageId}, node_weight::{traits::CorrectTransforms, NodeWeight}, NodeId, NodeInformation, }, diff --git a/lib/dal/src/workspace_snapshot/node_weight/attribute_value_node_weight.rs b/lib/dal/src/workspace_snapshot/node_weight/attribute_value_node_weight.rs index 49cc911dad..d273169caf 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/attribute_value_node_weight.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/attribute_value_node_weight.rs @@ -8,8 +8,7 @@ use crate::{ content_address::ContentAddress, graph::{ correct_transforms::add_dependent_value_root_updates, - deprecated::v1::DeprecatedAttributeValueNodeWeightV1, detect_updates::Update, - LineageId, + deprecated::v1::DeprecatedAttributeValueNodeWeightV1, detector::Update, LineageId, }, node_weight::traits::CorrectTransforms, NodeId, diff --git a/lib/dal/src/workspace_snapshot/node_weight/component_node_weight.rs b/lib/dal/src/workspace_snapshot/node_weight/component_node_weight.rs index af0742c9a8..16b15e00e6 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/component_node_weight.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/component_node_weight.rs @@ -15,7 +15,7 @@ use crate::{ content_address::{ContentAddress, ContentAddressDiscriminants}, graph::{ correct_transforms::add_dependent_value_root_updates, - deprecated::v1::DeprecatedComponentNodeWeightV1, detect_updates::Update, LineageId, + deprecated::v1::DeprecatedComponentNodeWeightV1, detector::Update, LineageId, }, node_weight::traits::CorrectTransforms, NodeInformation, diff --git a/lib/dal/src/workspace_snapshot/node_weight/content_node_weight.rs b/lib/dal/src/workspace_snapshot/node_weight/content_node_weight.rs index 95268b1b69..a732d1bcf6 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/content_node_weight.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/content_node_weight.rs @@ -8,7 +8,7 @@ use si_events::{ulid::Ulid, ContentHash}; use crate::{ workspace_snapshot::{ content_address::{ContentAddress, ContentAddressDiscriminants}, - graph::{deprecated::v1::DeprecatedContentNodeWeightV1, detect_updates::Update, LineageId}, + graph::{deprecated::v1::DeprecatedContentNodeWeightV1, detector::Update, LineageId}, node_weight::{traits::CorrectTransforms, NodeWeightError, NodeWeightResult}, NodeInformation, }, diff --git a/lib/dal/src/workspace_snapshot/node_weight/diagram_object_node_weight/v1.rs b/lib/dal/src/workspace_snapshot/node_weight/diagram_object_node_weight/v1.rs index 716c44dda0..50e66a71cf 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/diagram_object_node_weight/v1.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/diagram_object_node_weight/v1.rs @@ -4,7 +4,7 @@ use si_events::{merkle_tree_hash::MerkleTreeHash, ulid::Ulid, ContentHash}; use crate::{ workspace_snapshot::{ - graph::{detect_updates::Update, LineageId}, + graph::{detector::Update, LineageId}, node_weight::{ diagram_object_node_weight::DiagramObjectKind, traits::{ diff --git a/lib/dal/src/workspace_snapshot/node_weight/geometry_node_weight/v1.rs b/lib/dal/src/workspace_snapshot/node_weight/geometry_node_weight/v1.rs index f9c7b85d02..df0504b659 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/geometry_node_weight/v1.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/geometry_node_weight/v1.rs @@ -1,5 +1,5 @@ use super::{GeometryNodeWeight, NodeWeightDiscriminants}; -use crate::workspace_snapshot::graph::detect_updates::Update; +use crate::workspace_snapshot::graph::detector::Update; use crate::workspace_snapshot::node_weight::diagram_object_node_weight::DiagramObjectKind; use crate::workspace_snapshot::node_weight::traits::{ CorrectTransformsError, CorrectTransformsResult, diff --git a/lib/dal/src/workspace_snapshot/node_weight/ordering_node_weight.rs b/lib/dal/src/workspace_snapshot/node_weight/ordering_node_weight.rs index 76bf2928a7..08e431df84 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/ordering_node_weight.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/ordering_node_weight.rs @@ -7,7 +7,7 @@ use si_events::{merkle_tree_hash::MerkleTreeHash, ulid::Ulid, ContentHash}; use super::traits::CorrectTransformsError; use super::{NodeWeight, NodeWeightDiscriminants, NodeWeightError}; use crate::workspace_snapshot::graph::deprecated::v1::DeprecatedOrderingNodeWeightV1; -use crate::workspace_snapshot::graph::detect_updates::Update; +use crate::workspace_snapshot::graph::detector::Update; use crate::workspace_snapshot::node_weight::traits::{CorrectTransforms, CorrectTransformsResult}; use crate::workspace_snapshot::node_weight::NodeWeightResult; use crate::workspace_snapshot::NodeInformation; diff --git a/lib/dal/src/workspace_snapshot/node_weight/schema_variant_node_weight/v1.rs b/lib/dal/src/workspace_snapshot/node_weight/schema_variant_node_weight/v1.rs index ba3647dfd7..7d4b23d31c 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/schema_variant_node_weight/v1.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/schema_variant_node_weight/v1.rs @@ -12,7 +12,7 @@ use crate::{ layer_db_types::{SchemaVariantContent, SchemaVariantContentV3}, workspace_snapshot::{ content_address::ContentAddress, - graph::{detect_updates::Update, LineageId, WorkspaceSnapshotGraphError}, + graph::{detector::Update, LineageId, WorkspaceSnapshotGraphError}, node_weight::{ self, traits::{CorrectExclusiveOutgoingEdge, CorrectTransforms, SiNodeWeight}, @@ -192,10 +192,10 @@ impl CorrectTransforms for SchemaVariantNodeWeightV1 { fn correct_transforms( &self, workspace_snapshot_graph: &WorkspaceSnapshotGraphVCurrent, - mut updates: Vec, + mut updates: Vec, _from_different_change_set: bool, ) -> crate::workspace_snapshot::node_weight::traits::CorrectTransformsResult< - Vec, + Vec, > { use crate::workspace_snapshot::graph::SchemaVariantExt; diff --git a/lib/dal/src/workspace_snapshot/node_weight/secret_node_weight.rs b/lib/dal/src/workspace_snapshot/node_weight/secret_node_weight.rs index 2f5a580434..91b843b765 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/secret_node_weight.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/secret_node_weight.rs @@ -5,7 +5,7 @@ use si_events::{merkle_tree_hash::MerkleTreeHash, ulid::Ulid, ContentHash, Encry use crate::workspace_snapshot::content_address::ContentAddressDiscriminants; use crate::workspace_snapshot::graph::deprecated::v1::DeprecatedSecretNodeWeightV1; -use crate::workspace_snapshot::graph::detect_updates::Update; +use crate::workspace_snapshot::graph::detector::Update; use crate::workspace_snapshot::NodeId; use crate::workspace_snapshot::{ content_address::ContentAddress, diff --git a/lib/dal/src/workspace_snapshot/node_weight/traits.rs b/lib/dal/src/workspace_snapshot/node_weight/traits.rs index 5c1c46f56b..7e15348868 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/traits.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/traits.rs @@ -1,6 +1,6 @@ use crate::{ workspace_snapshot::{ - graph::{detect_updates::Update, WorkspaceSnapshotGraphError}, + graph::{detector::Update, WorkspaceSnapshotGraphError}, NodeInformation, }, EdgeWeightKindDiscriminants, WorkspaceSnapshotGraphVCurrent, diff --git a/lib/dal/src/workspace_snapshot/node_weight/traits/correct_exclusive_outgoing_edge.rs b/lib/dal/src/workspace_snapshot/node_weight/traits/correct_exclusive_outgoing_edge.rs index da46f51c76..b6575c1017 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/traits/correct_exclusive_outgoing_edge.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/traits/correct_exclusive_outgoing_edge.rs @@ -4,7 +4,7 @@ use petgraph::prelude::*; use crate::{ workspace_snapshot::{ - edge_weight::EdgeWeightKindDiscriminants, graph::detect_updates::Update, NodeInformation, + edge_weight::EdgeWeightKindDiscriminants, graph::detector::Update, NodeInformation, }, WorkspaceSnapshotGraphVCurrent, }; diff --git a/lib/dal/src/workspace_snapshot/node_weight/view_node_weight/v1.rs b/lib/dal/src/workspace_snapshot/node_weight/view_node_weight/v1.rs index b4dbc87d47..cf06d6afba 100644 --- a/lib/dal/src/workspace_snapshot/node_weight/view_node_weight/v1.rs +++ b/lib/dal/src/workspace_snapshot/node_weight/view_node_weight/v1.rs @@ -3,7 +3,7 @@ use std::collections::HashSet; use crate::{ workspace_snapshot::{ content_address::ContentAddress, - graph::{detect_updates::Update, LineageId, WorkspaceSnapshotGraphError}, + graph::{detector::Update, LineageId, WorkspaceSnapshotGraphError}, node_weight::{ traits::{CorrectExclusiveOutgoingEdge, CorrectTransforms, SiNodeWeight}, NodeWeight, NodeWeightDiscriminants, @@ -49,10 +49,10 @@ impl CorrectTransforms for ViewNodeWeightV1 { fn correct_transforms( &self, workspace_snapshot_graph: &crate::WorkspaceSnapshotGraphVCurrent, - mut updates: Vec, + mut updates: Vec, _from_different_change_set: bool, ) -> crate::workspace_snapshot::node_weight::traits::CorrectTransformsResult< - Vec, + Vec, > { let mut maybe_view_removal_update_idx = None; let mut removed_geometries = HashSet::new(); diff --git a/lib/dal/tests/integration_test/change_set.rs b/lib/dal/tests/integration_test/change_set.rs index bf7dd2d8d3..058740eff9 100644 --- a/lib/dal/tests/integration_test/change_set.rs +++ b/lib/dal/tests/integration_test/change_set.rs @@ -12,6 +12,8 @@ use itertools::Itertools; use pretty_assertions_sorted::assert_eq; use std::collections::HashSet; +mod approval; + #[test] async fn open_change_sets(ctx: &mut DalContext) { let view = OpenChangeSetsView::assemble(ctx) diff --git a/lib/dal/tests/integration_test/change_set/approval.rs b/lib/dal/tests/integration_test/change_set/approval.rs new file mode 100644 index 0000000000..ca47026bd0 --- /dev/null +++ b/lib/dal/tests/integration_test/change_set/approval.rs @@ -0,0 +1,58 @@ +use std::collections::HashSet; + +use dal::change_set::approval::{ChangeSetApproval, ChangeSetApprovalStatus}; +use dal::{DalContext, Ulid}; +use dal_test::color_eyre::eyre::OptionExt; +use dal_test::helpers::{ + create_component_for_default_schema_name_in_default_view, ChangeSetTestHelpers, +}; +use dal_test::{test, Result}; +use pretty_assertions_sorted::assert_eq; + +#[test] +async fn new(ctx: &mut DalContext) -> Result<()> { + create_component_for_default_schema_name_in_default_view(ctx, "fallout", "soken").await?; + ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx).await?; + + let status = ChangeSetApprovalStatus::Approved; + let checksum = ChangeSetApproval::generate_checksum(ctx).await?.to_string(); + + let new_approval = ChangeSetApproval::new(ctx, status, checksum).await?; + assert_eq!( + status, // expectd + new_approval.status() // actual + ); + + let mut approvals = ChangeSetApproval::list(ctx).await?; + let approval = approvals.pop().ok_or_eyre("unexpected empty approvals")?; + assert!(approvals.is_empty()); + assert_eq!( + new_approval.status(), // expected + approval.status() // actual + ); + assert_eq!( + new_approval.id(), // expected + approval.id() // actual + ); + + Ok(()) +} + +#[test] +async fn status(ctx: &mut DalContext) -> Result<()> { + create_component_for_default_schema_name_in_default_view(ctx, "fallout", "find the flame") + .await?; + ChangeSetTestHelpers::commit_and_update_snapshot_to_visibility(ctx).await?; + + let changes = ctx + .workspace_snapshot()? + .detect_changes_from_head(ctx) + .await?; + let seen: HashSet = HashSet::from_iter(changes.iter().map(|c| c.id)); + assert_eq!( + changes.len(), // expected + seen.len() // actual + ); + + Ok(()) +} diff --git a/lib/sdf-server/src/service/v2/change_set.rs b/lib/sdf-server/src/service/v2/change_set.rs index 6a7af8b654..bd5c216d82 100644 --- a/lib/sdf-server/src/service/v2/change_set.rs +++ b/lib/sdf-server/src/service/v2/change_set.rs @@ -27,9 +27,15 @@ mod rename; mod reopen; mod request_approval; +// NOTE(nick): move these to the above group and remove old modules once the feature flag has been removed; +mod approval_status; +mod approve_v2; + #[remain::sorted] #[derive(Debug, Error)] pub enum Error { + #[error("change set approval error: {0}")] + Approval(#[from] dal::change_set::approval::ChangeSetApprovalError), #[error("change set error: {0}")] ChangeSet(#[from] dal::ChangeSetError), #[error("change set apply error: {0}")] @@ -158,7 +164,9 @@ pub fn v2_routes(state: AppState) -> Router { permissions::Permission::Approve, )), ) - .route("/rename", post(rename::rename)), + .route("/rename", post(rename::rename)) + .route("/approval_status", get(approval_status::approval_status)) + .route("/approve_v2", post(approve_v2::approve)), ) .route("/", get(list::list_actionable)) } diff --git a/lib/sdf-server/src/service/v2/change_set/approval_status.rs b/lib/sdf-server/src/service/v2/change_set/approval_status.rs new file mode 100644 index 0000000000..0969936a7a --- /dev/null +++ b/lib/sdf-server/src/service/v2/change_set/approval_status.rs @@ -0,0 +1,33 @@ +use axum::{extract::Path, Json}; +use dal::{change_set::approval::ChangeSetApproval, ChangeSetId, WorkspacePk, WorkspaceSnapshot}; + +use crate::extract::{AccessBuilder, HandlerContext}; + +use super::Result; + +pub async fn approval_status( + HandlerContext(builder): HandlerContext, + AccessBuilder(access_builder): AccessBuilder, + Path((_workspace_pk, change_set_id)): Path<(WorkspacePk, ChangeSetId)>, +) -> Result> { + let ctx = builder + .build(access_builder.build(change_set_id.into())) + .await?; + + let approvals = ChangeSetApproval::list(&ctx).await?; + let mut current = Vec::with_capacity(approvals.len()); + for approval in approvals { + current.push(si_frontend_types::ChangeSetApproval { + user_id: approval.user_id(), + status: approval.status(), + // FIXME(nick): make this real! + is_valid: true, + }) + } + + Ok(Json(si_frontend_types::ChangeSetApprovals { + // FIXME(nick): get requirements. + required: Vec::new(), + current, + })) +} diff --git a/lib/sdf-server/src/service/v2/change_set/approve_v2.rs b/lib/sdf-server/src/service/v2/change_set/approve_v2.rs new file mode 100644 index 0000000000..3a10e63a85 --- /dev/null +++ b/lib/sdf-server/src/service/v2/change_set/approve_v2.rs @@ -0,0 +1,77 @@ +use axum::{ + extract::{Host, OriginalUri, Path}, + Json, +}; +use dal::{change_set::approval::ChangeSetApproval, ChangeSet, ChangeSetId, WorkspacePk, WsEvent}; +use serde::Deserialize; +use si_events::{audit_log::AuditLogKind, ChangeSetApprovalStatus}; + +use super::{post_to_webhook, Error, Result}; +use crate::{ + extract::{AccessBuilder, HandlerContext, PosthogClient}, + track, +}; + +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +pub struct Request { + pub status: ChangeSetApprovalStatus, +} + +pub async fn approve( + HandlerContext(builder): HandlerContext, + AccessBuilder(request_ctx): AccessBuilder, + PosthogClient(posthog_client): PosthogClient, + OriginalUri(original_uri): OriginalUri, + Host(host_name): Host, + Path((_workspace_pk, change_set_id)): Path<(WorkspacePk, ChangeSetId)>, + Json(request): Json, +) -> Result<()> { + let ctx = builder + .build(request_ctx.build(change_set_id.into())) + .await?; + + // Ensure that DVU roots are empty before continuing? + // todo(brit): maybe we can get away without this. Ex: Approve a PR before tests finish + if !ctx + .workspace_snapshot()? + .get_dependent_value_roots() + .await? + .is_empty() + { + // TODO(nick): we should consider requiring this check in integration tests too. Why did I + // not do this at the time of writing? Tests have multiple ways to call "apply", whether + // its via helpers or through the change set methods directly. In addition, they test + // for success and failure, not solely for success. We should still do this, but not within + // the PR corresponding to when this message was written. + return Err(Error::DvuRootsNotEmpty(ctx.change_set_id())); + } + + let change_set = ChangeSet::find(&ctx, ctx.visibility().change_set_id) + .await? + .ok_or(Error::ChangeSetNotFound(ctx.change_set_id()))?; + // FIXME(nick): put the real checksum here. + ChangeSetApproval::new(&ctx, request.status, "FIXME".to_string()).await?; + + track( + &posthog_client, + &ctx, + &original_uri, + &host_name, + "approve_change_set_apply", + serde_json::json!({ + "merged_change_set": change_set.id, + }), + ); + ctx.write_audit_log( + AuditLogKind::ApproveChangeSetApply { + from_status: change_set.status.into(), + }, + change_set.name, + ) + .await?; + + ctx.commit().await?; + + Ok(()) +} diff --git a/lib/si-events-rs/src/change_set_approval.rs b/lib/si-events-rs/src/change_set_approval.rs new file mode 100644 index 0000000000..14d05a71c9 --- /dev/null +++ b/lib/si-events-rs/src/change_set_approval.rs @@ -0,0 +1,23 @@ +use postgres_types::ToSql; +use serde::{Deserialize, Serialize}; +use strum::{AsRefStr, Display, EnumString}; + +use crate::create_xxhash_type; + +create_xxhash_type!(ChangesChecksum); + +#[remain::sorted] +#[derive( + AsRefStr, Deserialize, Serialize, Debug, Display, EnumString, PartialEq, Eq, Copy, Clone, ToSql, +)] +pub enum ChangeSetApprovalStatus { + Approved, +} + +#[remain::sorted] +#[derive(Debug, Clone, Deserialize, Serialize)] +pub enum ChangeSetApprovalKind { + Func, + SchemaVariant, + View, +} diff --git a/lib/si-events-rs/src/lib.rs b/lib/si-events-rs/src/lib.rs index dbd5629335..f5191ea211 100644 --- a/lib/si-events-rs/src/lib.rs +++ b/lib/si-events-rs/src/lib.rs @@ -10,6 +10,7 @@ pub use si_id::ulid; mod actor; mod cas; +mod change_set_approval; mod change_set_status; mod event_session; mod func; @@ -30,6 +31,8 @@ pub use crate::{ actor::Actor, actor::UserPk, cas::CasValue, + change_set_approval::ChangesChecksum, + change_set_approval::{ChangeSetApprovalKind, ChangeSetApprovalStatus}, change_set_status::ChangeSetStatus, content_hash::ContentHash, encrypted_secret::EncryptedSecretKey, diff --git a/lib/si-frontend-types-rs/src/change_set.rs b/lib/si-frontend-types-rs/src/change_set.rs index 606f84c8ab..c7c5bdaf7b 100644 --- a/lib/si-frontend-types-rs/src/change_set.rs +++ b/lib/si-frontend-types-rs/src/change_set.rs @@ -1,6 +1,9 @@ use chrono::{DateTime, Utc}; use serde::{Deserialize, Serialize}; -use si_events::{ChangeSetId, ChangeSetStatus}; +use si_events::{ + ulid::Ulid, ChangeSetApprovalKind, ChangeSetApprovalStatus, ChangeSetId, ChangeSetStatus, + UserPk, +}; #[derive(Clone, Debug, Deserialize, Serialize, Eq, PartialEq)] #[serde(rename_all = "camelCase")] @@ -19,3 +22,33 @@ pub struct ChangeSet { pub reviewed_by_user: Option, pub reviewed_at: Option>, } + +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct ChangeSetApprovals { + pub required: Vec, + pub current: Vec, +} + +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct ChangeSetRequiredApproval { + // What is the kind of the entity corresponding to the ID? + kind: ChangeSetApprovalKind, + // What is the ID of the entity that is requiring approvals? + id: Ulid, + // What is the minimum number needed? + number: usize, + // Is it satisfied? + is_satisfied: bool, + // Who can satisfy this? + users: Vec, +} + +#[derive(Debug, Clone, Deserialize, Serialize)] +pub struct ChangeSetApproval { + // Who approved this? + pub user_id: UserPk, + // What kind of approval did they do (including negative)? + pub status: ChangeSetApprovalStatus, + // Is this still valid? + pub is_valid: bool, +} diff --git a/lib/si-frontend-types-rs/src/lib.rs b/lib/si-frontend-types-rs/src/lib.rs index eaeeb6e6a7..d8e97f2ea2 100644 --- a/lib/si-frontend-types-rs/src/lib.rs +++ b/lib/si-frontend-types-rs/src/lib.rs @@ -9,6 +9,7 @@ mod workspace; pub use crate::audit_log::AuditLog; pub use crate::change_set::ChangeSet; +pub use crate::change_set::{ChangeSetApproval, ChangeSetApprovals, ChangeSetRequiredApproval}; pub use crate::component::{ ChangeStatus, ConnectionAnnotation, DiagramComponentView, DiagramSocket, DiagramSocketDirection, DiagramSocketNodeSide, GeometryAndView, GridPoint, RawGeometry, Size2D, diff --git a/lib/si-id/src/lib.rs b/lib/si-id/src/lib.rs index fefb0a7bc8..e52a58c005 100644 --- a/lib/si-id/src/lib.rs +++ b/lib/si-id/src/lib.rs @@ -66,6 +66,7 @@ id!(WorkspaceSnapshotNodeId); id_with_pg_types!(ActionId); id_with_pg_types!(CachedModuleId); id_with_pg_types!(ChangeSetId); +id_with_pg_types!(ChangeSetApprovalId); id_with_pg_types!(ComponentId); id_with_pg_types!(FuncId); id_with_pg_types!(FuncRunId);