aminghadersohi commented on PR #39922:
URL: https://github.com/apache/superset/pull/39922#issuecomment-4557326243
Hi @fitzee — all 4 items from your CHANGES_REQUESTED review have been
addressed in the commits pushed after 8a669c36. CI is green on dc73a0bf36eb.
Here's a per-item breakdown:
---
**1. HIGH — `register()` not lock-protected**
Fixed in commit e134b8380d. `register()` in `registry.py` now acquires
`_plugins_lock` before writing to `_REGISTRY`:
```python
with _plugins_lock:
if plugin.chart_type in _REGISTRY:
logger.warning("Overwriting existing plugin for chart_type=%r",
plugin.chart_type)
_REGISTRY[plugin.chart_type] = plugin
```
See `registry.py:158–163`.
---
**2. HIGH — Column name regex removal undocumented (relationship with PR
#39915)**
Documented in the PR description under "Column-name sanitization and PR
#39915". The section explains that this branch pre-dates #39915, carries
forward the same security boundary (SQL-pattern checks instead of strict ASCII
regex), and lists the real-world column name patterns now supported
(`1Q_revenue`, `order-date`, `events.created-at`).
---
**3. MEDIUM — `_ensure_plugins_loaded()` no circuit breaker**
Fixed in commit e134b8380d. A `_plugins_load_failed` flag was added at
module level. `_ensure_plugins_loaded()` now:
- Returns immediately if either `_plugins_loaded` or `_plugins_load_failed`
is set (line 81)
- Rolls back `_REGISTRY` to its pre-import state and sets
`_plugins_load_failed = True` on any import exception (lines 91–93)
- Logs the failure at `logger.exception` level so it doesn't disappear
silently
After a failed import, all subsequent lookups return `None` rather than
retrying the broken import.
---
**4. 5-chart-type validation behavioral change — link to Shortcut story**
Documented in the PR description under "Behavioral change — 5-type column
validation gap". There is no pre-existing Shortcut story for this: the gap was
discovered during the registry refactor itself. The PR description explains the
prior behavior (only `xy` and `table` validated column references; `pie`,
`pivot_table`, `mixed_timeseries`, `handlebars`, and `big_number` silently
passed non-existent columns), the new behavior (uniform validation for all 7
types), and why the tightening is intentional. If a separate tracking issue
would help, happy to file one — please let me know.
---
Current HEAD: `dc73a0bf36eb1add8ad8a6ff8906642bde37372a` — CI green. Would
appreciate a re-review when you have a chance.
--
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]
