fix(salesforce-api): #3938 RestDeploy raises on deploy failure instead of silently succeeding

cumulusci/salesforce_api/rest_deploy.py

@@ -9,6 +9,11 @@
 
 import requests
 
+from cumulusci.salesforce_api.exceptions import (
+    MetadataApiError,
+    MetadataComponentFailure,
+)
+
 PARENT_DIR_NAME = "metadata"
 
 
@@ -73,16 +78,18 @@ def __call__(self):
         body += f"\r\n--{self._boundary}--\r\n".encode("utf-8")
 
         response = requests.post(url, headers=headers, data=body)
-        response_json = response.json()
 
         if response.status_code == 201:
             self.task.logger.info("Deployment request successful")
-            deploy_request_id = response_json["id"]
+            deploy_request_id = response.json()["id"]
             self._monitor_deploy_status(deploy_request_id)
         else:
-            self.task.logger.error(
-                f"Deployment request failed with status code {response.status_code}"
+            message = (
+                f"Deployment request failed with status code {response.status_code}: "
+                f"{response.text}"
             )
+            self.task.logger.error(message)
+            raise MetadataApiError(message, response)
 
     # Set the purge_on_delete attribute based on org type
     def _set_purge_on_delete(self, purge_on_delete):
@@ -105,17 +112,30 @@ def _monitor_deploy_status(self, deploy_request_id):
         while True:
             response = requests.get(url, headers=headers)
             response_json = response.json()
-            self.task.logger.info(
-                f"Deployment {response_json['deployResult']['status']}"
-            )
-
-            if response_json["deployResult"]["status"] not in ["InProgress", "Pending"]:
-                # Handle the case when status has Failed
-                if response_json["deployResult"]["status"] == "Failed":
-                    for failure in response_json["deployResult"]["details"][
-                        "componentFailures"
-                    ]:
-                        self.task.logger.error(self._construct_error_message(failure))
+            status = response_json["deployResult"]["status"]
+            self.task.logger.info(f"Deployment {status}")
+
+            if status not in ["InProgress", "Pending"]:
+                if status == "Failed":
+                    component_failures = (
+                        response_json["deployResult"]
+                        .get("details", {})
+                        .get("componentFailures", [])
+                    )
+                    messages = []
+                    for failure in component_failures:
+                        message = self._construct_error_message(failure)
+                        self.task.logger.error(message)
+                        messages.append(message)
+                    # Mirror SOAP ApiDeploy._process_response: componentFailures
+                    # surface as MetadataComponentFailure; otherwise raise the
+                    # generic MetadataApiError. Either way, the deploy must
+                    # fail loudly so downstream callers (and `cci task run`)
+                    # see a non-zero exit instead of a silent success.
+                    log = "\n\n".join(messages) if messages else f"Deployment {status}"
+                    if component_failures:
+                        raise MetadataComponentFailure(log, response)
+                    raise MetadataApiError(log, response)
                 return
             time.sleep(5)
 

cumulusci/salesforce_api/tests/test_rest_deploy.py

@@ -4,6 +4,12 @@
 import zipfile
 from unittest.mock import MagicMock, Mock, call, patch
 
+import pytest
+
+from cumulusci.salesforce_api.exceptions import (
+    MetadataApiError,
+    MetadataComponentFailure,
+)
 from cumulusci.salesforce_api.rest_deploy import RestDeploy
 from cumulusci.tests.util import CURRENT_SF_API_VERSION
 
@@ -83,19 +89,25 @@ def test_deployment_success(self, mock_get, mock_post):
     @patch("requests.post")
     def test_deployment_failure(self, mock_post):
 
-        response_post = Mock(status_code=500)
+        response_post = Mock(status_code=500, text="Internal Server Error")
         response_post.json.return_value = {"id": "dummy_id"}
         mock_post.return_value = response_post
 
         deployer = RestDeploy(
             self.mock_task, self.mock_zip, False, False, "NoTestRun", []
         )
-        deployer()
-
-        # Assertions to verify log messages
-        assert (
-            call("Deployment request failed with status code 500")
-            in self.mock_logger.error.call_args_list
+        with pytest.raises(MetadataApiError) as exc_info:
+            deployer()
+
+        # The error message must include the status code and the response body
+        # so operators can see what Salesforce returned.
+        assert "500" in str(exc_info.value)
+        # The logger should still emit the error before the raise so log
+        # output stays informative.
+        assert any(
+            "500" in args[0]
+            for args, _ in self.mock_logger.error.call_args_list
+            if args
         )
 
         # Assertions to verify API Calls
@@ -142,7 +154,11 @@ def test_deployStatus_failure(self, mock_get, mock_post):
         deployer = RestDeploy(
             self.mock_task, self.mock_zip, False, False, "NoTestRun", []
         )
-        deployer()
+        # Failed REST deploys must surface as MetadataComponentFailure (a
+        # subclass of MetadataApiError) to mirror the SOAP ApiDeploy path so
+        # downstream callers see a non-zero result. See #3938.
+        with pytest.raises(MetadataComponentFailure):
+            deployer()
 
         # Assertions to verify log messages
         assert (
@@ -267,5 +283,118 @@ def test_purge_on_delete(self):
                 self.assertEqual(deployer.purge_on_delete, expected_result)
 
 
+class TestRestDeployRaisesOnFailure(unittest.TestCase):
+    """Regression tests for SFDO-Tooling/CumulusCI#3938.
+
+    The REST deploy path silently returned success when Salesforce reported a
+    deployment failure (either an initial-POST error or a polled "Failed"
+    status). It should raise MetadataApiError/MetadataComponentFailure to
+    mirror the SOAP path's behaviour so downstream callers see the failure.
+    """
+
+    def setUp(self):
+        self.mock_logger = Mock()
+        self.mock_task = MagicMock()
+        self.mock_task.logger = self.mock_logger
+        self.mock_task.org_config.instance_url = "https://example.com"
+        self.mock_task.org_config.access_token = "dummy_token"
+        self.mock_task.project_config.project__package__api_version = (
+            CURRENT_SF_API_VERSION
+        )
+        self.mock_zip = generate_sample_zip_data()
+
+    @patch("requests.post")
+    @patch("requests.get")
+    def test_rest_deploy_raises_on_failure(self, mock_get, mock_post):
+        response_post = Mock(status_code=201)
+        response_post.json.return_value = {"id": "dummy_id"}
+        mock_post.return_value = response_post
+
+        response_get = Mock(status_code=200)
+        response_get.json.side_effect = [
+            {"deployResult": {"status": "InProgress"}},
+            {
+                "deployResult": {
+                    "status": "Failed",
+                    "details": {
+                        "componentFailures": [
+                            {
+                                "problemType": "Error",
+                                "fileName": "metadata/classes/mockfile1.cls",
+                                "problem": "someproblem1",
+                                "lineNumber": 1,
+                                "columnNumber": 1,
+                            }
+                        ]
+                    },
+                }
+            },
+        ]
+        mock_get.return_value = response_get
+
+        deployer = RestDeploy(
+            self.mock_task, self.mock_zip, False, False, "NoTestRun", []
+        )
+
+        with pytest.raises(MetadataApiError) as exc_info:
+            deployer()
+
+        assert "someproblem1" in str(exc_info.value)
+        assert "mockfile1.cls" in str(exc_info.value)
+
+    @patch("requests.post")
+    @patch("requests.get")
+    def test_rest_deploy_raises_component_failure_subclass(self, mock_get, mock_post):
+        response_post = Mock(status_code=201)
+        response_post.json.return_value = {"id": "dummy_id"}
+        mock_post.return_value = response_post
+
+        response_get = Mock(status_code=200)
+        response_get.json.side_effect = [
+            {
+                "deployResult": {
+                    "status": "Failed",
+                    "details": {
+                        "componentFailures": [
+                            {
+                                "problemType": "Error",
+                                "fileName": "metadata/objects/mockfile2.obj",
+                                "problem": "someproblem2",
+                                "lineNumber": 2,
+                                "columnNumber": 2,
+                            }
+                        ]
+                    },
+                }
+            },
+        ]
+        mock_get.return_value = response_get
+
+        deployer = RestDeploy(
+            self.mock_task, self.mock_zip, False, False, "NoTestRun", []
+        )
+
+        with pytest.raises(MetadataComponentFailure):
+            deployer()
+
+    @patch("requests.post")
+    def test_rest_deploy_raises_on_initial_post_failure(self, mock_post):
+        response_post = Mock(status_code=500, text="Internal Server Error")
+        response_post.json.return_value = {
+            "message": "boom",
+            "errorCode": "INTERNAL_ERROR",
+        }
+        mock_post.return_value = response_post
+
+        deployer = RestDeploy(
+            self.mock_task, self.mock_zip, False, False, "NoTestRun", []
+        )
+
+        with pytest.raises(MetadataApiError) as exc_info:
+            deployer()
+
+        assert "500" in str(exc_info.value) or "boom" in str(exc_info.value)
+
+
 if __name__ == "__main__":
     unittest.main()