aminghadersohi commented on PR #39922: URL: https://github.com/apache/superset/pull/39922#issuecomment-4553619850
Thanks @fitzee — I re-reviewed the CHANGES_REQUESTED point-by-point instead of blindly applying it, and also had Claude + Codex subsessions review the follow-up commit. What was already addressed by earlier commits: - `register()` lock protection: addressed in `7f558486b8`. - `_ensure_plugins_loaded()` circuit breaker: addressed in `7f558486b8` via `_plugins_load_failed`. - `serialize_chart_object()` broad catch: addressed in `415b45dbd8` by separating the import failure path from plugin display-name errors. - `_reset_for_testing()`, module-level proxy singleton, and executable bits: addressed in `415b45dbd8`. Additional fixes pushed in `0b6b98dcf7` after the subsession reviews: - Switched the registry lock to `threading.RLock()`. This keeps `register()` lock-protected without risking a self-deadlock when lazy plugin import calls `register()` while `_ensure_plugins_loaded()` holds the lock. - Made plugin-load failure rollback partial registrations to preserve all-or-nothing registry state. - Fixed `_reset_for_testing()` to clear the cached `superset.mcp_service.chart.plugins` package so subsequent lazy loading can re-run built-in registration. - Completed the #39915-equivalent schema hardening: `FilterConfig.column` and `BigNumberChartConfig.temporal_column` now pass `check_sql_keywords=True`, matching `ColumnRef.name`. - Added regression tests covering SQL-pattern rejection, relaxed valid column names, partial-load rollback, re-entrant lock choice, and cached-plugin reset behavior. - Re-applied `100755 -> 100644` mode cleanup for the Python module files. I also updated the PR description to explicitly document the relationship to #39915 and the 5-chart-type validation behavioral change. I did not find a separate Shortcut/GitHub issue for that validation gap; it was discovered during this registry refactor. I documented that explicitly in the PR body rather than inventing/retroactively linking an unrelated issue. Validation: - `uvx pre-commit run ruff --files ...` ✅ - `uvx pre-commit run mypy --files ...` ✅ - Commit hook ran and all staged-file hooks passed except local `pylint`, which fails in this Agor/uvx environment because the custom Superset pylint plugin cannot import `superset` (`No module named 'superset'`). - `uvx pre-commit run --all-files` was attempted per repo guidance; it surfaced pre-existing unrelated mypy errors in `tests/unit_tests/semantic_layers/mapper_test.py` and `tests/unit_tests/mcp_service/chart/test_chart_utils.py`, then was stopped during all-frontend prettier because it was processing the entire repo. - CI for the previous commit was fully green; CI for `0b6b98dcf7` is running now. -- 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]
