[AIRFLOW-6728] Change various DAG info methods to POST #7364

madison-ookla · 2020-02-05T00:56:54Z

This PR changes the following DAG info methods to POST:

/dag_stats
/task_stats
/last_dagruns
/blocked

The reason for this is described in the JIRA ticket:

It appears that the URL uses a dag_ids parameter that is a comma separated list in the GET request. Since DAGs can have a dag_id length of 250 characters and the UI will show up to 100 DAGs per page, this means that the URL parameters alone could be 25,000 characters (not including domain, etc.). It seems that it might be best to switch this request over to POST to accommodate the potentially large request body.

Here is a small DAG file I used to generate DAGs with excessively large names:

EXPAND FOR CODE

from datetime import datetime

from airflow import DAG

from airflow.operators.dummy_operator import DummyOperator


def create_dag(dag_id,
               schedule,
               default_args):


    dag = DAG(dag_id,
              schedule_interval=schedule,
              default_args=default_args)

    with dag:
        t1 = DummyOperator(task_id='dummy_task')

    return dag


# build a dag for each number in range(100)
for n in range(0, 100):
    dag_id = 'long_name_dag__' + f"{n}".zfill(235)

    default_args = {'owner': 'airflow',
                    'start_date': datetime(2018, 1, 1)
                    }

    schedule = None

    globals()[dag_id] = create_dag(dag_id,
                                  schedule,
                                  default_args)

I was able to verify that the page loads correctly with the new changes. (BTW Breeze was extremely helpful for this 😀)

Issue link: AIRFLOW-6728

Make sure to mark the boxes below before creating PR: [x]

Description above provides context of the change
Commit message/PR title starts with [AIRFLOW-NNNN]. AIRFLOW-NNNN = JIRA ID^*
Unit tests coverage for changes (not needed for documentation changes)
Commits follow "How to write a good git commit message"
Relevant documentation is updated including usage instructions.
I will engage committers as explained in Contribution Workflow Example.

^* For document-only changes commit message can start with [AIRFLOW-XXXX].

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

This also involves sending in the CSRF token.

madison-ookla · 2020-02-05T03:44:00Z

The build failure looks like a timeout unrelated to the changes of this PR

codecov-io · 2020-02-05T06:56:22Z

Codecov Report

Merging #7364 into master will increase coverage by <.01%.
The diff coverage is 60%.

@@            Coverage Diff             @@
##           master    #7364      +/-   ##
==========================================
+ Coverage   86.35%   86.35%   +<.01%     
==========================================
  Files         871      871              
  Lines       40627    40636       +9     
==========================================
+ Hits        35082    35091       +9     
  Misses       5545     5545

Impacted Files	Coverage Δ
airflow/www/views.py	`76.22% <60%> (+0.14%)`	⬆️
airflow/www/app.py	`94.24% <0%> (ø)`	⬆️

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 2bfd7f2...8826960. Read the comment docs.

robinedwards · 2020-02-05T17:08:28Z

Have you actually encountered issues with the request body size? POST feels a bit weird here not very RESTy

madison-ookla · 2020-02-05T17:26:05Z

Have you actually encountered issues with the request body size? POST feels a bit weird here not very RESTy

I brought this issue forward because the Recent Tasks column in our production instance wasn't displaying any information. After investigation I found that it was because of the request URL size. Since the limit is 4096, to query 100 DAGs those DAGs only need to be 41 characters long to reach that limit.

ashb · 2020-02-05T17:45:14Z

Rather than a JSON body more "resty" would be to have this be a "form post" -- i.e. application/x-www-form-urlencoded body.

ashb · 2020-02-05T17:45:44Z

@mik-laj has a simiarl PR (but it keeps it as a GET which doesn't fix this problem)

madison-ookla · 2020-02-05T18:06:40Z

Rather than a JSON body more "resty" would be to have this be a "form post" -- i.e. application/x-www-form-urlencoded body.

I can change the request to use a form post instead of JSON

mik-laj · 2020-02-05T18:09:06Z

Here is my PR: https://github.com/apache/airflow/pulls/mik-laj
#6855
But this PR is better.

robinedwards · 2020-02-05T18:10:15Z

I guess another option is doing multiple GET requests...

mik-laj · 2020-02-05T18:32:43Z

I would like to point out that this endpoint belongs to the internal API so it didn't have to be pretty. This should work well. Only public APIs must be beautiful, standardized and backward compatible.

robinedwards · 2020-02-05T18:38:05Z

Fair point :-) Cheers for fixing @madison-ookla and apologies for any breakage my original change caused.

