sadpandajoe commented on code in PR #44955:
URL: https://github.com/apache/superset/pull/44955#discussion_r4192328862
##########
superset-frontend/packages/superset-ui-chart-controls/src/utils/headerGroups.ts:
##########
@@ -329,22 +358,52 @@ export function syncTimeComparisonGroups(
* Remap saved comparison-column config onto locale-independent keys.
*
* Charts created before comparison slots were stored as `Main …` may still
- * keep `t('Main') …` entries (for example `Principale revenue`). The column
- * editor lists the stored keys, so those legacy entries must be rewritten
- * before it filters `column_config` down to `colnames`.
+ * keep `t('Main') …` entries (for example `Principale revenue`). A buggy
+ * column editor also wrote `COLUMN_NAME_ALIASES` display names (for example
+ * `Time` for `__timestamp`). The column editor lists the stored keys, so
+ * those legacy entries must be rewritten before it filters `column_config`
+ * down to `colnames`.
*/
export function normalizeColumnConfigKeys<T>(
value: Record<string, T> | null | undefined,
colnames: string[],
+ knownKeys: string[] = colnames,
): Record<string, T> {
if (!value) {
return {};
}
const colnamesSet = new Set(colnames);
+ const knownKeySet = new Set(knownKeys);
+ const aliasToKey = new Map<string, string>();
+ const ambiguousAliases = new Set<string>();
+ colnames.forEach(col => {
+ getColumnNameAliasLabels(col).forEach(alias => {
+ // A saved key that names a real column or metric is not a legacy alias.
+ if (!alias || knownKeySet.has(alias) || ambiguousAliases.has(alias)) {
+ return;
+ }
+ const existing = aliasToKey.get(alias);
+ if (existing && existing !== col) {
+ aliasToKey.delete(alias);
+ ambiguousAliases.add(alias);
+ return;
+ }
+ aliasToKey.set(alias, col);
+ });
+ });
const next: Record<string, T> = {};
Object.entries(value).forEach(([key, config]) => {
+ if (colnamesSet.has(key)) {
+ if (!(key in next)) {
Review Comment:
With `Time` saved before `__timestamp` (e.g. `{ Time: { customColumnName:
'Legacy' }, __timestamp: { customColumnName: 'Canonical' } }`, `colnames =
['__timestamp']`), the legacy entry is remapped onto `__timestamp` first, and
this guard then skips the real-key entry because `key in next`. The result is
`{ __timestamp: { customColumnName: 'Legacy' } }`: Table/AG Grid render the
stale value, and the next edit of any column persists it and drops the
canonical one. The same ordering loses to a localized leftover like `Principale
revenue` placed before `Main revenue`. Before this commit the `key === target`
check let the real key overwrite. Should a key that is itself a current column
always win, i.e. assign `next[key] = config` here without the `!(key in next)`
check?
--
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]
