aminghadersohi commented on PR #39922:
URL: https://github.com/apache/superset/pull/39922#issuecomment-4552257304
Thanks for the detailed review @fitzee. All four blocking items and the
quick non-blocking NITs are addressed in commits pushed to this branch.
---
### Blocking findings
**HIGH — `register()` not lock-protected** ✅ Fixed in `c5d7bbb9`
`register()` now acquires `_plugins_lock` before reading and writing
`_REGISTRY`.
**HIGH — Column name regex removal / PR #39915 relationship** ✅ Fixed in
`71c7f8dac5`
The three regex removals in `d883b622` replaced restrictive patterns that
blocked valid column names with parentheses and slashes. The intent was always
to rely on `sanitize_user_input` validators instead — `ColumnRef.name` and
`FilterConfig.column` already had those validators, but
`BigNumberChartConfig.temporal_column` was missed. `71c7f8dac5` adds
`@field_validator('temporal_column')` that calls `sanitize_user_input(...,
allow_empty=True)`, closing the gap. PR #39915 ("relax column name regex,
improve validation errors") was closed without merging; the approach here
achieves equivalent coverage via Pydantic validators rather than regex.
**MEDIUM — No circuit breaker in `_ensure_plugins_loaded()`** ✅ Fixed in
`c5d7bbb9`
Added `_plugins_load_failed` flag. On `ImportError` the flag is set to
`True` and all subsequent calls short-circuit before acquiring the lock.
**Breaking change not documented** ✅ Fixed in PR description update
Added a "Behavioral change" note in ADDITIONAL INFORMATION explaining that
unknown `chart_type` values now receive a clear `unknown_chart_type` error
instead of falling through dispatch chains silently.
---
### Non-blocking NITs addressed
**`_reset_for_testing()` helper** — Added to `registry.py` in `71c7f8dac5`.
Resets `_REGISTRY`, `_plugins_loaded`, `_plugins_load_failed`, and
`_filter_config` in one call.
**Executable bits (mode 100755)** — Fixed in `71c7f8dac5` via `git
update-index --chmod=-x` on all 9 affected files (`plugin.py`, 7 plugin files,
`registry.py`, `initialization/__init__.py`).
**`noqa: BLE001` catches too broadly** — Fixed in `71c7f8dac5`. Split the
single `except Exception` into `except ImportError` (import-path failure) +
`except Exception` (third-party plugin errors, which may raise anything). The
second clause retains `# noqa: BLE001` with a comment.
**`get_registry()` allocates new `_RegistryProxy` on every call** — Fixed in
`71c7f8dac5`. `_RegistryProxy` is now instantiated once as `_PROXY` at module
level; `get_registry()` returns the singleton.
--
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]