madison-ookla · 2020-02-05T18:39:27Z

Thanks all for the feedback :) I would prefer to keep it as JSON just because it makes processing processing the request body a bit easier (rather than having to split a comma separated string). That said I don't do a lot of web-dev work so that practice may be commonplace.

madison-ookla · 2020-02-05T18:49:13Z

Although it looks like I could do request.form.getlist('dag_ids') inside the actual request handlers for a regular form POST, which is straightforward. I'd just have to add the form encoding onto all of the requests. I could go either way - let me know if JSON POST or form POST is preferred.

robinedwards · 2020-02-05T18:51:22Z

There is the FormData API for doing the encoding if your interested https://developer.mozilla.org/en-US/docs/Web/API/FormData/Using_FormData_Objects

ashb · 2020-02-05T23:33:48Z

There's also a postAsForm helper method already in Airflow

airflow/airflow/www/static/js/base.js

Lines 75 to 103 in 9e150cf

    
           function postAsForm(url, parameters) { 
        
             var form = $("<form></form>"); 
        
             form.attr("method", "POST"); 
        
             form.attr("action", url); 
        
             $.each(parameters || {}, function(key, value) { 
        
               var field = $('<input></input>'); 
        
               field.attr("type", "hidden"); 
        
               field.attr("name", key); 
        
               field.attr("value", value); 
        
               form.append(field); 
        
             }); 
        
             var field = $('<input></input>'); 
        
             field.attr("type", "hidden"); 
        
             field.attr("name", "csrf_token"); 
        
             field.attr("value", csrfToken); 
        
             form.append(field); 
        
             // The form needs to be a part of the document in order for us to be able 
        
             // to submit it. 
        
             $(document.body).append(form); 
        
             form.submit(); 
        
           }

if that's of use.

ashb · 2020-02-05T23:34:22Z

And yes, using request.form.getlist() is the way to go over json

madison-ookla · 2020-02-05T23:37:37Z

Gotcha, thanks @ashb. I'll move this over to a form POST then.

ashb · 2020-02-05T23:44:14Z

That helper method won't accept multiple fields of the same name just yet, so you might have to extend that or use something else

madison-ookla · 2020-02-05T23:50:03Z

That helper method won't accept multiple fields of the same name just yet, so you might have to extend that or use something else

Since the request is done with d3.json I'm not sure I'd be able to use that anyway

madison-ookla · 2020-02-06T02:26:22Z

I've converted the endpoints from JSON to form POST. It was easier than I thought!

ashb · 2020-02-06T09:55:45Z

airflow/www/templates/airflow/dags.html

+          .post("dag_ids="  + (encoded_dag_ids.join(',')), dagStatsHandler);
+        d3.json("{{ url_for('Airflow.task_stats') }}")
+          .header("X-CSRFToken", "{{ csrf_token() }}")
+          .post("dag_ids="  + (encoded_dag_ids.join(',')), taskStatsHandler);


Is this right? I would expect that this would post the encoded as a single parameter..

We probably don't have to handle the encoding and building of forms ourselves:

var p = new URLSearchParams(); p.append("dag_id", "not+encoded.here"); p.append("dag_id", "second dag/id"); d3.json("/a").post(p)

And that sends this request:

