No-op on parentUpdater() if child isn't in parent

This commit checks if child is in parent container at adjusted index or under the same key in parentUpdater() before modifying parent container. If child is no longer part of parent, parentUpdater() returns with no-op, and this callback is set to nil. This is to handle outdated child reference modifying parent after it is removed.
onflow · Oct 2, 2023 · 63ea7a7 · 63ea7a7
1 parent 1a2e69f
commit 63ea7a7
Show file tree

Hide file tree

Showing 5 changed files with 337 additions and 23 deletions.
diff --git 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,26 +2853,26 @@ 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,
 						vid))
 			}
 
 		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
@@ -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
@@ -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,25 +4753,26 @@ 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,
 						vid))
 			}
 
 		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) {