feat: Adds a key-value endpoint to store charts form data

[michael-s-molina]

SUMMARY

Currently, the Explore state is being stored in the URL to allow users to preserve their current interactions in case of a refresh and also to transfer context between the dashboard and Explore. 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 Explore 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 chart 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:

POST api/v1/chart/<id>/form_data

BODY 
{
   // Any type of JSON supported text.
   value: <string>,
}

RESPONSE
{
   // The key generated on the server-side
   key: <string>
}

PUT api/v1/chart/<id>/form_data/<key>

BODY
{
   // Any type of JSON supported text.
   value: <string>,
}

RESPONSE
{
   // The result of the operation
   message: <string>
}

GET api/v1/chart/<id>/form_data/<key>

RESPONSE
{
   // Any type of JSON supported text.
   value: <string>,
}

DELETE api/v1/chart/<id>/form_data/<key>

RESPONSE
{
   // The result of the operation
   message: <string>
}

All the endpoints accept an optional query parameter called dataset_id that is used for the cases when a chart is not saved yet but we want to preserve the state. In this case, the chart ID shall be set to 0:

POST api/v1/chart/0/form_data?dataset_id=<id>

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.

TESTING INSTRUCTIONS

1 - Execute Python tests
2 - All tests should pass

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]

Codecov Report

Merging #17882 (fca7ace) into master (3a9bd12) will decrease coverage by 0.38%.
The diff coverage is 98.28%.

@@            Coverage Diff             @@
##           master   #17882      +/-   ##
==========================================
- Coverage   67.10%   66.71%   -0.39%     
==========================================
  Files        1612     1627      +15     
  Lines       64999    65428     +429     
  Branches     6872     6872              
==========================================
+ Hits        43616    43653      +37     
- Misses      19513    19905     +392     
  Partials     1870     1870              

Flag	Coverage Δ
hive	52.15% <50.34%> (-1.17%)	⬇️
mysql	81.17% <98.28%> (-1.05%)	⬇️
postgres	81.22% <98.28%> (-1.05%)	⬇️
presto	52.00% <50.34%> (-1.16%)	⬇️
python	81.66% <98.28%> (-1.05%)	⬇️
sqlite	80.91% <98.28%> (-1.04%)	⬇️

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

Impacted Files	Coverage Δ
superset/key_value/commands/entry.py	100.00% <ø> (ø)
superset/charts/form_data/commands/update.py	90.90% <90.90%> (ø)
superset/charts/form_data/commands/create.py	94.11% <94.11%> (ø)
superset/charts/form_data/commands/delete.py	94.73% <94.73%> (ø)
superset/charts/form_data/commands/get.py	95.65% <95.65%> (ø)
superset/charts/commands/exceptions.py	91.48% <100.00%> (+0.37%)	⬆️
superset/charts/form_data/api.py	100.00% <100.00%> (ø)
superset/charts/form_data/utils.py	100.00% <100.00%> (ø)
superset/config.py	91.74% <100.00%> (+0.10%)	⬆️
superset/dashboards/filter_state/api.py	100.00% <100.00%> (ø)
... and 69 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3a9bd12...fca7ace. Read the comment docs.

[villebro]

First pass comments. The new API design looks good, but I think we may still need some further fine tuning.

[michael-s-molina]

First pass comments. The new API design looks good, but I think we may still need some further fine tuning.

Great comments @villebro. I tackled them all.

[villebro]

A few last comments, but looking really good 👍 LMKWYT

[villebro]

Approving to unblock this PR as the majority of my comments were not blocking.

[michael-s-molina]

I can create the same cache of form_data and not any limitation. Is it by design?

@zhaoyongjie Yes. It's by design. This is especially useful when a user shares a URL with someone. We save the same content over a different key so that any modification made by a user does not affect the other one.

The problem is that the same user session and the same query payload. It will generate the different cache keys. In the other word, EVERY HTTP request will generate a new record or entity in cache system. It will quickly consume the cache system.

That's not how it will work. A new key will be generated only when mounting. After that, we'll use the PUT method to replace the values.

[ktmud]

Thanks for tackling this problem. I have a couple of questions and would be curious to hear your thoughts:

