Merge pull request #3066 from onflow/supun/migration-error-handling

migrations/migration.go

@@ -139,6 +139,31 @@ func (m *StorageMigration) MigrateNestedValue(
 	valueMigrations []ValueMigration,
 	reporter Reporter,
 ) (newValue interpreter.Value) {
+
+	defer func() {
+		//	Here it catches the panics that may occur at the framework level,
+		// even before going to each individual migration. e.g: iterating the dictionary for elements.
+		//
+		// There is a similar recovery at the `StorageMigration.migrate()` method,
+		// which handles panics from each individual migrations (e.g: capcon migration, static type migration, etc.).
+
+		if r := recover(); r != nil {
+			switch r := r.(type) {
+			case error:
+				if reporter != nil {
+					reporter.Error(
+						storageKey,
+						storageMapKey,
+						"StorageMigration",
+						r,
+					)
+				}
+			default:
+				panic(r)
+			}
+		}
+	}()
+
 	switch value := value.(type) {
 	case *interpreter.SomeValue:
 		innerValue := value.InnerValue(m.interpreter, emptyLocationRange)
@@ -378,6 +403,11 @@ func (m *StorageMigration) migrate(
 	value interpreter.Value,
 ) (converted interpreter.Value, err error) {
 
+	// Handles panics from each individual migrations (e.g: capcon migration, static type migration, etc.).
+	// So even if one migration panics, others could still run (i.e: panics are caught inside the loop).
+	// Removing that would cause all migrations to stop for a particular value, if one of them panics.
+	// NOTE: this won't catch panics occur at the migration framework level.
+	// They are caught at `StorageMigration.MigrateNestedValue()`.
 	defer func() {
 		if r := recover(); r != nil {
 			switch r := r.(type) {

migrations/migration_test.go

@@ -33,6 +33,7 @@ import (
 	"github.com/onflow/cadence/runtime/common"
 	"github.com/onflow/cadence/runtime/interpreter"
 	"github.com/onflow/cadence/runtime/stdlib"
+	"github.com/onflow/cadence/runtime/tests/checker"
 	. "github.com/onflow/cadence/runtime/tests/runtime_utils"
 	"github.com/onflow/cadence/runtime/tests/utils"
 )
@@ -947,3 +948,148 @@ func TestContractMigration(t *testing.T) {
 		value,
 	)
 }
+
+func TestMigrationPanics(t *testing.T) {
+
+	t.Parallel()
+
+	t.Run("composite dictionary", func(t *testing.T) {
+		t.Parallel()
+
+		ledger := NewTestLedger(nil, nil)
+		account := common.Address{0x42}
+
+		t.Run("prepare", func(t *testing.T) {
+
+			fooContractLocation := common.NewAddressLocation(nil, account, "Foo")
+
+			storage := runtime.NewStorage(ledger, nil)
+			locationRange := interpreter.EmptyLocationRange
+
+			contractChecker, err := checker.ParseAndCheckWithOptions(t, `
+                access(all) contract Foo {
+                    access(all) struct Bar {}
+                }`,
+				checker.ParseAndCheckOptions{
+					Location: fooContractLocation,
+				},
+			)
+			require.NoError(t, err)
+
+			inter, err := interpreter.NewInterpreter(
+				nil,
+				utils.TestLocation,
+				&interpreter.Config{
+					Storage:                       storage,
+					AtreeValueValidationEnabled:   false,
+					AtreeStorageValidationEnabled: false,
+					ImportLocationHandler: func(inter *interpreter.Interpreter, location common.Location) interpreter.Import {
+						program := interpreter.ProgramFromChecker(contractChecker)
+						subInterpreter, err := inter.NewSubInterpreter(program, location)
+						if err != nil {
+							panic(err)
+						}
+
+						return interpreter.InterpreterImport{
+							Interpreter: subInterpreter,
+						}
+					},
+				},
+			)
+			require.NoError(t, err)
+
+			const fooBarQualifiedIdentifier = "Foo.Bar"
+
+			dictionaryAnyStructStaticType :=
+				interpreter.NewDictionaryStaticType(
+					nil,
+					interpreter.PrimitiveStaticTypeString,
+					interpreter.NewCompositeStaticTypeComputeTypeID(
+						nil,
+						fooContractLocation,
+						fooBarQualifiedIdentifier,
+					),
+				)
+
+			storedValue := interpreter.NewDictionaryValue(
+				inter,
+				emptyLocationRange,
+				dictionaryAnyStructStaticType,
+			)
+
+			transferredValue := storedValue.Transfer(
+				inter,
+				locationRange,
+				atree.Address(account),
+				false,
+				nil,
+				nil,
+			)
+
+			inter.WriteStored(
+				account,
+				common.PathDomainStorage.Identifier(),
+				interpreter.StringStorageMapKey("dictionary_value"),
+				transferredValue,
+			)
+
+			err = storage.Commit(inter, true)
+			require.NoError(t, err)
+		})
+
+		t.Run("migrate", func(t *testing.T) {
+			storage := runtime.NewStorage(ledger, nil)
+
+			inter, err := interpreter.NewInterpreter(
+				nil,
+				utils.TestLocation,
+				&interpreter.Config{
+					Storage:                       storage,
+					AtreeValueValidationEnabled:   false,
+					AtreeStorageValidationEnabled: false,
+					ImportLocationHandler: func(_ *interpreter.Interpreter, _ common.Location) interpreter.Import {
+						// During the migration, treat the imported `Foo` contract as un-migrated.
+						panic(errors.New("type checking error in Foo"))
+					},
+				},
+			)
+			require.NoError(t, err)
+
+			migration := NewStorageMigration(inter, storage)
+
+			reporter := newTestReporter()
+
+			migration.Migrate(
+				&AddressSliceIterator{
+					Addresses: []common.Address{
+						account,
+					},
+				},
+				migration.NewValueMigrationsPathMigrator(
+					reporter,
+					testStringMigration{},
+					testInt8Migration{},
+					testCapMigration{},
+				),
+			)
+
+			err = migration.Commit()
+			require.NoError(t, err)
+
+			expectedErrors := map[struct {
+				interpreter.StorageKey
+				interpreter.StorageMapKey
+			}][]string{
+				{
+					StorageKey: interpreter.StorageKey{
+						Key:     common.PathDomainStorage.Identifier(),
+						Address: account,
+					},
+					StorageMapKey: interpreter.StringStorageMapKey("dictionary_value"),
+				}: {"StorageMigration"},
+			}
+
+			assert.Equal(t, expectedErrors, reporter.errored)
+		})
+	})
+}