feat(store/v2): route to the commitment during migration #22290
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request enhance the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
|
@cool-develope your pull request is missing a changelog! |
![coderabbitai[bot]](https://github.com/avatars/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 (7)
store/v2/root/store.go (2)
121-140: Excellent addition ofgetVersionedReadermethodThe new
getVersionedReadermethod effectively encapsulates the logic for retrieving a versioned reader from either state storage or state commitment. This improves code organization and reusability.Consider combining the error checks for both state storage and state commitment to provide a more informative error message. For example:
if err != nil { - return nil, err + return nil, fmt.Errorf("failed to check version existence: %w", err) }This change would provide more context in case of an error.
157-161: Effective refactoring ofStateAtmethodThe
StateAtmethod has been successfully refactored to use the newgetVersionedReadermethod, which simplifies the code and improves consistency withStateLatest.Consider improving error handling by wrapping the error returned from
getVersionedReader. This would provide more context about where the error occurred:func (s *Store) StateAt(v uint64) (corestore.ReaderMap, error) { vReader, err := s.getVersionedReader(v) - return NewReaderMap(v, vReader), err + if err != nil { + return nil, fmt.Errorf("failed to get versioned reader for version %d: %w", v, err) + } + return NewReaderMap(v, vReader), nil }This change would make debugging easier by providing more information about the error's origin.
store/v2/commitment/tree.go (2)
40-41: Clarify the comment for theReaderinterface.To adhere to Go documentation conventions, the comment can be more concise and start with the interface name. Consider rephrasing it as:
// Reader is an optional interface used to read data from the tree during the migration process.
43-44: Add comments to exported methods in theReaderinterface.According to the Go style guidelines, all exported methods should have comments. Please add comments to
GetandIteratormethods to improve code documentation.Example:
// Get retrieves the value for a given version and key. Get(version uint64, key []byte) ([]byte, error) // Iterator returns an iterator for a given version and key range. Iterator(version uint64, start, end []byte, ascending bool) (corestore.Iterator, error)store/v2/commitment/iavl/tree.go (2)
80-80: Improve function comments to adhere to GoDoc conventionsThe comments for the
GetandIteratormethods currently state that they implement theReaderinterface. To enhance clarity and conform to GoDoc conventions, consider updating the comments to describe the functionality of each method, starting with the method name.Apply the following changes:
-// Get implements the Reader interface. +// Get retrieves the value associated with the specified key at the given version. ... -// Iterator implements the Reader interface. +// Iterator returns an iterator over a range of keys for a specified version.Also applies to: 90-91
Line range hint
80-87: Potential issue with castingversionfromuint64toint64In both the
GetandIteratormethods, theversionparameter is cast fromuint64toint64when callingGetImmutable:immutableTree, err := t.tree.GetImmutable(int64(version))If
versionexceeds the maximum value ofint64, this casting could lead to incorrect behavior due to integer overflow.Consider adding a validation check to ensure that
versiondoes not exceedmath.MaxInt64before casting. For example:if version > math.MaxInt64 { return nil, fmt.Errorf("version %d is too large", version) }Alternatively, evaluate whether the underlying
iavlmethods can be updated to acceptuint64to avoid this issue.Also applies to: 90-98
store/v2/commitment/store.go (1)
302-309: Ensure proper formatting of byte slices in error messagesUsing
%sto format byte slices in error messages may not display the keys correctly if they contain non-printable characters. Consider formatting thekeyandstoreKeyusing%Xfor hexadecimal representation or%qfor a quoted string.Suggested change:
- return nil, fmt.Errorf("failed to get key %s from store %s: %w", key, storeKey, err) + return nil, fmt.Errorf("failed to get key %X from store %s: %w", key, storeKey, err)Or, if safe to convert to string:
- return nil, fmt.Errorf("failed to get key %s from store %s: %w", key, storeKey, err) + return nil, fmt.Errorf("failed to get key %s from store %s: %w", string(key), storeKey, err)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (6)
- store/v2/commitment/iavl/tree.go (3 hunks)
- store/v2/commitment/store.go (2 hunks)
- store/v2/commitment/tree.go (2 hunks)
- store/v2/database.go (0 hunks)
- store/v2/root/reader.go (2 hunks)
- store/v2/root/store.go (1 hunks)
💤 Files with no reviewable changes (1)
- store/v2/database.go
🧰 Additional context used
📓 Path-based instructions (5)
store/v2/commitment/iavl/tree.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/commitment/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/commitment/tree.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/reader.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (14)
store/v2/root/store.go (1)
143-155: Well-executed refactoring ofStateLatestmethodThe refactoring of the
StateLatestmethod improves code clarity and maintainability. By utilizing the newgetVersionedReadermethod, it reduces code duplication and centralizes the logic for retrieving versioned readers.The error handling is concise and appropriate. Good job on returning early in case of errors, which improves readability.
store/v2/commitment/tree.go (1)
9-9: Import statements are correctly added.The new import statements for
corestoreandsnapshotstypesare necessary and properly formatted.store/v2/root/reader.go (3)
17-18: Field alignment and documentation look good.The struct fields in
ReaderMapandReaderare well-defined with clear comments, adhering to Go documentation standards and promoting code readability.Also applies to: 34-36
48-50: Efficient error handling inHasmethod.The
Hasmethod correctly handles errors returned fromvReader.Has, ensuring that any encountered error is promptly returned.
57-57: Methods correctly delegate tovReader.The
Get,Iterator, andReverseIteratormethods properly call the corresponding methods onvReader, maintaining the intended functionality.Also applies to: 61-61, 65-65
store/v2/commitment/iavl/tree.go (3)
17-17: Confirming implementation ofcommitment.Readerinterface
The addition ofvar _ commitment.Reader = (*IavlTree)(nil)correctly asserts thatIavlTreeimplements thecommitment.Readerinterface.
Line range hint
80-87:Getmethod logic and error handling are correctThe
Getmethod appropriately retrieves the immutable tree at the specified version and returns the value associated with the key. The error handling is properly implemented, and the method aligns with the expected functionality of thecommitment.Readerinterface.
90-98:Iteratormethod logic and error handling are appropriateThe
Iteratormethod correctly retrieves the immutable tree at the specified version and returns an iterator over the specified key range. Error handling is properly managed, ensuring that any issues in retrieving the immutable tree are surfaced.store/v2/commitment/store.go (6)
28-31: Verification ofstore.VersionedReaderimplementationThe
CommitStorecorrectly implements thestore.VersionedReaderinterface, and the accompanying comment accurately explains its usage during the migration process.
282-294:getReadermethod retrievesReaderinterface appropriatelyThe
getReadermethod efficiently fetches the tree associated with the givenstoreKeyand asserts that it implements theReaderinterface. Error handling is properly implemented.
296-301:VersionExistsmethod correctly checks version existenceThe
VersionExistsmethod accurately determines if a given version exists in the metadata by checking if the commit info is not nil.
317-321:Hasmethod correctly determines key presenceThe
Hasmethod appropriately checks for the existence of a key by utilizing theGetmethod and evaluating the length of the returned value.
323-331:Iteratormethod implemented correctlyThe
Iteratormethod correctly obtains an iterator over the key range for the specified version, ensuring forward iteration by passingtruefor the direction.
333-341:ReverseIteratormethod implemented correctlyThe
ReverseIteratormethod correctly obtains a reverse iterator over the key range for the specified version by passingfalsefor the direction.
| // Reader is the optional interface that is only used to read data from the tree | ||
| // during the migration process. | ||
| type Reader interface { | ||
| Get(version uint64, key []byte) ([]byte, error) | ||
| Iterator(version uint64, start, end []byte, ascending bool) (corestore.Iterator, error) | ||
| } | ||
|
|
There was a problem hiding this comment.
Consider the impact of removing Get from Tree interface on existing implementations.
By removing the Get method from the Tree interface and introducing the Reader interface, existing code that relies on Tree.Get may break. Ensure that all implementations and usages of Tree are updated to reflect this change, and consider providing guidance or deprecation notices to assist with the migration.
| func NewReaderMap(v uint64, vr store.VersionedReader) *ReaderMap { | ||
| return &ReaderMap{ | ||
| rootStore: rs, | ||
| version: v, | ||
| vReader: vr, | ||
| version: v, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use descriptive parameter names for clarity.
In the constructors NewReaderMap and NewReader, the parameters v and vr are abbreviated and may reduce readability. According to the Uber Go Style Guide, parameter names should be self-explanatory. Consider renaming v to version and vr to versionedReader for better clarity.
Apply this diff to update the parameter names:
-func NewReaderMap(v uint64, vr store.VersionedReader) *ReaderMap {
+func NewReaderMap(version uint64, versionedReader store.VersionedReader) *ReaderMap {
return &ReaderMap{
- vReader: vr,
- version: v,
+ vReader: versionedReader,
+ version: version,
}
}
-func NewReader(v uint64, vr store.VersionedReader, actor []byte) *Reader {
+func NewReader(version uint64, versionedReader store.VersionedReader, actor []byte) *Reader {
return &Reader{
- version: v,
- vReader: vr,
+ version: version,
+ vReader: versionedReader,
actor: actor,
}
}Also applies to: 39-43
| } | ||
| } | ||
|
|
||
| func (roa *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) { | ||
| return NewReader(roa.version, roa.rootStore, actor), nil | ||
| return NewReader(roa.version, roa.vReader, actor), nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use conventional receiver names for consistency.
The receiver name roa is used for both ReaderMap and Reader methods, which can be confusing. As per Go conventions and the Uber Go Style Guide, receiver names should be a concise abbreviation of the type name. Consider using rm for ReaderMap and r for Reader to enhance code readability.
Apply this diff to update the receiver names:
-func (roa *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) {
+func (rm *ReaderMap) GetReader(actor []byte) (corestore.Reader, error) {
- return NewReader(roa.version, roa.vReader, actor), nil
+ return NewReader(rm.version, rm.vReader, actor), nil
}
-func (roa *Reader) Has(key []byte) (bool, error) {
+func (r *Reader) Has(key []byte) (bool, error) {
- val, err := roa.vReader.Has(roa.actor, roa.version, key)
+ val, err := r.vReader.Has(r.actor, r.version, key)
if err != nil {
return false, err
}
return val, nil
}
-func (roa *Reader) Get(key []byte) ([]byte, error) {
+func (r *Reader) Get(key []byte) ([]byte, error) {
- return roa.vReader.Get(roa.actor, roa.version, key)
+ return r.vReader.Get(r.actor, r.version, key)
}
-func (roa *Reader) Iterator(start, end []byte) (corestore.Iterator, error) {
+func (r *Reader) Iterator(start, end []byte) (corestore.Iterator, error) {
- return roa.vReader.Iterator(roa.actor, roa.version, start, end)
+ return r.vReader.Iterator(r.actor, r.version, start, end)
}
-func (roa *Reader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
+func (r *Reader) ReverseIterator(start, end []byte) (corestore.Iterator, error) {
- return roa.vReader.ReverseIterator(roa.actor, roa.version, start, end)
+ return r.vReader.ReverseIterator(r.actor, r.version, start, end)
}Also applies to: 48-48, 57-57, 61-61, 65-65
| bz, err := tree.Get(version, key) | ||
| reader, ok := tree.(Reader) | ||
| if !ok { | ||
| return nil, errors.New("tree does not implement Reader") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error message for better context
Including the storeKey in the error message when the tree does not implement Reader can provide more context for debugging.
Suggested change:
- return nil, errors.New("tree does not implement Reader")
+ return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey)📝 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.
| return nil, errors.New("tree does not implement Reader") | |
| return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey) |
store/v2/commitment/store.go
Outdated
| // Has implements store.VersionedReader. | ||
| func (c *CommitStore) Has(storeKey []byte, version uint64, key []byte) (bool, error) { | ||
| val, err := c.Get(storeKey, version, key) | ||
| return len(val) > 0, err |
There was a problem hiding this comment.
This is not correct. Empty slice is an supported value but not nil
| return len(val) > 0, err | |
| return val != nil, err |
You can use this test to verify:
func (s *CommitStoreTestSuite) TestStore_Has() {
storeKeys := []string{storeKey1}
myKey := []byte("myKey")
const initialVersion = 1
specs := map[string]struct {
src *corestore.Changeset
queryVersion uint64
expExists bool
expErr bool
}{
"known key with some value": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
storeKey1: {{Key: myKey, Value: []byte("my-value")}},
}),
queryVersion: initialVersion,
expExists: true,
},
"known key with empty value": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
storeKey1: {{Key: myKey, Value: []byte("")}},
}),
queryVersion: initialVersion,
expExists: true,
},
"unknown key": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{}),
queryVersion: initialVersion,
expExists: false,
},
"unknown version": {
src: corestore.NewChangesetWithPairs(map[string]corestore.KVPairs{
storeKey1: {{Key: myKey, Value: []byte("")}},
}),
queryVersion: initialVersion + 1,
expErr: true,
},
}
for name, spec := range specs {
s.T().Run(name, func(t *testing.T) {
commitStore, err := s.NewStore(dbm.NewMemDB(), storeKeys, nil, coretesting.NewNopLogger())
require.NoError(t, err)
require.NoError(t, commitStore.WriteChangeset(spec.src))
_, err = commitStore.Commit(initialVersion)
require.NoError(t, err)
// when
gotResult, gotErr := commitStore.Has([]byte(storeKey1), spec.queryVersion, myKey)
// then
if spec.expErr {
require.Error(t, gotErr)
return
}
require.NoError(t, gotErr)
require.Equal(t, spec.expExists, gotResult)
})
}
}There was a problem hiding this comment.
good catch, let me try to add this test case!
There was a problem hiding this comment.
I had added the test to this file locally: https://github.com/cosmos/cosmos-sdk/blob/main/store/v2/commitment/store_test_suite.go
| } | ||
|
|
||
| return v, NewReaderMap(v, s), nil | ||
| if vReader, ok := s.stateCommitment.(store.VersionedReader); ok { |
There was a problem hiding this comment.
How about adding the interface to the Committer interface so that you can get rid of the type cast?
type Committer interface {
VersionedReader
...
There was a problem hiding this comment.
we dont want to force commitment structures to implement iteration. This is here because of iavl mainly. If iavl didnt have iteration then this wouldnt be needed because iteration wouldnt be used in the state machine like today
| } | ||
| } | ||
|
|
||
| return nil, fmt.Errorf("version %d does not exist", version) |
There was a problem hiding this comment.
note: this error is also returned when the type cast failed.
There was a problem hiding this comment.
yes, it is expected
| @@ -118,27 +118,46 @@ func (s *Store) SetInitialVersion(v uint64) error { | |||
| return s.stateCommitment.SetInitialVersion(v) | |||
| } | |||
|
|
|||
| func (s *Store) StateLatest() (uint64, corestore.ReaderMap, error) { | |||
| v, err := s.GetLatestVersion() | |||
| func (s *Store) getVersionedReader(version uint64) (store.VersionedReader, error) { | |||
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
store/v2/commitment/iavl/tree_test.go (1)
137-214: Good test coverage, with room for improvements.The
TestIavlTreeIteratorfunction provides good coverage of the basic iterator functionality, including forward and reverse iteration across different versions. However, there are several areas where the test could be enhanced:
- Consider adding checks for the values associated with the keys, not just the keys themselves.
- Add tests for iterating with start and end key boundaries to ensure proper handling of partial ranges.
- Include checks to verify that the iterator is exhausted after the last key (i.e.,
Valid()returnsfalseafter the lastNext()call).- Add tests for error cases, such as attempting to iterate over a non-existent version.
Here's an example of how you could add a check for iterator exhaustion:
// After the last iter.Next() call in each iteration test require.False(t, iter.Valid(), "Iterator should be exhausted")And an example of testing with start and end boundaries:
// Add this test case // iterate over keys with start and end boundaries iter, err = tree.Iterator(3, []byte("key3"), []byte("key7"), true) require.NoError(t, err) expectedKeys = []string{"key3", "key4", "key5", "key6"} for i := 0; iter.Valid(); i++ { require.Equal(t, expectedKeys[i], string(iter.Key())) iter.Next() } require.False(t, iter.Valid(), "Iterator should be exhausted") require.NoError(t, iter.Close())store/v2/root/store.go (2)
121-145: LGTM with a minor suggestion for improvementThe new
getVersionedReadermethod effectively centralizes the version retrieval logic, improving code organization. It correctly checks both the state storage and state commitment for the requested version.Consider combining the error checks for
VersionExistscalls to reduce code duplication:func (s *Store) getVersionedReader(version uint64) (store.VersionedReader, error) { - isExist, err := s.stateStorage.VersionExists(version) - if err != nil { - return nil, err - } - if isExist { + if isExist, err := s.stateStorage.VersionExists(version); err != nil { + return nil, fmt.Errorf("failed to check version existence in state storage: %w", err) + } else if isExist { return s.stateStorage, nil } if vReader, ok := s.stateCommitment.(store.VersionedReader); ok { - isExist, err := vReader.VersionExists(version) - if err != nil { - return nil, err - } - if isExist { + if isExist, err := vReader.VersionExists(version); err != nil { + return nil, fmt.Errorf("failed to check version existence in state commitment: %w", err) + } else if isExist { return vReader, nil } } return nil, fmt.Errorf("version %d does not exist", version) }This change would make the error handling more consistent and informative.
162-166: LGTM with a minor suggestion for error handlingThe refactored
StateAtmethod effectively uses the newgetVersionedReaderfunction, aligning well with the changes inStateLatest. The implementation is concise and clear.Consider wrapping the error returned by
getVersionedReaderto provide more context:func (s *Store) StateAt(v uint64) (corestore.ReaderMap, error) { vReader, err := s.getVersionedReader(v) - return NewReaderMap(v, vReader), err + if err != nil { + return nil, fmt.Errorf("failed to get versioned reader for version %d: %w", v, err) + } + return NewReaderMap(v, vReader), nil }This change would make the error more informative for debugging purposes.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
📒 Files selected for processing (3)
- store/v2/commitment/iavl/tree_test.go (1 hunks)
- store/v2/commitment/store.go (2 hunks)
- store/v2/root/store.go (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
store/v2/commitment/iavl/tree_test.go (2)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
Pattern
**/*_test.go: "Assess the unit test code assessing sufficient code coverage for the changes associated in the pull request"store/v2/commitment/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.store/v2/root/store.go (1)
Pattern
**/*.go: Review the Golang code for conformity with the Uber Golang style guide, highlighting any deviations.
🔇 Additional comments (5)
store/v2/commitment/store.go (4)
28-31: LGTM: Correct implementation ofstore.VersionedReaderThe addition of the
store.VersionedReaderinterface implementation is correct and well-documented. The comment provides valuable context about its usage during the migration process.
296-300: LGTM: Correct implementation ofVersionExistsThe
VersionExistsmethod is well-implemented. It correctly uses themetadata.GetCommitInfomethod and properly handles the returned error.
302-316: LGTM: ImprovedGetmethod implementationThe updated
Getmethod is well-implemented. It correctly uses the newgetReadermethod, improving code reusability. The error handling is comprehensive and provides informative error messages.
323-341: LGTM: Well-implementedIteratorandReverseIteratormethodsThe
IteratorandReverseIteratormethods are well-implemented. They correctly use thegetReadermethod and provide consistent implementations. The error handling is appropriate, and the code is clean and readable.store/v2/root/store.go (1)
148-160: LGTM: Improved StateLatest implementationThe refactored
StateLatestmethod effectively utilizes the newgetVersionedReaderfunction, improving code clarity and reusability. The error handling is consistent, and the method correctly creates and returns aReaderMap.
| func (c *CommitStore) getReader(storeKey string) (Reader, error) { | ||
| tree, ok := c.multiTrees[storeKey] | ||
| if !ok { | ||
| return nil, fmt.Errorf("store %s not found", storeKey) | ||
| } | ||
|
|
||
| bz, err := tree.Get(version, key) | ||
| reader, ok := tree.(Reader) | ||
| if !ok { | ||
| return nil, errors.New("tree does not implement Reader") | ||
| } | ||
|
|
||
| return reader, nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error message for better context
The getReader method is well-implemented and improves code reusability. However, the error message when the tree doesn't implement Reader could be more informative.
Consider including the storeKey in the error message:
- return nil, errors.New("tree does not implement Reader")
+ return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey)📝 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 (c *CommitStore) getReader(storeKey string) (Reader, error) { | |
| tree, ok := c.multiTrees[storeKey] | |
| if !ok { | |
| return nil, fmt.Errorf("store %s not found", storeKey) | |
| } | |
| bz, err := tree.Get(version, key) | |
| reader, ok := tree.(Reader) | |
| if !ok { | |
| return nil, errors.New("tree does not implement Reader") | |
| } | |
| return reader, nil | |
| } | |
| func (c *CommitStore) getReader(storeKey string) (Reader, error) { | |
| tree, ok := c.multiTrees[storeKey] | |
| if !ok { | |
| return nil, fmt.Errorf("store %s not found", storeKey) | |
| } | |
| reader, ok := tree.(Reader) | |
| if !ok { | |
| return nil, fmt.Errorf("tree for store %s does not implement Reader", storeKey) | |
| } | |
| return reader, nil | |
| } |
| // Has implements store.VersionedReader. | ||
| func (c *CommitStore) Has(storeKey []byte, version uint64, key []byte) (bool, error) { | ||
| val, err := c.Get(storeKey, version, key) | ||
| return val != nil, err | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider handling empty values in Has method
The Has method is implemented concisely. However, it assumes that a nil value means the key doesn't exist, which might not always be the case. Some stores might use empty slices ([]byte{}) to represent existing keys with no value.
Consider updating the implementation to handle both nil and empty slice cases:
func (c *CommitStore) Has(storeKey []byte, version uint64, key []byte) (bool, error) {
val, err := c.Get(storeKey, version, key)
- return val != nil, err
+ return val != nil && len(val) > 0, err
}This change ensures that keys with empty values are still considered to exist.
📝 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.
| // Has implements store.VersionedReader. | |
| func (c *CommitStore) Has(storeKey []byte, version uint64, key []byte) (bool, error) { | |
| val, err := c.Get(storeKey, version, key) | |
| return val != nil, err | |
| } | |
| // Has implements store.VersionedReader. | |
| func (c *CommitStore) Has(storeKey []byte, version uint64, key []byte) (bool, error) { | |
| val, err := c.Get(storeKey, version, key) | |
| return val != nil && len(val) > 0, err | |
| } |
Description
Closes: #22243
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!in the type prefix if API or client breaking changeCHANGELOG.mdReviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
Please see Pull Request Reviewer section in the contributing guide for more information on how to review a pull request.
I have...
Summary by CodeRabbit
New Features
Bug Fixes