Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 11 additions & 18 deletions test/benchmarks_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ func BenchmarkV1BulkInsert100KDocs(b *testing.B) {
}

func bulkRead(b *testing.B, docSize int) {
db, col := setup(b)
_, col := setup(b)

// -----------------------------
// Prepare and insert documents
Expand All @@ -110,31 +110,24 @@ func bulkRead(b *testing.B, docSize int) {
require.NoError(b, err)

// -----------------------------------------
// Sub-benchmark 1: Read entire collection
// Sub-benchmark 1: Read entire collection using ReadDocuments
// -----------------------------------------
b.Run("ReadAllDocsOnce", func(b *testing.B) {
query := fmt.Sprintf("FOR d IN %s RETURN d", col.Name())
// Prepare keys for reading
keys := make([]string, docSize)
for j := 0; j < docSize; j++ {
keys[j] = fmt.Sprintf("doc_%d", j)
}

b.ResetTimer()
for i := 0; i < b.N; i++ {
cursor, err := db.Query(ctx, query, nil)
readDocs := make([]TestDoc, docSize)
_, _, err := col.ReadDocuments(ctx, keys, readDocs)
require.NoError(b, err)

count := 0
for {
var doc TestDoc
_, err := cursor.ReadDocument(ctx, &doc)
if driver.IsNoMoreDocuments(err) {
break
}
require.NoError(b, err)
count++
}
// require.Equal(b, docSize, count, "expected to read all documents")
_ = cursor.Close()
// sanity check
if count != docSize {
b.Fatalf("expected to read %d docs, got %d", docSize, count)
if len(readDocs) != docSize {
b.Fatalf("expected to read %d docs, got %d", docSize, len(readDocs))
}
}
})
Expand Down
1 change: 1 addition & 0 deletions v2/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
- Disabled V8 related testcases in V1 and V2
- Added new ConsolidationPolicy attributes to support updated configuration options for ArangoSearch Views properties and Inverted Indexes
- Add Vector index feature
- Add len() for Read methods

## [2.1.6](https://github.com/arangodb/go-driver/tree/v2.1.6) (2025-11-06)
- Add missing endpoints from replication
Expand Down
2 changes: 2 additions & 0 deletions v2/arangodb/collection_documents_create.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,7 +71,9 @@ type CollectionDocumentCreate interface {
}

type CollectionDocumentCreateResponseReader interface {
shared.ReadAllReadable[CollectionDocumentCreateResponse]
Read() (CollectionDocumentCreateResponse, error)
Len() int
}

type CollectionDocumentCreateResponse struct {
Expand Down
66 changes: 58 additions & 8 deletions v2/arangodb/collection_documents_create_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"context"
"io"
"net/http"
"reflect"
"sync"

"github.com/pkg/errors"

Expand All @@ -49,6 +51,13 @@ func (c collectionDocumentCreate) CreateDocumentsWithOptions(ctx context.Context
return nil, errors.Errorf("Input documents should be list")
}

// Get document count from input (same as v1 approach)
documentsVal := reflect.ValueOf(documents)
if documentsVal.Kind() == reflect.Ptr {
documentsVal = documentsVal.Elem()
}
documentCount := documentsVal.Len()

url := c.collection.url("document")

req, err := c.collection.connection().NewRequest(http.MethodPost, url)
Expand All @@ -73,7 +82,7 @@ func (c collectionDocumentCreate) CreateDocumentsWithOptions(ctx context.Context
case http.StatusCreated:
fallthrough
case http.StatusAccepted:
return newCollectionDocumentCreateResponseReader(&arr, opts), nil
return newCollectionDocumentCreateResponseReader(&arr, opts, documentCount), nil
default:
return nil, shared.NewResponseStruct().AsArangoErrorWithCode(code)
}
Expand Down Expand Up @@ -125,44 +134,64 @@ func (c collectionDocumentCreate) CreateDocument(ctx context.Context, document i
return c.CreateDocumentWithOptions(ctx, document, nil)
}

func newCollectionDocumentCreateResponseReader(array *connection.Array, options *CollectionDocumentCreateOptions) *collectionDocumentCreateResponseReader {
c := &collectionDocumentCreateResponseReader{array: array, options: options}
func newCollectionDocumentCreateResponseReader(array *connection.Array, options *CollectionDocumentCreateOptions, documentCount int) *collectionDocumentCreateResponseReader {
c := &collectionDocumentCreateResponseReader{
array: array,
options: options,
documentCount: documentCount,
}

if c.options != nil {
c.response.Old = newUnmarshalInto(c.options.OldObject)
c.response.New = newUnmarshalInto(c.options.NewObject)
}

c.ReadAllReader = shared.ReadAllReader[CollectionDocumentCreateResponse, *collectionDocumentCreateResponseReader]{Reader: c}
return c
}

var _ CollectionDocumentCreateResponseReader = &collectionDocumentCreateResponseReader{}

type collectionDocumentCreateResponseReader struct {
array *connection.Array
options *CollectionDocumentCreateOptions
response struct {
array *connection.Array
options *CollectionDocumentCreateOptions
documentCount int // Store input document count for Len() without caching
response struct {
*DocumentMeta
*shared.ResponseStruct `json:",inline"`
Old *UnmarshalInto `json:"old,omitempty"`
New *UnmarshalInto `json:"new,omitempty"`
}
shared.ReadAllReader[CollectionDocumentCreateResponse, *collectionDocumentCreateResponseReader]
mu sync.Mutex
}

func (c *collectionDocumentCreateResponseReader) Read() (CollectionDocumentCreateResponse, error) {
c.mu.Lock()
defer c.mu.Unlock()

if !c.array.More() {
return CollectionDocumentCreateResponse{}, shared.NoMoreDocumentsError{}
}

var meta CollectionDocumentCreateResponse

// Create new instances for each document to avoid pointer reuse
if c.options != nil {
meta.Old = c.options.OldObject
meta.New = c.options.NewObject
if c.options.OldObject != nil {
oldObjectType := reflect.TypeOf(c.options.OldObject).Elem()
meta.Old = reflect.New(oldObjectType).Interface()
}
if c.options.NewObject != nil {
newObjectType := reflect.TypeOf(c.options.NewObject).Elem()
meta.New = reflect.New(newObjectType).Interface()
}
Comment on lines +181 to +188
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reflection code assumes that OldObject and NewObject are non-nil pointers, but doesn't validate this before calling Elem(). If OldObject is nil, reflect.TypeOf(c.options.OldObject) will return nil, and calling .Elem() on it will panic. This same issue exists in lines 181-188 and 209-218. Consider adding nil checks or handling the nil case gracefully.

Copilot uses AI. Check for mistakes.
}

c.response.DocumentMeta = &meta.DocumentMeta
c.response.ResponseStruct = &meta.ResponseStruct
c.response.Old = newUnmarshalInto(meta.Old)
c.response.New = newUnmarshalInto(meta.New)

if err := c.array.Unmarshal(&c.response); err != nil {
if err == io.EOF {
Expand All @@ -175,5 +204,26 @@ func (c *collectionDocumentCreateResponseReader) Read() (CollectionDocumentCreat
return meta, meta.AsArangoError()
}

// Copy data from the new instances back to the original option objects for backward compatibility
if c.options != nil {
if c.options.OldObject != nil && meta.Old != nil {
oldValue := reflect.ValueOf(meta.Old).Elem()
originalValue := reflect.ValueOf(c.options.OldObject).Elem()
originalValue.Set(oldValue)
}
if c.options.NewObject != nil && meta.New != nil {
newValue := reflect.ValueOf(meta.New).Elem()
originalValue := reflect.ValueOf(c.options.NewObject).Elem()
originalValue.Set(newValue)
}
}

Comment on lines +207 to +220
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The back-propagation to c.options.OldObject and c.options.NewObject via reflection (lines 207-219) creates a potential data race if the same options object is shared across multiple concurrent Read() calls or readers. While the mutex protects the Read() method itself, if the same options object is passed to multiple readers, concurrent writes to the same options object could race. Consider documenting that options objects should not be shared across concurrent operations, or design this differently to avoid the race.

Suggested change
// Copy data from the new instances back to the original option objects for backward compatibility
if c.options != nil {
if c.options.OldObject != nil && meta.Old != nil {
oldValue := reflect.ValueOf(meta.Old).Elem()
originalValue := reflect.ValueOf(c.options.OldObject).Elem()
originalValue.Set(oldValue)
}
if c.options.NewObject != nil && meta.New != nil {
newValue := reflect.ValueOf(meta.New).Elem()
originalValue := reflect.ValueOf(c.options.NewObject).Elem()
originalValue.Set(newValue)
}
}

Copilot uses AI. Check for mistakes.
return meta, nil
}

// Len returns the number of items in the response.
// Returns the input document count immediately without reading/caching (same as v1 behavior).
// After calling Len(), you can still use Read() to iterate through items.
func (c *collectionDocumentCreateResponseReader) Len() int {
return c.documentCount
}
3 changes: 3 additions & 0 deletions v2/arangodb/collection_documents_delete.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,9 @@ type CollectionDocumentDeleteResponse struct {
}

type CollectionDocumentDeleteResponseReader interface {
shared.ReadAllIntoReadable[CollectionDocumentDeleteResponse]
Read(i interface{}) (CollectionDocumentDeleteResponse, error)
Len() int
}

type CollectionDocumentDeleteOptions struct {
Expand All @@ -82,6 +84,7 @@ type CollectionDocumentDeleteOptions struct {
WithWaitForSync *bool

// Return additionally the complete previous revision of the changed document
// Should be a pointer to an object
OldObject interface{}

// If set to true, an empty object is returned as response if the document operation succeeds.
Expand Down
49 changes: 43 additions & 6 deletions v2/arangodb/collection_documents_delete_impl.go
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import (
"context"
"io"
"net/http"
"reflect"
"sync"

"github.com/pkg/errors"

Expand All @@ -42,6 +44,7 @@ var _ CollectionDocumentDelete = &collectionDocumentDelete{}

type collectionDocumentDelete struct {
collection *collection
shared.ReadAllIntoReader[CollectionDocumentDeleteResponse, *collectionDocumentDeleteResponseReader]
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The embedded ReadAllIntoReader field in this struct appears to be unused and unnecessary. The collectionDocumentDelete methods return *collectionDocumentDeleteResponseReader, not use this embedded reader. This field should be removed to avoid confusion.

Suggested change
shared.ReadAllIntoReader[CollectionDocumentDeleteResponse, *collectionDocumentDeleteResponseReader]

Copilot uses AI. Check for mistakes.
}

func (c collectionDocumentDelete) DeleteDocument(ctx context.Context, key string) (CollectionDocumentDeleteResponse, error) {
Expand Down Expand Up @@ -78,6 +81,13 @@ func (c collectionDocumentDelete) DeleteDocumentsWithOptions(ctx context.Context
return nil, errors.Errorf("Input documents should be list")
}

// Get document count from input (same as v1 approach)
documentsVal := reflect.ValueOf(documents)
if documentsVal.Kind() == reflect.Ptr {
documentsVal = documentsVal.Elem()
}
documentCount := documentsVal.Len()

url := c.collection.url("document")

req, err := c.collection.connection().NewRequest(http.MethodDelete, url)
Expand All @@ -97,23 +107,34 @@ func (c collectionDocumentDelete) DeleteDocumentsWithOptions(ctx context.Context
if err != nil {
return nil, err
}
return newCollectionDocumentDeleteResponseReader(&arr, opts), nil
return newCollectionDocumentDeleteResponseReader(&arr, opts, documentCount), nil
}

func newCollectionDocumentDeleteResponseReader(array *connection.Array, options *CollectionDocumentDeleteOptions) *collectionDocumentDeleteResponseReader {
c := &collectionDocumentDeleteResponseReader{array: array, options: options}
func newCollectionDocumentDeleteResponseReader(array *connection.Array, options *CollectionDocumentDeleteOptions, documentCount int) *collectionDocumentDeleteResponseReader {
c := &collectionDocumentDeleteResponseReader{
array: array,
options: options,
documentCount: documentCount,
}

c.ReadAllIntoReader = shared.ReadAllIntoReader[CollectionDocumentDeleteResponse, *collectionDocumentDeleteResponseReader]{Reader: c}
return c
}

var _ CollectionDocumentDeleteResponseReader = &collectionDocumentDeleteResponseReader{}

type collectionDocumentDeleteResponseReader struct {
array *connection.Array
options *CollectionDocumentDeleteOptions
array *connection.Array
options *CollectionDocumentDeleteOptions
documentCount int // Store input document count for Len() without caching
shared.ReadAllIntoReader[CollectionDocumentDeleteResponse, *collectionDocumentDeleteResponseReader]
mu sync.Mutex
}

func (c *collectionDocumentDeleteResponseReader) Read(i interface{}) (CollectionDocumentDeleteResponse, error) {
c.mu.Lock()
defer c.mu.Unlock()

if !c.array.More() {
return CollectionDocumentDeleteResponse{}, shared.NoMoreDocumentsError{}
}
Expand Down Expand Up @@ -146,11 +167,27 @@ func (c *collectionDocumentDeleteResponseReader) Read(i interface{}) (Collection
}

if c.options != nil && c.options.OldObject != nil {
meta.Old = c.options.OldObject
// Create a new instance for each document to avoid pointer reuse
oldObjectType := reflect.TypeOf(c.options.OldObject).Elem()
meta.Old = reflect.New(oldObjectType).Interface()
Comment on lines +171 to +172
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reflection code assumes that OldObject is a non-nil pointer, but doesn't validate this before calling Elem(). If OldObject is nil (despite the nil check on line 169), reflect.TypeOf(c.options.OldObject) will return nil, and calling .Elem() on it will panic. Consider adding validation that the pointer is not nil after the options check.

Copilot uses AI. Check for mistakes.

// Extract old data into the new instance
if err := response.Object.Object.Extract("old").Inject(meta.Old); err != nil {
return CollectionDocumentDeleteResponse{}, err
}

// Copy data from the new instance to the original OldObject for backward compatibility
oldValue := reflect.ValueOf(meta.Old).Elem()
originalValue := reflect.ValueOf(c.options.OldObject).Elem()
originalValue.Set(oldValue)
Comment on lines +178 to +182
Copy link

Copilot AI Dec 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The back-propagation to c.options.OldObject via reflection (lines 179-182) creates a potential data race if the same options object is shared across multiple concurrent Read() calls or readers. While the mutex protects the Read() method itself, if the same options object is passed to multiple readers, concurrent writes to the same options object could race. Consider documenting that options objects should not be shared across concurrent operations, or design this differently to avoid the race.

Suggested change
// Copy data from the new instance to the original OldObject for backward compatibility
oldValue := reflect.ValueOf(meta.Old).Elem()
originalValue := reflect.ValueOf(c.options.OldObject).Elem()
originalValue.Set(oldValue)

Copilot uses AI. Check for mistakes.
}

return meta, nil
}

// Len returns the number of items in the response.
// Returns the input document count immediately without reading/caching (same as v1 behavior).
// After calling Len(), you can still use Read() to iterate through items.
func (c *collectionDocumentDeleteResponseReader) Len() int {
return c.documentCount
}
2 changes: 2 additions & 0 deletions v2/arangodb/collection_documents_read.go
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,8 @@ type CollectionDocumentRead interface {

type CollectionDocumentReadResponseReader interface {
Read(i interface{}) (CollectionDocumentReadResponse, error)
shared.ReadAllIntoReadable[CollectionDocumentReadResponse]
Len() int
}

type CollectionDocumentReadResponse struct {
Expand Down
Loading