1. We already has a short url generator that produces a numeric ID. Will this replace it?
2. Since the key is after a chart id, does this mean when users create a new chart, we'd have to create a new chart object first, too? IIRC, currently we don't do that.
3. We already have a KeyValue model that is more persistent than Flask-Caching. Is there a reason why we are not reusing it? It feels to me both dashboard filter states and explore chart states should be persistent since they will produce sharable URLs. We may need a mechanism to periodically delete old records to reduce database size, but that's better than cache server failure causing a URL to go stale at an unpredictable time.

[ktmud]

According to the doc, the cache type name filesystem is deprecated since 1.9.1. Superset is using 1.10.1.

[michael-s-molina]

Hi @ktmud. These are all great questions that also appeared while we were discussing this feature internally. This is a great opportunity to post the conclusions of those discussions 😄

1. We already have a short URL generator that produces a numeric ID. Will this replace it?

We considered using this generator but the problem was its numerical characteristics. It allows a user to guess potential IDs and it's not ideal for the type of information that could be stored. The idea is to replace the short URL generator with this new approach while maintaining backward compatibility. This will be handled by another PR that is already in development.

2. Since the key is after a chart id, does this mean when users create a new chart, we'd have to create a new chart object first, too? IIRC, currently we don't do that.

We won't need to create a new chart object first. This part in the PR's description talks about it:

All the endpoints accept an optional query parameter called dataset_id that is used for the cases when a chart is not saved yet but we want to preserve the state. In this case, the chart ID shall be set to 0:

POST api/v1/chart/0/form_data?dataset_id=<id>

In this case, the primary resource is the dataset and all its security rules are applied.

3. We already have a KeyValue model that is more persistent than Flask-Caching. Is there a reason why we are not reusing it? It feels to me both dashboard filter states and explore chart states should be persistent since they will produce sharable URLs. We may need a mechanism to periodically delete old records to reduce database size, but that's better than cache server failure causing a URL to go stale at an unpredictable time.

That was in fact our first approach. We discarded this solution because:

• The numerical nature of the IDs
• The lack of management properties associated with the entries prevents the maintainability of the records.
• This feature is behind a feature flag and is disabled by default. Changing its state constitutes a breaking change.
• We would need to develop the management system (we even did that during the POC). This logic is already handled by Flask-Caching.
• We wanted to provide administrators with the possibility of choosing which persistence mechanism is best suited for their use cases. Some will use Redis as the main persistence mechanism while periodically storing the memory content on the disk. This can ensure performance and persistence at the same time. Others will use the file system to store this type of information and so on.

According to the doc, the cache type name filesystem is deprecated since 1.9.1. Superset is using 1.10.1.

Oh, you're right! Thanks for pointing that out. I'll replace it with FileSystemCache.

[ktmud]

@michael-s-molina Thanks for the additional context! This answers some of my questions but I'm having a couple of more:

1. I understand why numerical IDs can be problematic in some cases but I'm not sure why would we treat temporary chart states differently than we do for a chart or dashboard itself? If we should not, then in the long run, do we plan to change how chart/dashboard/dataset id are generated as well?
2. /api/v1/chart/0/form_data?dataset_id=<id> works but the id == 0 check feels hacky. Why not just use a new endpoint? You may not even need chart_id or dataset_id in the API URI anyway since you can save that information in form data (or a wrapper payload for the whole temporary Explore state). I.e., when creating the new chart, users see the URL /explore/?dataset_id=xxx, the client makes a POST request to /api/v1/chart/0/form_data, API returns a form data key, then client URL is updated to /explore/?key=abceded, where the key would contain all the information the client needs---starting chart id or dataset id + updated full form data.

[ktmud]

I'm not sure we want to limit Explore state access only to those who can create charts. If users can read a chart, then they should be able to explore a chart as well.

This check is also inconsistent in terms of whether dataset access is used to prevent access to an Explore state. The Explore page also checks dataset access so the resource_id == 0 check seems redundant.

[michael-s-molina]

1. I understand why numerical IDs can be problematic in some cases but I'm not sure why would we treat temporary chart states differently than we do for a chart or dashboard itself? If we should not, then in the long run, do we plan to change how chart/dashboard/dataset id are generated as well?

