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

fix(Jinja): Extra cache keys for Jinja columns #30715

Merged
merged 3 commits into from
Oct 25, 2024

Conversation

geido
Copy link
Member

@geido geido commented Oct 25, 2024

SUMMARY

This PR fixes an issue where Jinja in columns was not considered during cache key checks, leading to cache spillover.

TESTING INSTRUCTIONS

  1. Create an aggregated table chart using the Vehicle Sales dataset from the examples connection.
  2. Create below dimensions:
    a. '{{current_user_id()}}' as id
    a. '{{current_username()}}' as username
    a. '{{current_user_email()}}' as email
    a. '{{url_param('test', 'default')}}' as url_param
  3. Set the row limit to 1.
  4. Save the chart to a dashboard and access it.
  5. Notice that the user-related macros will show your account data, and the url_param column will show default (which is set in the macro to be returned when the param is not defined).
  6. Copy the dashboard URL, and append &test=blah to it (after the native_filters_key value) and access it.
  7. The url_param column should now how the param value (blah).

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

@dosubot dosubot bot added the global:jinja Related to Jinja templating label Oct 25, 2024
@geido
Copy link
Member Author

geido commented Oct 25, 2024

/testenv up

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM but I suggest using a type guard for the check

superset/connectors/sqla/models.py Outdated Show resolved Hide resolved
Copy link
Contributor

@geido Ephemeral environment spinning up at http://34.210.89.190:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

Copy link
Member

@villebro villebro left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this critical fix!

@geido geido merged commit a12ccf2 into master Oct 25, 2024
33 checks passed
Copy link
Contributor

Ephemeral environment shutdown and build artifacts deleted.

@michael-s-molina michael-s-molina added the v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch label Oct 25, 2024
@michael-s-molina
Copy link
Member

LGTM, thanks for this critical fix!

@sadpandajoe I'm adding this PR to the release blockers based on this comment.

@Vitor-Avila
Copy link
Contributor

Thank you so much for working on this!!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
global:jinja Related to Jinja templating size/M v4.1 Label added by the release manager to track PRs to be included in the 4.1 branch
Projects
Status: Release Blockers
Development

Successfully merging this pull request may close these issues.

5 participants