-
Notifications
You must be signed in to change notification settings - Fork 81
add http end points related to tasks #690
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?
Conversation
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: root.
|
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 support for task management endpoints in the ArangoDB Go driver, including listing, retrieving, creating (with and without explicit IDs), and removing tasks. It also provides associated test coverage for these operations.
- Implemented
ClientTasks
interface and methods intasks_impl.go
- Updated
Client
to include the new task client - Added end-to-end tests for task creation, retrieval, listing, and deletion
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
File | Description |
---|---|
v2/tests/tasks_test.go | Tests for task endpoints: create (with/without ID), get, list, delete |
v2/arangodb/tasks_impl.go | Implementation of Tasks , Task , CreateTask , RemoveTask , CreateTaskWithID |
v2/arangodb/tasks.go | Definition of ClientTasks , TaskOptions , and Task interfaces |
v2/arangodb/client_impl.go | Registration of the task client in the main client struct |
v2/arangodb/client.go | Extension of the public Client interface to include ClientTasks |
Comments suppressed due to low confidence (2)
v2/tests/tasks_test.go:38
- [nitpick] The variable name
createtdTask
contains a typo and is ambiguous. Rename it tocreatedTask
for clarity.
createtdTask, err := client.CreateTask(ctx, options)
v2/tests/tasks_test.go:45
- After retrieving
taskInfo
, add a test that callstaskInfo.Params(&result)
and validates the unmarshaled parameters to ensureParams()
works correctly.
require.NotNil(t, taskInfo)
} | ||
|
||
func (c clientTask) Tasks(ctx context.Context) ([]Task, error) { | ||
urlEndpoint := connection.NewUrl("_api", "tasks") // Note: This should include database context, see below |
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 endpoint URL lacks the database context, so operations may target the system database. Use a helper to include the current database in the path (e.g., _db/{dbName}/_api/tasks
).
urlEndpoint := connection.NewUrl("_api", "tasks") // Note: This should include database context, see below | |
dbName := url.PathEscape(c.client.databaseName) // Ensure database name is URL-safe | |
urlEndpoint := connection.NewUrl("_db", dbName, "_api", "tasks") // Include database context |
Copilot uses AI. Check for mistakes.
case http.StatusOK: | ||
result := make([]Task, len(response)) | ||
for i, task := range response { | ||
fmt.Printf("Task %d: %+v\n", i, task) |
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] This debug fmt.Printf
may clutter output in production. Remove it or replace it with a structured logger at the appropriate log level.
Copilot uses AI. Check for mistakes.
func (c clientTask) CreateTaskWithID(ctx context.Context, id string, options *TaskOptions) (Task, error) { | ||
// Check if task already exists | ||
existingTask, err := c.Task(ctx, id) | ||
fmt.Printf("Checking existing task with ID: %s, existingTask: %v, Error:%v", id, existingTask, 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] This debug fmt.Printf
should be removed or converted to a proper log statement to avoid leaking internal state.
fmt.Printf("Checking existing task with ID: %s, existingTask: %v, Error:%v", id, existingTask, err) | |
log.Printf("DEBUG: Checking existing task with ID: %s, existingTask: %v, Error: %v", id, existingTask, err) |
Copilot uses AI. Check for mistakes.
// Check if task already exists | ||
existingTask, err := c.Task(ctx, id) | ||
fmt.Printf("Checking existing task with ID: %s, existingTask: %v, Error:%v", id, existingTask, err) | ||
if err == nil && existingTask != nil { |
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.
When checking for an existing task, only a NotFound error should be ignored. Currently any error lets creation proceed, potentially hiding real failures. Explicitly detect NotFound and propagate other errors.
if err == nil && existingTask != nil { | |
if err != nil { | |
if !shared.IsNotFound(err) { | |
return nil, errors.WithStack(err) | |
} | |
} else if existingTask != nil { |
Copilot uses AI. Check for mistakes.
require.NotNil(t, tasks) | ||
require.Greater(t, len(tasks), 0, "Expected at least one task to be present") | ||
t.Logf("Found tasks: %v", tasks) | ||
fmt.Printf("Number of tasks: %s\n", tasks[0].ID()) |
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] Use t.Logf
instead of fmt.Printf
in tests for consistent test logging and output capture.
fmt.Printf("Number of tasks: %s\n", tasks[0].ID()) | |
t.Logf("Number of tasks: %s", tasks[0].ID()) |
Copilot uses AI. Check for mistakes.
Thank you for your pull request and welcome to our community. We could not parse the GitHub identity of the following contributors: root.
|
This PR includes the implementation and test cases for the following task-related endpoints: