[GitHub] [incubator-superset] villebro opened a new pull request #7816: Add cache_key_wrapper to Jinja template processor

[GitBox <gi...@apache.org>]

villebro opened a new pull request #7816: Add cache_key_wrapper to Jinja template processor
URL: https://github.com/apache/incubator-superset/pull/7816
 
 
   ### CATEGORY
   
   Choose one
   
   - [x] Enhancement (new features, refinement)
   - [x] Add tests
   - [x] Documentation
   
   ### SUMMARY
   Currently using dynamic filters that change based on the logged in user can unintended consequences when caching is enabled. For example, if a where-clause references the `currenct_username()` function to filter only rows for the currently logged in user, the result will be cached with the same key that another user would get, despite both users getting different results from the `current_username()` function. This is because the rendered result of calling `current_username()` is never stored in the `query_obj` that is the basis for the cache key.
   
   This PR adds a new function `cache_key_wrapper` to the jinja context, which can be wrapped around any function call, and stores the called values in a list `extra_cache_keys`, which are added to the `cache_dict` prior to hashing. This ensures that both users get unique values when referencing the same datasource. In practice this is done by "compiling" the query before calculation of the cache key, and storing all values that have been passed to `cache_key_wrapper`, which is then considered when calling the `cache_key` function in `viz.py` (legacy) and `query_context.py`/`query_object.py` (future). This adds some overhead, as the full SQLAlchemy selectable has to be generated and compiled once prior to cache key calculation, and again if there isn't a cache hit. This could be easily optimized, but since the overhead is rather unnoticeable, I decided against it in favor of code readability.
   
   ### SCREENSHOT OF NEW DOCS
   ![image](https://user-images.githubusercontent.com/33317356/60618894-114ced00-9de0-11e9-9020-745f974f935c.png)
   
   ### TEST PLAN
   Tested locally + CI
   
   ### ADDITIONAL INFORMATION
   - [x] Has associated issue: Fixed #7580 
   - [x] Introduces new feature or API
   
   ### REVIEWERS
   @mistercrunch @betodealmeida @john-bodley @duffar12

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org