SQL Lab "select all" tables is dangerous

[giftig]

In SQL Lab, when you select a table it will automatically show the table schema and add a preview tab for some rows for the table. It makes a few backend requests for each one, as well as running a couple of queries against the database to fetch this data. It'll take at least a few seconds to do this, typically.

In 2.1, I've noticed there's a new feature: "Select All". This is problematic for us as we have thousands of tables, and the result of any one of our thousands of users (even accidentally) hitting "select all" will be locking up both their browser and our database cluster for a significant length of time.

This needs to be feature flagged at least, as we need it disabled, but in my opinion it's a mis-feature. It's likely our thousands of tables are more than many users have, but even with comparatively small numbers of tables it's implemented in a very inefficient way in terms of backend calls (each preview actually does a GET to one endpoint which retrieves a query to run, and then a POST to the general execute SQL json endpoint to execute that query, and performs a total of 6-7 backend requests for every individual table). And even if it were made more efficient, it's still necessary to run many distinct queries in quick succession.

With a handful of tables it's quite easy to select them all from the list, and with more than a handful it's unlikely to be useful to preview all the tables (and also unlikely you'd need to join across all the tables).

Any thoughts? Are others finding this feature useful, or problematic?

[giftig]

I've just had a little dig around in src/components/Select/Select.tsx and it looks like this "select all" feature is now included in the select component as a whole, and can't be disabled if the select mode is not "single". So it looks like a wide-reaching change and the impact of having it enabled in this specific case wasn't considered.

I'd suggest the way to resolve this, then, is add another option to the Select component to allow explicitly disabling the select all feature, and have selectAllEnabled respect that flag even in multiple-select mode. Then have renderTableSelect in src/components/TableSelector/index.tsx provide that option and disable the feature.

I'll look into making this change tomorrow and raise a PR for discussion.

[rusackas]

You CAN disable the Select All functionality on a per-component basis.

In superset-frontend/src/components/TableSelector/index.tsx on line 283 (as of this writing, anyway), you'll find the Select component in question. Just add allowSelectAll={false} and you'll be all set.

Now... for the bigger issue, of "should it even be there?"

In this case, I think I'm on your side, that this does more harm than good. If you want to open a PR disabling it, we can hash out any discussion there. Another option might be to find a way to make it dynamic, i.e. allowing Select All if there are less than ten tables or something along those lines. Then it can provide the handy utility for smaller instances that like it, but not explode anyone's server.

CC @yousoph @kasiazjc in case they have particular context around how much people are clamoring to have that Select All in this particular menu.

[giftig]

allowSelectAll was added just last month, so looks like I was behind on that -- I was looking at our fork based on the 2.1 branch, where it doesn't exist yet. The fact that the flag now already exists certainly makes things easier (and maybe validates that this has already been spotted as a problem in other places).

Making it dynamic the way you describe should be easy enough; it already does a check like

        selectOptions.length > 0 &&
        enabledOptions.length > 1 &&

which could have something like an enabledOptions.length < x condition added. It'd probably need to be configurable, though; the sensible maximum number to allow would probably differ quite a bit in different contexts. My feeling is it might be overkill; just making use of allowSelectAll and turning it off here is probably ok.

[giftig]

Looks like @justinpark beat me to raising the PR for this one (#24120); thanks for the quick turnaround!