hkc-8010 commented on code in PR #76012:
URL: https://github.com/apache/airflow/pull/76012#discussion_r4438766283
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/variables.py:
##########
@@ -40,6 +42,24 @@
from airflow.models.variable import Variable
+def _restore_redacted_value(new_value: str, old_value: str | None, key: str)
-> str:
+ """
+ Put the stored secret back wherever ``new_value`` still holds the mask
``VariableResponse`` sent out.
+
+ The UI edit form is filled from the redacted response, so saving it
unchanged would otherwise
+ write ``***`` over every sensitive field.
+ """
+ if old_value is None:
+ return new_value
+ try:
+ new_obj, old_obj = json.loads(new_value), json.loads(old_value)
+ except json.JSONDecodeError:
+ return merge(new_value, old_value, key)
+ merged = merge(new_obj, old_obj, key)
Review Comment:
Done in da1cfd6961. `_merge` now walks dicts and lists at any depth, same as
`_redact`, so both of your payloads round-trip. A subtree that `_redact_all`
cuts off at `max_depth` arrives as a single `***` under a sensitive key and is
restored whole. Covered by `test_merge_round_trip_deeply_nested` (your two
payloads plus that case) and
`test_patch_restores_secret_nested_deeper_than_max_depth` through the API. The
Connections extra merge picks this up too.
One side effect: the old early return restored any `***` past depth 5 even
under a non-sensitive key, and now only sensitive positions are restored, which
matches what shallower levels already did. I kept `max_depth` on `merge()` so
the signature doesn't change, it just no longer limits the walk.
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/variables.py:
##########
@@ -40,6 +42,24 @@
from airflow.models.variable import Variable
+def _restore_redacted_value(new_value: str, old_value: str | None, key: str)
-> str:
+ """
+ Put the stored secret back wherever ``new_value`` still holds the mask
``VariableResponse`` sent out.
+
+ The UI edit form is filled from the redacted response, so saving it
unchanged would otherwise
+ write ``***`` over every sensitive field.
+ """
+ if old_value is None:
+ return new_value
+ try:
+ new_obj, old_obj = json.loads(new_value), json.loads(old_value)
+ except json.JSONDecodeError:
+ return merge(new_value, old_value, key)
+ merged = merge(new_obj, old_obj, key)
+ # Keep the caller's formatting when there was nothing to restore.
+ return new_value if merged == new_obj else json.dumps(merged)
Review Comment:
Done, restored values are written with `indent=2`.
`test_patch_with_redacted_value_applies_other_changes` now checks the stored
text is the same as `json.dumps(..., indent=2)`.
##########
airflow-core/tests/unit/api_fastapi/core_api/routes/public/test_variables.py:
##########
@@ -622,6 +622,53 @@ def test_patch_variable_rejects_blank_key(self,
test_client, session, key):
assert response.status_code == 422
assert Variable.get(key, session=session) == "old value"
+ @pytest.mark.enable_redact
+ @pytest.mark.parametrize(
+ ("key", "stored_value"),
+ [
+ (TEST_VARIABLE_KEY2, TEST_VARIABLE_VALUE2),
+ (TEST_VARIABLE_KEY3, TEST_VARIABLE_VALUE3),
+ (TEST_VARIABLE_KEY5, TEST_VARIABLE_VALUE5),
+ (TEST_VARIABLE_KEY6, TEST_VARIABLE_VALUE6),
+ ],
+ ids=["sensitive-key", "sensitive-json-field",
"nested-sensitive-json-fields", "sensitive-key-json"],
+ )
+ def test_patch_with_redacted_value_keeps_stored_secrets(self, test_client,
key, stored_value):
Review Comment:
Good catch. Both tests now run with and without `update_mask=["value"]`, and
switching the condition to `and not update_mask` makes the masked cases fail. I
also rewrote the docstrings to describe what the form actually sends.
##########
airflow-core/src/airflow/api_fastapi/core_api/services/public/variables.py:
##########
Review Comment:
Thanks, agreed. A positional merge can't tell a reordered list from an
edited one, and Connections extra has the same behaviour, so I'd leave it as a
known limitation rather than change the list handling in this PR. Retyping the
entries after reordering a list of secrets avoids it.
--
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]
