aminghadersohi commented on PR #43309: URL: https://github.com/apache/superset/pull/43309#issuecomment-5348868452
Confirmed, and that kills the gating idea — thanks, I'd have shipped a fix with a hole in it. The Advanced tab that hosts the checkbox is gated on `useTabLayout = isEditMode || useSqlAlchemyForm` (`DatabaseModal/index.tsx#L763`), and the only other render of `ExtraOptions` is inside `renderFinishState()` (`#L1999-L2002`), which runs after the connection exists. So on the create path through the dynamic form — exactly the path this bug is on — `impersonate_user` is never user-settable, and at Connect time it's whatever `#L1051-L1053` hardcodes. Reading the flag during validation would just be reading our own hardcoded `true`. Making the URL check loose to compensate would trade a false negative for a false positive, which is worse: we'd tell people the sheet is fine and let them find out at query time. Your clarification also lands differently than I'd read it the first time. "SA + DWD deprecated, checkbox unchecked and no longer wired to SA auth, queries always SA-only" is a coherent model, and it's better than what's there now — the current design authenticates validation as one identity and queries as another, and no amount of patching inside `validate_parameters` fixes that split. It does mean an existing SA + DWD connection that relies on per-user delegation stops working and has to move to native OAuth2, so I agree it's SIP territory rather than something to slip into a bug fix. So I don't think this PR should carry the behavior change. Two ways to land it, and I'd rather you pick than guess: 1. **Close it** in favor of the SIP, and let the deprecation carry the fix. Cleanest if the SIP is likely to happen soon. 2. **Narrow it to the error message only** — no behavior change at all, just stop reporting every failure in `_can_read_url` as "check for typos and make sure that 'Type of Google Sheets allowed' selection matches the input" and surface the actual cause. That message is wrong for `invalid_grant` today, and it's equally wrong for a permission error or a network blip; it sends people to re-check a URL that was never the problem. This is useful whichever way the SIP goes, and it's non-breaking. Happy to do either, or to help with the SIP write-up if that's where this is heading. Not asking for a merge on the current diff either way — the two commits on it now are the incomplete fix plus a test, and I'll strip or rewrite them to match whichever option you want. -- 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]
