diff --git a/array.go b/array.go index c0706563..c6f47872 100644 --- a/array.go +++ b/array.go @@ -2790,27 +2790,51 @@ func (a *Array) setCallbackWithChild(i uint64, child Value, maxInlineSize uint64 // Index i will be updated with array operations, which affects element index. a.mutableElementIndex[vid] = i - c.setParentUpdater(func() error { + c.setParentUpdater(func() (found bool, err error) { // Avoid unnecessary write operation on parent container. // Child value was stored as SlabIDStorable (not inlined) in parent container, // and continues to be stored as SlabIDStorable (still not inlinable), // so no update to parent container is needed. if !c.Inlined() && !c.Inlinable(maxInlineSize) { - return nil + return true, nil } - // Get latest index by child value ID. - index, exist := a.getIndexByValueID(vid) + // Get latest adjusted index by child value ID. + adjustedIndex, exist := a.getIndexByValueID(vid) if !exist { - return NewFatalError(fmt.Errorf("failed to get index for child element with value id %s", vid)) + return false, nil + } + + storable, err := a.root.Get(a.Storage, adjustedIndex) + if err != nil { + // Don't need to wrap error as external error because err is already categorized by ArraySlab.Get(). + return false, err + } + + // Verify retrieved element is either SlabIDStorable or Slab, with identical value ID. + switch x := storable.(type) { + case SlabIDStorable: + sid := SlabID(x) + if !vid.equal(sid) { + return false, nil + } + + case Slab: + sid := x.SlabID() + if !vid.equal(sid) { + return false, nil + } + + default: + return false, nil } // Set child value with parent array using updated index. // Set() calls c.Storable() which returns inlined or not-inlined child storable. - existingValueStorable, err := a.set(index, c) + existingValueStorable, err := a.set(adjustedIndex, c) if err != nil { - return err + return false, err } // Verify overwritten storable has identical value ID. @@ -2819,7 +2843,7 @@ func (a *Array) setCallbackWithChild(i uint64, child Value, maxInlineSize uint64 case SlabIDStorable: sid := SlabID(x) if !vid.equal(sid) { - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten SlabIDStorable %s != value ID %s", sid, @@ -2829,7 +2853,7 @@ func (a *Array) setCallbackWithChild(i uint64, child Value, maxInlineSize uint64 case Slab: sid := x.SlabID() if !vid.equal(sid) { - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten Slab ID %s != value ID %s", sid, @@ -2837,18 +2861,18 @@ func (a *Array) setCallbackWithChild(i uint64, child Value, maxInlineSize uint64 } case nil: - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten value is nil")) default: - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten value is wrong type %T", existingValueStorable)) } - return nil + return true, nil }) } @@ -2857,7 +2881,18 @@ func (a *Array) notifyParentIfNeeded() error { if a.parentUpdater == nil { return nil } - return a.parentUpdater() + + // If parentUpdater() doesn't find child array (a), then no-op on parent container + // and unset parentUpdater callback in child array. This can happen when child + // array is an outdated reference (removed or overwritten in parent container). + found, err := a.parentUpdater() + if err != nil { + return err + } + if !found { + a.parentUpdater = nil + } + return nil } func (a *Array) Get(i uint64) (Value, error) { diff --git a/array_test.go b/array_test.go index 45d7a2f3..6026f5d8 100644 --- a/array_test.go +++ b/array_test.go @@ -7200,3 +7200,118 @@ func TestArrayRemoveReturnedValue(t *testing.T) { verifyEmptyArray(t, storage, typeInfo, address, parentArray) }) } + +func TestArrayWithOutdatedCallback(t *testing.T) { + typeInfo := testTypeInfo{42} + address := Address{1, 2, 3, 4, 5, 6, 7, 8} + + t.Run("overwritten child array", func(t *testing.T) { + + storage := newTestPersistentStorage(t) + + // Create parent array + parentArray, err := NewArray(storage, address, typeInfo) + require.NoError(t, err) + + var expectedValues arrayValue + + // Create child array + childArray, err := NewArray(storage, address, typeInfo) + require.NoError(t, err) + + // Insert child array to parent array + err = parentArray.Append(childArray) + require.NoError(t, err) + + v := NewStringValue(strings.Repeat("a", 10)) + + err = childArray.Append(v) + require.NoError(t, err) + + expectedValues = append(expectedValues, arrayValue{v}) + + verifyArray(t, storage, typeInfo, address, parentArray, expectedValues, true) + + // Overwrite child array value from parent + valueStorable, err := parentArray.Set(0, Uint64Value(0)) + require.NoError(t, err) + + id, ok := valueStorable.(SlabIDStorable) + require.True(t, ok) + + child, err := id.StoredValue(storage) + require.NoError(t, err) + + valueEqual(t, expectedValues[0], child) + + expectedValues[0] = Uint64Value(0) + + // childArray.parentUpdater isn't nil before callback is invoked. + require.NotNil(t, childArray.parentUpdater) + + // modify overwritten child array + err = childArray.Append(Uint64Value(0)) + require.NoError(t, err) + + // childArray.parentUpdater is nil after callback is invoked. + require.Nil(t, childArray.parentUpdater) + + // No-op on parent + valueEqual(t, expectedValues, parentArray) + }) + + t.Run("removed child array", func(t *testing.T) { + + storage := newTestPersistentStorage(t) + + // Create parent array + parentArray, err := NewArray(storage, address, typeInfo) + require.NoError(t, err) + + var expectedValues arrayValue + + // Create child array + childArray, err := NewArray(storage, address, typeInfo) + require.NoError(t, err) + + // Insert child array to parent array + err = parentArray.Append(childArray) + require.NoError(t, err) + + v := NewStringValue(strings.Repeat("a", 10)) + + err = childArray.Append(v) + require.NoError(t, err) + + expectedValues = append(expectedValues, arrayValue{v}) + + verifyArray(t, storage, typeInfo, address, parentArray, expectedValues, true) + + // Remove child array value from parent + valueStorable, err := parentArray.Remove(0) + require.NoError(t, err) + + id, ok := valueStorable.(SlabIDStorable) + require.True(t, ok) + + child, err := id.StoredValue(storage) + require.NoError(t, err) + + valueEqual(t, expectedValues[0], child) + + expectedValues = arrayValue{} + + // childArray.parentUpdater isn't nil before callback is invoked. + require.NotNil(t, childArray.parentUpdater) + + // modify removed child array + err = childArray.Append(Uint64Value(0)) + require.NoError(t, err) + + // childArray.parentUpdater is nil after callback is invoked. + require.Nil(t, childArray.parentUpdater) + + // No-op on parent + valueEqual(t, expectedValues, parentArray) + }) +} diff --git a/map.go b/map.go index 3a8d855e..d9ce5e54 100644 --- a/map.go +++ b/map.go @@ -4690,21 +4690,51 @@ func (m *OrderedMap) setCallbackWithChild( vid := c.ValueID() - c.setParentUpdater(func() error { + c.setParentUpdater(func() (found bool, err error) { // Avoid unnecessary write operation on parent container. // Child value was stored as SlabIDStorable (not inlined) in parent container, // and continues to be stored as SlabIDStorable (still not inlinable), // so no update to parent container is needed. if !c.Inlined() && !c.Inlinable(maxInlineSize) { - return nil + return true, nil + } + + // Retrieve element value under the same key and + // verify retrieved value is this child (c). + _, valueStorable, err := m.get(comparator, hip, key) + if err != nil { + var knf *KeyNotFoundError + if errors.As(err, &knf) { + return false, nil + } + // Don't need to wrap error as external error because err is already categorized by OrderedMap.Get(). + return false, err + } + + // Verify retrieved element value is either SlabIDStorable or Slab, with identical value ID. + switch x := valueStorable.(type) { + case SlabIDStorable: + sid := SlabID(x) + if !vid.equal(sid) { + return false, nil + } + + case Slab: + sid := x.SlabID() + if !vid.equal(sid) { + return false, nil + } + + default: + return false, nil } // Set child value with parent map using same key. // Set() calls c.Storable() which returns inlined or not-inlined child storable. existingValueStorable, err := m.set(comparator, hip, key, c) if err != nil { - return err + return false, err } // Verify overwritten storable has identical value ID. @@ -4713,7 +4743,7 @@ func (m *OrderedMap) setCallbackWithChild( case SlabIDStorable: sid := SlabID(x) if !vid.equal(sid) { - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten SlabIDStorable %s != value ID %s", sid, @@ -4723,7 +4753,7 @@ func (m *OrderedMap) setCallbackWithChild( case Slab: sid := x.SlabID() if !vid.equal(sid) { - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten Slab ID %s != value ID %s", sid, @@ -4731,17 +4761,18 @@ func (m *OrderedMap) setCallbackWithChild( } case nil: - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten value is nil")) default: - return NewFatalError( + return false, NewFatalError( fmt.Errorf( "failed to reset child value in parent updater callback: overwritten value is wrong type %T", existingValueStorable)) } - return nil + + return true, nil }) } @@ -4751,7 +4782,18 @@ func (m *OrderedMap) notifyParentIfNeeded() error { if m.parentUpdater == nil { return nil } - return m.parentUpdater() + + // If parentUpdater() doesn't find child map (m), then no-op on parent container + // and unset parentUpdater callback in child map. This can happen when child + // map is an outdated reference (removed or overwritten in parent container). + found, err := m.parentUpdater() + if err != nil { + return err + } + if !found { + m.parentUpdater = nil + } + return nil } func (m *OrderedMap) Has(comparator ValueComparator, hip HashInputProvider, key Value) (bool, error) { diff --git a/map_test.go b/map_test.go index 095f9f00..5be038b8 100644 --- a/map_test.go +++ b/map_test.go @@ -13373,3 +13373,125 @@ func TestMapRemoveReturnedValue(t *testing.T) { verifyMap(t, storage, typeInfo, address, parentMap, expectedKeyValues, nil, true) }) } + +func TestMapWithOutdatedCallback(t *testing.T) { + typeInfo := testTypeInfo{42} + address := Address{1, 2, 3, 4, 5, 6, 7, 8} + + t.Run("overwritten child array", func(t *testing.T) { + + storage := newTestPersistentStorage(t) + + // Create parent map + parentMap, err := NewMap(storage, address, newBasicDigesterBuilder(), typeInfo) + require.NoError(t, err) + + expectedKeyValues := make(mapValue) + + // Create child array + childArray, err := NewArray(storage, address, typeInfo) + require.NoError(t, err) + + k := Uint64Value(0) + + // Insert child array to parent map + existingStorable, err := parentMap.Set(compare, hashInputProvider, k, childArray) + require.NoError(t, err) + require.Nil(t, existingStorable) + + v := NewStringValue(strings.Repeat("a", 10)) + + err = childArray.Append(v) + require.NoError(t, err) + + expectedKeyValues[k] = arrayValue{v} + + verifyMap(t, storage, typeInfo, address, parentMap, expectedKeyValues, nil, true) + + // Overwrite child array value from parent + valueStorable, err := parentMap.Set(compare, hashInputProvider, k, Uint64Value(0)) + require.NoError(t, err) + + id, ok := valueStorable.(SlabIDStorable) + require.True(t, ok) + + child, err := id.StoredValue(storage) + require.NoError(t, err) + + valueEqual(t, expectedKeyValues[k], child) + + expectedKeyValues[k] = Uint64Value(0) + + // childArray.parentUpdater isn't nil before callback is invoked. + require.NotNil(t, childArray.parentUpdater) + + // modify overwritten child array + err = childArray.Append(Uint64Value(0)) + require.NoError(t, err) + + // childArray.parentUpdater is nil after callback is invoked. + require.Nil(t, childArray.parentUpdater) + + // No-op on parent + valueEqual(t, expectedKeyValues, parentMap) + }) + + t.Run("removed child array", func(t *testing.T) { + + storage := newTestPersistentStorage(t) + + // Create parent map + parentMap, err := NewMap(storage, address, newBasicDigesterBuilder(), typeInfo) + require.NoError(t, err) + + expectedKeyValues := make(mapValue) + + // Create child array + childArray, err := NewArray(storage, address, typeInfo) + require.NoError(t, err) + + k := Uint64Value(0) + + // Insert child array to parent map + existingStorable, err := parentMap.Set(compare, hashInputProvider, k, childArray) + require.NoError(t, err) + require.Nil(t, existingStorable) + + v := NewStringValue(strings.Repeat("a", 10)) + + err = childArray.Append(v) + require.NoError(t, err) + + expectedKeyValues[k] = arrayValue{v} + + verifyMap(t, storage, typeInfo, address, parentMap, expectedKeyValues, nil, true) + + // Remove child array value from parent + keyStorable, valueStorable, err := parentMap.Remove(compare, hashInputProvider, k) + require.NoError(t, err) + require.Equal(t, keyStorable, k) + + id, ok := valueStorable.(SlabIDStorable) + require.True(t, ok) + + child, err := id.StoredValue(storage) + require.NoError(t, err) + + valueEqual(t, expectedKeyValues[k], child) + + delete(expectedKeyValues, k) + + // childArray.parentUpdater isn't nil before callback is invoked. + require.NotNil(t, childArray.parentUpdater) + + // modify removed child array + err = childArray.Append(Uint64Value(0)) + require.NoError(t, err) + + // childArray.parentUpdater is nil after callback is invoked. + require.Nil(t, childArray.parentUpdater) + + // No-op on parent + valueEqual(t, expectedKeyValues, parentMap) + }) +} diff --git a/value.go b/value.go index ec590c0c..c8be86e3 100644 --- a/value.go +++ b/value.go @@ -26,7 +26,7 @@ type ValueComparator func(SlabStorage, Value, Storable) (bool, error) type StorableComparator func(Storable, Storable) bool -type parentUpdater func() error +type parentUpdater func() (found bool, err error) // mutableValueNotifier is an interface that allows mutable child value to notify and update parent. type mutableValueNotifier interface {