sha174n commented on PR #42412: URL: https://github.com/apache/superset/pull/42412#issuecomment-5368855393
Thanks for picking this up. One blocking issue on the mechanism: Database.get_raw_connection is a @contextmanager, so it has to be entered with `with ... as conn:`. Wrapping it in contextlib.closing(...) never triggers __enter__, so the generator body (engine setup + the per-user connection-context handling this PR is meant to route through) does not run, and conn ends up being the context-manager wrapper rather than the DBAPI connection, so conn.cursor() fails. Suggested: `with merged_database.get_raw_connection(catalog=catalog, schema=schema) as conn:` and keep cursor.close() in the finally. The current unit tests do not catch this because they mock get_raw_connection to return a bare mock whose .cursor() always resolves; worth asserting the connection context manager is actually entered. Two more: run the mutator on the merged instance (after session.merge) so a session-bound mutator can't hit DetachedInstanceError during streaming; and pre-commit is red on ruff-format in the t est file, plus the branch needs a rebase on master. -- 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]
