rebenitez1802 commented on PR #40955: URL: https://github.com/apache/superset/pull/40955#issuecomment-4971736749
Approve — all three points from my earlier pass are addressed in `5b5c9b9`. Nice iteration. Resolved: - **Medium (UUID id rejection):** you've made this a deliberate fail-closed decision rather than a bug — `test_update_chart_query_context_mismatched_datasource_is_rejected` now codifies a non-numeric id as a rejected mismatch. That's the right call for a background-worker path: a UUID that *can't* be verified against the chart's integer `datasource_id` produces a visible 400 instead of silently letting the stored context drift. Good tradeoff. - **Low (non-string `query_context`):** the expanded `except (TypeError, ValueError)` comment now clearly separates the "not a string / already-parsed dict, intentionally out of scope" case from the "unparseable string" case. 👍 - **Low (test gaps):** matching test now parametrizes a non-`table` (`query`) type, and the rejection test covers the non-numeric id. Coverage matches the guarantee. 🟢 Low (non-blocking) — the `"5f7b3c1a-...-uuid"` fixture isn't a syntactically valid UUID, so the comment `# non-numeric id` is more accurate than "uuid"; the code path (`int()` → `ValueError`) is identical either way, so no functional impact. Fine to leave as-is. No blocking issues. LGTM once CI is green. -- 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]
