chore: Migrate warm up cache endpoint to api v1 #23853

jfrag1 · 2023-04-27T16:31:42Z

SUMMARY

This PR deprecates the superset/warm_up_cache endpoint and creates the api/v1/charts/warm_up_cache and api/v1/dataset/warm_up_cache endpoints to replace it.

The new endpoint has the following key differences:

Follows command pattern
uses PUT instead of GET
accepts a json body validated by marshmallow instead of query params
added openapi spec

Also improves test coverage

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

codecov · 2023-04-27T17:07:06Z

Codecov Report

Merging #23853 (079ff1b) into master (3e76736) will increase coverage by 0.10%.
The diff coverage is 87.57%.

❗ Current head 079ff1b differs from pull request most recent head fcb89ad. Consider uploading reports for the commit fcb89ad to get more accurate results

@@            Coverage Diff             @@
##           master   #23853      +/-   ##
==========================================
+ Coverage   68.91%   69.01%   +0.10%     
==========================================
  Files        1899     1901       +2     
  Lines       73843    73969     +126     
  Branches     8119     8119              
==========================================
+ Hits        50892    51053     +161     
+ Misses      20840    20805      -35     
  Partials     2111     2111

Flag	Coverage Δ
hive	`53.90% <46.58%> (+<0.01%)`	⬆️
mysql	`79.40% <87.57%> (+0.17%)`	⬆️
postgres	`79.49% <87.57%> (+0.17%)`	⬆️
presto	`53.82% <46.58%> (+<0.01%)`	⬆️
python	`83.47% <87.57%> (+0.15%)`	⬆️
sqlite	`77.99% <87.57%> (+0.17%)`	⬆️
unit	`54.59% <42.23%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
superset/tasks/cache.py	`59.34% <48.64%> (-1.81%)`	⬇️
superset/charts/commands/warm_up_cache.py	`97.50% <97.50%> (ø)`
superset/charts/api.py	`87.78% <100.00%> (+0.70%)`	⬆️
superset/charts/commands/exceptions.py	`94.00% <100.00%> (+0.38%)`	⬆️
superset/charts/schemas.py	`99.38% <100.00%> (+0.01%)`	⬆️
superset/datasets/api.py	`88.65% <100.00%> (+0.70%)`	⬆️
superset/datasets/commands/exceptions.py	`94.36% <100.00%> (+0.24%)`	⬆️
superset/datasets/commands/warm_up_cache.py	`100.00% <100.00%> (ø)`
superset/datasets/schemas.py	`97.64% <100.00%> (+0.16%)`	⬆️

... and 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

superset/cachekeys/api.py

superset/cachekeys/commands/exceptions.py

superset/cachekeys/api.py

tests/integration_tests/cachekeys/api_tests.py

dpgaspar

LGTM.
Can you add the following line on UPDATING.md

- [22809](https://github.com/apache/superset/pull/23853): Migrated endpoint `/superset/warm_up_cache` to `/api/v1/cachekey/warm_up_cache/`. Corresponding permissions are `can warm_up_cache on Superset` to `can warm_up_cache on cachekey`. Make sure you add/replace the necessary permissions on any custom roles you may have.

superset/cachekeys/commands/warm_up_cache.py

jfrag1 · 2023-05-18T06:43:35Z

An alternative could be to place it on api/v1/chart and/or api/v1/dataset

I would model these endpoints under the resources they refer to. I feels really weird for me treating cache warm up as a first-level resource.

Moved to api/v1/chart

dpgaspar · 2023-05-26T08:35:34Z

tests/integration_tests/charts/api_tests.py

+                "table_name": "energy_usage",
+                "db_name": get_example_database().database_name,
+            },
+        )


Seems weird to be warming up cache for a dataset on /api/v1/chart/.... The idea would be to create a new endpoint on dataset's and perhaps use the same Command, or split it your call. the PUT payload would be restricted to it's resource

dpgaspar

LGTM, I'll defer to @john-bodley since he has more context here

dpgaspar · 2023-06-14T14:40:37Z

superset/tasks/cache.py

-        req = request.Request(url, headers=headers)
+        baseurl = "{WEBDRIVER_BASEURL}".format(**app.config)
+        url = f"{baseurl}api/v1/chart/warm_up_cache"
+        logger.info("Fetching %s with payload %s", url, data)


nit: checked that data contains the get_payload that only contains chart_id and dashboard_id so it's probably safe to log, but I would just remove it from the logs, it can also generate a huge log entry

First draft

91ee895

pull-request-size bot added the size/L label Apr 27, 2023

Cleanup refs to old endpoint

51725d5

jfrag1 added 6 commits April 27, 2023 13:31

Fix test

ea265bf

Fix lint

db02b9c

Merge branch 'master' into jack/migrate-warm-up-cache-to-api-v1

ab1150e

Merge branch 'master' into jack/migrate-warm-up-cache-to-api-v1

8f633bd

Fix formatting

698e2d6

Run isort

5bf548f

dpgaspar reviewed May 3, 2023

View reviewed changes

jfrag1 added 10 commits May 5, 2023 08:36

slice -> chart

778abd8

fix desc and summary

3aa8ac9

No user-provided input in errors

5b583bc

get -> put and move query params to json payload

9cec54f

Add api/commands tests

f08d7bd

Formatting

213a72a

Merge branch 'master' into jack/migrate-warm-up-cache-to-api-v1

2665079

Fix merge conflict

6c10de8

isort

0061947

Adjust warm up cache task

f7c297f

pull-request-size bot added size/XL and removed size/L labels May 5, 2023

jfrag1 added 3 commits May 5, 2023 11:41

Formatting

9606c28

Fix doc string

cfe2e30

Fix urllib request

b53339c

dpgaspar reviewed May 8, 2023

View reviewed changes

superset/cachekeys/commands/warm_up_cache.py Outdated Show resolved Hide resolved

dpgaspar requested review from villebro, michael-s-molina and john-bodley May 8, 2023 08:56

Fix cache warmup task

f4df657

pull-request-size bot added size/XL and removed size/L labels May 18, 2023

jfrag1 added 5 commits May 17, 2023 23:19

Merge branch 'master' into jack/migrate-warm-up-cache-to-api-v1

0423fa0

Address comments

582e311

Run black

b6c6f46

Fix import order

01dd08b

Fix permission test

7779d27

jfrag1 added 3 commits May 17, 2023 23:51

Fix type annotations

1e7d008

Merge branch 'master' into jack/migrate-warm-up-cache-to-api-v1

5f06fe1

Fix merge conflict

fe5b65c

dpgaspar reviewed May 26, 2023

View reviewed changes

jfrag1 added 7 commits June 9, 2023 15:03

Fix merge conflicts

54c4cf6

Split into two endpoints/commands

4dcd2fc

Dict -> dict

0d3f58a

Fix openapi schema ref

13d1529

Fix lint

c9bc42f

Fix test

dff0d4f

Fix security test

bcb88d1

dpgaspar approved these changes Jun 14, 2023

View reviewed changes

dpgaspar requested a review from john-bodley June 14, 2023 14:50

john-bodley approved these changes Jun 16, 2023

View reviewed changes

Fix merge conflicts

fcb89ad

dpgaspar merged commit 5af298e into apache:master Jun 20, 2023

dpgaspar deleted the jack/migrate-warm-up-cache-to-api-v1 branch June 20, 2023 11:08

mapledan mentioned this pull request Nov 16, 2023

chore: exempt cache-warmup endpoint from CSRF protection #25978

Closed

9 tasks

mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 3.0.0 labels Mar 13, 2024

vinothkumar66 pushed a commit to vinothkumar66/superset that referenced this pull request Nov 11, 2024

chore: Migrate warm up cache endpoint to api v1 (apache#23853)

0a1e22c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Migrate warm up cache endpoint to api v1 #23853

chore: Migrate warm up cache endpoint to api v1 #23853

jfrag1 commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading

dpgaspar left a comment

jfrag1 commented May 18, 2023

dpgaspar May 26, 2023

dpgaspar left a comment •

edited

Loading

dpgaspar Jun 14, 2023

chore: Migrate warm up cache endpoint to api v1 #23853

chore: Migrate warm up cache endpoint to api v1 #23853

Conversation

jfrag1 commented Apr 27, 2023 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Apr 27, 2023 • edited Loading

Codecov Report

dpgaspar left a comment

Choose a reason for hiding this comment

jfrag1 commented May 18, 2023

dpgaspar May 26, 2023

Choose a reason for hiding this comment

dpgaspar left a comment • edited Loading

Choose a reason for hiding this comment

dpgaspar Jun 14, 2023

Choose a reason for hiding this comment

jfrag1 commented Apr 27, 2023 •

edited

Loading

codecov bot commented Apr 27, 2023 •

edited

Loading

dpgaspar left a comment •

edited

Loading