-
Notifications
You must be signed in to change notification settings - Fork 81
Feature/add mis endpoints in query #696
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Feature/add mis endpoints in query #696
Conversation
… specified database
02974e4
to
f6a6c13
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds 16 missing query-related endpoints to the ArangoDB Go driver v2, enabling comprehensive query management, caching, optimizer rules, and user-defined functions functionality. The implementation includes comprehensive test coverage for all new endpoints.
- Adds query properties management (GET/PUT endpoints)
- Implements query monitoring and control (current/slow queries, kill operations)
- Adds query caching and plan caching management endpoints
- Integrates user-defined function management capabilities
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
v2/utils/type.go | Adds ToJSONString utility function for debugging and logging |
v2/tests/database_query_test.go | Comprehensive test suite for all 16 new query endpoints |
v2/arangodb/utils.go | Adds RequiredFieldError utility for validation |
v2/arangodb/database_query_impl.go | Core implementation of all query-related endpoints |
v2/arangodb/database_query.go | Interface definitions and data structures for query operations |
v2/CHANGELOG.md | Documents the addition of missing query endpoints |
v2/arangodb/database_query_impl.go
Outdated
// Log the raw response for debugging (remove in production) | ||
fmt.Printf("DEBUG: Raw response from %s: %s\n", endpoint, string(rawResult)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debug logging statement should be removed from production code. Consider using a proper logging framework or removing this debug print statement.
// Log the raw response for debugging (remove in production) | |
fmt.Printf("DEBUG: Raw response from %s: %s\n", endpoint, string(rawResult)) | |
// Raw response available in rawResult for debugging if needed |
Copilot uses AI. Check for mistakes.
v2/arangodb/database_query.go
Outdated
GetAllOptimizerRules(ctx context.Context) ([]OptimizerRules, error) | ||
|
||
// GetQueryPlanCache returns a list of cached query plans. | ||
// The result is a list of QueryPlanChcheRespObject objects. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typo in the comment on line 83: 'QueryPlanChcheRespObject' should be 'QueryPlanCacheRespObject'.
// The result is a list of QueryPlanChcheRespObject objects. | |
// The result is a list of QueryPlanCacheRespObject objects. |
Copilot uses AI. Check for mistakes.
v2/arangodb/database_query.go
Outdated
} | ||
|
||
type QueryCacheProperties struct { | ||
// IncludesSystem indicates whether the query cache includes system collections. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment inconsistency: the comment says 'IncludesSystem' but the field name is 'IncludeSystem'. The comment should match the field name.
// IncludesSystem indicates whether the query cache includes system collections. | |
// IncludeSystem indicates whether the query cache includes system collections. |
Copilot uses AI. Check for mistakes.
func validateQueryPropertiesFields(options QueryProperties) error { | ||
if options.Enabled == nil { | ||
return RequiredFieldError("enabled") | ||
} | ||
if options.TrackSlowQueries == nil { | ||
return RequiredFieldError("trackSlowQueries") | ||
} | ||
if options.TrackBindVars == nil { | ||
return RequiredFieldError("trackBindVars") | ||
} | ||
if options.MaxSlowQueries == nil { | ||
return RequiredFieldError("maxSlowQueries") | ||
} | ||
if options.SlowQueryThreshold == nil { | ||
return RequiredFieldError("slowQueryThreshold") | ||
} | ||
if options.MaxQueryStringLength == nil { | ||
return RequiredFieldError("maxQueryStringLength") | ||
} | ||
return nil | ||
} | ||
|
||
func (d databaseQuery) UpdateQueryProperties(ctx context.Context, options QueryProperties) (QueryProperties, error) { | ||
url := d.db.url("_api", "query", "properties") | ||
|
||
// Validate all fields are set | ||
if err := validateQueryPropertiesFields(options); err != nil { | ||
return QueryProperties{}, err | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] The validation function requires all fields to be non-nil, but this may be overly restrictive for partial updates. Consider allowing optional fields or having separate validation for create vs update operations.
func validateQueryPropertiesFields(options QueryProperties) error { | |
if options.Enabled == nil { | |
return RequiredFieldError("enabled") | |
} | |
if options.TrackSlowQueries == nil { | |
return RequiredFieldError("trackSlowQueries") | |
} | |
if options.TrackBindVars == nil { | |
return RequiredFieldError("trackBindVars") | |
} | |
if options.MaxSlowQueries == nil { | |
return RequiredFieldError("maxSlowQueries") | |
} | |
if options.SlowQueryThreshold == nil { | |
return RequiredFieldError("slowQueryThreshold") | |
} | |
if options.MaxQueryStringLength == nil { | |
return RequiredFieldError("maxQueryStringLength") | |
} | |
return nil | |
} | |
func (d databaseQuery) UpdateQueryProperties(ctx context.Context, options QueryProperties) (QueryProperties, error) { | |
url := d.db.url("_api", "query", "properties") | |
// Validate all fields are set | |
if err := validateQueryPropertiesFields(options); err != nil { | |
return QueryProperties{}, err | |
} | |
// Validation for all fields is no longer required; partial updates are allowed. | |
func (d databaseQuery) UpdateQueryProperties(ctx context.Context, options QueryProperties) (QueryProperties, error) { | |
url := d.db.url("_api", "query", "properties") | |
// Partial updates are allowed; no need to validate all fields are set |
Copilot uses AI. Check for mistakes.
v2/tests/database_query_test.go
Outdated
MaxSlowQueries: utils.NewType(*res.MaxSlowQueries + *utils.NewType(1)), | ||
SlowQueryThreshold: utils.NewType(*res.SlowQueryThreshold + *utils.NewType(0.1)), | ||
MaxQueryStringLength: utils.NewType(*res.MaxQueryStringLength + *utils.NewType(100)), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Complex pointer arithmetic with nested NewType calls makes the code hard to read. Consider simplifying: utils.NewType(*res.MaxSlowQueries + 1)
.
MaxSlowQueries: utils.NewType(*res.MaxSlowQueries + *utils.NewType(1)), | |
SlowQueryThreshold: utils.NewType(*res.SlowQueryThreshold + *utils.NewType(0.1)), | |
MaxQueryStringLength: utils.NewType(*res.MaxQueryStringLength + *utils.NewType(100)), | |
MaxSlowQueries: utils.NewType(*res.MaxSlowQueries + 1), | |
SlowQueryThreshold: utils.NewType(*res.SlowQueryThreshold + 0.1), | |
MaxQueryStringLength: utils.NewType(*res.MaxQueryStringLength + 100), |
Copilot uses AI. Check for mistakes.
v2/tests/database_query_test.go
Outdated
|
||
for cursor.HasMore() { | ||
var doc map[string]interface{} | ||
cursor.ReadDocument(ctx, &doc) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error from ReadDocument is being ignored, which could mask issues during cursor iteration. The error should be handled properly.
cursor.ReadDocument(ctx, &doc) | |
err := cursor.ReadDocument(ctx, &doc) | |
require.NoError(t, err) |
Copilot uses AI. Check for mistakes.
This includes the following Query-related endpoints: