Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions merge/schema_change_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -204,24 +204,14 @@ func TestAtomicToGranularSchemaChanges(t *testing.T) {
},
},
ChangeParser{Parser: structParser},
// No conflict after changing struct to a granular schema
Apply{
Manager: "two",
Object: `
struct:
string: "b"
`,
APIVersion: "v1",
Conflicts: merge.Conflicts{
merge.Conflict{Manager: "one", Path: _P("struct", "string")},
},
},
ForceApply{
Manager: "two",
Object: `
struct:
string: "b"
`,
APIVersion: "v1",
},
},
Object: `
Expand All @@ -231,9 +221,21 @@ func TestAtomicToGranularSchemaChanges(t *testing.T) {
`,
APIVersion: "v1",
Managed: fieldpath.ManagedFields{
// Note that manager one previously owned
// the top level _P("struct")
// which included all of its subfields
// when the struct field was atomic.
//
// Upon changing the schema of struct from
// atomic to granular, manager one continues
// to own the same fieldset as before,
// but does not retain ownership of any of the subfields.
Copy link
Contributor

@jpbetz jpbetz Apr 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also make it clear that this is a known limitation and not the ideal behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added

//
// This is a known limitation due to the inability
// to accurately determine whether an empty field
// was previously atomic or not.
"one": fieldpath.NewVersionedSet(_NS(
_P("struct"),
_P("struct", "numeric"),
), "v1", true),
"two": fieldpath.NewVersionedSet(_NS(
_P("struct", "string"),
Expand Down
43 changes: 12 additions & 31 deletions typed/reconcile_schema.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,19 +187,17 @@ func (v *reconcileWithSchemaWalker) visitListItems(t *schema.List, element *fiel
}

func (v *reconcileWithSchemaWalker) doList(t *schema.List) (errs ValidationErrors) {
// reconcile lists changed from granular to atomic
// reconcile lists changed from granular to atomic.
// Note that migrations from atomic to granular are not recommended and will
// be treated as if they were always granular.
//
// In this case, the manager that owned the previously atomic field (and all subfields),
// will now own just the top-level field and none of the subfields.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect

if !v.isAtomic && t.ElementRelationship == schema.Atomic {
v.toRemove = fieldpath.NewSet(v.path) // remove all root and all children fields
v.toAdd = fieldpath.NewSet(v.path) // add the root of the atomic
return errs
}
// reconcile lists changed from atomic to granular
if v.isAtomic && t.ElementRelationship == schema.Associative {
v.toAdd, errs = buildGranularFieldSet(v.path, v.value)
if errs != nil {
return errs
}
}
if v.fieldSet != nil {
errs = v.visitListItems(t, v.fieldSet)
}
Expand Down Expand Up @@ -231,42 +229,25 @@ func (v *reconcileWithSchemaWalker) visitMapItems(t *schema.Map, element *fieldp
}

func (v *reconcileWithSchemaWalker) doMap(t *schema.Map) (errs ValidationErrors) {
// reconcile maps and structs changed from granular to atomic
// reconcile maps and structs changed from granular to atomic.
// Note that migrations from atomic to granular are not recommended and will
// be treated as if they were always granular.
//
// In this case the manager that owned the previously atomic field (and all subfields),
// will now own just the top-level field and none of the subfields.
if !v.isAtomic && t.ElementRelationship == schema.Atomic {
if v.fieldSet != nil && v.fieldSet.Size() > 0 {
v.toRemove = fieldpath.NewSet(v.path) // remove all root and all children fields
v.toAdd = fieldpath.NewSet(v.path) // add the root of the atomic
}
return errs
}
// reconcile maps changed from atomic to granular
if v.isAtomic && (t.ElementRelationship == schema.Separable || t.ElementRelationship == "") {
v.toAdd, errs = buildGranularFieldSet(v.path, v.value)
if errs != nil {
return errs
}
}
if v.fieldSet != nil {
errs = v.visitMapItems(t, v.fieldSet)
}
return errs
}

func buildGranularFieldSet(path fieldpath.Path, value *TypedValue) (*fieldpath.Set, ValidationErrors) {

valueFieldSet, err := value.ToFieldSet()
if err != nil {
return nil, errorf("toFieldSet: %v", err)
}
if valueFieldSetAtPath, ok := fieldSetAtPath(valueFieldSet, path); ok {
result := fieldpath.NewSet(path)
resultAtPath := descendToPath(result, path)
*resultAtPath = *valueFieldSetAtPath
return result, nil
}
return nil, nil
}

func fieldSetAtPath(node *fieldpath.Set, path fieldpath.Path) (*fieldpath.Set, bool) {
ok := true
for _, pe := range path {
Expand Down
33 changes: 0 additions & 33 deletions typed/reconcile_schema_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -207,39 +207,6 @@ var reconcileCases = []reconcileTestCase{{
_P("stringMap"),
_P("unchanged", "numeric"),
),
}, {
name: "atomic-to-granular",
rootTypeName: "v1",
oldSchema: atomicSchema("v1"),
newSchema: granularSchema("v1"),
liveObject: basicLiveObject,
oldFields: _NS(
_P("struct"),
_P("list"),
_P("objectList"),
_P("stringMap"),
_P("unchanged", "numeric"),
),
fixedFields: _NS(
_P("struct"),
_P("struct", "numeric"),
_P("struct", "string"),
_P("list"),
_P("list", _V("one")),
_P("list", _V("two")),
_P("objectList"),
_P("objectList", _KBF("keyA", "a1", "keyB", "b1")),
_P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "value"),
_P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "keyA"),
_P("objectList", _KBF("keyA", "a1", "keyB", "b1"), "keyB"),
_P("objectList", _KBF("keyA", "a2", "keyB", "b2")),
_P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "value"),
_P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "keyA"),
_P("objectList", _KBF("keyA", "a2", "keyB", "b2"), "keyB"),
_P("stringMap"),
_P("stringMap", "key1"),
_P("unchanged", "numeric"),
),
}, {
name: "no-change-granular",
rootTypeName: "v1",
Expand Down