Copilot commented on code in PR #61878: URL: https://github.com/apache/airflow/pull/61878#discussion_r2850721535
########## shared/template_rendering/src/airflow_shared/template_rendering/__init__.py: ########## @@ -0,0 +1,86 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import logging + +log = logging.getLogger(__name__) + +# Public truncation configuration used by ``truncate_rendered_value``. +# Exposed as module-level constants to avoid duplicating literals in callers/tests. +TRUNCATE_MIN_CONTENT_LENGTH = 7 +TRUNCATE_PREFIX = "Truncated. You can change this behaviour in [core]max_templated_field_length. " +TRUNCATE_SUFFIX = "..." + + +def truncate_rendered_value(rendered: str, max_length: int) -> str: + """ + Truncate a rendered template value to approximately ``max_length`` characters. + + Behavior: + + * If ``max_length <= 0``, an empty string is returned. + * A fixed prefix (``TRUNCATE_PREFIX``) and suffix (``TRUNCATE_SUFFIX``) are always + included when truncation occurs. The minimal truncation-only message is:: + + f"{TRUNCATE_PREFIX}{TRUNCATE_SUFFIX}" + + * If ``max_length`` is smaller than the length of this truncation-only message, that + message is returned in full, even though its length may exceed ``max_length``. + * Otherwise, space remaining after the prefix and suffix is allocated to the original + ``rendered`` content. Content is only appended if at least + ``TRUNCATE_MIN_CONTENT_LENGTH`` characters are available; if fewer are available, + the truncation-only message is returned instead. + + Note: this function is best-effort — the return value is intended to be no longer than + ``max_length``, but when ``max_length < len(TRUNCATE_PREFIX + TRUNCATE_SUFFIX)`` it + intentionally returns a longer string to preserve the full truncation message. + """ Review Comment: As written, the function always returns a truncation message+suffix (and possibly content) regardless of whether the input actually needs truncation. Since this is now a public shared helper, please document the expected contract explicitly (e.g., “call only when `len(rendered) > max_length`”), or adjust the implementation to return `rendered` unchanged when it already fits. ########## airflow-core/tests/unit/models/test_renderedtifields.py: ########## @@ -124,13 +125,14 @@ def teardown_method(self): pytest.param(datetime(2018, 12, 6, 10, 55), "2018-12-06 10:55:00+00:00", id="datetime"), pytest.param( "a" * 5000, - f"Truncated. You can change this behaviour in [core]max_templated_field_length. {('a' * 5000)[: max_length - 79]!r}... ", + truncate_rendered_value("a" * 5000, conf.getint("core", "max_templated_field_length")), Review Comment: Calling `conf.getint(...)` inline inside `pytest.param(...)` bakes the configured value at collection time, which can diverge from any runtime overrides/fixtures that adjust config for the test. Reuse the existing `max_length` variable (previously used here) or compute the expected value inside the test function/body where config overrides are active. ########## airflow-core/tests/unit/models/test_renderedtifields.py: ########## @@ -124,13 +125,14 @@ def teardown_method(self): pytest.param(datetime(2018, 12, 6, 10, 55), "2018-12-06 10:55:00+00:00", id="datetime"), pytest.param( "a" * 5000, - f"Truncated. You can change this behaviour in [core]max_templated_field_length. {('a' * 5000)[: max_length - 79]!r}... ", + truncate_rendered_value("a" * 5000, conf.getint("core", "max_templated_field_length")), id="large_string", ), pytest.param( LargeStrObject(), - f"Truncated. You can change this behaviour in " - f"[core]max_templated_field_length. {str(LargeStrObject())[: max_length - 79]!r}... ", + truncate_rendered_value( + str(LargeStrObject()), conf.getint("core", "max_templated_field_length") + ), Review Comment: Calling `conf.getint(...)` inline inside `pytest.param(...)` bakes the configured value at collection time, which can diverge from any runtime overrides/fixtures that adjust config for the test. Reuse the existing `max_length` variable (previously used here) or compute the expected value inside the test function/body where config overrides are active. ########## task-sdk/tests/task_sdk/execution_time/test_task_runner.py: ########## @@ -2690,18 +2690,24 @@ def execute(self, context): runtime_ti = create_runtime_ti(task=task, dag_id="test_truncation_masking_dag") run(runtime_ti, context=runtime_ti.get_template_context(), log=mock.MagicMock()) - assert ( - call( - msg=SetRenderedFields( - rendered_fields={ - "env_vars": "Truncated. You can change this behaviour in [core]max_templated_field_length. \"{'TEST_URL_0': '***', 'TEST_URL_1': '***', 'TEST_URL_10': '***', 'TEST_URL_11': '***', 'TEST_URL_12': '***', 'TEST_URL_13': '***', 'TEST_URL_14': '***', 'TEST_URL_15': '***', 'TEST_URL_16': '***', 'TEST_URL_17': '***', 'TEST_URL_18': '***', 'TEST_URL_19': '***', 'TEST_URL_2': '***', 'TEST_URL_20': '***', 'TEST_URL_21': '***', 'TEST_URL_22': '***', 'TEST_URL_23': '***', 'TEST_URL_24': '***', 'TEST_URL_25': '***', 'TEST_URL_26': '***', 'TEST_URL_27': '***', 'TEST_URL_28': '***', 'TEST_URL_29': '***', 'TEST_URL_3': '***', 'TEST_URL_30': '***', 'TEST_URL_31': '***', 'TEST_URL_32': '***', 'TEST_URL_33': '***', 'TEST_URL_34': '***', 'TEST_URL_35': '***', 'TEST_URL_36': '***', 'TEST_URL_37': '***', 'TEST_URL_38': '***', 'TEST_URL_39': '***', 'TEST_URL_4': '***', 'TEST_URL_40': '***', 'TEST_URL_41': '***', 'TEST_URL_42': '***', 'TEST_URL_43': '***', 'TEST_URL_44': '***', 'TES T_URL_45': '***', 'TEST_URL_46': '***', 'TEST_URL_47': '***', 'TEST_URL_48': '***', 'TEST_URL_49': '***', 'TEST_URL_5': '***', 'TEST_URL_6': '***', 'TEST_URL_7': '***', 'TEST_URL_8': '***', 'TEST_URL_9': '***'}\"... ", - "region": "us-west-2", - }, - type="SetRenderedFields", - ) - ) - in mock_supervisor_comms.send.mock_calls + msg = next( + c.kwargs["msg"] + for c in mock_supervisor_comms.send.mock_calls + if c.kwargs.get("msg") and getattr(c.kwargs["msg"], "type", None) == "SetRenderedFields" + ) + rendered_fields = msg.rendered_fields + + # region is short enough to not be truncated + assert rendered_fields["region"] == "us-west-2" + + # env_vars exceeds max_templated_field_length and must be truncated with secrets redacted + env_vars_value = rendered_fields["env_vars"] + assert isinstance(env_vars_value, str) + assert env_vars_value.startswith( + "Truncated. You can change this behaviour in [core]max_templated_field_length. " ) + assert env_vars_value.endswith("...") Review Comment: This re-introduces duplicated truncation literals in tests even though the PR exports `TRUNCATE_PREFIX`/`TRUNCATE_SUFFIX`. Import and assert against the shared constants to avoid brittleness if the message changes (and to keep Core + SDK expectations aligned). ########## shared/template_rendering/tests/template_rendering/test_truncate_rendered_value.py: ########## @@ -0,0 +1,108 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +from airflow_shared.template_rendering import ( + TRUNCATE_MIN_CONTENT_LENGTH, + TRUNCATE_PREFIX, + TRUNCATE_SUFFIX, + truncate_rendered_value, +) + + +def test_truncate_rendered_value_prioritizes_message(): + """Test that truncation message is always shown first, content only if space allows.""" + test_cases = [ + (1, "test", "Minimum value"), + (3, "test", "At ellipsis length"), + (5, "test", "Very small"), + (10, "password123", "Small"), + (20, "secret_value", "Small with content"), + (50, "This is a test string", "Medium"), + (83, "Hello World", "At prefix+suffix boundary v1"), + (84, "Hello World", "Just above boundary v1"), + (86, "Hello World", "At overhead boundary v2"), Review Comment: These hard-coded boundary lengths (`83`, `84`, `86`) are magic numbers that will break if `TRUNCATE_PREFIX`/`TRUNCATE_SUFFIX` or `TRUNCATE_MIN_CONTENT_LENGTH` changes. Prefer deriving these values from `len(TRUNCATE_PREFIX)`, `len(TRUNCATE_SUFFIX)`, and `TRUNCATE_MIN_CONTENT_LENGTH` so the tests remain stable while still covering the boundaries. ########## shared/template_rendering/src/airflow_shared/template_rendering/__init__.py: ########## @@ -0,0 +1,86 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + +import logging + +log = logging.getLogger(__name__) + +# Public truncation configuration used by ``truncate_rendered_value``. +# Exposed as module-level constants to avoid duplicating literals in callers/tests. +TRUNCATE_MIN_CONTENT_LENGTH = 7 +TRUNCATE_PREFIX = "Truncated. You can change this behaviour in [core]max_templated_field_length. " +TRUNCATE_SUFFIX = "..." + + +def truncate_rendered_value(rendered: str, max_length: int) -> str: + """ + Truncate a rendered template value to approximately ``max_length`` characters. + + Behavior: + + * If ``max_length <= 0``, an empty string is returned. + * A fixed prefix (``TRUNCATE_PREFIX``) and suffix (``TRUNCATE_SUFFIX``) are always + included when truncation occurs. The minimal truncation-only message is:: + + f"{TRUNCATE_PREFIX}{TRUNCATE_SUFFIX}" + + * If ``max_length`` is smaller than the length of this truncation-only message, that + message is returned in full, even though its length may exceed ``max_length``. + * Otherwise, space remaining after the prefix and suffix is allocated to the original + ``rendered`` content. Content is only appended if at least + ``TRUNCATE_MIN_CONTENT_LENGTH`` characters are available; if fewer are available, + the truncation-only message is returned instead. + + Note: this function is best-effort — the return value is intended to be no longer than + ``max_length``, but when ``max_length < len(TRUNCATE_PREFIX + TRUNCATE_SUFFIX)`` it + intentionally returns a longer string to preserve the full truncation message. + """ + if max_length <= 0: + return "" + + trunc_only = f"{TRUNCATE_PREFIX}{TRUNCATE_SUFFIX}" + + if max_length < len(trunc_only): + return trunc_only + + # Compute available space for content + overhead = len(TRUNCATE_PREFIX) + len(TRUNCATE_SUFFIX) + available = max_length - overhead + + if available < TRUNCATE_MIN_CONTENT_LENGTH: + return trunc_only + + # Slice content to fit and construct final string + content = rendered[:available].rstrip() Review Comment: `rstrip()` changes the truncated content beyond simple length limiting (it can remove meaningful trailing spaces/newlines from the rendered value). If the intent is “truncate to fit” while preserving the original content slice, drop the `rstrip()` (or make whitespace-stripping an explicit opt-in behavior). ```suggestion # Slice content to fit and construct final string without altering trailing whitespace content = rendered[:available] ``` ########## airflow-core/tests/unit/serialization/test_helpers.py: ########## @@ -0,0 +1,31 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +from __future__ import annotations + + +def test_serialize_template_field_with_very_small_max_length(monkeypatch): + """Test that truncation message is prioritized even for very small max_length.""" + monkeypatch.setenv("AIRFLOW__CORE__MAX_TEMPLATED_FIELD_LENGTH", "1") + + from airflow.serialization.helpers import serialize_template_field + + result = serialize_template_field("This is a long string", "test") + + # The truncation message should be shown even if it exceeds max_length + # This ensures users always see why content is truncated + assert result + assert "Truncated. You can change this behaviour" in result Review Comment: This assertion is quite loose and could pass even if the returned format regresses (e.g., missing suffix, wrong prefix, extra content). Since the shared module exports constants, consider asserting exact output for this case (e.g., `TRUNCATE_PREFIX + TRUNCATE_SUFFIX`) to make the regression test precise. ```suggestion from airflow.serialization.helpers import TRUNCATE_PREFIX, TRUNCATE_SUFFIX, serialize_template_field result = serialize_template_field("This is a long string", "test") # The truncation message should be shown even if it exceeds max_length # This ensures users always see why content is truncated assert result == TRUNCATE_PREFIX + TRUNCATE_SUFFIX ``` -- 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]
