aminghadersohi commented on PR #36013: URL: https://github.com/apache/superset/pull/36013#issuecomment-3499068729
I've addressed the reviewer feedback with the latest commit: ## Changes Made ### 1. Replaced `_` Variable Names (@eschutho) - Changed `_ = user.roles` to `user_roles = user.roles # noqa: F841` - Changed `_ = user.groups` to `user_groups = user.groups # noqa: F841` - This avoids confusion with the translation function `_()` commonly used in Superset - Added `noqa: F841` because these variables are intentionally unused (they trigger eager loading) ### 2. Fixed Pydantic Alias Configuration (@korbit-ai) - Added `serialization_alias="schema"` to `DatasetContext.schema_name` field - This ensures the field serializes correctly as `"schema"` in output, not `"schema_name"` - The field can now accept input as either `"schema"` or `"schema_name"` (via `alias`) ### 3. Improved Exception Handling Comments (@eschutho) - Added clarifying comments to explain why we log cleanup errors but re-raise the original exception - Renamed `e` to `cleanup_error` for clarity - The pattern preserves the original tool error while logging any session cleanup failures ### About the Performance Concern (@korbit-ai) The eager loading of user relationships (`user.roles`, `user.groups`) is **intentional** and necessary in this context: - **Why**: SQLAlchemy sessions close after the tool execution, causing lazy-loading errors - **When**: Only happens once per tool call, not repeatedly - **Cost**: Minimal - two simple relationship queries per request - **Benefit**: Prevents runtime errors when accessing user permissions later This is a session lifecycle management pattern, not a performance issue. The relationships are needed by Superset's security layer. -- 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]
