diff --git a/compose/service/module.go b/compose/service/module.go index 65fef5607..b5bd00394 100644 --- a/compose/service/module.go +++ b/compose/service/module.go @@ -316,7 +316,7 @@ func (svc module) updater(ctx context.Context, namespaceID, moduleID uint64, act hasRecords = len(set) > 0 - if err = updateModuleFields(ctx, s, m, m.Fields, hasRecords); err != nil { + if err = updateModuleFields(ctx, s, m, old, hasRecords); err != nil { return err } } @@ -488,86 +488,93 @@ func (svc module) handleUndelete(ctx context.Context, ns *types.Namespace, m *ty // updates module fields // expecting to receive all module fields, as it deletes the rest // also, sort order of the fields is also important as this fn stores and updates field's place as send -func updateModuleFields(ctx context.Context, s store.Storer, m *types.Module, newFields types.ModuleFieldSet, hasRecords bool) (err error) { - for _, f := range newFields { - // Set module ID to all new fields +func updateModuleFields(ctx context.Context, s store.Storer, new, old *types.Module, hasRecords bool) (err error) { + // Go over new to assure field integrity + for _, f := range new.Fields { if f.ModuleID == 0 { - f.ModuleID = m.ID + f.ModuleID = new.ID } - // Make sure all updating fields belong here - if f.ModuleID != m.ID { + if f.ModuleID != new.ID { return fmt.Errorf("module id of field %q does not match the module", f.Name) } } - if err = loadModuleFields(ctx, s, m); err != nil { - return + // Delete any missing module fields + n := now() + ff := make(types.ModuleFieldSet, 0, len(old.Fields)) + for _, of := range old.Fields { + nf := new.Fields.FindByID(of.ID) + + if nf == nil { + of.DeletedAt = n + ff = append(ff, of) + } else if nf.DeletedAt != nil { + of.DeletedAt = n + ff = append(ff, of) + } } - for _, ef := range m.Fields { - f := newFields.FindByID(ef.ID) - if f == nil || f.DeletedAt == nil { - continue - } - - ef.DeletedAt = now() - err = store.UpdateComposeModuleField(ctx, s, ef) + if len(ff) > 0 { + err = store.DeleteComposeModuleField(ctx, s, ff...) if err != nil { return err } } - for idx, f := range newFields { - f.Place = idx - f.DeletedAt = nil + // Assure; create/update remaining fields + idx := 0 + ff = make(types.ModuleFieldSet, 0, len(old.Fields)) + for _, f := range new.Fields { + if f.DeletedAt != nil { + continue + } - if e := m.Fields.FindByID(f.ID); e != nil { - f.CreatedAt = e.CreatedAt + f.Place = idx + if of := old.Fields.FindByID(f.ID); of != nil { + f.CreatedAt = of.CreatedAt // We do not have any other code in place that would handle changes of field name and kind, so we need // to reset any changes made to the field. // @todo remove when we are able to handle field rename & type change if hasRecords { - f.Name = e.Name - f.Kind = e.Kind + f.Name = of.Name + f.Kind = of.Kind } f.UpdatedAt = now() err = store.UpdateComposeModuleField(ctx, s, f) - if err != nil { return err } - if label.Changed(f.Labels, e.Labels) { + if label.Changed(f.Labels, of.Labels) { if err = label.Update(ctx, s, f); err != nil { return } } - // override existing with new field - *e = *f - + ff = append(ff, f) } else { f.ID = nextID() f.CreatedAt = *now() - err = store.CreateComposeModuleField(ctx, s, f) - if err != nil { + if err = store.CreateComposeModuleField(ctx, s, f); err != nil { return err } - if err = label.Update(ctx, s, f); err != nil { return } - m.Fields = append(m.Fields, f) + ff = append(ff, f) } + + idx++ } - sort.Sort(m.Fields) + sort.Sort(ff) + new.Fields = ff return nil } diff --git a/tests/compose/module_test.go b/tests/compose/module_test.go index d3e0018d2..3505979a6 100644 --- a/tests/compose/module_test.go +++ b/tests/compose/module_test.go @@ -265,6 +265,63 @@ func TestModuleFieldsUpdate(t *testing.T) { h.a.Equal(m.Fields[1].Kind, "DateTime") } +func TestModuleFieldsUpdate_removed(t *testing.T) { + h := newHelper(t) + h.clearModules() + + h.allow(types.NamespaceRBACResource.AppendWildcard(), "read") + ns := h.makeNamespace("some-namespace") + m := h.makeModule(ns, "some-module", &types.ModuleField{ID: id.Next(), Kind: "String", Name: "a"}, &types.ModuleField{ID: id.Next(), Kind: "String", Name: "b"}) + h.allow(types.ModuleRBACResource.AppendWildcard(), "update") + + f := m.Fields[0] + fjs := fmt.Sprintf(`{ "name": "%s", "fields": [{ "fieldID": "%d", "name": "a", "kind": "String" }] }`, m.Name, f.ID) + h.apiInit(). + Post(fmt.Sprintf("/namespace/%d/module/%d", ns.ID, m.ID)). + JSON(fjs). + Expect(t). + Status(http.StatusOK). + Assert(helpers.AssertNoErrors). + End() + + m = h.lookupModuleByID(m.ID) + h.a.NotNil(m) + h.a.NotNil(m.Fields) + h.a.Len(m.Fields, 1) + + h.a.NotNil(m.Fields[0].UpdatedAt) + h.a.Equal(m.Fields[0].Name, "a") +} + +func TestModuleFieldsUpdate_removedHasRecords(t *testing.T) { + h := newHelper(t) + h.clearModules() + + h.allow(types.NamespaceRBACResource.AppendWildcard(), "read") + ns := h.makeNamespace("some-namespace") + m := h.makeModule(ns, "some-module", &types.ModuleField{ID: id.Next(), Kind: "String", Name: "a"}, &types.ModuleField{ID: id.Next(), Kind: "String", Name: "b"}) + h.makeRecord(m, &types.RecordValue{Name: "a", Value: "va"}, &types.RecordValue{Name: "b", Value: "vb"}) + h.allow(types.ModuleRBACResource.AppendWildcard(), "update") + + f := m.Fields[0] + fjs := fmt.Sprintf(`{ "name": "%s", "fields": [{ "fieldID": "%d", "name": "a", "kind": "String" }] }`, m.Name, f.ID) + h.apiInit(). + Post(fmt.Sprintf("/namespace/%d/module/%d", ns.ID, m.ID)). + JSON(fjs). + Expect(t). + Status(http.StatusOK). + Assert(helpers.AssertNoErrors). + End() + + m = h.lookupModuleByID(m.ID) + h.a.NotNil(m) + h.a.NotNil(m.Fields) + h.a.Len(m.Fields, 1) + + h.a.NotNil(m.Fields[0].UpdatedAt) + h.a.Equal(m.Fields[0].Name, "a") +} + func TestModuleFieldsUpdateExpressions(t *testing.T) { h := newHelper(t) h.clearModules()