durgaprasadml commented on PR #41120: URL: https://github.com/apache/superset/pull/41120#issuecomment-4761758627
Thanks for the work here. The overall approach makes sense to me, especially the move toward consistent denylist enforcement across SQL Lab, executor, estimation, and datasource validation paths, along with the schema-qualified matching support and expanded test coverage. I noticed the transient Query construction + db.session.expunge(...) pattern now appears in multiple places (estimate.py, helpers.py, and executor.py) to resolve the effective schema through get_default_schema_for_query(). Would it make sense to centralize this logic into a shared helper/utility so future changes to query-aware schema resolution only need to be made in one place? It feels like these code paths are intentionally trying to maintain execution-parity, and a common helper could help prevent divergence over time. Not a blocker—just wondering if there’s an opportunity to reduce duplication and keep the schema-resolution behavior synchronized across these security-sensitive paths. -- 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]
