-
Notifications
You must be signed in to change notification settings - Fork 16.2k
Fix small max templated field length #59999
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Fix small max templated field length #59999
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
jscheffl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code is duplicated 4-times. Can you please make it in a way with less redundancy?
|
Hi reviewers, Thank you for catching the code duplication. I've updated the solution to use a centralized approach:
This eliminates all duplication while preserving the original fix for issue #59877. Let me know if you have any feedback. |
|
Some static checks via MyPy are executed and failing in CI, can you check for them? |
|
Hi @jscheffl and all reviewers, I've addressed the CI check failures:
The implementation now passes all type checks while maintaining the fix for the original issue. All CI checks (MyPy, static, PostgreSQL, MySQL, SQLite, serialization, low-dep) should now pass. |
Description
This PR fixes issue #59877 by addressing incorrect rendered template truncation when
AIRFLOW__CORE__MAX_TEMPLATED_FIELD_LENGTHis set to very small values (like 1, 5, 10).Problem
When
max_templated_field_lengthwas set to very small values, the original code used a fixed offset of 79 characters (max_length - 79), which resulted in:max_lengthwas smaller than 79Context and Previous Work
I've carefully reviewed the existing issue #59877 and the previous PR #59882 by @Jeevankumar-s. While their approach provided a good foundation, I identified that there were additional edge cases with the
!rformatting (repr() adding quotes to strings) that needed proper handling to ensure the output never exceeds the configured maximum length. The conversation in the issue also highlighted the importance of prioritizing clear communication to users over potentially confusing partial results.Solution
I've replaced the problematic logic with a comprehensive solution that:
!rformatting (repr() adds 2 quotes around strings)My approach specifically addresses the
!rformatting issue that could cause output to exceed the max_length when the repr() adds additional quote characters around strings.Files Changed
task-sdk/src/airflow/sdk/execution_time/task_runner.py- Runtime truncation logicairflow-core/src/airflow/serialization/helpers.py- Serialization truncation logicBoth paths now have consistent behavior.
Testing
I've verified the fix with various test cases including the edge cases mentioned in the issue, ensuring output length never exceeds the configured maximum.
Personal Note
As a new contributor to Apache Airflow, I've spent considerable time understanding the issue, reviewing the existing PR, and carefully considering the community's feedback. I aimed to create a solution that not only fixes the technical issue but also follows the principle of prioritizing clear communication to users, as discussed in the issue comments.
Closes #59877