aminghadersohi commented on code in PR #44765:
URL: https://github.com/apache/superset/pull/44765#discussion_r4129978961
##########
superset/models/core.py:
##########
@@ -1220,8 +1263,13 @@ 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:
+ yield sqla.inspect(engine)
Review Comment:
Not a valid finding — the login this PR targets is inside the retry.
`sqla.inspect(engine)` does log in. SQLAlchemy 2.0.52
`Inspector._init_engine` (`sqlalchemy/engine/reflection.py`):
```python
def _init_engine(self, engine: Engine) -> None:
self.bind = self.engine = engine
engine.connect().close()
self._op_context_requires_connect = True
```
So the `engine.connect()` that presents the token runs inside
`_open_with_oauth2_retry`, and a token rejected at login is refreshed and the
inspector rebuilt.
`tests/unit_tests/models/core_test.py::test_get_inspector_refreshes_a_token_rejected_at_login`
covers exactly that.
The lazy calls you point at (`Inspector._operation_context` →
`self.bind.connect()`) do reconnect outside the retry, but that is deliberate,
not an oversight:
- they reconnect with a token that just authenticated successfully at
`inspect()` time, so the login-rejection failure mode does not recur there;
- retrying them would mean replaying the caller's block. The wrapper cannot
know whether that block is idempotent, so it only ever retries opening — see
the `_open_with_oauth2_retry` docstring (`superset/models/core.py:1276-1283`)
and `test_get_raw_connection_does_not_replay_the_callers_block`.
Engines that reject the token later, per statement (Google Sheets, for
example), are out of scope and called out as such in the PR description.
--
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]
