fix: scenes and properties are not updated properly when plugin is up…

…dated
reearth · Feb 18, 2022 · 861c4bb · 861c4bb
1 parent 5009c5e
commit 861c4bb
Show file tree

Hide file tree

Showing 12 changed files with 105 additions and 84 deletions.
diff --git a/internal/usecase/interactor/plugin_upload.go b/internal/usecase/interactor/plugin_upload.go
@@ -90,14 +90,20 @@ func (i *Plugin) upload(ctx context.Context, p *pluginpack.Package, sid id.Scene
 		}
 	}()
 
+	var oldPManifest *manifest.Manifest
 	newpid := p.Manifest.Plugin.ID()
 	oldpid := s.Plugins().PluginByName(newpid.Name()).PluginRef()
-	var oldp *plugin.Plugin
 	if oldpid != nil {
-		oldp, err = i.pluginRepo.FindByID(ctx, *oldpid, []id.SceneID{sid})
+		oldPlugin, err := i.pluginRepo.FindByID(ctx, *oldpid, []id.SceneID{sid})
 		if err != nil {
 			return nil, nil, err
 		}
+
+		oldPManifest2, err := i.pluginManifestFromPlugin(ctx, oldPlugin)
+		if err != nil {
+			return nil, nil, err
+		}
+		oldPManifest = &oldPManifest2
 	}
 
 	// new (oldpid == nil): upload files, save plugin and properties -> install
@@ -136,19 +142,19 @@ func (i *Plugin) upload(ctx context.Context, p *pluginpack.Package, sid id.Scene
 		return nil, nil, err
 	}
 
