recursive atomicity checking
diff --git a/merge/extract_apply_test.go b/merge/extract_apply_test.go index 8e0f4ca..50e19e1 100644 --- a/merge/extract_apply_test.go +++ b/merge/extract_apply_test.go
@@ -46,7 +46,7 @@ map: elementType: scalar: string - elementRelationship: associative + elementRelationship: separable - name: atomicMap type: map: @@ -428,7 +428,6 @@ ), }, }, - // BROKEN "extract_apply_empty_structure_remove_list": { Ops: []Operation{ ExtractApply{ @@ -456,29 +455,15 @@ APIVersion: "v1", }, }, - // BROKEN: expected: - //Object: ` - // list: - // - b - //`, - // but actually got: Object: ` list: - - a - b `, APIVersion: "v1", Managed: fieldpath.ManagedFields{ "apply-one": fieldpath.NewVersionedSet( - // BROKEN expected: - //_NS( - // _P("list"), - //), - // but actually got: _NS( _P("list"), - _P("list", _V("a")), - _P("list", _V("b")), ), "v1", false,
diff --git a/typed/helpers.go b/typed/helpers.go index 7ac603a..6b2b2cb 100644 --- a/typed/helpers.go +++ b/typed/helpers.go
@@ -221,18 +221,6 @@ return pe, nil } -func retainOnlyListKeys(keys []string, child value.Map) { - child.Iterate(func(k string, _ value.Value) bool { - for _, fieldName := range keys { - if k == fieldName { - return true - } - } - child.Delete(k) - return true - }) -} - func setItemToPathElement(list *schema.List, index int, child value.Value) (fieldpath.PathElement, error) { pe := fieldpath.PathElement{} switch { @@ -266,22 +254,3 @@ // Use the index as a key for atomic lists. return fieldpath.PathElement{Index: &index}, nil } - -// isAtomic lets you determine the atomicity of a value without -// recursing into another call of extractItemsWithSchema -func isAtomic(val value.Value, s *schema.Schema, tr schema.TypeRef) (bool, ValidationErrors) { - a, ok := s.Resolve(tr) - if !ok { - return false, errorf("schema error: no type found matching: %v", *tr.NamedType) - - } - a = deduceAtom(a, val) - switch { - case a.Map != nil: - return a.Map.ElementRelationship == schema.Atomic, nil - case a.List != nil: - return a.List.ElementRelationship == schema.Atomic, nil - default: - return true, nil - } -}
diff --git a/typed/remove.go b/typed/remove.go index c70618e..a338d76 100644 --- a/typed/remove.go +++ b/typed/remove.go
@@ -51,10 +51,22 @@ } func (w *removingWalker) doList(t *schema.List) (errs ValidationErrors) { + if !w.value.IsList() { + return nil + } l := w.value.AsListUsing(w.allocator) defer w.allocator.Free(l) - // If list is null, empty, or atomic just return - if l == nil || l.Length() == 0 || t.ElementRelationship == schema.Atomic { + // If list is null or empty just return + if l == nil || l.Length() == 0 { + return nil + } + + // atomic lists should return everything in the case of extract + // and nothing in the case of remove (!w.shouldExtract) + if t.ElementRelationship == schema.Atomic { + if w.shouldExtract { + w.out = w.value.Unstructured() + } return nil } @@ -70,14 +82,7 @@ // but ignore them when we are removing (i.e. !w.shouldExtract) if w.toRemove.Has(path) { if w.shouldExtract { - itemIsAtomic, err := isAtomic(item, w.schema, t.ElementType) - if err != nil { - return err - } - if !itemIsAtomic && item.IsMap() { - retainOnlyListKeys(t.Keys, item.AsMap()) - } - newItems = append(newItems, item.Unstructured()) + newItems = append(newItems, removeItemsWithSchema(item, w.toRemove, w.schema, t.ElementType, w.shouldExtract).Unstructured()) } else { continue } @@ -99,12 +104,24 @@ } func (w *removingWalker) doMap(t *schema.Map) ValidationErrors { + if !w.value.IsMap() { + return nil + } m := w.value.AsMapUsing(w.allocator) if m != nil { defer w.allocator.Free(m) } - // If map is null, empty, or atomic just return - if m == nil || m.Empty() || t.ElementRelationship == schema.Atomic { + // If map is null or empty just return + if m == nil || m.Empty() { + return nil + } + + // atomic maps should return everything in the case of extract + // and nothing in the case of remove (!w.shouldExtract) + if t.ElementRelationship == schema.Atomic { + if w.shouldExtract { + w.out = w.value.Unstructured() + } return nil } @@ -114,7 +131,6 @@ } newMap := map[string]interface{}{} - var errors ValidationErrors m.Iterate(func(k string, val value.Value) bool { pe := fieldpath.PathElement{FieldName: &k} path, _ := fieldpath.MakePath(pe) @@ -126,17 +142,8 @@ // but ignore them when we are removing (i.e. !w.shouldExtract) if w.toRemove.Has(path) { if w.shouldExtract { - valIsAtomic, err := isAtomic(val, w.schema, fieldType) - if err != nil { - errors = err - return false - } + newMap[k] = removeItemsWithSchema(val, w.toRemove, w.schema, fieldType, w.shouldExtract).Unstructured() - if !valIsAtomic && (val.IsMap() || val.IsList()) { - newMap[k] = nil - } else { - newMap[k] = val.Unstructured() - } } return true } @@ -151,9 +158,6 @@ newMap[k] = val.Unstructured() return true }) - if errors != nil { - return errors - } if len(newMap) > 0 { w.out = newMap }
diff --git a/typed/remove_test.go b/typed/remove_test.go index af46cb0..e31ee06 100644 --- a/typed/remove_test.go +++ b/typed/remove_test.go
@@ -288,7 +288,6 @@ quadruplets: []removeQuadruplet{{ `{"setBool":[false]}`, _NS(_P("setBool", _V(false))), - // is this the right remove output? `{"setBool":null}`, `{"setBool":[false]}`, }, { @@ -305,8 +304,6 @@ `{"setBool":[true,false]}`, _NS(_P("setBool")), ``, - // old - //`{"setBool":[true,false]}`, `{"setBool":null}`, }, { `{"setNumeric":[1,2,3,4.5]}`, @@ -324,11 +321,23 @@ rootTypeName: "myRoot", schema: typed.YAMLObject(associativeAndAtomicSchema), quadruplets: []removeQuadruplet{{ + // extract a struct from an associative list `{"list":[{"key":"a","id":1},{"key":"a","id":2},{"key":"b","id":1}]}`, - _NS(_P("list", _KBF("key", "a", "id", 1))), - `{"list":[{"key":"a","id":2},{"key":"b","id":1}]}`, + _NS( + _P("list", _KBF("key", "a", "id", 1), "key"), + _P("list", _KBF("key", "a", "id", 1), "id"), + ), + `unparseable`, `{"list":[{"key":"a","id":1}]}`, }, { + // remove structs from an associative list + `{"list":[{"key":"a","id":1},{"key":"a","id":2},{"key":"b","id":1}]}`, + _NS( + _P("list", _KBF("key", "a", "id", 1)), + ), + `{"list":[{"key":"a","id":2},{"key":"b","id":1}]}`, + `unparseable`, + }, { `{"atomicList":["a", "a", "a"]}`, _NS(_P("atomicList")), ``, @@ -352,7 +361,7 @@ _P("listOfLists", _KBF("name", "a"), "name"), _P("listOfLists", _KBF("name", "a"), "value", _V("b")), _P("listOfLists", _KBF("name", "a"), "value", _V("c")), - _P("listOfLists", _KBF("name", "d")), + _P("listOfLists", _KBF("name", "d"), "name"), ), `unparseable`, `{"listOfLists": [{"name": "a", "value": ["b", "c"]}, {"name": "d"}]}`, @@ -365,19 +374,34 @@ ``, `{"listOfLists": null}`, }, { - // path to a top-level element + // path to a top-level element (extract) + `{"listOfLists": [{"name": "a", "value": ["b", "c"]}, {"name": "d"}]}`, + _NS(_P("listOfLists", _KBF("name", "d"), "name")), + //`{"listOfLists": [{"name": "a", "value": ["b", "c"]}, null]}`, + `unparseable`, + `{"listOfLists": [{"name": "d"}]}`, + }, { + // path to a top-level element (remove) `{"listOfLists": [{"name": "a", "value": ["b", "c"]}, {"name": "d"}]}`, _NS(_P("listOfLists", _KBF("name", "d"))), `{"listOfLists": [{"name": "a", "value": ["b", "c"]}]}`, - `{"listOfLists": [{"name": "d"}]}`, + `unparseable`, }, { - // same as previous with the other top-level element containing nested elements. + // same as previous with the other top-level element containing nested elements. (extract) + `{"listOfLists": [{"name": "a", "value": ["b", "c"]}, {"name": "d"}]}`, + _NS( + _P("listOfLists", _KBF("name", "a"), "name"), + ), + `unparseable`, + `{"listOfLists": [{"name": "a"}]}`, + }, { + // same as previous with the other top-level element containing nested elements. (remove) `{"listOfLists": [{"name": "a", "value": ["b", "c"]}, {"name": "d"}]}`, _NS( _P("listOfLists", _KBF("name", "a")), ), `{"listOfLists": [{"name": "d"}]}`, - `{"listOfLists": [{"name": "a"}]}`, + `unparseable`, }, { // just one path to leaf element `{"listOfLists": [{"name": "a", "value": ["b", "c"]}, {"name": "d"}]}`, @@ -449,13 +473,21 @@ ``, `{"listOfMaps"}`, }, { - // path to a top-level element + // path to a top-level element (extract) + `{"listOfMaps": [{"name": "a", "value": {"b":"x", "c":"y"}}, {"name": "d", "value": {"e":"z"}}]}`, + _NS( + _P("listOfMaps", _KBF("name", "a"), "name"), + ), + `unparseable`, + `{"listOfMaps": [{"name": "a"}]}`, + }, { + // path to a top-level element (remove) `{"listOfMaps": [{"name": "a", "value": {"b":"x", "c":"y"}}, {"name": "d", "value": {"e":"z"}}]}`, _NS( _P("listOfMaps", _KBF("name", "a")), ), `{"listOfMaps": [{"name": "d", "value": {"e":"z"}}]}`, - `{"listOfMaps": [{"name": "a"}]}`, + `unparseable`, }, { // just one path to leaf element `{"listOfMaps": [{"name": "a", "value": {"b":"x", "c":"y"}}, {"name": "d", "value": {"e":"z"}}]}`, @@ -477,7 +509,7 @@ // path to non-existant top-level element `{"listOfMaps": [{"name": "a", "value": {"b":"x", "c":"y"}}, {"name": "d", "value": {"e":"z"}}]}`, _NS( - _P("listOfMaps", _KBF("name", "q")), + _P("listOfMaps", _KBF("name", "q"), "name"), ), `{"listOfMaps": [{"name": "a", "value": {"b":"x", "c":"y"}}, {"name": "d", "value": {"e":"z"}}]}`, // doesn't remove anything `{"listOfMaps":null}`, // extract only the root type @@ -508,7 +540,6 @@ _P("mapOfLists", "d", _V("f")), ), `unparseable`, - // old `{"mapOfLists": {"b":["a","c"], "d":["e", "f"]}}`, }, { // path to root type @@ -517,7 +548,7 @@ _P("mapOfLists"), ), ``, - `{"mapOfLists"}`, + `{"mapOfLists":null}`, }, { // path to a top-level element `{"mapOfLists": {"b":["a","c"], "d":["e", "f"]}}`, @@ -525,7 +556,7 @@ _P("mapOfLists", "b"), ), `{"mapOfLists": {"d":["e", "f"]}}`, - `{"mapOfLists": {"b"}}`, + `{"mapOfLists": {"b":null}}`, }, { // just one path to leaf element `{"mapOfLists": {"b":["a","c"], "d":["e", "f"]}}`, @@ -584,7 +615,7 @@ _P("mapOfMaps"), ), ``, - `{"mapOfMaps"}`, + `{"mapOfMaps":null}`, }, { // path to a top-level element `{"mapOfMaps": {"b":{"a":"x","c":"z"}, "d":{"e":"y", "f":"w"}}}`, @@ -592,7 +623,7 @@ _P("mapOfMaps", "b"), ), `{"mapOfMaps": {"d":{"e":"y", "f":"w"}}}`, - `{"mapOfMaps": {"b"}}`, + `{"mapOfMaps": {"b":null}}`, }, { // just one path to leaf element `{"mapOfMaps": {"b":{"a":"x","c":"z"}, "d":{"e":"y", "f":"w"}}}`, @@ -640,7 +671,7 @@ _P("mapOfMapsRecursive"), ), ``, - `{"mapOfMapsRecursive"}`, + `{"mapOfMapsRecursive":null}`, }, { // top-level map `{"mapOfMapsRecursive": {"a":{"b":{"c":null}}}}`, @@ -648,7 +679,7 @@ _P("mapOfMapsRecursive", "a"), ), `{"mapOfMapsRecursive"}`, - `{"mapOfMapsRecursive": {"a"}}`, + `{"mapOfMapsRecursive": {"a":null}}`, }, { // second-level map `{"mapOfMapsRecursive": {"a":{"b":{"c":null}}}}`, @@ -656,7 +687,7 @@ _P("mapOfMapsRecursive", "a", "b"), ), `{"mapOfMapsRecursive":{"a":null}}`, - `{"mapOfMapsRecursive": {"a":{"b"}}}`, + `{"mapOfMapsRecursive": {"a":{"b":null}}}`, }, { // third-level map `{"mapOfMapsRecursive": {"a":{"b":{"c":null}}}}`,