diff --git a/cumulusci/salesforce_api/rest_deploy.py b/cumulusci/salesforce_api/rest_deploy.py index 9276f42db2..19412e5f0a 100644 --- a/cumulusci/salesforce_api/rest_deploy.py +++ b/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) diff --git a/cumulusci/salesforce_api/tests/test_rest_deploy.py b/cumulusci/salesforce_api/tests/test_rest_deploy.py index edd747ae8d..6f958d41c5 100644 --- a/cumulusci/salesforce_api/tests/test_rest_deploy.py +++ b/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()