Repository: great-expectations/great_expectations
Pull request #7021 (2023-02-01 16:09:24)

[BUGFIX] Patch logic error in new `add_or_update` methods
Changes proposed in this pull request:
- Previously, changes did not persist - this was an oversight during the initial implementation.
- Additional tests to ensure this now works.


### Definition of Done
- [x] My code follows the Great Expectations [style guide](https://docs.greatexpectations.io/docs/contributing/style_guides/code_style)
- [x] I have performed a [self-review](https://docs.greatexpectations.io/docs/contributing/contributing_checklist) of my own code
- [x] I have commented my code, particularly in hard-to-understand areas
- [x] I have added [unit tests](https://docs.greatexpectations.io/docs/contributing/contributing_test#writing-unit-and-integration-tests) where applicable and made sure that new and existing tests are passing.
- [x] I have run any local integration tests and made sure that nothing is broken.


netlify[bot]: ### <span aria-hidden="true">👷</span> Deploy Preview for *niobium-lead-7998* processing.


|  Name | Link |
|---------------------------------|------------------------|
|<span aria-hidden="true">🔨</span> Latest commit | f97ed08d097c6f57e69ffd71e0f05db3e42d4897 |
|<span aria-hidden="true">🔍</span> Latest deploy log | https://app.netlify.com/sites/niobium-lead-7998/deploys/63da8eb5d25a6500086b569a |
codesee-maps[bot]: <details>
<summary><b>👇 <a href="https://app.codesee.io/r/reviews?pr=7021&src=https%3A%2F%2Fgithub.com%2Fgreat-expectations%2Fgreat_expectations">Click on the image</a> for a new way to code review</b>
</summary>

  - Make big changes easier — review code in small groups of related files
  - Know where to start — see the whole change at a glance
  - Take a code tour — explore the change with an interactive tour
  - Make comments and review — all fully sync’ed with github

    [Try it now!](https://app.codesee.io/r/reviews?pr=7021&src=https%3A%2F%2Fgithub.com%2Fgreat-expectations%2Fgreat_expectations)
</details>

[![Review these changes using an interactive CodeSee Map](https://s3.us-east-2.amazonaws.com/maps.codesee.io/images/github/great-expectations/great_expectations/7021/06d57516/f97ed08d097c6f57e69ffd71e0f05db3e42d4897.svg)](https://app.codesee.io/r/reviews?pr=7021&src=https%3A%2F%2Fgithub.com%2Fgreat-expectations%2Fgreat_expectations)

#### Legend
![CodeSee Map Legend](https://app.codesee.io/Legend_v2.png)

Patch:
diff --git a/great_expectations/data_context/data_context/abstract_data_context.py b/great_expectations/data_context/data_context/abstract_data_context.py
index 14d80ea99..ea3459b99 100644
--- a/great_expectations/data_context/data_context/abstract_data_context.py
+++ b/great_expectations/data_context/data_context/abstract_data_context.py
@@ -124,6 +124,9 @@ from great_expectations.rule_based_profiler.config.base import (
 from great_expectations.rule_based_profiler.data_assistant.data_assistant_dispatcher import (
     DataAssistantDispatcher,
 )
+from great_expectations.rule_based_profiler.helpers.util import (
+    convert_variables_to_dict,
+)
 from great_expectations.rule_based_profiler.rule_based_profiler import RuleBasedProfiler
 from great_expectations.util import load_class, verify_dynamic_loading_support
 from great_expectations.validator.validator import BridgeValidator, Validator
@@ -2300,10 +2303,22 @@ class AbstractDataContext(ConfigPeer, ABC):
             )
 
         if existing_suite:
-            self.update_expectation_suite(existing_suite)
-            return self.get_expectation_suite(
-                expectation_suite_name=expectation_suite_name, ge_cloud_id=id
+            self.delete_expectation_suite(
+                expectation_suite_name=existing_suite.name,
+                ge_cloud_id=existing_suite.ge_cloud_id,
             )
+            if id is None:
+                id = existing_suite.ge_cloud_id
+            if expectations is None:
+                expectations = existing_suite.expectations
+            if evaluation_parameters is None:
+                evaluation_parameters = existing_suite.evaluation_parameters
+            if data_asset_type is None:
+                data_asset_type = existing_suite.data_asset_type
+            if execution_engine_type is None:
+                execution_engine_type = existing_suite.execution_engine_type
+            if meta is None:
+                meta = existing_suite.meta
 
         return self.add_expectation_suite(
             expectation_suite_name=expectation_suite_name,
@@ -2496,10 +2511,10 @@ class AbstractDataContext(ConfigPeer, ABC):
     def add_or_update_profiler(
         self,
         name: str,
+        id: str | None = None,
         config_version: float | None = None,
         rules: dict[str, dict] | None = None,
         variables: dict | None = None,
-        id: str | None = None,
     ) -> RuleBasedProfiler:
         """Add a new Profiler or update an existing one on the context depending on whether it already exists or not.
 
@@ -2522,10 +2537,17 @@ class AbstractDataContext(ConfigPeer, ABC):
             )
 
         if existing_profiler:
-            self.update_profiler(existing_profiler)
-            return self.get_profiler(
+            self.delete_profiler(
                 name=existing_profiler.name, ge_cloud_id=existing_profiler.ge_cloud_id
             )
+            if id is None:
+                id = existing_profiler.ge_cloud_id
+            if config_version is None:
+                config_version = existing_profiler.config_version
+            if rules is None:
+                rules = {rule.name: rule.to_dict() for rule in existing_profiler.rules}
+            if variables is None:
+                variables = convert_variables_to_dict(existing_profiler.variables)
 
         config_version = config_version or 1.0
         rules = rules or {}


Test patch:
diff --git a/tests/data_context/test_data_context_state_management.py b/tests/data_context/test_data_context_state_management.py
index 252a06467..fee5ecbb6 100644
--- a/tests/data_context/test_data_context_state_management.py
+++ b/tests/data_context/test_data_context_state_management.py
@@ -381,24 +381,48 @@ def test_add_or_update_expectation_suite_adds_successfully(
 
 
 @pytest.mark.unit
+@pytest.mark.parametrize(
+    "new_expectations",
+    [
+        pytest.param(
+            [],
+            id="empty expectations",
+        ),
+        pytest.param(
+            [
+                ExpectationConfiguration(
+                    expectation_type="expect_column_to_exist",
+                    kwargs={"column": "pickup_datetime"},
+                )
+            ],
+            id="new expectations",
+        ),
+    ],
+)
 def test_add_or_update_expectation_suite_updates_successfully(
     in_memory_data_context: EphemeralDataContextSpy,
+    new_expectations: list[ExpectationConfiguration],
 ):
     context = in_memory_data_context
 
     suite_name = "default"
-    suite = context.add_expectation_suite(suite_name)
+    suite = context.add_expectation_suite(
+        expectation_suite_name=suite_name,
+        expectations=[
+            ExpectationConfiguration(
+                expectation_type="expect_column_values_to_be_in_set",
+                kwargs={"column": "x", "value_set": [1, 2, 4]},
+            ),
+        ],
+    )
 
     assert context.expectations_store.save_count == 1
 
-    suite.expectations = [
-        ExpectationConfiguration(
-            expectation_type="expect_column_values_to_be_in_set",
-            kwargs={"column": "x", "value_set": [1, 2, 4]},
-        ),
-    ]
-    _ = context.add_or_update_expectation_suite(expectation_suite_name=suite_name)
+    suite = context.add_or_update_expectation_suite(
+        expectation_suite_name=suite_name, expectations=new_expectations
+    )
 
+    assert suite.expectations == new_expectations
     assert context.expectations_store.save_count == 2
 
 
@@ -465,9 +489,45 @@ def test_add_or_update_profiler_adds_successfully(
 
 
 @pytest.mark.unit
+@pytest.mark.parametrize(
+    "new_rules",
+    [
+        pytest.param(
+            {},
+            id="empty rules",
+        ),
+        pytest.param(
+            {
+                "my_new_rule": {
+                    "variables": {},
+                    "domain_builder": {
+                        "class_name": "TableDomainBuilder",
+                    },
+                    "parameter_builders": [
+                        {
+                            "class_name": "MetricMultiBatchParameterBuilder",
+                            "name": "my_parameter",
+                            "metric_name": "my_metric",
+                        },
+                    ],
+                    "expectation_configuration_builders": [
+                        {
+                            "class_name": "DefaultExpectationConfigurationBuilder",
+                            "expectation_type": "expect_column_pair_values_A_to_be_greater_than_B",
+                            "column_A": "$domain.domain_kwargs.column_A",
+                            "column_B": "$domain.domain_kwargs.column_B",
+                        },
+                    ],
+                },
+            },
+            id="new rule",
+        ),
+    ],
+)
 def test_add_or_update_profiler_updates_successfully(
     in_memory_data_context: EphemeralDataContextSpy,
     profiler_rules: dict,
+    new_rules: dict,
 ):
     context = in_memory_data_context
 
@@ -481,7 +541,8 @@ def test_add_or_update_profiler_updates_successfully(
 
     assert context.profiler_store.save_count == 1
 
-    profiler.rules = {}
-    _ = context.add_or_update_profiler(name=name)
+    profiler = context.add_or_update_profiler(name=name, rules=new_rules)
 
+    # Rules get converted to a list within the RBP constructor
+    assert sorted(rule.name for rule in profiler.rules) == sorted(new_rules.keys())
     assert context.profiler_store.save_count == 2