[AIRFLOW-4294] fix missing dag & task runs in UI when . in dag_id

[yourhiro]

Make sure you have checked all steps below.

Jira

• My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-XXX] My Airflow PR"
  • https://issues.apache.org/jira/browse/AIRFLOW-4294
  • https://issues.apache.org/jira/browse/AIRFLOW-4325

Description

• Here are some details about my PR, including screenshots of any UI changes:

This fixes a regression when a dag_id has a '.' character in its name. The new UI uses "safe" dag_ids for the IDs of the corresponding DOM elements and when the endpoint /dag_stats returns the payload keyed on the regular dag_id, d3 is unable to update the elements.

The fix is to convert the dag_ids in the payload to a 'safe' dag_id by replacing all the . characters with __dot__ so the relevant DOM elements can be selected and updated

Before:

After change:

Tests

• My PR adds the following unit tests OR does not need testing for this extremely good reason:

Manual testing confirms that it show dag runs and task runs correctly

Commits

• My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• [] In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain docstrings that explain what it does
  • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

N/A

Code Quality

• Passes flake8

N/A

[codecov-io]

Codecov Report

Merging #5111 into master will increase coverage by 0.26%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #5111      +/-   ##
==========================================
+ Coverage   77.01%   77.28%   +0.26%     
==========================================
  Files         465      465              
  Lines       29962    29962              
==========================================
+ Hits        23076    23156      +80     
+ Misses       6886     6806      -80

Impacted Files	Coverage Δ
airflow/models/taskinstance.py	92.42% <0%> (-0.18%)	⬇️
airflow/hooks/dbapi_hook.py	88.79% <0%> (+0.86%)	⬆️
airflow/models/connection.py	65.73% <0%> (+1.12%)	⬆️
airflow/hooks/hive_hooks.py	75.13% <0%> (+1.85%)	⬆️
airflow/utils/sqlalchemy.py	81.81% <0%> (+4.54%)	⬆️
airflow/operators/mysql_operator.py	100% <0%> (+100%)	⬆️
airflow/operators/mysql_to_hive.py	100% <0%> (+100%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c63ddcc...e2315e8. Read the comment docs.

[ashb]

Does the old UI not suffer from this? It looks like it should be equally at a fault (though doesn't exist in master anymore).

I'm a little bit under the weather: could you explain to me again why the classic UI doesn't suffer from this bug?

[yourhiro]

interesting, it does appear that the bug has existed for some time, but I was running 1.10.0 and my dag and task run histories were showing fine. I've been using Airflow since 1.8 I believe, and this is the first time I've seen this issue, but this is my first time looking at any of the web UI code.

I encountered this bug because I upgraded to 1.10.3 and all my histories disappeared since I have a . character in all my DAG names.

[yourhiro]

aha, I see that up until 1.10.2, the payload was indeed keyed on safe_dag_id.

as of 1.10.2, it was working in both old and new UIs:
https://github.com/apache/airflow/blob/1.10.2/airflow/www/views.py#L577-L585
https://github.com/apache/airflow/blob/1.10.2/airflow/www_rbac/views.py#L337-L345

looks like it was changed in this commit:
ad7a702

I would argue that the coupling of the endpoint response to the way it's used on the client led to this being accidentally changed but I'm not familiar with the API design in Airflow so maybe that's a normal thing.

Either way, I would personally prefer a client-side fix because I feel it keeps the endpoint response clean but don't feel too strongly about it so I'll defer to you on which is more consistent with the rest of the web app.

[ashb]

Client-side fix sounds like the right thing to do, because it then allows other consumers than the Airflow frontend to use this endpoint (at least in theory, in practice the auth might be problematic)