Use WebEncoder to encode DagRun.conf in DagRun's list view #32385
Conversation
xuganyu96
requested review from
ashb,
bbovenzi,
pierrejeambrun and
ryanahamilton
as code owners
airflow/www/utils.py
Outdated
There was a problem hiding this comment.
I think we should always use WebEncoder here, so the extra argument is not needed.
Right now json_f is actually only used once, but since json_f is only for the web UI, all usages in the future should use WebEncoder anyway.
There was a problem hiding this comment.
Personally speaking I've typically found it easier to keep parameterization open since it's more flexible and we should probably not be so sure about future usage of json_f, but I am open to suggestions. Maybe we can wait for a second opinion?
There was a problem hiding this comment.
@uranusjr I've updated the code so that json_f always uses WebEncoder and updated the relevant func calls + unit tests. Please review at your convenience. Thank you!
xuganyu96
force-pushed
the
dagrun-list-view-conf-serialization
branch
from
c97c90f to
b30d0c6
Compare
|
@uranusjr ? I think that might be a good candidate to add to 2.6.3 |
* Use WebEncoder to encode DagRun.conf in DagRun's list view * test case for json_f with WebEncoder * Always use WebEncoder in web views (cherry picked from commit 4bbb633)
4 participants
This PR is related to the following issues and PRs: #32287, #28777, #32288, #28772.
Previous efforts related to JSON-serializing DagRun configuration have provided an elegant solution in the form of a custom JSON encoder class named
WebEncoder, which is used to encode non-JSON serializable objects stored inDagRun.conf. This allows DAG run conf to be correctly rendered in the grid view, but the fix was not applied to the list view, hence breaking/dagrun/listif there are conf values that are not JSON serializable.This PR attempts to address the incompleteness of the previous fix by using
WebEncoderinformatters_columns, which should allow non-JSON-serializable conf to be rendered in list view in the same way it is rendered in grid view.Note that
www.utils.json_fis used exactly once where the code change happened, and the code change seems trivial, so I am not sure if unit tests or other forms of validation is warranted, but I am happy to write tests if necessary.Thank you for considering my contribution!
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in newsfragments.