mikebridge commented on PR #42540:
URL: https://github.com/apache/superset/pull/42540#issuecomment-5689115972
Thanks — reproduced this before fixing it, and the composition is exactly as
described: the caller set `{saved, sqlExpression}` unions with a saved-only
semantic view's `{simple, sqlExpression}` to disable all three, and
`defaultActiveTabKey`'s `.find(...) ?? preferredTabKey` then resolved to
`simple`, which is disabled.
Fixed in `e7af87697a`. Two parts, since one of them is a bug independent of
the reconciliation:
**The reconciliation.** A caller's `saved` restriction is now lifted only
when the datasource uses Saved classification *and* the provider has not itself
disabled `saved`. Those callers hide Saved to exclude calculated expressions;
under Saved classification it holds ordinary dimensions instead, so the
restriction no longer matches its own intent. Provider capability still wins,
so no unsupported expression mode is ever enabled.
I considered relaxing the *semantic* restriction on `simple` instead, and
rejected it: that would override a provider's prohibition on adhoc column
expressions, and leave the dimensions in the wrong classification. A generic
"drop the least justified restriction when everything is disabled" rule was
also rejected — it cannot tell provider capability from caller intent.
**The independent bug.** `?? preferredTabKey` could resolve to a disabled
tab whenever nothing was enabled, regardless of semantic layers.
`defaultActiveTabKey` is now `undefined` in that case, the effect no longer
emits a tab callback with a disabled key, and a genuinely all-disabled control
renders a warning with Close rather than unusable tabs. That case is now
impossible by construction rather than patched at the call site.
Regression uses the callers' actual `disabledTabs` value, asserting Saved is
selected, Simple and Custom SQL stay disabled, a dimension is reachable, and
saving returns the original dimension — plus cases for table and
expression-capable semantic datasources keeping the caller's Simple-only
restriction, and for the all-disabled configuration. Control was run by
removing every production change and confirming with `git diff --exit-code`
that the file matched this PR's prior head before re-running.
Good catch — the union only bites with those specific caller values, which
is why the existing tests missed it.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