I would advocate for changing them in the long run. Numerical IDs can facilitate Insecure Direct Object References and Cross Site Scripting (XSS) attacks and also open the door for Business Intelligence Leaks.

2. /api/v1/chart/0/form_data?dataset_id=<id> works but the id == 0 check feels hacky. Why not just use a new endpoint?

We considered using different endpoints for the saved/unsaved states, but it felt like overkill for a very specific use case because a new endpoint means new docs, tests, API, security rules, etc.

You may not even need chart_id or dataset_id in the API URI anyway since you can save that information in form data (or a wrapper payload for the whole temporary Explore state).

That's true and it was one of the options on the table but we preferred the more RESTy approach with the resource identifier and the query parameter. We also didn't want to force any kind of schema to the value.

I.e., when creating the new chart, users see the URL /explore/?dataset_id=xxx, the client makes a POST request to /api/v1/chart/0/form_data, API returns a form data key, then client URL is updated to /explore/?key=abceded, where the key would contain all the information the client needs---starting chart id or dataset id + updated full form data.

That's pretty much the current flow developed in the follow-up PR that I'm working on related to the client-side part 😉. Since the client is the owner of the value, they can control what type of information is stored, so I like your idea of also saving the chart ID and dataset ID alongside the value to keep only the key in the URL. I'll follow your suggestion in the client-side PR. The only thing is that the server shouldn’t account for this information and should continue to work with the resource identifier and query parameter.

I'm not sure we want to limit Explore state access only to those who can create charts. If users can read a chart, then they should be able to explore a chart as well.

Makes sense. Will change it.

This check is also inconsistent in terms of whether dataset access is used to prevent access to an Explore state.

Here we're assuming that if a user has access to a chart, it means that they have access to the dataset. That's why we only check the dataset when the chart is not saved.

The Explore page also checks dataset access so the resource_id == 0 check seems redundant.

That's true but we also need to consider a call to the API directly.

[ktmud]

Here we're assuming that if a user has access to a chart, it means that they have access to the dataset. That's why we only check the dataset when the chart is not saved.

What I meant is your access control when chart id is present doesn't seem to check dataset access, then why would it be necessary when chart id is not? We may want to change the former if the answer for my next question is yes (see below) but that would be a separate PR since it'd break some current workflows.

That's true but we also need to consider a call to the API directly.

The only information leaked would be the chart configuration, maybe some axis label names and metrics/columns used, which is different than accessing the dataset itself. Do we consider these sensitive--for those with global read access to charts?

[michael-s-molina]

What I meant is your access control when chart id is present doesn't seem to check dataset access, then why would it be necessary when chart id is not?

When the chart ID is present, the dataset access is indirectly checked because if the user has access to the chart it means they can see the content that is coming from the dataset, so if a user has access to a chart it means they also have access to the underlying dataset. If the chart ID is not present, it's necessary to check for dataset access so we can store dataset-related information.

The only information leaked would be the chart configuration, maybe some axis label names and metrics/columns used, which is different than accessing the dataset itself. Do we consider these sensitive--for those with global read access to charts?

We can store sensitive information that directly correlates to the data such as selected filter values, emitted filters, color properties associated with values, etc. We can also store information about the underlying database like schemas, tables, columns, etc.

[ktmud]

if the user has access to the chart it means they can see the content that is coming from the dataset, so if a user has access to a chart it means they also have access to the underlying dataset

This is not true. A user can select any dataset to be the datasource of their chart. We cannot automatically assign dataset access to anyone who was added as the owner (or reader, if that role exists) of a chart. These two has to be separate.

[ktmud]

I may have missed it, but I don't think we have RBAC for charts right now. I'm okay with blocking user access to temporary form data if they don't have access to a chart, but IMO users shouldn't automatically gain access to the dataset just because they have access to chart metadata. Knowing what columns or possible values may be in the dataset is very different than viewing the actual data in the dataset.

[michael-s-molina]

I was discussing your point internally and you were right! We have some cases where it's better to check for dataset access independently of chart access. I updated the check_access function accordingly and also the tests to reflect this change. Thank you for bringing this to my attention 🙂

[michael-s-molina]

Thank you reviewers for your great contributions. I will continue the work on the client-side now, where further adjustments can be made.