Revert "Improve graphman remove feedback (#6281)" (#6350)

This reverts commit 8c87348.

Author: incrypto32

graph/src/components/store/err.rs

@@ -56,8 +56,6 @@ 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(
@@ -121,7 +119,6 @@ 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)
@@ -205,7 +202,6 @@ 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: DeploymentSearch,
+        name: String,
     },
     /// Create a subgraph name
     Create {
@@ -1745,7 +1745,7 @@ fn make_deployment_selector(
     use graphman::deployment::DeploymentSelector::*;
 
     match deployment {
-        DeploymentSearch::Name { name } => Name(name.to_string()),
+        DeploymentSearch::Name { name } => Name(name),
         DeploymentSearch::Hash { hash, shard } => Subgraph { hash, shard },
         DeploymentSearch::All => All,
         DeploymentSearch::Deployment { namespace } => Schema(namespace),

node/src/manager/commands/remove.rs

@@ -3,24 +3,11 @@ use std::sync::Arc;
 use graph::prelude::{anyhow, Error, SubgraphName, SubgraphStore as _};
 use graph_store_postgres::SubgraphStore;
 
-use crate::manager::deployment::DeploymentSearch;
+pub async fn run(store: Arc<SubgraphStore>, name: &str) -> Result<(), Error> {
+    let name = SubgraphName::new(name).map_err(|name| anyhow!("illegal subgraph name `{name}`"))?;
 
-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
-            ))
-        }
-    }
+    println!("Removing subgraph {}", name);
+    store.remove_subgraph(name).await?;
 
     Ok(())
 }

server/graphman/src/resolvers/deployment_mutation/remove.rs

@@ -24,10 +24,7 @@ pub async fn run(ctx: &GraphmanContext, name: &String) -> Result<()> {
         }
     };
 
-    let changes = catalog_conn
-        .remove_subgraph(name)
-        .await
-        .map_err(GraphmanError::from)?;
+    let changes = catalog_conn.remove_subgraph(name).await?;
     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,8 +358,6 @@ 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 {
@@ -405,44 +403,17 @@ fn graphql_cannot_remove_subgraph_with_invalid_name() {
         )
         .await;
 
-        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
-                        }
+        let success_resp = json!({
+            "data": {
+                "deployment": {
+                    "remove": {
+                        "success": true,
                     }
-                }"#
-            }),
-            VALID_TOKEN,
-        )
-        .await;
-
-        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"
-        );
+        assert_ne!(resp, success_resp);
     });
 }
 

store/postgres/src/primary.rs

@@ -1124,7 +1124,7 @@ impl Connection {
                 .await?;
             self.remove_unused_assignments().await
         } else {
-            Err(StoreError::SubgraphNotFound(name.to_string()))
+            Ok(vec![])
         }
     }
 

store/test-store/src/store.rs

@@ -260,19 +260,9 @@ 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());
-    // Ignore SubgraphNotFound errors during cleanup
-    match SUBGRAPH_STORE.remove_subgraph(name).await {
-        Ok(_) | Err(StoreError::SubgraphNotFound(_)) => {}
-        Err(e) => panic!("unexpected error removing subgraph: {}", e),
-    }
+    SUBGRAPH_STORE.remove_subgraph(name).await.unwrap();
     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,18 +1232,3 @@ 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,12 +627,7 @@ pub async fn cleanup(
     hash: &DeploymentHash,
 ) -> Result<(), Error> {
     let locators = subgraph_store.locators(hash).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()),
-    }
-
+    subgraph_store.remove_subgraph(name.clone()).await?;
     for locator in locators {
         subgraph_store.remove_deployment(locator.id.into()).await?;
     }