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.

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

[cache] Fixing cache key w/ merged extra filters #3809

Merged
merged 1 commit into from
Nov 15, 2017

Conversation

john-bodley
Copy link
Member

I was alerted to this issue when after force refreshing a dashboard and then exploring the slice, I observed that the slice wasn't cache and/or there were inconsistencies in the cached values.

It turns out that the reason is the cache key for a slice is different from these views, as the dashboard key contained the extra_filters which were absent from the slice view. The remedy was to simply update the cache key logic to mutate the form data which is being used as the cache key by calling the merge_extra_filters method. Note I don't have full context over these extra filters or whether this call should be abstracted in some way.

Also the merge_extra_filters method previously only handled the case when the extra_filters field was non-empty, however this logic needed to be loosen to handle the case of the field being present to ensure that the field is removed.

Note I'm uncertain whether an additional test case is required. I had something of the form,

    def test_cache_payload_from_dashboard(self):
        dash = (
            db.session.query(models.Dashboard)
            .filter_by(slug='births')
            .first()
        )

        # An HTTP request for the dashboard caches all the slices.
        self.client.get(dash.url)

        for slice in dash.slices:
            payload = slice.viz.get_payload()
            assert payload['is_cached']

however the dash.url leverages async functionality and thus I'm uncertain how best to wait for the page content to load.

@coveralls
Copy link

coveralls commented Nov 9, 2017

Coverage Status

Coverage increased (+0.008%) to 71.453% when pulling 34266aca2787091ec0c1f2a9dba89ab7d5c8ce0d on john-bodley:john-bodley-cache-debug into 70c7315 on apache:master.

@mistercrunch
Copy link
Member

Good catch. I was trying to understand whether we could just run the merge function once only early on in the stack (early in the view perhaps). I'm ok merging this as-is as well.

Whatever we do we should make sure that this feature will work as expected and show the altered tag shows when loading a slice from a filtered dash (I think it would work either way):
#3668

I found a low-pri bug around that same funcitonality/method today:
#3824

@mistercrunch
Copy link
Member

Happy merging once conflicts are resolved.

@john-bodley
Copy link
Member Author

@mistercrunch I've resolved the merge conflicts.

@mistercrunch mistercrunch merged commit ba89b2d into apache:master Nov 15, 2017
michellethomas pushed a commit to michellethomas/panoramix that referenced this pull request May 24, 2018
wenchma pushed a commit to wenchma/incubator-superset that referenced this pull request Nov 16, 2018
@mistercrunch mistercrunch added 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0 labels Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 0.21.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants