[fix] Ensuring cache warmup respects request form data

[john-bodley]

CATEGORY

Choose one

• Bug Fix
• Enhancement (new features, refinement)
• Refactor
• Add tests
• Build / Development Environment
• Documentation

SUMMARY

Fixes #9664.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TEST PLAN

ADDITIONAL INFORMATION

• Has associated issue: Jinja filter_values macro does not work with cache warmup #9664
• Changes UI
• Requires DB Migration.
• Confirm DB Migration upgrade and downgrade tested.
• Introduces new feature or API
• Removes existing feature or API

REVIEWERS

to: @dpgaspar @etr2460 @villebro

[john-bodley]

@villebro this a draft fix for #9664. I would love your thoughts on how best to address this problem. Note this solution works, however it seems somewhat egregious to be leveraging the Flask g global for this context.

[villebro]

@john-bodley I agree that it's not optimal to be passing around form_data in many different contexts, but in the short term this is probably the best solution. To make this more robust, perhaps we should introduce a util that has this logic in one central place? There we could also add any necessary documentation to explain why this is necessary.

[john-bodley]

@villebro I wonder if it should be added to get_form_data?

[villebro]

@john-bodley I didn't remember that method; You're right, it should probably go in there. I'm assuming g context should only be used if form_data is missing in both POST and url params?

Mental note: that method is really bloated/complex and doesn't have any unit tests, should probably be broken up at some point.

[john-bodley]

@villebro I've updated the PR to use the somewhat scary get_form_data method. Though the method is complex I do prefer having this all housed in one place as it minimizes potential regressions (which is kind of how we ended up here).

I've added some basic unit tests to get_from_data as well as addition unit tests to the Jinja macros.

[john-bodley]

@villebro I was spinning my wheels for quite some time trying to get test_request_context() to work and then realize that this isn't explicitly related to the POST method and not is the body of the request a JSON object, i.e., the form_data field is actually further encoded as a JSON object.

The form-data is messy and we probably should strive to standardize this at some stage. For now I've renamed post_data as request_form_data which is more accurate.

[codecov-io]

Codecov Report

Merging #9665 into master will increase coverage by 4.89%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #9665      +/-   ##
==========================================
+ Coverage   65.70%   70.60%   +4.89%     
==========================================
  Files         581      581              
  Lines       30219    30318      +99     
  Branches     3071     3096      +25     
==========================================
+ Hits        19856    21406    +1550     
+ Misses      10182     8801    -1381     
+ Partials      181      111      -70     

Flag	Coverage Δ
#cypress	53.66% <ø> (?)
#javascript	58.79% <ø> (+0.04%)	⬆️
#python	70.72% <100.00%> (+0.12%)	⬆️

Impacted Files	Coverage Δ
superset/jinja_context.py	84.69% <100.00%> (+10.69%)	⬆️
superset/views/core.py	75.11% <100.00%> (-0.14%)	⬇️
superset/views/utils.py	82.91% <100.00%> (+0.21%)	⬆️
superset-frontend/src/components/ListView/utils.ts	82.95% <0.00%> (-2.05%)	⬇️
superset/utils/pandas_postprocessing.py	88.32% <0.00%> (-1.98%)	⬇️
superset/viz.py	71.49% <0.00%> (ø)
superset/viz_sip38.py	0.00% <0.00%> (ø)
superset/views/base.py	72.35% <0.00%> (ø)
superset/charts/schemas.py	100.00% <0.00%> (ø)
... and 155 more

---

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 f07ca7d...38f9c12. Read the comment docs.

[villebro]

This really improves code robustness/readability/quality! I'm surprised this worked out so well, I was expecting more trouble to surface as g was added to get_form_data. If it doesn't completely blow up mypy, adding types to get_form_data would really put the icing on this PR.

[villebro]

While we're at it, let's put in type annotations here; I was surprised to see that this was returning a tuple.

[john-bodley]

@villebro unquote_plus was raising a typing issue as it expected errors to be of type str. Per here it seems if errors is None it gets set to "replace" which is the default, thus there's no need to specify it.