sadpandajoe commented on PR #43354: URL: https://github.com/apache/superset/pull/43354#issuecomment-5373426209
Thanks for digging into this — good catch, and you're right that the literal repro steps alone won't trigger it on every dashboard/build. Answering both parts: **Which flow actually leaks the key (not the "Edit chart" link):** You're correct that `EXCLUDED_URL_PARAMS` in the filter bar already strips `permalink_key` from the dashboard's own URL, and that "Edit chart" navigates via a clean `dashboard_page_id`/`slice_id` URL with no dashboard query params attached. Neither of those is the leak vector. The actual carrier is the memoized cache in `dashboard/util/charts/getFormDataWithExtraFilters.ts`, keyed only by `sliceId` (`cachedFormdataByChart[sliceId]` / `cachedFiltersByChart[sliceId]`). While the dashboard is mounted with a permalink in its URL, that cache gets populated with the chart's full form data, including `url_params.permalink_key` (from `merge_request_params`, same mechanism you found). When Explore mounts for that same `sliceId` and hits a cache match (`cachedFiltersByChart[sliceId] === filters`), it reuses the dashboard's cached form data rather than fetching fresh — leaking `url_params.permalink_key` along with it. `getFormDataWithDashboardContext.ts` then spreads `...exploreFormData, . ..dashboardContextFormData` (dashboard context last), so the leaked key wins and reaches `ExploreViewContainer`'s URL-rewrite. That cache-hit is a real precondition, not automatic — it depends on dashboard/filter state at the moment "Edit chart" is clicked (matching cached filters for that slice). In testing this reproduced on 1 of 8 seeded example dashboards, so a clean build/dashboard with no prior matching cache entry for that slice — which sounds like what you hit — will genuinely take the clean path and never carry `permalink_key` into Explore at all. That's consistent with your result, not a discrepancy in the fix. **Branch / backport:** This was reproduced and fixed against `master`. I checked whether the same code exists on the current release branch (`6.2`) before answering the backport question, rather than guessing: - `constants.ts` on `6.2` has the identical asymmetry — `RESERVED_CHART_URL_PARAMS` omits `permalink_key`, `RESERVED_DASHBOARD_URL_PARAMS` already includes it. - `ExploreViewContainer/index.tsx` on `6.2` consumes `RESERVED_CHART_URL_PARAMS` the same way. - `getFormDataWithExtraFilters.ts`'s `sliceId`-keyed cache and `getFormDataWithDashboardContext.ts`'s `...exploreFormData, ...dashboardContextFormData` override order are both present on `6.2` as well. So yes — this looks like a genuine backport candidate for `6.2` (and likely earlier 6.x branches sharing the same code), not a master-only issue. I haven't opened a backport PR; let me know if you'd like one against `6.2`. -- 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]
