@W-19854606: Strict Mode & Raise validation errors for missing schema and permissions (#3937)

Author: aditya-balachander

cumulusci/tasks/bulkdata/generate_from_yaml.py

@@ -95,6 +95,9 @@ def _init_options(self, kwargs):
         self.working_directory = self.options.get("working_directory")
         loading_rules = process_list_arg(self.options.get("loading_rules")) or []
         self.loading_rules = [Path(path) for path in loading_rules if path]
+        # These flags may be provided internally by callers (e.g., Snowfakery task)
+        self.strict_mode = bool(self.options.get("strict_mode", False))
+        self.validate_only = bool(self.options.get("validate_only", False))
 
     def _generate_data(self, db_url, mapping_file_path, num_records, current_batch_num):
         """Generate all of the data"""
@@ -171,6 +174,7 @@ def generate_data(self, dburl, num_records, current_batch_num):
                     "project_config": self.project_config,
                     **self.plugin_options,
                 },
+                strict_mode=self.strict_mode or self.validate_only,
             )
 
         if (

cumulusci/tasks/bulkdata/mapping_parser.py

@@ -444,9 +444,9 @@ def replace_if_necessary(dct, name, replacement):
             except KeyError:
                 message = f"Field {self.sf_object}.{f} does not exist or is not visible to the current user."
                 if validation_result:
-                    validation_result.add_warning(message)
+                    validation_result.add_error(message)
                 else:
-                    logger.warning(message)
+                    logger.error(message)
             else:
                 del field_dict[f]
                 field_dict[new_name] = entry
@@ -463,9 +463,9 @@ def replace_if_necessary(dct, name, replacement):
             if f not in describe:
                 message = f"Field {self.sf_object}.{f} does not exist or is not visible to the current user."
                 if validation_result:
-                    validation_result.add_warning(message)
+                    validation_result.add_error(message)
                 else:
-                    logger.warning(message)
+                    logger.error(message)
                 error_in_f = True
             elif not self._check_field_permission(
                 describe,
@@ -480,9 +480,9 @@ def replace_if_necessary(dct, name, replacement):
                     + f"{relevant_permissions} for this operation."
                 )
                 if validation_result:
-                    validation_result.add_warning(message)
+                    validation_result.add_error(message)
                 else:
-                    logger.warning(message)
+                    logger.error(message)
                 error_in_f = True
 
             if error_in_f:
@@ -514,9 +514,9 @@ def _validate_sobject(
         except KeyError:
             message = f"sObject {self.sf_object} does not exist or is not visible to the current user."
             if validation_result:
-                validation_result.add_warning(message)
+                validation_result.add_error(message)
             else:
-                logger.warning(message)
+                logger.error(message)
             return False
 
         # Validate our access to this sObject.
@@ -525,9 +525,9 @@ def _validate_sobject(
         ):
             message = f"sObject {self.sf_object} does not have the correct permissions for {data_operation_type}."
             if validation_result:
-                validation_result.add_warning(message)
+                validation_result.add_error(message)
             else:
-                logger.warning(message)
+                logger.error(message)
             return False
 
         return True

cumulusci/tasks/bulkdata/snowfakery.py

@@ -141,9 +141,12 @@ class Snowfakery(BaseSalesforceApiTask):
             "Defaults to False."
         },
         "validate_only": {
-            "description": "Boolean: if True, only validate the generated mapping against the org schema without loading data. "
+            "description": "Boolean: if True, validates snowfakery recipe and generated mapping against the org schema without loading data. "
             "Defaults to False."
         },
+        "strict_mode": {
+            "description": "Boolean: If True, validates the Snowfakery recipe and generated mapping against the org schema (strict mode) and then proceeds with the run",
+        },
     }
 
     def _validate_options(self):
@@ -165,6 +168,7 @@ def _validate_options(self):
             self.options.get("drop_missing_schema", False)
         )
         self.validate_only = process_bool_arg(self.options.get("validate_only", False))
+        self.strict_mode = process_bool_arg(self.options.get("strict_mode", False))
 
         loading_rules = process_list_arg(self.options.get("loading_rules")) or []
         self.loading_rules = [Path(path) for path in loading_rules if path]
