chore(query): exec query string (raw Query Builder) #1412
Conversation
WalkthroughThe changes introduce two new files, Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant QueryString
participant QueryStringExec
participant TxQueryStringResult
Client->>QueryString: NewQueryString()
QueryString->>QueryString: Exec(ctx, into)
QueryString->>QueryString: Do(ctx, payload, into)
QueryStringExec->>Client: Exec(ctx, into)
QueryStringExec->>TxQueryStringResult: Tx()
TxQueryStringResult->>Client: Into(v)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (4)
runtime/builder/query_string.go (2)
25-29: Consider initializing all fields in constructorThe constructor only initializes the
Startfield. Consider:
- Initializing the
TxResultchannel with a buffer size- Adding parameters for
EngineandQuery- Or documenting why these fields are left as zero values
-func NewQueryString() QueryString { +func NewQueryString(engine engine.Engine, query string) QueryString { return QueryString{ Start: time.Now(), + Engine: engine, + Query: query, + TxResult: make(chan []byte, 1), } }
39-49: Enhance logging and context handlingConsider the following improvements:
- Add context cancellation check
- Include query details in timing logs (sanitized)
- Add structured logging for better observability
func (q QueryString) Do(ctx context.Context, payload interface{}, into interface{}) error { if q.Engine == nil { return fmt.Errorf("client.Prisma.Connect() needs to be called before sending queries") } + if err := ctx.Err(); err != nil { + return fmt.Errorf("context error before execution: %w", err) + } + err := q.Engine.Do(ctx, payload, into) now := time.Now() totalDuration := now.Sub(q.Start) - logger.Debug.Printf("[timing] TOTAL %q", totalDuration) + logger.Debug.Printf("[timing] Query execution completed: duration=%v", totalDuration) return err }runtime/raw/query_string.go (2)
19-25: Add documentation and improve error handling.The QueryStringExec type and its methods would benefit from:
- GoDoc comments explaining the purpose and usage
- More detailed error handling in the Exec method
+// QueryStringExec represents a raw query execution context type QueryStringExec struct { query builder.QueryString } +// ExtractQuery returns the underlying query object func (r QueryStringExec) ExtractQuery() builder.QueryString { return r.query } +// Exec executes the raw query with the given context and populates the result into the provided interface func (r QueryStringExec) Exec(ctx context.Context, into interface{}) error { + if ctx == nil { + return fmt.Errorf("context cannot be nil") + } + if into == nil { + return fmt.Errorf("destination interface cannot be nil") + } return r.query.Exec(ctx, into) }Also applies to: 55-57
1-57: Add comprehensive test coverage.This new functionality for raw query execution needs thorough testing, including:
- Unit tests for all public methods
- Integration tests for transaction handling
- Error case testing
- Concurrent execution testing
Would you like me to help create a test suite for this implementation?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
runtime/builder/query_string.go(1 hunks)runtime/raw/query_string.go(1 hunks)
🔇 Additional comments (5)
runtime/builder/query_string.go (2)
3-10: Verify engine package compatibility
The implementation depends on the engine package. Let's ensure we're using a compatible version.
✅ Verification successful
Engine package integration is consistent and compatible
The implementation in query_string.go follows the established patterns in the codebase:
- Uses the same
protocol.GQLRequeststructure as other query builders - Follows consistent engine integration pattern seen in
raw,transaction, and other packages - Maintains the same interface contract with the engine package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check engine package version and recent changes
# Look for any breaking changes in the engine package
# Check for recent changes in the engine package
fd -t f . | rg "engine/protocol" | xargs rg "GQLRequest" -A 5
Length of output: 452
Script:
#!/bin/bash
# Let's verify the engine package usage in the codebase
# First, check how GQLRequest is used in query_string.go
ast-grep --pattern 'protocol.GQLRequest'
# Also check for any other files using similar query execution patterns
rg "engine\.NewEngine|engine\.Engine" -A 3
Length of output: 3063
12-23: Consider thread safety for TxResult channel
The TxResult channel might be accessed concurrently. Consider documenting thread-safety guarantees or adding synchronization mechanisms if needed.
runtime/raw/query_string.go (3)
1-8: LGTM! Clean package structure and imports.
The package structure follows Go best practices with clear organization and necessary imports.
42-42: Clarify the purpose of the empty IsTx method.
The empty IsTx method seems to serve no purpose. If it's a marker interface method, it should be documented. Otherwise, consider removing it.
✅ Verification successful
Let me gather more context about the interface that defines this method.
Empty IsTx method is a valid marker interface implementation
The empty IsTx() method is intentionally empty as it serves as a marker interface implementation. It's part of the Transaction interface defined in runtime/transaction/transaction.go which is used to mark and identify transaction-related types. This is a common pattern in Go to implement type-based behavior identification.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to IsTx to understand its purpose
rg -A 5 "IsTx()"
Length of output: 1770
Script:
#!/bin/bash
# Search for the interface definition that requires IsTx()
ast-grep --pattern 'interface {
IsTx()
$$$
}'
Length of output: 272
48-53: Consider improving transaction handling.
The current transaction implementation could benefit from:
- Documentation explaining the channel buffer size choice
- Context/timeout handling for transaction operations
- Cleanup mechanism for abandoned transactions
Also, verify that the transaction implementation is thread-safe and handles concurrent access properly.
| func (q QueryString) Exec(ctx context.Context, into interface{}) error { | ||
| payload := protocol.GQLRequest{ | ||
| Query: q.Query, | ||
| Variables: map[string]interface{}{}, | ||
| } | ||
| return q.Do(ctx, payload, into) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and error handling
The Exec method should validate the query string before execution. Consider:
- Checking for empty query strings
- Basic GraphQL syntax validation
- Proper error wrapping for context tracing
func (q QueryString) Exec(ctx context.Context, into interface{}) error {
+ if q.Query == "" {
+ return fmt.Errorf("query string cannot be empty")
+ }
+ if ctx == nil {
+ return fmt.Errorf("context cannot be nil")
+ }
payload := protocol.GQLRequest{
Query: q.Query,
Variables: map[string]interface{}{},
}
- return q.Do(ctx, payload, into)
+ if err := q.Do(ctx, payload, into); err != nil {
+ return fmt.Errorf("failed to execute query: %w", err)
+ }
+ return nil
}📝 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.
| func (q QueryString) Exec(ctx context.Context, into interface{}) error { | |
| payload := protocol.GQLRequest{ | |
| Query: q.Query, | |
| Variables: map[string]interface{}{}, | |
| } | |
| return q.Do(ctx, payload, into) | |
| } | |
| func (q QueryString) Exec(ctx context.Context, into interface{}) error { | |
| if q.Query == "" { | |
| return fmt.Errorf("query string cannot be empty") | |
| } | |
| if ctx == nil { | |
| return fmt.Errorf("context cannot be nil") | |
| } | |
| payload := protocol.GQLRequest{ | |
| Query: q.Query, | |
| Variables: map[string]interface{}{}, | |
| } | |
| if err := q.Do(ctx, payload, into); err != nil { | |
| return fmt.Errorf("failed to execute query: %w", err) | |
| } | |
| return nil | |
| } |
| package builder | ||
|
|
||
| import ( | ||
| "context" | ||
| "fmt" | ||
| "github.com/steebchen/prisma-client-go/engine" | ||
| "github.com/steebchen/prisma-client-go/engine/protocol" | ||
| "github.com/steebchen/prisma-client-go/logger" | ||
| "time" | ||
| ) | ||
|
|
||
| type QueryString struct { | ||
| // Engine holds the implementation of how queries are processed | ||
| Engine engine.Engine | ||
|
|
||
| // Start time of the request for tracing | ||
| Start time.Time | ||
|
|
||
| // Query string | ||
| Query string | ||
|
|
||
| TxResult chan []byte | ||
| } | ||
|
|
||
| func NewQueryString() QueryString { | ||
| return QueryString{ | ||
| Start: time.Now(), | ||
| } | ||
| } | ||
|
|
||
| func (q QueryString) Exec(ctx context.Context, into interface{}) error { | ||
| payload := protocol.GQLRequest{ | ||
| Query: q.Query, | ||
| Variables: map[string]interface{}{}, | ||
| } | ||
| return q.Do(ctx, payload, into) | ||
| } | ||
|
|
||
| func (q QueryString) Do(ctx context.Context, payload interface{}, into interface{}) error { | ||
| if q.Engine == nil { | ||
| return fmt.Errorf("client.Prisma.Connect() needs to be called before sending queries") | ||
| } | ||
|
|
||
| err := q.Engine.Do(ctx, payload, into) | ||
| now := time.Now() | ||
| totalDuration := now.Sub(q.Start) | ||
| logger.Debug.Printf("[timing] TOTAL %q", totalDuration) | ||
| return err | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the gathered information, I can now provide a final response.
Raw query execution requires additional security measures
The implementation currently lacks built-in security measures for raw query execution. Since this is part of Prisma's query engine which already handles query validation and security at the protocol level, the primary concerns should be:
- Document that this is a power-user feature that bypasses Prisma's usual safety mechanisms
- Add input validation for the query string to prevent empty or malformed queries
- Consider adding a warning in the logger when raw queries are used
Specific locations requiring attention:
runtime/builder/query_string.go: Add basic input validation in theExecmethod before query execution- Add documentation comments explaining the security implications of using raw queries
The previous suggestions about rate limiting and query complexity analysis are not necessary as these would typically be handled at the application level, not within the Prisma client implementation.
🔗 Analysis chain
Consider security implications of raw queries
Since this implementation allows execution of raw query strings, consider:
- Adding query sanitization
- Implementing rate limiting
- Adding query complexity analysis to prevent DoS attacks
- Documenting security best practices for users
Let me gather more specific information about the query execution and validation mechanisms.
Let me check one more aspect about the query execution flow to understand the complete picture.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing security measures in the codebase
# Look for sanitization or rate limiting implementations
# Check for security-related code
rg -i "sanitize|rate.?limit|complexity"
# Check for security documentation
fd -t f . | rg -i "readme|security|doc" | xargs rg -i "security|best.?practices"
Length of output: 1846
Script:
#!/bin/bash
# Check for any existing query validation or security measures in the engine
ast-grep --pattern 'func ($x *Engine) Do($ctx, $payload, $into) {
$$$
}'
# Look for any query validation or security measures in the protocol
rg -A 10 "type GQLRequest"
# Check if there are any existing query validation mechanisms
rg -i "validate.*query|query.*validate"
Length of output: 760
Script:
#!/bin/bash
# Check the engine implementation for any security measures
rg -A 10 "type Engine interface"
# Look for any query preprocessing or validation in the engine package
fd -t f . -e go | grep "engine/" | xargs rg -i "func.*query|process.*query"
# Check if there are any existing security measures in the engine implementation
rg -i "security|protect|prevent" engine/
Length of output: 1582
| func (r TxQueryStringResult) Into(v interface{}) error { | ||
| return r.result.Get(r.query.TxResult, &v) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil checks in the Into method.
The Into method should validate its inputs and the internal state before proceeding.
func (r TxQueryStringResult) Into(v interface{}) error {
+ if r.result == nil {
+ return fmt.Errorf("transaction result is nil")
+ }
+ if v == nil {
+ return fmt.Errorf("destination interface cannot be nil")
+ }
return r.result.Get(r.query.TxResult, &v)
}📝 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.
| func (r TxQueryStringResult) Into(v interface{}) error { | |
| return r.result.Get(r.query.TxResult, &v) | |
| } | |
| func (r TxQueryStringResult) Into(v interface{}) error { | |
| if r.result == nil { | |
| return fmt.Errorf("transaction result is nil") | |
| } | |
| if v == nil { | |
| return fmt.Errorf("destination interface cannot be nil") | |
| } | |
| return r.result.Get(r.query.TxResult, &v) | |
| } |
| func (r Raw) QueryString(query string) QueryStringExec { | ||
| queryStr := builder.NewQueryString() | ||
| queryStr.Engine = r.Engine | ||
| queryStr.Query = query | ||
|
|
||
| return QueryStringExec{ | ||
| query: queryStr, | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for the query string parameter.
The raw query string should be validated to prevent potential SQL injection or malformed queries. Consider:
- Checking for empty strings
- Basic query structure validation
- Adding documentation about security considerations when using raw queries
func (r Raw) QueryString(query string) QueryStringExec {
+ if query == "" {
+ panic("query string cannot be empty")
+ }
queryStr := builder.NewQueryString()
queryStr.Engine = r.Engine
queryStr.Query = query
return QueryStringExec{
query: queryStr,
}
}Committable suggestion skipped: line range outside the PR's diff.
|
What's the use case for this? Unfortunately Prisma is moving away from GQL to a json protocol, so this will be deprecated soon. I just did not have the chance to refactor the codebase from gql to the new json protocol as I didn't have the time. Although in the future this new API could be used to do raw json protocol queries. |
in my system. A serivce will call db service (Repo service) to get data. So i want to reuse query builder to query data from A service (from A service i will send raw query to repo service) |
Thanks for info about prisma is moving GQL to json protocol, i will check and implement that |
steebchen
changed the title
|
You can remove this line prisma-client-go/engine/lifecycle.go Line 256 in c207dc0 to see how the protocol looks. It's basically very similar to gql but it's JSON. Note that involves lots of changes all over the codebase. You can also check out this RobertCraigie/prisma-client-py#707 for more info. I'll send more info as well from the linked notion doc which I can't access, but I asked Prisma to send this over. |
Add feat to exec query from QueryString (Raw Query Builder)
example:
var users []db.UserModel
client.Prisma.QueryString(
query {result: findManyUser(where:{AND:[{email:{equals:"truanv@gmail",}},{lastName:{equals:"Van",}},],}) {id createdAt firstName lastName email }}).Exec(ctx, &users);Summary by CodeRabbit
New Features
QueryStringfunctionality.QueryStringExecandTxQueryStringResulttypes.Bug Fixes
Documentation