refactor: use AST pattern matching instead of string matching for sta… #262
+84
−60
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…tement detection Fixes datafusion-contrib#159 The codebase was using starts_with() string checks to detect statement types (SET, SHOW, INSERT, etc.) which is fragile - it can break with leading whitespace, comments, or case variations. Since we already have the parsed Statement AST from sqlparser, this switches to using pattern matching directly on the Statement enum. Changes in permissions.rs: - Renamed check_query_permission to check_statement_permission, now takes &Statement instead of &str - Permission detection (SELECT/INSERT/UPDATE/DELETE/CREATE/DROP/ALTER) now uses match on Statement variants - Added should_skip_permission_check() helper using matches!() macro for SET, SHOW, and transaction statements - Removed all the to_lowercase().starts_with() chains Changes in handlers.rs: - INSERT detection now uses matches!(statement, Statement::Insert(_)) - Removed unnecessary query_lower variable construction - Fixed extended query handler to properly destructure statement from the portal tuple All 12 unit tests pass. The existing integration test failures (dbeaver, metabase, psql) are unrelated - they fail due to missing DataFusion functions like array_length and array_contains.
Adds a test file to verify pgAdmin startup queries work correctly. The test covers: - SELECT version() query - The CASE expression checking pg_extension for 'bdr' extension and pg_replication_slots for replication slot count Both pg_extension and pg_replication_slots tables are already implemented in pg_catalog, so these queries now pass.
mjgarton
reviewed
Dec 15, 2025
Use reference instead of calling .to_string() on query string as suggested by mjgarton - the query is only used for logging and doesn't need to be owned.
mjgarton
reviewed
Dec 15, 2025
Remove comments that don't fully describe what's being skipped. The function name should_skip_permission_check is self-documenting. Suggested by mjgarton.
Member
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #159
The codebase was using starts_with() string checks to detect statement types (SET, SHOW, INSERT, etc.) which is fragile - it can break with leading whitespace, comments, or case variations.
Since we already have the parsed Statement AST from sqlparser, this switches to using pattern matching directly on the Statement enum.
Changes in permissions.rs:
Changes in handlers.rs:
All 12 unit tests pass. The existing integration test failures (dbeaver, metabase, psql) are unrelated - they fail due to missing DataFusion functions like array_length and array_contains.