Improve graphman remove feedback (#6281)

* graph, store: return SubgraphNotFound error when removing non-existent subgraph

Previously, remove_subgraph returned Ok(vecsilently when the
subgraph didn't exist. Now it returns a proper StoreError::SubgraphNotFound
error with the subgraph name, giving users clear feedback.

Signed-off-by: Maksim Dimitrov <[email protected]>

* node: validate remove command accepts only subgraph names

Change the remove command to accept DeploymentSearch and validate
that only subgraph names are accepted. Passing an IPFS hash or schema
namespace now returns a clear error message instead of being silently
treated as a name.

Signed-off-by: Maksim Dimitrov <[email protected]>

* server: propagate errors from remove_subgraph in graphman GraphQL resolver

Add missing .map_err(GraphmanError::from) to properly convert
store errors (including the new SubgraphNotFound) to GraphQL errors.

Signed-off-by: Maksim Dimitrov <[email protected]>

* test: verify SubgraphNotFound error for non-existent subgraph removal

Signed-off-by: Maksim Dimitrov <[email protected]>

* test: fix graphql_can_remove_subgraph test

The test was silently passing because remove_subgraph returned Ok(vecfor non-existent subgraphs. Now that SubgraphNotFound error is properly
returned, the test needs to actually create the subgraph first.

Signed-off-by: Maksim Dimitrov <[email protected]>

* test(integration): Ignore SubgraphNotFound errors when removing subgraphs that may not exist

Signed-off-by: Maksim Dimitrov <[email protected]>

* test(unit): Ignore SubgraphNotFound for missing subgraphs in cleanup

Signed-off-by: Maksim Dimitrov <[email protected]>

---------

Signed-off-by: Maksim Dimitrov <[email protected]>

Author: dimitrovmaksim

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

@@ -260,9 +260,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

@@ -627,7 +627,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?;
     }