Improve graphman remove feedback

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(_)

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),

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<SubgraphStore>, 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<SubgraphStore>, 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(())
 }

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?;

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"
+        );
     });
 }
 

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()))
         }
     }
 

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 {

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()
+        ));
+    })
+}

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?;
     }