@@ -614,6 +618,7 @@ def _run_generate_and_load_subtask(
             "ignore_row_errors": self.ignore_row_errors,
             "drop_missing_schema": self.drop_missing_schema,
             "validate_only": validate_only,
+            "strict_mode": self.strict_mode,
         }
         subtask_config = TaskConfig({"options": options})
         subtask = GenerateAndLoadDataFromYaml(

cumulusci/tasks/bulkdata/tests/test_generate_from_snowfakery_task.py

@@ -206,6 +206,102 @@ def test_exception_handled_cleanly(self, generate_data):
             assert "Foo" in str(e.value)
         assert len(generate_data.mock_calls) == 1
 
+    @mock.patch("cumulusci.tasks.bulkdata.generate_from_yaml.generate_data")
+    def test_validate_only_forces_strict_mode(self, generate_data):
+        with temp_sqlite_database_url() as database_url:
+            task = _make_task(
+                GenerateDataFromYaml,
+                {
+                    "options": {
+                        "generator_yaml": simple_yaml,
+                        "database_url": database_url,
+                        "validate_only": True,
+                    }
+                },
+            )
+            task()
+
+        assert len(generate_data.mock_calls) == 1
+        _, kwargs = generate_data.call_args
+        assert kwargs["strict_mode"] is True
+
+    @mock.patch("cumulusci.tasks.bulkdata.generate_from_yaml.generate_data")
+    def test_strict_mode_only(self, generate_data):
+        with temp_sqlite_database_url() as database_url:
+            task = _make_task(
+                GenerateDataFromYaml,
+                {
+                    "options": {
+                        "generator_yaml": simple_yaml,
+                        "database_url": database_url,
+                        "strict_mode": True,
+                    }
+                },
+            )
+            task()
+
+        assert len(generate_data.mock_calls) == 1
+        _, kwargs = generate_data.call_args
+        assert kwargs["strict_mode"] is True
+
+    @mock.patch("cumulusci.tasks.bulkdata.generate_from_yaml.generate_data")
+    def test_defaults_no_strict_no_validate(self, generate_data):
+        with temp_sqlite_database_url() as database_url:
+            task = _make_task(
+                GenerateDataFromYaml,
+                {
+                    "options": {
+                        "generator_yaml": simple_yaml,
+                        "database_url": database_url,
+                    }
+                },
+            )
+            task()
+
+        assert len(generate_data.mock_calls) == 1
+        _, kwargs = generate_data.call_args
+        assert kwargs["strict_mode"] is False
+
+    def test_validate_only_runs_and_creates_mapping(self):
+        with temporary_file_path("mapping.yml") as mapping_path:
+            with temp_sqlite_database_url() as database_url:
+                task = _make_task(
+                    GenerateDataFromYaml,
+                    {
+                        "options": {
+                            "generator_yaml": simple_yaml,
+                            "database_url": database_url,
+                            "generate_mapping_file": mapping_path,
+                            "validate_only": True,
+                        }
+                    },
+                )
+                task()
+
+            assert mapping_path.exists()
+            mapping = yaml.safe_load(open(mapping_path))
+            assert mapping  # ensure something was written
+
+    def test_strict_mode_runs_and_creates_mapping(self):
+        with temporary_file_path("mapping.yml") as mapping_path:
+            with temp_sqlite_database_url() as database_url:
+                task = _make_task(
+                    GenerateDataFromYaml,
+                    {
+                        "options": {
+                            "generator_yaml": simple_yaml,
+                            "database_url": database_url,
+                            "generate_mapping_file": mapping_path,
+                            "strict_mode": True,
+                        }
+                    },
+                )
+                task()
+
+            assert mapping_path.exists()
+            mapping = yaml.safe_load(open(mapping_path))
+            assert mapping  # ensure something was written
+
     @mock.patch(
         "cumulusci.tasks.bulkdata.generate_and_load_data_from_yaml.GenerateAndLoadDataFromYaml._dataload"
     )

cumulusci/tasks/bulkdata/tests/test_mapping_parser.py

@@ -1868,8 +1868,9 @@ def test_validate_only_collects_missing_field_errors(self):
 
         assert result is not None
         assert isinstance(result, ValidationResult)
-        # Should have warnings about missing field
-        assert any("Nonsense__c" in warning for warning in result.warnings)
+        assert result.has_errors()
+        # Should have errors about missing field
+        assert any("Nonsense__c" in error for error in result.errors)
 
     @responses.activate
     def test_validate_only_collects_missing_required_field_errors(self):
@@ -1930,8 +1931,9 @@ def test_validate_only_early_return_on_sobject_error(self):
 
         assert result is not None
         assert isinstance(result, ValidationResult)
-        # Should have warning about missing object
-        assert any("InvalidObject__c" in warning for warning in result.warnings)
+        assert result.has_errors()
+        # Should have error about missing object
+        assert any("InvalidObject__c" in error for error in result.errors)
 
     @responses.activate
     def test_validate_only_collects_lookup_errors(self):
@@ -2068,7 +2070,7 @@ def test_check_required_without_validation_result_logs(self, caplog):
         assert "Name" in caplog.text
 
     def test_validate_sobject_with_validation_result(self):
-        """Test _validate_sobject adds warnings to ValidationResult"""
+        """Test _validate_sobject adds errors to ValidationResult"""
         from cumulusci.tasks.bulkdata.mapping_parser import ValidationResult
 
         ms = MappingStep(
@@ -2087,13 +2089,11 @@ def test_validate_sobject_with_validation_result(self):
         )
 
         assert not result
-        assert len(validation_result.warnings) > 0
-        assert any(
-            "InvalidObject__c" in warning for warning in validation_result.warnings
-        )
+        assert validation_result.has_errors()
+        assert any("InvalidObject__c" in error for error in validation_result.errors)
 
     def test_validate_field_dict_with_validation_result(self):
-        """Test _validate_field_dict adds warnings to ValidationResult"""
+        """Test _validate_field_dict adds errors to ValidationResult"""
         from cumulusci.tasks.bulkdata.mapping_parser import ValidationResult
 
         ms = MappingStep(
@@ -2114,10 +2114,8 @@ def test_validate_field_dict_with_validation_result(self):
         )
 
         assert not result
-        assert len(validation_result.warnings) > 0
-        assert any(
-            "NonexistentField__c" in warning for warning in validation_result.warnings
-        )
+        assert validation_result.has_errors()
+        assert any("NonexistentField__c" in error for error in validation_result.errors)
 
     def test_infer_and_validate_lookups_with_validation_result(self):
         """Test _infer_and_validate_lookups adds errors to ValidationResult"""
@@ -2235,10 +2233,11 @@ def test_validate_field_dict_permission_error_with_validation_result(self):
         )
 
         assert not result
