-
Notifications
You must be signed in to change notification settings - Fork 13.4k
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.
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
feat: Adds a key-value endpoint to store the state of dashboard filters #17536
feat: Adds a key-value endpoint to store the state of dashboard filters #17536
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17536 +/- ##
==========================================
- Coverage 77.01% 76.71% -0.30%
==========================================
Files 1049 1062 +13
Lines 56682 56997 +315
Branches 7851 7851
==========================================
+ Hits 43651 43723 +72
- Misses 12778 13021 +243
Partials 253 253
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great!
Can you add some tests for PUT and DELETE as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Quick first pass, looking really good! Will review/test more tomorrow!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a few comments around moving logic out of DAO layer and into the CMDs. Would also be great to see a few more tests, especially around the CMDs themselves.
@betodealmeida @villebro @craig-rueda Tackled all your comments. Can you review it again? Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great! I left a few small comments, but it looks good to go, IMHO.
d81faf1
to
b9004e0
Compare
f2b70e5
to
7c76172
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments - tested, and works really nicely, really looking forward to this!
06d3836
to
458d468
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…rd filters (apache#17536)" This reverts commit 2f2e8fe.
from tests.integration_tests.fixtures.world_bank_dashboard import ( | ||
load_world_bank_dashboard_with_slices, | ||
) | ||
from tests.integration_tests.test_app import app |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
app
is reimported here.
mypy hook should have given error in ci too as my local does.
not sure how it was skipped.(different version or ignore config or mypy doesn't run in pipeline ?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing that out @mayurnewase. I submitted a fix in #17887. @villebro Do you have any idea how pre-commit
and CI missed that?
SUMMARY
Currently, the filters state is being stored in the URL to allow users to preserve their current interactions in case of a refresh and also to share specific states with other users. The most common example is when a user, after exploring a dashboard using filters, wishes to share their findings with other users.
The problem with this approach is that when the size of the state is big, it provokes URL length problems with the major browsers.
The proposed solution in this PR is to store the filters state on the server-side and keep in the URL only a secure key that gives access to the stored content. To do that, we'll extend the current dashboard endpoint with an additional path that supports the storage of information in the form of key-value entries. By making this endpoint relative to the main resource we can reuse the security model and access grants.
The additional endpoint and the supported operations are:
The keys are generated on the server-side using the https://docs.python.org/3/library/secrets.html#secrets.token_urlsafe function and the value can be any JSON supported text.
To store the key-value entries, this PR uses Flask-Caching, which enables support to different backends like Redis, Memcached, file system, etc. It also allows the configuration of expiration times for the stored entries in order to discard the ones that are not being used anymore and optimize the required storage space.
This PR also allows a configuration to preserve actively accessed entries by renewing the expiration time every time a value is retrieved.
Follow-up of #17337
TESTING INSTRUCTIONS
1 - Execute Python tests
2 - All tests should pass
ADDITIONAL INFORMATION