Improve Trino Query Validation and Table Schema Handling #50
Conversation
WalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Makefile
participant Docker
participant TestSuite
User->>Makefile: make docker-test
Makefile->>Docker: Build image (Dockerfile.test)
Docker-->>Makefile: Image built
Makefile->>Docker: Run container with test suite
Docker->>TestSuite: Execute tests
TestSuite-->>Docker: Test results
Docker-->>User: Output test results
sequenceDiagram
participant Client
participant isReadOnlyQuery
participant GetTableSchema
Client->>isReadOnlyQuery: Check if SQL query is read-only
isReadOnlyQuery-->>Client: true/false
Client->>GetTableSchema: Request table schema with flexible table name
GetTableSchema->>GetTableSchema: Parse table string (catalog.schema.table)
GetTableSchema-->>Client: Return schema or error
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@petrtsatsin-branch could you sign DCO, check https://github.com/tuannvm/mcp-trino/pull/50/checks?check_run_id=43647570913 Thanks! |
…ons. Signed-off-by: Petr Tsatsin <petr.tsatsin@branch.io>
d784c1e to
c9f8ba8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Makefile (1)
54-58: Consider caching layers / adding--targetfor fasterdocker-testruns (optional)
docker-testrebuilds the entire image every invocation. If theDockerfile.testuses a multi-stage build (common for Go projects), adding--target testand ensuring proper layer caching will speed up local and CI runs:-docker build -f Dockerfile.test -t $(BINARY_NAME)-test:$(VERSION) . +docker build --target test -f Dockerfile.test -t $(BINARY_NAME)-test:$(VERSION) .This is purely a QoL improvement and can be skipped if the build is already fast enough.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
Makefile(2 hunks)internal/trino/client.go(2 hunks)internal/trino/client_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/trino/client_test.go
- internal/trino/client.go
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 60-60: Target "all" should be declared PHONY.
(phonydeclared)
Makefile
Outdated
| @@ -1,4 +1,4 @@ | |||
| .PHONY: build test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint | |||
| .PHONY: build test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test | |||
There was a problem hiding this comment.
Add all to the .PHONY list to satisfy checkmake and avoid accidental file collisions
The all target is defined later in the file (line 60) but is not declared as phony. This triggers the checkmake warnings (minphony, phonydeclared) and, more importantly, prevents make from ignoring a real file named all in the working directory.
-.PHONY: build test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test
+.PHONY: build test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test all🧰 Tools
🪛 checkmake (0.2.2)
[warning] 1-1: Missing required phony target "all"
(minphony)
🤖 Prompt for AI Agents
In the Makefile at line 1, add the target `all` to the `.PHONY` declaration list
to prevent checkmake warnings and avoid conflicts with any real file named `all`
in the directory. This ensures that `make` treats `all` as a phony target and
always executes its commands.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
Makefile (1)
1-1:alltarget still missing from.PHONYlist
This was flagged in the previous review and is still unresolved. Declaringallas phony silences checkmake warnings and prevents collisions with a real file namedall.-.PHONY: build build-dxt pack-dxt test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test +.PHONY: build build-dxt pack-dxt test clean run-dev release-snapshot run-docker run docker-compose-up docker-compose-down lint docker-test all
🧹 Nitpick comments (1)
Makefile (1)
73-77: Minor enhancements for thedocker-testtarget
- Add
--pull=missing(or similar) to ensure the base image is up-to-date when CI runs infrequently.- Consider passing the binary version as a build-arg instead of baking it into the tag only – this makes the version available inside the Dockerfile if needed for caching layers or metadata.
Nit-level; feel free to ignore if unnecessary.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Makefile(2 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
Makefile
[warning] 1-1: Missing required phony target "all"
(minphony)
[warning] 79-79: Target "all" should be declared PHONY.
(phonydeclared)
|
@claude review PR |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Summary
This PR enhances the MCP Trino HTTP server by improving query validation logic and fixing table schema retrieval for fully qualified table names. The changes allow complex SELECT queries to be properly recognized as read-only and support various table name formats in schema retrieval operations.
Changes
Improved the isReadOnlyQuery function to correctly identify SELECT\n queries as read-only
Added normalization of whitespace by replacing newlines and carriage returns with spaces
Ensured proper detection of SQL keywords regardless of spacing
Added detection of embedded write operations within SELECT queries
Added protection against SQL injection attempts using multiple statements (semicolons)
Enhanced GetTableSchema function to properly handle fully qualified table names:
Supports catalog.schema.table format without prepending defaults
Supports schema.table format with default catalog
Maintains backward compatibility with simple table names
Prevents "Too many dots in table name" errors when using fully qualified table names
Created a new test file client_test.go with extensive test cases for isReadOnlyQuery
Tests cover various query formats, whitespace variations, and embedded write operations
Added tests for case insensitivity and SQL injection attempts
Problem Solved
Previously, SELECT\n queries were incorrectly rejected as non-read-only
Queries with newlines or unusual spacing were not properly recognized
Table schema retrieval failed when using fully qualified table names
Testing
Added unit tests for query validation logic
Manually tested with various query formats
Verified schema retrieval works with different table name formats
Summary by CodeRabbit