aminghadersohi commented on code in PR #42390:
URL: https://github.com/apache/superset/pull/42390#discussion_r3717100423
##########
superset-frontend/src/components/ErrorMessage/OAuth2RedirectMessage.tsx:
##########
@@ -121,6 +121,7 @@ export function OAuth2RedirectMessage({
{ type: 'Schemas', id: 'LIST' },
{ type: 'Catalogs', id: 'LIST' },
'Tables',
+ 'TableMetadatas',
Review Comment:
Considered and intentionally keeping the unscoped tag. Three reasons:
**1. No resource identity is available here.** `OAuth2RedirectMessage` is
generic across all four `source` values, and the only thing it receives is the
`SupersetError`. The `OAUTH2_REDIRECT` payload carries `{url, tab_id,
redirect_uri}` and nothing else (`superset/exceptions.py:360-368`) — no
database, catalog, schema, or table. Passing resource identity "through the
CRUD error flow" would mean adding it to the backend error `extra` and
threading it through `ErrorMessageWithStackTrace` into every `source="crud"`
consumer. That's a much larger change than this fix, and it isn't free of risk.
**2. Unscoped is the established convention on this branch, not a new
pattern.** The `crud` arm already invalidates `{Schemas, LIST}`, `{Catalogs,
LIST}`, and the bare `'Tables'` tag — all equally unscoped, added in #41913.
Scoping only `TableMetadatas` would make this line the odd one out without
addressing the same breadth in its three siblings.
**3. The blast radius is bounded, and the refetch is mostly desirable.**
`invalidateTags` only refetches queries with *active subscribers*; unsubscribed
cache entries are just marked invalid and refetched on next mount. The only
subscriber of `useTableMetadataQuery` is `TablePreview`, which mounts once per
pinned table tab in the South Pane (`SouthPane/index.tsx:191-213`) — a handful
at most, not an application-wide burst. And OAuth2 credentials are
per-database, so after re-authentication the sibling previews against that same
database were failing for exactly the same reason and *should* refetch. The
genuine over-invalidation is limited to pinned tables on other databases, which
is bounded by the same small tab count.
Happy to revisit as a separate change if the error payload ever grows
database identity — that's the prerequisite for doing this properly rather than
approximating 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]
