Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore: Migrate /superset/user_slices and /superset/fave_slices to API v1 #22964

Merged
merged 8 commits into from
Apr 3, 2023
Merged

chore: Migrate /superset/user_slices and /superset/fave_slices to API v1 #22964

merged 8 commits into from
Apr 3, 2023

Conversation

diegomedina248
Copy link
Contributor

SUMMARY

Continuing the effort on deprecating all /superset/ REST API endpoints
Deprecates /superset/user_slices for /api/v1/chart/user_slices/
Deprecates /superset/fave_slices for /api/v1/chart/user_slices/

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
Copy link

codecov bot commented Feb 2, 2023 •
edited
Loading

Codecov Report

Merging #22964 (d343bf4) into master (81b32d1) will increase coverage by 0.01%.
The diff coverage is 86.48%.

❗ Current head d343bf4 differs from pull request most recent head 6c282be. Consider uploading reports for the commit 6c282be to get more accurate results

@@            Coverage Diff             @@
##           master   #22964      +/-   ##
==========================================
+ Coverage   67.68%   67.69%   +0.01%     
==========================================
  Files        1914     1914              
  Lines       73928    73954      +26     
  Branches     8021     8021              
==========================================
+ Hits        50036    50062      +26     
  Misses      21849    21849              
  Partials     2043     2043
Flag Coverage Δ
hive 52.73% <57.69%> (+<0.01%) ⬆️
javascript 53.89% <81.81%> (+<0.01%) ⬆️
mysql 78.49% <88.46%> (+0.01%) ⬆️
postgres 78.56% <88.46%> (+0.01%) ⬆️
presto 52.65% <57.69%> (+<0.01%) ⬆️
python 82.40% <88.46%> (+0.01%) ⬆️
sqlite 77.06% <88.46%> (+0.01%) ⬆️
unit 52.63% <57.69%> (+<0.01%) ⬆️

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

Impacted Files Coverage Δ
superset/charts/api.py 85.71% <ø> (ø)
superset/constants.py 100.00% <ø> (ø)
...rset-frontend/src/profile/components/Favorites.tsx 80.00% <66.66%> (+2.22%) ⬆️
superset/models/slice.py 86.79% <80.00%> (+1.28%) ⬆️
...ontrols/AnnotationLayerControl/AnnotationLayer.jsx 74.40% <87.50%> (ø)
superset/models/helpers.py 38.65% <88.88%> (+0.48%) ⬆️
superset/charts/filters.py 88.40% <90.00%> (+0.07%) ⬆️
superset/views/core.py 74.58% <100.00%> (+0.03%) ⬆️

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

@diegomedina248 diegomedina248 force-pushed the dm/chore/migrate-user-fav-slices-to-api-v1 branch 8 times, most recently from 6a52aa5 to 425df90 Compare February 2, 2023 16:12
@diegomedina248 diegomedina248 force-pushed the dm/chore/migrate-user-fav-slices-to-api-v1 branch from 425df90 to 17667df Compare February 2, 2023 16:39
john-bodley
john-bodley reviewed Feb 3, 2023
View reviewed changes
superset/charts/commands/get_slices.py Outdated
slices = ChartDAO.user_slices(cast(int, self._user_id))

payload = []
for o in slices:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a tendency to use slc rather than the more opaque o to represent a slice. That said I’m not sure what o actually represents given that is presented as o.Slice.

superset/charts/commands/get_slices.py Outdated
return payload

def validate(self) -> None:
if not self._user_id:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we do this elsewhere but mutating state inside validate seems wrong to me.

superset/charts/dao.py Outdated
)
.filter( # pylint: disable=comparison-with-callable
or_(
Slice.id.in_(owner_ids_query),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason this isn’t a join as opposed to a subquery? I suspect the subquery likely fetches significantly more slices than the user favorited.

dpgaspar
dpgaspar reviewed Feb 7, 2023
View reviewed changes
superset/charts/api.py Outdated
@@ -914,3 +916,43 @@ def import_(self) -> Response:
)
command.run()
return self.response(200, message="OK")

@expose("/user_slices/")
@expose("/user_slices/<int:user_id>/")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could we just use api/v1/chart/?q=(filters:!((col:owners,opr:rel_m_m,value:<USER_ID>)),order_direction:desc,page:0,page_size:25)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mm, would that return favorited slices too?

@pull-request-size pull-request-size bot added size/L and removed size/XL labels Feb 8, 2023
@diegomedina248 diegomedina248 force-pushed the dm/chore/migrate-user-fav-slices-to-api-v1 branch from 68f537d to 72fb5bb Compare February 8, 2023 00:50
@diegomedina248 diegomedina248 force-pushed the dm/chore/migrate-user-fav-slices-to-api-v1 branch from 72fb5bb to 4a027f2 Compare February 8, 2023 00:57
Antonio-RiveroMartnez
Antonio-RiveroMartnez approved these changes Apr 3, 2023
View reviewed changes
Copy link
Member

@Antonio-RiveroMartnez Antonio-RiveroMartnez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

jfrag1
jfrag1 approved these changes Apr 3, 2023
View reviewed changes
Copy link
Member

@jfrag1 jfrag1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@hughhhh hughhhh merged commit cdc7af1 into master Apr 3, 2023
@mistercrunch 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
@mistercrunch mistercrunch deleted the dm/chore/migrate-user-fav-slices-to-api-v1 branch March 26, 2024 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dpgaspar dpgaspar dpgaspar left review comments

@john-bodley john-bodley john-bodley left review comments

@Antonio-RiveroMartnez Antonio-RiveroMartnez Antonio-RiveroMartnez approved these changes

@jfrag1 jfrag1 jfrag1 approved these changes

Assignees
No one assigned
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels size/L 🚢 3.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@diegomedina248 @dpgaspar @john-bodley @Antonio-RiveroMartnez @jfrag1 @mistercrunch @hughhhh