-        # Should have warning about incorrect permissions
+        assert validation_result.has_errors()
+        # Should have error about incorrect permissions
         assert any(
-            "does not have the correct permissions" in warning
-            for warning in validation_result.warnings
+            "does not have the correct permissions" in error
+            for error in validation_result.errors
         )
 
     def test_validate_sobject_permission_error_with_validation_result(self):
@@ -2261,10 +2260,11 @@ def test_validate_sobject_permission_error_with_validation_result(self):
         )
 
         assert not result
-        # Should have warning about incorrect permissions
+        assert validation_result.has_errors()
+        # Should have error about incorrect permissions
         assert any(
-            "does not have the correct permissions" in warning
-            for warning in validation_result.warnings
+            "does not have the correct permissions" in error
+            for error in validation_result.errors
         )
 
     def test_infer_and_validate_lookups_invalid_reference_with_validation_result(self):

cumulusci/tasks/bulkdata/tests/test_snowfakery.py

@@ -237,6 +237,59 @@ def _run_snowfakery_and_inspect_mapping(**options):
     return _run_snowfakery_and_inspect_mapping
 
 
+@mock.patch("cumulusci.tasks.bulkdata.snowfakery.GenerateAndLoadDataFromYaml")
+def test_snowfakery_validate_only_passes_flags(mock_subtask_cls, snowfakery):
+    mock_subtask = mock.Mock()
+    mock_subtask.__call__ = mock.Mock(return_value=None)
+    mock_subtask.return_values = {"validation_result": "ok"}
+    mock_subtask_cls.return_value = mock_subtask
+
+    task = snowfakery(
+        recipe=str(simple_salesforce_yaml),
+        validate_only=True,
+    )
+
+    with TemporaryDirectory() as tmpdir:
+        task._run_generate_and_load_subtask(
+            Path(tmpdir),
+            DummyOrgConfig({}, "test"),
+            options={},
+            validate_only=True,
+        )
+
+    # task_config passed into subtask should carry validate_only=True and strict_mode flag
+    call_kwargs = mock_subtask_cls.call_args.kwargs
+    task_config = call_kwargs["task_config"]
+    assert task_config.options["validate_only"] is True
+    assert task_config.options["strict_mode"] is False
+
+
+@mock.patch("cumulusci.tasks.bulkdata.snowfakery.GenerateAndLoadDataFromYaml")
+def test_snowfakery_strict_mode_passes_flags(mock_subtask_cls, snowfakery):
+    mock_subtask = mock.Mock()
+    mock_subtask.__call__ = mock.Mock(return_value=None)
+    mock_subtask.return_values = {"validation_result": "ok"}
+    mock_subtask_cls.return_value = mock_subtask
+
+    task = snowfakery(
+        recipe=str(simple_salesforce_yaml),
+        strict_mode=True,
+    )
+
+    with TemporaryDirectory() as tmpdir:
+        task._run_generate_and_load_subtask(
+            Path(tmpdir),
+            DummyOrgConfig({}, "test"),
+            options={},
+            validate_only=False,
+        )
+
+    call_kwargs = mock_subtask_cls.call_args.kwargs
+    task_config = call_kwargs["task_config"]
+    assert task_config.options["validate_only"] is False
+    assert task_config.options["strict_mode"] is True
+
+
 def get_mapping_from_snowfakery_task_results(results: SnowfakeryTaskResults):
     """Find the shared mapping file and return it."""
     template_dir = SnowfakeryWorkingDirectory(results.working_dir / "template_1/")

pyproject.toml

@@ -53,7 +53,7 @@ dependencies = [
     "sarge",
     "selenium<4",
     "simple-salesforce==1.11.4",
-    "snowfakery>=4.1.0",
+    "snowfakery>=4.2.0",
     "xmltodict",
     "docutils<=0.21.2",
 ]