POST /a HTTP/1.1 Host: 0.0.0.0:8080 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0 Accept: application/json,*/* Accept-Language: en-US,en;q=0.5 Accept-Encoding: gzip, deflate Content-Type: application/x-www-form-urlencoded;charset=UTF-8 Content-Length: 48 Origin: http://0.0.0.0:8080 Connection: keep-alive Referer: http://0.0.0.0:8080/home dag_id=not%2Bencoded.here&dag_id=second+dag%2Fid

Could you make this change @madison-ookla ?

Sure, can do

Done! I've tested this locally and it works 👍

Ready for you now @ashb @mik-laj 😁

ashb

Nice, looking good (and quite a short change in the end!)

madison-ookla · 2020-02-07T16:30:46Z

Nice, looking good (and quite a short change in the end!)

Thanks! Any chance this can be backported to the v1.10.x branch?

Edit: Nevermind, I see it's now part of the 1.10.10 fix version on Jira 🙂

If the number of dags was large and/or the length of the DAG ids were too large this would exceed the maximum possible query string limit. To work around that we have made these endpoints always make POST requests

ashb · 2020-04-09T23:01:02Z

@madison-ookla And included in 1.10.10 which was just released :)

madison-ookla · 2020-04-09T23:08:20Z

@madison-ookla And included in 1.10.10 which was just released :)

Thanks, very excited about this release! 😁

madison-ookla added 2 commits February 4, 2020 16:17

[AIRFLOW-6728] Change various stats methods from GET to POST

6070377

[AIRFLOW-6728] Update d3.json requests on front end to use POST

8b72c8c

This also involves sending in the CSRF token.

boring-cyborg bot added the area:webserver Webserver related Issues label Feb 5, 2020

ashb requested a review from mik-laj February 5, 2020 17:11

[AIRFLOW-6728] Convert endpoints from JSON POST to form POST

e812504

ashb reviewed Feb 6, 2020

View reviewed changes

[AIRFLOW-6728] Use UrlSearchParams object for DAG IDs

8826960

ashb approved these changes Feb 7, 2020

View reviewed changes

ashb merged commit b738c9e into apache:master Feb 7, 2020

madison-ookla deleted the bugfix/ui-dag-stats branch February 7, 2020 16:30

kaxil added a commit that referenced this pull request Mar 17, 2020

[AIRFLOW-6728] Change various DAG info methods to POST (#7364)

0be3c5e

kaxil added a commit that referenced this pull request Mar 19, 2020

[AIRFLOW-6728] Change various DAG info methods to POST (#7364)

874207a

kaxil added a commit to astronomer/airflow that referenced this pull request Mar 19, 2020

[AIRFLOW-6728] Change various DAG info methods to POST (apache#7364)

c1e346b

ashb mentioned this pull request Apr 30, 2020

Request Line is too large #8636

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-6728] Change various DAG info methods to POST #7364

[AIRFLOW-6728] Change various DAG info methods to POST #7364

madison-ookla commented Feb 5, 2020 •

edited

Loading

madison-ookla commented Feb 5, 2020

codecov-io commented Feb 5, 2020 •

edited

Loading

robinedwards commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

ashb commented Feb 5, 2020

ashb commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

mik-laj commented Feb 5, 2020

robinedwards commented Feb 5, 2020

mik-laj commented Feb 5, 2020

robinedwards commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

robinedwards commented Feb 5, 2020 •

edited

Loading

ashb commented Feb 5, 2020

ashb commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

ashb commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

madison-ookla commented Feb 6, 2020

ashb Feb 6, 2020

ashb Feb 6, 2020

madison-ookla Feb 6, 2020

madison-ookla Feb 6, 2020

madison-ookla Feb 6, 2020

ashb left a comment

madison-ookla commented Feb 7, 2020 •

edited

Loading

ashb commented Apr 9, 2020

madison-ookla commented Apr 9, 2020 •

edited

Loading

[AIRFLOW-6728] Change various DAG info methods to POST #7364

[AIRFLOW-6728] Change various DAG info methods to POST #7364

Conversation

madison-ookla commented Feb 5, 2020 • edited Loading

madison-ookla commented Feb 5, 2020

codecov-io commented Feb 5, 2020 • edited Loading

Codecov Report

robinedwards commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

ashb commented Feb 5, 2020

ashb commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

mik-laj commented Feb 5, 2020

robinedwards commented Feb 5, 2020

mik-laj commented Feb 5, 2020

robinedwards commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

robinedwards commented Feb 5, 2020 • edited Loading

ashb commented Feb 5, 2020

ashb commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

ashb commented Feb 5, 2020

madison-ookla commented Feb 5, 2020

madison-ookla commented Feb 6, 2020

ashb Feb 6, 2020

Choose a reason for hiding this comment

ashb Feb 6, 2020

Choose a reason for hiding this comment

madison-ookla Feb 6, 2020

Choose a reason for hiding this comment

madison-ookla Feb 6, 2020

Choose a reason for hiding this comment

madison-ookla Feb 6, 2020

Choose a reason for hiding this comment

ashb left a comment

Choose a reason for hiding this comment

madison-ookla commented Feb 7, 2020 • edited Loading

ashb commented Apr 9, 2020

madison-ookla commented Apr 9, 2020 • edited Loading

madison-ookla commented Feb 5, 2020 •

edited

Loading

codecov-io commented Feb 5, 2020 •

edited

Loading

robinedwards commented Feb 5, 2020 •

edited

Loading

madison-ookla commented Feb 7, 2020 •

edited

Loading

madison-ookla commented Apr 9, 2020 •

edited

Loading