Skip to content

fix(collections): add IterateRaw() implementation for Multi index #25123

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(collections): add IterateRaw() implementation for Multi index #25123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
8 changes: 8 additions & 0 deletions collections/indexes/multi.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,6 +119,14 @@ func (m *Multi[ReferenceKey, PrimaryKey, Value]) Iterate(ctx context.Context, ra
return (MultiIterator[ReferenceKey, PrimaryKey])(iter), err
}

func (m *Multi[ReferenceKey, PrimaryKey, Value]) IterateRaw(
ctx context.Context, start, end []byte, order collections.Order,
) (
iter collections.Iterator[collections.Pair[ReferenceKey, PrimaryKey], collections.NoValue], err error,
) {
return m.refKeys.IterateRaw(ctx, start, end, order)
}

func (m *Multi[ReferenceKey, PrimaryKey, Value]) Walk(
ctx context.Context,
ranger collections.Ranger[collections.Pair[ReferenceKey, PrimaryKey]],
Expand Down
107 changes: 107 additions & 0 deletions collections/indexes/multi_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,3 +110,110 @@
require.NoError(t, err)
require.Equal(t, []byte{}, rawValue)
}

func TestMulti_PairPrimaryKey(t *testing.T) {
sk, ctx := deps()
schema := collections.NewSchemaBuilder(sk)

mi := NewMulti(schema,
collections.NewPrefix(1), "multi_index",
collections.Uint64Key,
collections.PairKeyCodec(collections.Uint64Key, collections.Uint64Key),
func(pk collections.Pair[uint64, uint64], _ collections.NoValue) (uint64, error) {
return pk.K2(), nil
},
)

// we create two reference keys for primary key 1 and 2 associated with "milan"
require.NoError(t, mi.Reference(ctx, collections.Join(uint64(1), uint64(1)), collections.NoValue{}, func() (collections.NoValue, error) { return collections.NoValue{}, collections.ErrNotFound }))
require.NoError(t, mi.Reference(ctx, collections.Join(uint64(2), uint64(1)), collections.NoValue{}, func() (collections.NoValue, error) { return collections.NoValue{}, collections.ErrNotFound }))

iter, err := mi.MatchExact(ctx, uint64(1))
require.NoError(t, err)
pks, err := iter.PrimaryKeys()
require.NoError(t, err)
expectedPks := []collections.Pair[uint64, uint64]{
collections.Join(uint64(1), uint64(1)),
collections.Join(uint64(2), uint64(1)),
}
require.Equal(t, expectedPks, pks)
Comment on lines +131 to +139
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Ensure iterator from MatchExact is closed

The iterator created at Line 131 isn’t closed. Add a defer right after the error check.

 iter, err := mi.MatchExact(ctx, uint64(1))
 require.NoError(t, err)
+defer iter.Close()
 pks, err := iter.PrimaryKeys()
📝 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.

Suggested change
iter, err := mi.MatchExact(ctx, uint64(1))
require.NoError(t, err)
pks, err := iter.PrimaryKeys()
require.NoError(t, err)
expectedPks := []collections.Pair[uint64, uint64]{
collections.Join(uint64(1), uint64(1)),
collections.Join(uint64(2), uint64(1)),
}
require.Equal(t, expectedPks, pks)
iter, err := mi.MatchExact(ctx, uint64(1))
require.NoError(t, err)
defer iter.Close()
pks, err := iter.PrimaryKeys()
require.NoError(t, err)
expectedPks := []collections.Pair[uint64, uint64]{
collections.Join(uint64(1), uint64(1)),
collections.Join(uint64(2), uint64(1)),
}
require.Equal(t, expectedPks, pks)
🤖 Prompt for AI Agents 
In collections/indexes/multi_test.go around lines 131 to 139, the iterator
returned by mi.MatchExact is not closed, which can lead to resource leaks. After
checking that err is nil, add a defer statement to close the iterator to ensure
it is properly released after use.


rawIter, err := mi.IterateRaw(ctx, nil, nil, collections.OrderAscending)
require.NoError(t, err)
defer iter.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Bug: Closing the wrong iterator (resource leak)

Line 143 defers iter.Close(), but the iterator created at Line 141 is rawIter. rawIter is never closed.

- defer iter.Close()
+ defer rawIter.Close()
📝 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.

Suggested change
defer iter.Close()
defer rawIter.Close()
🤖 Prompt for AI Agents 
In collections/indexes/multi_test.go at line 143, the deferred call incorrectly
closes iter instead of rawIter, causing a resource leak. Replace defer
iter.Close() with defer rawIter.Close() to ensure the correct iterator is closed
and resources are properly released.


count := 0
for ; rawIter.Valid(); rawIter.Next() {
key, err := rawIter.Key()
require.NoError(t, err)
require.Equal(t, uint64(1), key.K1())
expectedKey := collections.Join(uint64(count+1), uint64(1))
require.Equal(t, expectedKey, key.K2())
count++
}
require.Equal(t, 2, count)
}

Check failure on line 155 in collections/indexes/multi_test.go

View workflow job for this annotation

GitHub Actions / Analyze 

File is not properly formatted (gofumpt)

Check failure on line 155 in collections/indexes/multi_test.go

View workflow job for this annotation

GitHub Actions / golangci-lint 

File is not properly formatted (gofumpt)
func TestMulti_IterateRaw(t *testing.T) {
sk, ctx := deps()
schema := collections.NewSchemaBuilder(sk)

mi := NewMulti(schema, collections.NewPrefix(1), "multi_index", collections.StringKey, collections.Uint64Key, func(_ uint64, value company) (string, error) {
return value.City, nil
})

// Insert some test data
company1 := company{City: "milan"}
company2 := company{City: "new york"}
company3 := company{City: "milan"}
companies := []company{company1, company2, company3}

for i, c := range companies {
ref := uint64(i) + 1
err := mi.Reference(ctx, ref, c, func() (company, error) { return c, nil })
require.NoError(t, err)
}

// Test IterateRaw with ascending order
iter, err := mi.IterateRaw(ctx, nil, nil, collections.OrderAscending)
require.NoError(t, err)
defer iter.Close()

var count int
for ; iter.Valid(); iter.Next() {
key, err := iter.Key()
require.NoError(t, err)
require.NotEmpty(t, key.K1())
require.NotEmpty(t, key.K2())
count++
}
require.Equal(t, 3, count)

// Test IterateRaw with descending order
iter, err = mi.IterateRaw(ctx, nil, nil, collections.OrderDescending)
require.NoError(t, err)
defer iter.Close()

count = 0
for ; iter.Valid(); iter.Next() {
key, err := iter.Key()
require.NoError(t, err)
require.NotEmpty(t, key.K1())
require.NotEmpty(t, key.K2())
count++
}
require.Equal(t, 3, count)

// Test with specific range - use MatchExact to get the correct keys
matchIter, err := mi.MatchExact(ctx, "milan")
require.NoError(t, err)
defer matchIter.Close()

count = 0
for ; matchIter.Valid(); matchIter.Next() {
fullKey, err := matchIter.FullKey()
require.NoError(t, err)
require.Equal(t, "milan", fullKey.K1())
count++
}
require.Equal(t, 2, count)
}
Loading