amoghrajesh commented on code in PR #53943:
URL: https://github.com/apache/airflow/pull/53943#discussion_r2244632290
##########
task-sdk/tests/task_sdk/definitions/test_secrets_masker.py:
##########
@@ -729,3 +731,356 @@ def test_mixed_structured_unstructured_data(self):
assert "***" in redacted_data["description"]
assert redacted_data["nested"]["token"] == "***"
assert redacted_data["nested"]["info"] == "No secrets here"
+
+
+class TestSecretsMaskerMerge:
+ """Test the merge functionality for restoring original values from
redacted data."""
+
+ @pytest.mark.parametrize(
+ ("new_value", "old_value", "name", "expected"),
+ [
+ ("***", "original_secret", "password", "original_secret"),
+ ("new_secret", "original_secret", "password", "new_secret"),
+ ("***", "original_value", "normal_field", "***"),
+ ("new_value", "original_value", "normal_field", "new_value"),
+ ("***", "original_value", None, "***"),
+ ("new_value", "original_value", None, "new_value"),
+ ],
+ )
+ def test_merge_simple_strings(self, new_value, old_value, name, expected):
+ secrets_masker = SecretsMasker()
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_value, old_value, name)
+ assert result == expected
+
+ def test_merge_dictionaries(self):
+ secrets_masker = SecretsMasker()
+
+ old_data = {
+ "password": "original_password",
+ "api_key": "original_api_key",
+ "normal_field": "original_normal",
+ "token": "original_token",
+ }
+
+ new_data = {
+ "password": "***",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "***",
+ }
+
+ expected = {
+ "password": "original_password",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "original_token",
+ }
Review Comment:
```suggestion
new_data = {
"password": "***",
"api_key": "new_api_key",
"normal_field": "new_normal",
"token": "new_token_value",
}
expected = {
"password": "original_password",
"api_key": "new_api_key",
"normal_field": "new_normal",
"token": "new_token_value",
}
```
Lets do this to cover more area?
##########
task-sdk/tests/task_sdk/definitions/test_secrets_masker.py:
##########
@@ -729,3 +731,356 @@ def test_mixed_structured_unstructured_data(self):
assert "***" in redacted_data["description"]
assert redacted_data["nested"]["token"] == "***"
assert redacted_data["nested"]["info"] == "No secrets here"
+
+
+class TestSecretsMaskerMerge:
+ """Test the merge functionality for restoring original values from
redacted data."""
+
+ @pytest.mark.parametrize(
+ ("new_value", "old_value", "name", "expected"),
+ [
+ ("***", "original_secret", "password", "original_secret"),
+ ("new_secret", "original_secret", "password", "new_secret"),
+ ("***", "original_value", "normal_field", "***"),
+ ("new_value", "original_value", "normal_field", "new_value"),
+ ("***", "original_value", None, "***"),
+ ("new_value", "original_value", None, "new_value"),
+ ],
+ )
+ def test_merge_simple_strings(self, new_value, old_value, name, expected):
+ secrets_masker = SecretsMasker()
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_value, old_value, name)
+ assert result == expected
+
+ def test_merge_dictionaries(self):
+ secrets_masker = SecretsMasker()
+
+ old_data = {
+ "password": "original_password",
+ "api_key": "original_api_key",
+ "normal_field": "original_normal",
+ "token": "original_token",
+ }
+
+ new_data = {
+ "password": "***",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "***",
+ }
+
+ expected = {
+ "password": "original_password",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "original_token",
+ }
+
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
Review Comment:
In all the tests
##########
task-sdk/tests/task_sdk/definitions/test_secrets_masker.py:
##########
@@ -729,3 +731,356 @@ def test_mixed_structured_unstructured_data(self):
assert "***" in redacted_data["description"]
assert redacted_data["nested"]["token"] == "***"
assert redacted_data["nested"]["info"] == "No secrets here"
+
+
+class TestSecretsMaskerMerge:
+ """Test the merge functionality for restoring original values from
redacted data."""
+
+ @pytest.mark.parametrize(
+ ("new_value", "old_value", "name", "expected"),
+ [
+ ("***", "original_secret", "password", "original_secret"),
+ ("new_secret", "original_secret", "password", "new_secret"),
+ ("***", "original_value", "normal_field", "***"),
+ ("new_value", "original_value", "normal_field", "new_value"),
+ ("***", "original_value", None, "***"),
+ ("new_value", "original_value", None, "new_value"),
+ ],
+ )
+ def test_merge_simple_strings(self, new_value, old_value, name, expected):
+ secrets_masker = SecretsMasker()
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_value, old_value, name)
+ assert result == expected
+
+ def test_merge_dictionaries(self):
+ secrets_masker = SecretsMasker()
+
+ old_data = {
+ "password": "original_password",
+ "api_key": "original_api_key",
+ "normal_field": "original_normal",
+ "token": "original_token",
+ }
+
+ new_data = {
+ "password": "***",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "***",
+ }
+
+ expected = {
+ "password": "original_password",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "original_token",
+ }
+
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_data, old_data)
+ assert result == expected
+
+ def test_merge_nested_dictionaries(self):
+ secrets_masker = SecretsMasker()
+
+ old_data = {
+ "config": {"password": "original_password", "host":
"original_host"},
+ "credentials": {"api_key": "original_api_key", "username":
"original_user"},
+ }
+
+ new_data = {
+ "config": {
+ "password": "***",
+ "host": "new_host",
+ },
+ "credentials": {
+ "api_key": "new_api_key",
+ "username": "new_user",
+ },
+ }
+
+ expected = {
+ "config": {
+ "password": "original_password",
+ "host": "new_host",
+ },
+ "credentials": {
+ "api_key": "new_api_key",
+ "username": "new_user",
+ },
+ }
+
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_data, old_data)
+ assert result == expected
Review Comment:
Oh i see you are covering it here? Maybe merge the two tests into one with
pytest params?
##########
task-sdk/tests/task_sdk/definitions/test_secrets_masker.py:
##########
@@ -729,3 +731,356 @@ def test_mixed_structured_unstructured_data(self):
assert "***" in redacted_data["description"]
assert redacted_data["nested"]["token"] == "***"
assert redacted_data["nested"]["info"] == "No secrets here"
+
+
+class TestSecretsMaskerMerge:
+ """Test the merge functionality for restoring original values from
redacted data."""
+
+ @pytest.mark.parametrize(
+ ("new_value", "old_value", "name", "expected"),
+ [
+ ("***", "original_secret", "password", "original_secret"),
+ ("new_secret", "original_secret", "password", "new_secret"),
+ ("***", "original_value", "normal_field", "***"),
+ ("new_value", "original_value", "normal_field", "new_value"),
+ ("***", "original_value", None, "***"),
+ ("new_value", "original_value", None, "new_value"),
+ ],
+ )
+ def test_merge_simple_strings(self, new_value, old_value, name, expected):
+ secrets_masker = SecretsMasker()
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_value, old_value, name)
+ assert result == expected
+
+ def test_merge_dictionaries(self):
+ secrets_masker = SecretsMasker()
+
+ old_data = {
+ "password": "original_password",
+ "api_key": "original_api_key",
+ "normal_field": "original_normal",
+ "token": "original_token",
+ }
+
+ new_data = {
+ "password": "***",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "***",
+ }
+
+ expected = {
+ "password": "original_password",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "original_token",
+ }
+
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_data, old_data)
+ assert result == expected
+
+ def test_merge_nested_dictionaries(self):
+ secrets_masker = SecretsMasker()
+
+ old_data = {
+ "config": {"password": "original_password", "host":
"original_host"},
+ "credentials": {"api_key": "original_api_key", "username":
"original_user"},
+ }
+
+ new_data = {
+ "config": {
+ "password": "***",
+ "host": "new_host",
+ },
+ "credentials": {
+ "api_key": "new_api_key",
+ "username": "new_user",
+ },
+ }
+
+ expected = {
+ "config": {
+ "password": "original_password",
+ "host": "new_host",
+ },
+ "credentials": {
+ "api_key": "new_api_key",
+ "username": "new_user",
+ },
+ }
+
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_data, old_data)
+ assert result == expected
+
+ @pytest.mark.parametrize(
+ ("old_data", "new_data", "name", "expected"),
+ [
+ # Lists
+ (
+ ["original_item1", "original_item2", "original_item3"],
+ ["new_item1", "new_item2"],
+ None,
+ ["new_item1", "new_item2"],
+ ),
+ (
+ ["original_item1", "original_item2"],
+ ["new_item1", "new_item2", "new_item3", "new_item4"],
+ None,
+ ["new_item1", "new_item2", "new_item3", "new_item4"],
+ ),
+ (
+ ["secret1", "secret2", "secret3"],
+ ["***", "new_secret2", "***"],
+ "password",
+ ["secret1", "new_secret2", "secret3"],
+ ),
+ (
+ ["value1", "value2", "value3"],
+ ["***", "new_value2", "***"],
+ "normal_list",
+ ["***", "new_value2", "***"],
+ ),
+ # Tuples
+ (
+ ("original_item1", "original_item2", "original_item3"),
+ ("new_item1", "new_item2"),
+ None,
+ ("new_item1", "new_item2"),
+ ),
+ (
+ ("original_item1", "original_item2"),
+ ("new_item1", "new_item2", "new_item3", "new_item4"),
+ None,
+ ("new_item1", "new_item2", "new_item3", "new_item4"),
+ ),
+ (
+ ("secret1", "secret2", "secret3"),
+ ("***", "new_secret2", "***"),
+ "password",
+ ("secret1", "new_secret2", "secret3"),
+ ),
+ (
+ ("value1", "value2", "value3"),
+ ("***", "new_value2", "***"),
+ "normal_tuple",
+ ("***", "new_value2", "***"),
+ ),
+ # Sets
+ (
+ {"original_item1", "original_item2", "original_item3"},
+ {"new_item1", "new_item2"},
+ None,
+ {"new_item1", "new_item2"},
+ ),
+ (
+ {"original_item1", "original_item2"},
+ {"new_item1", "new_item2", "new_item3", "new_item4"},
+ None,
+ {"new_item1", "new_item2", "new_item3", "new_item4"},
+ ),
+ (
+ {"secret1", "secret2", "secret3"},
+ {"***", "new_secret2", "***"},
+ "password",
+ {"***", "new_secret2", "***"},
Review Comment:
Yeah, we should not be raising an error.
Maybe clarify in the helper text?
##########
task-sdk/tests/task_sdk/definitions/test_secrets_masker.py:
##########
@@ -729,3 +731,356 @@ def test_mixed_structured_unstructured_data(self):
assert "***" in redacted_data["description"]
assert redacted_data["nested"]["token"] == "***"
assert redacted_data["nested"]["info"] == "No secrets here"
+
+
+class TestSecretsMaskerMerge:
+ """Test the merge functionality for restoring original values from
redacted data."""
+
+ @pytest.mark.parametrize(
+ ("new_value", "old_value", "name", "expected"),
+ [
+ ("***", "original_secret", "password", "original_secret"),
+ ("new_secret", "original_secret", "password", "new_secret"),
+ ("***", "original_value", "normal_field", "***"),
+ ("new_value", "original_value", "normal_field", "new_value"),
+ ("***", "original_value", None, "***"),
+ ("new_value", "original_value", None, "new_value"),
+ ],
+ )
+ def test_merge_simple_strings(self, new_value, old_value, name, expected):
+ secrets_masker = SecretsMasker()
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
+ result = merge(new_value, old_value, name)
+ assert result == expected
+
+ def test_merge_dictionaries(self):
+ secrets_masker = SecretsMasker()
+
+ old_data = {
+ "password": "original_password",
+ "api_key": "original_api_key",
+ "normal_field": "original_normal",
+ "token": "original_token",
+ }
+
+ new_data = {
+ "password": "***",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "***",
+ }
+
+ expected = {
+ "password": "original_password",
+ "api_key": "new_api_key",
+ "normal_field": "new_normal",
+ "token": "original_token",
+ }
+
+ with
patch("airflow.sdk.execution_time.secrets_masker._secrets_masker",
return_value=secrets_masker):
Review Comment:
You could use the fixture: `patched_secrets_masker`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
