aminghadersohi commented on PR #43478: URL: https://github.com/apache/superset/pull/43478#issuecomment-5399780922
Thanks — both flagged issues were accurate, confirmed by reproducing them locally. Fixed in dfedf65. **1. Test asserted the wrong error message (codeant + bito).** Confirmed real; the test failed: ``` assert 'Invalid chart query data' in "Invalid extra_form_data filter: 'op'" ``` The malformed-filter regression test was written against the fallback path's message, but a later commit in this branch added the saved-context error branch, which reports the malformed filter directly. Updated the assertion to expect `Invalid extra_form_data filter` — the more precise message is the correct behavior here. **2. `ChartError` bypasses the form_data fallback (codeant).** The underlying problem is real, though the suggested fix would trade one bug for another. `qc_json["datasource"]["id"]` sat inside the same `try` as the merge, so both causes collapsed into one error: - a *stale* saved context (missing/malformed `datasource`) → should fall back to `form_data` - *malformed request filters* → should surface a `ValidationError` Returning `None` for everything, as suggested, would fix the stale case but silently swallow bad user filters at this path. Instead the datasource is now resolved up front — failure there returns `None` so the caller rebuilds from `form_data` — and the merge keeps its `ValidationError`. Both cases now behave correctly. **3. Test gap on the invalid-`extra_form_data` path (bito).** That path was already covered end-to-end by `test_malformed_extra_form_data_filter_returns_clean_error`. Added `test_stale_query_context_falls_back_instead_of_erroring` to cover the newly separated stale-context branch, which was the genuinely untested one. `pytest tests/unit_tests/mcp_service/` — 3514 passed. `pre-commit run` — clean. -- 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]
