gabotorresruiz commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4158794776
##########
superset/models/core.py:
##########
@@ -1260,8 +1303,14 @@ def get_inspector(
catalog: str | None = None,
schema: str | None = None,
) -> Inspector:
- with self.get_sqla_engine(catalog=catalog, schema=schema) as engine:
- yield sqla.inspect(engine)
+ @contextmanager
+ def open_inspector() -> Iterator[Inspector]:
+ with self.get_sqla_engine(catalog=catalog, schema=schema) as
engine:
+ with check_for_oauth2(self):
Review Comment:
NIT, definitely not a blocker. I went looking for what this guard changes in
the real path and could not find a case: `get_sqla_engine` already wraps its
own `yield engine` in `check_for_oauth2`, so it catches both a rejection raised
by `sqla.inspect()` and one thrown back in from the caller's block. I removed
this line on `ed86e158` and re-ran the login-rejection,
rejected-replacement-token, caller-block and schema-discovery scenarios against
a real metadata database: identical outcomes, exactly one `start_oauth2_dance`
in every case. The only test that fails without it is
`test_get_inspector_does_not_replay_the_callers_block`, and that one stubs
`get_sqla_engine` out with `nullcontext(engine)`, so it pins the guard rather
than the behaviour.
Happy to keep it as cheap insurance, and the symmetry with `open_connection`
reads well. Worth a short comment saying it is redundant with `get_sqla_engine`
today, so a later reader does not take that test as proof the real path depends
on 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]
