#3588 fix: gRPC query and streaming query now propagate the language parameter#3589
#3588 fix: gRPC query and streaming query now propagate the language parameter#3589
Conversation
…parameter The language parameter was ignored in query() and queryStream() paths, causing all queries to be executed as SQL regardless of the specified language. - Added language field to ExecuteQueryRequest proto (field 9, backward-compatible) - Server: executeQuery() and streamQuery() modes now use the language from the request - Client: query(), queryStream(), and streamQuery() now pass language to the proto builders - Enabled Gremlin and Cypher e2e tests that were disabled pending this fix Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical limitation in the gRPC client where query and streaming query operations did not correctly propagate the specified language, effectively restricting them to SQL. By introducing a new language field in the gRPC proto, and updating both the server and client implementations to utilize this parameter, the system now fully supports diverse query languages like Gremlin and Cypher through gRPC. This enhancement significantly broadens the capabilities of the gRPC interface, allowing for more flexible and powerful database interactions. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
🧪 CI InsightsHere's what we observed from your CI run for 2ca180b. 🟢 All jobs passed!But CI Insights is watching 👀 |
Code ReviewThe core fix is correct and clean — adding Bug:
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively addresses the issue of propagating the language parameter in gRPC query and streaming query operations, with well-structured changes and enabled e2e tests. However, the implementation introduces security concerns: logging unsanitized user input in executeQuery can lead to log injection, and the streamPaged pagination logic is vulnerable to a SQL injection-style bypass that could cause a denial-of-service via an infinite loop. Furthermore, the streamPaged implementation is currently SQL-specific and will fail for other query languages. A medium-severity suggestion has also been provided to simplify and deduplicate some new server-side logic.
| private void streamPaged(Database db, StreamQueryRequest request, int batchSize, | ||
| ServerCallStreamObserver<QueryResult> scso, | ||
| AtomicBoolean cancelled, ProjectionConfig projectionConfig) { | ||
| AtomicBoolean cancelled, ProjectionConfig projectionConfig, String language) { |
There was a problem hiding this comment.
By adding the language parameter, this method now incorrectly suggests it supports paged streaming for any language. However, the implementation calls wrapWithSkipLimit which generates SQL-specific syntax for pagination. This will cause queries in other languages like Gremlin or Cypher to fail because the wrapped query is not valid in those languages.
To fix this, you should add a check at the beginning of the method to throw an UnsupportedOperationException if the language is not SQL, making it clear that this mode is not yet supported for other languages.
| final String language = (request.getLanguage() == null || request.getLanguage().isEmpty()) ? "sql" : request.getLanguage(); | ||
|
|
||
| ResultSet resultSet = database.query("sql", request.getQuery(), | ||
| LogManager.instance().log(this, Level.FINE, "executeQuery(): language = %s query = %s", language, request.getQuery()); |
There was a problem hiding this comment.
The executeQuery method logs user-supplied language and query parameters without sanitization, creating a log injection vulnerability where attackers can spoof log entries or corrupt log files. Additionally, logging the full query string may inadvertently expose sensitive information. It is crucial to sanitize input by removing or replacing newline characters and consider masking sensitive data before logging. While request.getLanguage() == null is technically redundant for proto3 string fields (which default to empty strings), the provided suggestion prioritizes robust sanitization. This pattern is also present in streamQuery; consider extracting this logic into a private helper method to avoid duplication and improve maintainability and consistent security practices.
| final String language = (request.getLanguage() == null || request.getLanguage().isEmpty()) ? "sql" : request.getLanguage(); | |
| ResultSet resultSet = database.query("sql", request.getQuery(), | |
| LogManager.instance().log(this, Level.FINE, "executeQuery(): language = %s query = %s", language, request.getQuery()); | |
| final String language = (request.getLanguage() == null || request.getLanguage().isEmpty()) ? "sql" : request.getLanguage(); | |
| LogManager.instance().log(this, Level.FINE, "executeQuery(): language = %s query = %s", language.replace('\n', '_').replace('\r', '_'), request.getQuery().replace('\n', '_').replace('\r', '_')); |
| QueryResult.Builder b = QueryResult.newBuilder(); | ||
|
|
||
| try (ResultSet rs = db.query("sql", pagedSql, params)) { | ||
| try (ResultSet rs = db.query(language, pagedSql, params)) { |
There was a problem hiding this comment.
In streamPaged mode, the server constructs a SQL query by concatenating the user-supplied query into a SELECT FROM (...) wrapper to apply pagination. An attacker can provide a query that escapes the subquery (e.g., by using a closing parenthesis and a comment marker like ) --) to comment out the SKIP and LIMIT clauses added by the server. This can lead to an infinite loop on the server, as it will repeatedly execute the query and receive the same full result set, causing a denial-of-service and resource exhaustion. Furthermore, the code now passes the user-supplied language to this SQL-specific wrapper, which will cause syntax errors or unpredictable behavior if a non-SQL language is specified. It is recommended to validate the query string or use a more robust method for applying pagination that does not rely on simple string concatenation.
- Guard streamPaged against non-SQL languages (falls back to CURSOR mode) - Use langOrDefault() in client query/queryStream to prevent NPE on null language - Extract langOrDefault() helper on server side to deduplicate language defaulting - Add streaming query e2e tests for SQL and OpenCypher Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Code ReviewThis is a clean, well-scoped fix that correctly propagates the CorrectnessPAGED mode silent fallback (
Tests
Missing stream tests for Gremlin and Cypher: Design / ProtoProto field placement: adding
Documentation artefacts
SummaryThe core fix (proto field + server + client plumbing) is correct and backward-compatible. Actionable items before merging:
|
Coverage summary from CodacySee diff coverage on Codacy
Coverage variation details
Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: Diff coverage details
Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: See your quality gate settings Change summary preferences |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3589 +/- ##
==========================================
+ Coverage 65.55% 65.75% +0.19%
==========================================
Files 1514 1514
Lines 103672 103683 +11
Branches 21457 21457
==========================================
+ Hits 67967 68178 +211
+ Misses 26467 26282 -185
+ Partials 9238 9223 -15 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Summary
Fixes #3588
string language = 9;toExecuteQueryRequestproto (additive, backward-compatible)executeQuery()and all threestreamQuerymodes (streamCursor,streamMaterialized,streamPaged) now use the language from the request instead of hardcoding"sql"query(),queryStream(), and privatestreamQuery()now pass thelanguageparameter to the proto buildersTest plan
RemoteGrpcDatabaseTest.simpleGremlinQuery()enabled (was@Disabled)RemoteGrpcDatabaseTest.simpleCypherQuery()enabled (was@Disabled)RemoteGrpcDatabaseTest.simpleOpenCypherQuery()already passingcd e2e && mvn test -Dtest=RemoteGrpcDatabaseTest🤖 Generated with Claude Code