-	if oldpid == nil {
+	if oldPManifest == nil {
 		// new: install plugin
 		if err := i.installScenePlugin(ctx, p, s); err != nil {
 			return nil, nil, err
 		}
 	} else {
 		// same, diff: migrate
-		if err := i.migrateScenePlugin(ctx, p, s, oldp); err != nil {
+		if err := i.migrateScenePlugin(ctx, *oldPManifest, p, s); err != nil {
 			return nil, nil, err
 		}
 	}
 
-	if oldpid != nil && !oldpid.Equal(newpid) {
+	if oldpid != nil && oldPManifest != nil && !oldpid.Equal(newpid) {
 		// diff only: delete old files
 		if err := i.file.RemovePlugin(ctx, *oldpid); err != nil {
 			return nil, nil, err
@@ -159,7 +165,7 @@ func (i *Plugin) upload(ctx context.Context, p *pluginpack.Package, sid id.Scene
 			if err := i.pluginRepo.Remove(ctx, *oldpid); err != nil {
 				return nil, nil, err
 			}
-			if ps := oldp.PropertySchemas(); len(ps) > 0 {
+			if ps := oldPManifest.Plugin.PropertySchemas(); len(ps) > 0 {
 				if err := i.propertySchemaRepo.RemoveAll(ctx, ps); err != nil {
 					return nil, nil, err
 				}
@@ -195,17 +201,12 @@ func (i *Plugin) installScenePlugin(ctx context.Context, p *pluginpack.Package,
 	return nil
 }
 
-func (i *Plugin) migrateScenePlugin(ctx context.Context, p *pluginpack.Package, s *scene.Scene, oldp *plugin.Plugin) (err error) {
-	if oldp == nil || p.Manifest == nil {
+func (i *Plugin) migrateScenePlugin(ctx context.Context, oldm manifest.Manifest, p *pluginpack.Package, s *scene.Scene) (err error) {
+	if oldm.Plugin == nil || p.Manifest == nil {
 		return nil
 	}
 
-	oldPManifest, err := i.pluginManifestFromPlugin(ctx, oldp)
-	if err != nil {
-		return err
-	}
-
-	diff := manifest.DiffFrom(oldPManifest, *p.Manifest)
+	diff := manifest.DiffFrom(oldm, *p.Manifest)
 	updatedProperties := property.List{}
 
 	// update scene
@@ -220,18 +221,14 @@ func (i *Plugin) migrateScenePlugin(ctx context.Context, p *pluginpack.Package,
 		updatedProperties = append(updatedProperties, p)
 	}
 
-	sp := s.Plugins().Plugin(diff.From)
-	if sp != nil && sp.Property() != nil && diff.PropertySchemaDeleted {
+	if sp := s.Plugins().Plugin(diff.From); sp != nil && sp.Property() != nil && diff.PropertySchemaDeleted {
 		// plugin property should be removed
 		if err := i.propertyRepo.Remove(ctx, *sp.Property()); err != nil {
 			return err
 		}
 	}
 
 	s.Plugins().Upgrade(diff.From, diff.To, spp, diff.PropertySchemaDeleted)
-	if err := i.sceneRepo.Save(ctx, s); err != nil {
-		return err
-	}
 
 	// delete layers, blocks and widgets
 	for _, e := range diff.DeletedExtensions {
@@ -255,6 +252,10 @@ func (i *Plugin) migrateScenePlugin(ctx context.Context, p *pluginpack.Package,
 		}
 	}
 
+	if err := i.sceneRepo.Save(ctx, s); err != nil {
+		return err
+	}
+
 	// migrate layers
 	if err := i.layerRepo.UpdatePlugin(ctx, diff.From, diff.To, []id.SceneID{s.ID()}); err != nil {
 		return err

diff --git a/internal/usecase/interactor/plugin_upload_test.go b/internal/usecase/interactor/plugin_upload_test.go
@@ -137,6 +137,8 @@ func TestPlugin_Upload_SameVersion(t *testing.T) {
 	sid := id.NewSceneID()
 	pid := mockPluginID.WithScene(sid.Ref())
 	eid := id.PluginExtensionID("marker")
+	eid2 := id.PluginExtensionID("widget")
+	wid := id.NewWidgetID()
 
 	repos := memory.InitRepos(nil)
 	mfs := mockFS(map[string]string{
@@ -146,16 +148,21 @@ func TestPlugin_Upload_SameVersion(t *testing.T) {
 	assert.NoError(t, err)
 
 	ps := property.NewSchema().ID(property.NewSchemaID(pid, eid.String())).MustBuild()
+	ps2 := property.NewSchema().ID(property.NewSchemaID(pid, eid2.String())).MustBuild()
 	pl := plugin.New().ID(pid).Extensions([]*plugin.Extension{
 		plugin.NewExtension().ID(eid).Type(plugin.ExtensionTypePrimitive).Schema(ps.ID()).MustBuild(),
+		plugin.NewExtension().ID(eid2).Type(plugin.ExtensionTypeWidget).Schema(ps2.ID()).MustBuild(),
 	}).MustBuild()
 
 	p := property.New().NewID().Schema(ps.ID()).Scene(sid).MustBuild()
+	p2 := property.New().NewID().Schema(ps2.ID()).Scene(sid).MustBuild()
 	pluginLayer := layer.NewItem().NewID().Scene(sid).Plugin(pid.Ref()).Extension(eid.Ref()).Property(p.IDRef()).MustBuild()
 	rootLayer := layer.NewGroup().NewID().Scene(sid).Layers(layer.NewIDList([]layer.ID{pluginLayer.ID()})).Root(true).MustBuild()
 	scene := scene.New().ID(sid).Team(team).RootLayer(rootLayer.ID()).Plugins(scene.NewPlugins([]*scene.Plugin{
 		scene.NewPlugin(pid, nil),
-	})).MustBuild()
+	})).Widgets(scene.NewWidgets([]*scene.Widget{
+		scene.MustNewWidget(wid, pid, eid2, p2.ID(), false, false),
+	}, nil)).MustBuild()
 
 	_ = repos.PropertySchema.Save(ctx, ps)
 	_ = repos.Plugin.Save(ctx, pl)
@@ -188,6 +195,11 @@ func TestPlugin_Upload_SameVersion(t *testing.T) {
 	nscene, err := repos.Scene.FindByID(ctx, scene.ID(), nil)
 	assert.NoError(t, err)
 	assert.True(t, nscene.Plugins().HasPlugin(pl.ID()))
+	assert.Nil(t, nscene.Widgets().Widget(wid))
+
+	nlp2, err := repos.Property.FindByID(ctx, p.ID(), nil)
+	assert.Nil(t, nlp2) // deleted
+	assert.Equal(t, rerror.ErrNotFound, err)
 
 	// plugin
 	npl, err := repos.Plugin.FindByID(ctx, pid, []id.SceneID{scene.ID()})
@@ -198,6 +210,10 @@ func TestPlugin_Upload_SameVersion(t *testing.T) {
 	assert.Nil(t, nlps) // deleted
 	assert.Equal(t, rerror.ErrNotFound, err)
 
+	nlps2, err := repos.PropertySchema.FindByID(ctx, ps2.ID())
+	assert.Nil(t, nlps2) // deleted
+	assert.Equal(t, rerror.ErrNotFound, err)
+
 	_, err = mfs.Open("plugins/" + pid.String() + "/hogehoge")
 	assert.True(t, os.IsNotExist(err)) // deleted
 

diff --git a/pkg/scene/widgets_test.go b/pkg/scene/widgets_test.go
@@ -155,30 +155,31 @@ func TestWidgets_RemoveAllByPlugin(t *testing.T) {
 	w3 := MustNewWidget(NewWidgetID(), pid2, "e1", NewPropertyID(), true, false)
 
 	tests := []struct {
-		Name           string
-		PID            PluginID
-		EID            *PluginExtensionID
-		WS, Expected   *Widgets
-		ExpectedResult []PropertyID
+		Name             string
+		ArgsPID          PluginID
+		ArgsEID          *PluginExtensionID
+		Target, Expected *Widgets
+		ExpectedResult   []PropertyID
 	}{
 		{
 			Name:           "remove widgets",
-			PID:            pid,
-			WS:             NewWidgets([]*Widget{w1, w2, w3}, nil),
+			ArgsPID:        pid,
+			ArgsEID:        nil,
+			Target:         NewWidgets([]*Widget{w1, w2, w3}, nil),
 			Expected:       NewWidgets([]*Widget{w3}, nil),
 			ExpectedResult: []PropertyID{w1.Property(), w2.Property()},
 		},
 		{
-			Name:           "remove widgets",
-			PID:            pid,
-			EID:            PluginExtensionID("e2").Ref(),
-			WS:             NewWidgets([]*Widget{w1, w2, w3}, nil),
+			Name:           "remove widgets of extension",
+			ArgsPID:        pid,
+			ArgsEID:        PluginExtensionID("e2").Ref(),
+			Target:         NewWidgets([]*Widget{w1, w2, w3}, nil),
 			Expected:       NewWidgets([]*Widget{w1, w3}, nil),
 			ExpectedResult: []PropertyID{w2.Property()},
 		},
 		{
 			Name:           "remove from nil widgets",
-			WS:             nil,
+			Target:         nil,
 			Expected:       nil,
 			ExpectedResult: nil,
 		},
@@ -188,8 +189,8 @@ func TestWidgets_RemoveAllByPlugin(t *testing.T) {
 		tc := tc
 		t.Run(tc.Name, func(t *testing.T) {
 			t.Parallel()
-			assert.Equal(t, tc.ExpectedResult, tc.WS.RemoveAllByPlugin(tc.PID, tc.EID))
-			assert.Equal(t, tc.Expected, tc.WS)
+			assert.Equal(t, tc.ExpectedResult, tc.Target.RemoveAllByPlugin(tc.ArgsPID, tc.ArgsEID))
+			assert.Equal(t, tc.Expected, tc.Target)
 		})
 	}
 }

diff --git a/pkg/value/bool.go b/pkg/value/bool.go
@@ -6,7 +6,7 @@ var TypeBool Type = "bool"
 
 type propertyBool struct{}
 
-func (*propertyBool) I2V(i interface{}) (interface{}, bool) {
+func (p *propertyBool) I2V(i interface{}) (interface{}, bool) {
 	switch v := i.(type) {
 	case bool:
 		return v, true
@@ -16,13 +16,11 @@ func (*propertyBool) I2V(i interface{}) (interface{}, bool) {
 		}
 	case *bool:
 		if v != nil {
-			return *v, true
+			return p.I2V(*v)
 		}
 	case *string:
 		if v != nil {
-			if b, err := strconv.ParseBool(*v); err == nil {
-				return b, true
-			}
+			return p.I2V(*v)
 		}
 	}
 	return nil, false

diff --git a/pkg/value/coordinates.go b/pkg/value/coordinates.go
@@ -32,12 +32,12 @@ var TypeCoordinates Type = "coordinates"
 
 type propertyCoordinates struct{}
 
-func (*propertyCoordinates) I2V(i interface{}) (interface{}, bool) {
+func (p *propertyCoordinates) I2V(i interface{}) (interface{}, bool) {
 	if v, ok := i.(Coordinates); ok {
 		return v, true
 	} else if v, ok := i.(*Coordinates); ok {
 		if v != nil {
-			return *v, true
+			return p.I2V(*v)
 		}
 		return nil, false
 	} else if v2, ok := i.([]float64); ok {

diff --git a/pkg/value/latlng.go b/pkg/value/latlng.go
@@ -21,19 +21,19 @@ var TypeLatLng Type = "latlng"
 
 type propertyLatLng struct{}
 
-func (*propertyLatLng) I2V(i interface{}) (interface{}, bool) {
+func (p *propertyLatLng) I2V(i interface{}) (interface{}, bool) {
 	switch v := i.(type) {
 	case LatLng:
 		return v, true
 	case LatLngHeight:
 		return LatLng{Lat: v.Lat, Lng: v.Lng}, true
 	case *LatLng:
 		if v != nil {
-			return *v, true
+			return p.I2V(*v)
 		}
 	case *LatLngHeight:
 		if v != nil {
-			return LatLng{Lat: v.Lat, Lng: v.Lng}, true
+			return p.I2V(*v)
 		}
 	}
 

diff --git a/pkg/value/latlngheight.go b/pkg/value/latlngheight.go
@@ -23,19 +23,19 @@ var TypeLatLngHeight Type = "latlngheight"
 
 type propertyLatLngHeight struct{}
 
-func (*propertyLatLngHeight) I2V(i interface{}) (interface{}, bool) {
+func (p *propertyLatLngHeight) I2V(i interface{}) (interface{}, bool) {
 	switch v := i.(type) {
 	case LatLngHeight:
 		return v, true
 	case LatLng:
 		return LatLngHeight{Lat: v.Lat, Lng: v.Lng, Height: 0}, true
 	case *LatLngHeight:
 		if v != nil {
-			return *v, true
+			return p.I2V(*v)
 		}
 	case *LatLng:
 		if v != nil {
-			return LatLngHeight{Lat: v.Lat, Lng: v.Lng, Height: 0}, true
+			return p.I2V(*v)
 		}
 	}