SQL: Use cratedb-sqlparse for implementing read-only mode
#14
Conversation
|
Warning Rate limit exceeded@amotl has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe changes introduce the use of the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant SqlStatementClassifier
participant cratedb_sqlparse
participant sqlparse
User->>SqlStatementClassifier: Create instance with SQL expression
User->>SqlStatementClassifier: Access is_dql/operation
SqlStatementClassifier->>cratedb_sqlparse: parse(expression)
cratedb_sqlparse-->>SqlStatementClassifier: Parsed statement(s)
SqlStatementClassifier-->>User: Return DQL status/operation type
User->>SqlStatementClassifier: Access is_select_into
SqlStatementClassifier->>sqlparse: parse(expression)
sqlparse-->>SqlStatementClassifier: Parsed tokens
SqlStatementClassifier-->>User: Return SELECT INTO detection
Poem
✨ Finishing Touches
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (3)
pyproject.toml (1)
23-28: Consider loosening / future-proofing the version pin forcratedb-sqlparse.A hard pin (
==0.0.14) guarantees reproducibility today but can block consumers from upgrading the library when bug-fixes or security patches are released.
Typical options:
- Pin an upper bound instead of an exact version:
"cratedb-sqlparse>=0.0.14,<0.1"
- Keep the hard pin here but add a comment (or dependabot ignore) explaining why we must stay on
0.0.14.Either approach makes maintenance intent explicit.
cratedb_mcp/util/sql.py (2)
78-88: Minor optimisation: avoid double parsing.
is_dqlcurrently:
- Parses via
cratedb-sqlparse.- Calls
self.is_select→operation→parse_cratedb()again (after the fix above).Because the result is cached the second call is cheap, but we can still skip it:
- parsed = self.parse_cratedb() + parsed = self.parse_cratedb() # cached # ... later - return self.is_select and not self.is_camouflage + is_select = parsed and parsed[0].type.upper() == "SELECT" + return is_select and not self.is_camouflageNot critical, but trims a property indirection and keeps the logic self-contained.
123-125: Token flattening may miss nestedINTOs.
[str(item).upper() for item in parsed[0]]iterates only the first-level tokens; nested groups (e.g., within parentheses) are ignored.
If full accuracy is required you might want to recurse withtoken.flatten()provided bysqlparse.Not blocking, just flagging a potential false-negative edge-case.
🛑 Comments failed to post (1)
cratedb_mcp/util/sql.py (1)
43-57:
⚠️ Potential issueThread-safe lazy cache is good – but only half of the class uses it.
You introduced
_parsed_cratedbandparse_cratedb()– nice!
However, some read-paths (seeoperationbelow) access_parsed_cratedbwithout guaranteeing thatparse_cratedb()has been called first, which will crash on the first directSqlStatementClassifier(...).operationcall.Consider always funnelling access through the helper:
@property def operation(self) -> str: """ The SQL operation: SELECT, INSERT, UPDATE, DELETE, CREATE, etc. """ - return self._parsed_cratedb[0].type.upper() + parsed = self.parse_cratedb() + if not parsed: + return "" + return parsed[0].type.upper()This keeps the lazy cache intact and removes
NoneTypesurprises.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.@property def operation(self) -> str: """ The SQL operation: SELECT, INSERT, UPDATE, DELETE, CREATE, etc. """ parsed = self.parse_cratedb() if not parsed: return "" return parsed[0].type.upper()🤖 Prompt for AI Agents (early access)
In cratedb_mcp/util/sql.py around lines 43 to 57, the _parsed_cratedb cache is accessed directly in some places like the operation property without ensuring parse_cratedb() has been called, which can cause NoneType errors. To fix this, update all accesses to _parsed_cratedb to go through the parse_cratedb() method, ensuring the cache is initialized before use and preventing crashes on first access.
About
Problem
I have not been able to remove the traditional
sqlparseyet. The patch needs more work.