diff --git a/graph/src/components/store/err.rs b/graph/src/components/store/err.rs index cbf500884df..dcf92f6f461 100644 --- a/graph/src/components/store/err.rs +++ b/graph/src/components/store/err.rs @@ -56,6 +56,8 @@ pub enum StoreError { ForkFailure(String), #[error("subgraph writer poisoned by previous error")] Poisoned, + #[error("subgraph not found: {0}")] + SubgraphNotFound(String), #[error("panic in subgraph writer: {0}")] WriterPanic(JoinError), #[error( @@ -119,6 +121,7 @@ impl Clone for StoreError { Self::DatabaseUnavailable => Self::DatabaseUnavailable, Self::ForkFailure(arg0) => Self::ForkFailure(arg0.clone()), Self::Poisoned => Self::Poisoned, + Self::SubgraphNotFound(arg0) => Self::SubgraphNotFound(arg0.clone()), Self::WriterPanic(arg0) => Self::Unknown(anyhow!("writer panic: {}", arg0)), Self::UnsupportedDeploymentSchemaVersion(arg0) => { Self::UnsupportedDeploymentSchemaVersion(*arg0) @@ -202,6 +205,7 @@ impl StoreError { | Canceled | DatabaseUnavailable | ForkFailure(_) + | SubgraphNotFound(_) | Poisoned | WriterPanic(_) | UnsupportedDeploymentSchemaVersion(_) diff --git a/node/src/bin/manager.rs b/node/src/bin/manager.rs index 792df8853c9..8894f18ff56 100644 --- a/node/src/bin/manager.rs +++ b/node/src/bin/manager.rs @@ -165,7 +165,7 @@ pub enum Command { /// Remove a named subgraph Remove { /// The name of the subgraph to remove - name: String, + name: DeploymentSearch, }, /// Create a subgraph name Create { @@ -1745,7 +1745,7 @@ fn make_deployment_selector( use graphman::deployment::DeploymentSelector::*; match deployment { - DeploymentSearch::Name { name } => Name(name), + DeploymentSearch::Name { name } => Name(name.to_string()), DeploymentSearch::Hash { hash, shard } => Subgraph { hash, shard }, DeploymentSearch::All => All, DeploymentSearch::Deployment { namespace } => Schema(namespace), diff --git a/node/src/manager/commands/remove.rs b/node/src/manager/commands/remove.rs index bcf9417569a..4311e824cbb 100644 --- a/node/src/manager/commands/remove.rs +++ b/node/src/manager/commands/remove.rs @@ -3,11 +3,24 @@ use std::sync::Arc; use graph::prelude::{anyhow, Error, SubgraphName, SubgraphStore as _}; use graph_store_postgres::SubgraphStore; -pub async fn run(store: Arc, name: &str) -> Result<(), Error> { - let name = SubgraphName::new(name).map_err(|name| anyhow!("illegal subgraph name `{name}`"))?; +use crate::manager::deployment::DeploymentSearch; - println!("Removing subgraph {}", name); - store.remove_subgraph(name).await?; +pub async fn run(store: Arc, name: &DeploymentSearch) -> Result<(), Error> { + match name { + DeploymentSearch::Name { name } => { + let subgraph_name = SubgraphName::new(name) + .map_err(|name| anyhow!("illegal subgraph name `{name}`"))?; + println!("Removing subgraph {}", name); + store.remove_subgraph(subgraph_name).await?; + println!("Subgraph {} removed", name); + } + _ => { + return Err(anyhow!( + "Remove command expects a subgraph name, but got either hash or namespace: {}", + name + )) + } + } Ok(()) } diff --git a/server/graphman/src/resolvers/deployment_mutation/remove.rs b/server/graphman/src/resolvers/deployment_mutation/remove.rs index c7997b6885f..ba1b50a2adb 100644 --- a/server/graphman/src/resolvers/deployment_mutation/remove.rs +++ b/server/graphman/src/resolvers/deployment_mutation/remove.rs @@ -24,7 +24,10 @@ pub async fn run(ctx: &GraphmanContext, name: &String) -> Result<()> { } }; - let changes = catalog_conn.remove_subgraph(name).await?; + let changes = catalog_conn + .remove_subgraph(name) + .await + .map_err(GraphmanError::from)?; catalog_conn .send_store_event(&ctx.notification_sender, &StoreEvent::new(changes)) .await?; diff --git a/server/graphman/tests/deployment_mutation.rs b/server/graphman/tests/deployment_mutation.rs index fd2020ee740..e4d89633976 100644 --- a/server/graphman/tests/deployment_mutation.rs +++ b/server/graphman/tests/deployment_mutation.rs @@ -6,8 +6,8 @@ use graph::components::store::SubgraphStore; use graph::prelude::DeploymentHash; use serde::Deserialize; use serde_json::json; -use test_store::create_test_subgraph; use test_store::SUBGRAPH_STORE; +use test_store::{create_subgraph_name, create_test_subgraph}; use tokio::time::sleep; use self::util::client::send_graphql_request; @@ -358,6 +358,8 @@ fn graphql_cannot_create_new_subgraph_with_invalid_name() { #[test] fn graphql_can_remove_subgraph() { run_test(|| async { + create_subgraph_name("subgraph_1").await.unwrap(); + let resp = send_graphql_request( json!({ "query": r#"mutation RemoveSubgraph { @@ -403,17 +405,44 @@ fn graphql_cannot_remove_subgraph_with_invalid_name() { ) .await; - let success_resp = json!({ - "data": { - "deployment": { - "remove": { - "success": true, + let data = &resp["data"]["deployment"]; + let errors = resp["errors"].as_array().unwrap(); + + assert!(data.is_null()); + assert_eq!(errors.len(), 1); + assert_eq!( + errors[0]["message"].as_str().unwrap(), + "store error: Subgraph name must contain only a-z, A-Z, 0-9, '-' and '_'" + ); + }); +} + +#[test] +fn graphql_remove_returns_error_for_non_existing_subgraph() { + run_test(|| async { + let resp = send_graphql_request( + json!({ + "query": r#"mutation RemoveNonExistingSubgraph { + deployment { + remove(name: "non_existing_subgraph") { + success + } } - } - } - }); + }"# + }), + VALID_TOKEN, + ) + .await; - assert_ne!(resp, success_resp); + let data = &resp["data"]["deployment"]; + let errors = resp["errors"].as_array().unwrap(); + + assert!(data.is_null()); + assert_eq!(errors.len(), 1); + assert_eq!( + errors[0]["message"].as_str().unwrap(), + "store error: subgraph not found: non_existing_subgraph" + ); }); } diff --git a/store/postgres/src/primary.rs b/store/postgres/src/primary.rs index d7f506ff024..21cbef3b9a1 100644 --- a/store/postgres/src/primary.rs +++ b/store/postgres/src/primary.rs @@ -1124,7 +1124,7 @@ impl Connection { .await?; self.remove_unused_assignments().await } else { - Ok(vec![]) + Err(StoreError::SubgraphNotFound(name.to_string())) } } diff --git a/store/test-store/src/store.rs b/store/test-store/src/store.rs index af973c32993..b2a1ebb2744 100644 --- a/store/test-store/src/store.rs +++ b/store/test-store/src/store.rs @@ -259,9 +259,19 @@ pub async fn create_test_subgraph_with_features( locator } +pub async fn create_subgraph_name(name: &str) -> Result<(), StoreError> { + let subgraph_name = SubgraphName::new_unchecked(name.to_string()); + SUBGRAPH_STORE.create_subgraph(subgraph_name).await?; + Ok(()) +} + pub async fn remove_subgraph(id: &DeploymentHash) { let name = SubgraphName::new_unchecked(id.to_string()); - SUBGRAPH_STORE.remove_subgraph(name).await.unwrap(); + // Ignore SubgraphNotFound errors during cleanup + match SUBGRAPH_STORE.remove_subgraph(name).await { + Ok(_) | Err(StoreError::SubgraphNotFound(_)) => {} + Err(e) => panic!("unexpected error removing subgraph: {}", e), + } let locs = SUBGRAPH_STORE.locators(id.as_str()).await.unwrap(); let mut conn = primary_connection().await; for loc in locs { diff --git a/store/test-store/tests/postgres/subgraph.rs b/store/test-store/tests/postgres/subgraph.rs index 23b60ecc52c..5fa9b8c89ba 100644 --- a/store/test-store/tests/postgres/subgraph.rs +++ b/store/test-store/tests/postgres/subgraph.rs @@ -1232,3 +1232,18 @@ fn fail_unfail_non_deterministic_error_noop() { test_store::remove_subgraphs().await; }) } + +#[test] +fn remove_nonexistent_subgraph_returns_not_found() { + test_store::run_test_sequentially(|store| async move { + remove_subgraphs().await; + + let name = SubgraphName::new("nonexistent/subgraph").unwrap(); + let result = store.subgraph_store().remove_subgraph(name.clone()).await; + + assert!(matches!( + result, + Err(StoreError::SubgraphNotFound(n)) if n == name.to_string() + )); + }) +} diff --git a/tests/src/fixture/mod.rs b/tests/src/fixture/mod.rs index 62000dc5e8e..da89ced9fa0 100644 --- a/tests/src/fixture/mod.rs +++ b/tests/src/fixture/mod.rs @@ -626,7 +626,12 @@ pub async fn cleanup( hash: &DeploymentHash, ) -> Result<(), Error> { let locators = subgraph_store.locators(hash).await?; - subgraph_store.remove_subgraph(name.clone()).await?; + // Remove subgraph if it exists, ignore not found errors + match subgraph_store.remove_subgraph(name.clone()).await { + Ok(_) | Err(graph::prelude::StoreError::SubgraphNotFound(_)) => {} + Err(e) => return Err(e.into()), + } + for locator in locators { subgraph_store.remove_deployment(locator.id.into()).await?; }