From 73149680d44a5376b76aee49248bdccad4746aa1 Mon Sep 17 00:00:00 2001 From: Adam Eijdenberg Date: Thu, 3 Aug 2017 16:54:20 +1000 Subject: [PATCH 1/4] Refactor replace loop in preparation for wildcards --- patch/replace_op.go | 99 +++++++++++++++++++++++++++++---------------- 1 file changed, 65 insertions(+), 34 deletions(-) diff --git a/patch/replace_op.go b/patch/replace_op.go index 48f68c3..452f7bc 100644 --- a/patch/replace_op.go +++ b/patch/replace_op.go @@ -11,6 +11,12 @@ type ReplaceOp struct { Value interface{} // will be cloned using yaml library } +type replaceCtx struct { + PrevUpdate func(interface{}) + I int + Obj interface{} +} + func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { // Ensure that value is not modified by future operations clonedValue, err := op.cloneValue(op.Value) @@ -24,19 +30,31 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { return clonedValue, nil } - obj := doc - prevUpdate := func(newObj interface{}) { doc = newObj } + ctxStack := []*replaceCtx{&replaceCtx{ + PrevUpdate: func(newObj interface{}) { doc = newObj }, + I: 0, + Obj: doc, + }} + for len(ctxStack) != 0 { + // Pop the next context off the stack + ctx := ctxStack[len(ctxStack)-1] + ctxStack = ctxStack[:len(ctxStack)-1] + + // Terminate if done + if ctx.I+1 >= len(tokens) { + continue + } - for i, token := range tokens[1:] { - isLast := i == len(tokens)-2 + token := tokens[ctx.I+1] + isLast := ctx.I == len(tokens)-2 switch typedToken := token.(type) { case IndexToken: idx := typedToken.Index - typedObj, ok := obj.([]interface{}) + typedObj, ok := ctx.Obj.([]interface{}) if !ok { - return nil, newOpArrayMismatchTypeErr(tokens[:i+2], obj) + return nil, newOpArrayMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) } if idx >= len(typedObj) { @@ -46,26 +64,29 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { if isLast { typedObj[idx] = clonedValue } else { - obj = typedObj[idx] - prevUpdate = func(newObj interface{}) { typedObj[idx] = newObj } + ctxStack = append(ctxStack, &replaceCtx{ + PrevUpdate: func(newObj interface{}) { typedObj[idx] = newObj }, + I: ctx.I + 1, + Obj: typedObj[idx], + }) } case AfterLastIndexToken: - typedObj, ok := obj.([]interface{}) + typedObj, ok := ctx.Obj.([]interface{}) if !ok { - return nil, newOpArrayMismatchTypeErr(tokens[:i+2], obj) + return nil, newOpArrayMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) } if isLast { - prevUpdate(append(typedObj, clonedValue)) + ctx.PrevUpdate(append(typedObj, clonedValue)) } else { return nil, fmt.Errorf("Expected after last index token to be last in path '%s'", op.Path) } case MatchingIndexToken: - typedObj, ok := obj.([]interface{}) + typedObj, ok := ctx.Obj.([]interface{}) if !ok { - return nil, newOpArrayMismatchTypeErr(tokens[:i+2], obj) + return nil, newOpArrayMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) } var idxs []int @@ -81,15 +102,19 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { if typedToken.Optional && len(idxs) == 0 { if isLast { - prevUpdate(append(typedObj, clonedValue)) + ctx.PrevUpdate(append(typedObj, clonedValue)) } else { - obj = map[interface{}]interface{}{typedToken.Key: typedToken.Value} - prevUpdate(append(typedObj, obj)) - // no need to change prevUpdate since matching item can only be a map + o := map[interface{}]interface{}{typedToken.Key: typedToken.Value} + ctx.PrevUpdate(append(typedObj, o)) + ctxStack = append(ctxStack, &replaceCtx{ + PrevUpdate: ctx.PrevUpdate, // no need to change prevUpdate since matching item can only be a map + I: ctx.I + 1, + Obj: o, + }) } } else { if len(idxs) != 1 { - return nil, opMultipleMatchingIndexErr{NewPointer(tokens[:i+2]), idxs} + return nil, opMultipleMatchingIndexErr{NewPointer(tokens[:ctx.I+2]), idxs} } idx := idxs[0] @@ -97,49 +122,55 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { if isLast { typedObj[idx] = clonedValue } else { - obj = typedObj[idx] // no need to change prevUpdate since matching item can only be a map + ctxStack = append(ctxStack, &replaceCtx{ + PrevUpdate: ctx.PrevUpdate, // no need to change prevUpdate since matching item can only be a map + I: ctx.I + 1, + Obj: typedObj[idx], + }) } } case KeyToken: - typedObj, ok := obj.(map[interface{}]interface{}) + typedObj, ok := ctx.Obj.(map[interface{}]interface{}) if !ok { - return nil, newOpMapMismatchTypeErr(tokens[:i+2], obj) + return nil, newOpMapMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) } - var found bool - - obj, found = typedObj[typedToken.Key] + o, found := typedObj[typedToken.Key] if !found && !typedToken.Optional { - return nil, opMissingMapKeyErr{typedToken.Key, NewPointer(tokens[:i+2]), typedObj} + return nil, opMissingMapKeyErr{typedToken.Key, NewPointer(tokens[:ctx.I+2]), typedObj} } if isLast { typedObj[typedToken.Key] = clonedValue } else { - prevUpdate = func(newObj interface{}) { typedObj[typedToken.Key] = newObj } - if !found { // Determine what type of value to create based on next token - switch tokens[i+2].(type) { + switch tokens[ctx.I+2].(type) { case AfterLastIndexToken: - obj = []interface{}{} + o = []interface{}{} case MatchingIndexToken: - obj = []interface{}{} + o = []interface{}{} case KeyToken: - obj = map[interface{}]interface{}{} + o = map[interface{}]interface{}{} default: errMsg := "Expected to find key, matching index or after last index token at path '%s'" - return nil, fmt.Errorf(errMsg, NewPointer(tokens[:i+3])) + return nil, fmt.Errorf(errMsg, NewPointer(tokens[:ctx.I+3])) } - typedObj[typedToken.Key] = obj + typedObj[typedToken.Key] = o } + + ctxStack = append(ctxStack, &replaceCtx{ + PrevUpdate: func(newObj interface{}) { typedObj[typedToken.Key] = newObj }, + I: ctx.I + 1, + Obj: o, + }) } default: - return nil, opUnexpectedTokenErr{token, NewPointer(tokens[:i+2])} + return nil, opUnexpectedTokenErr{token, NewPointer(tokens[:ctx.I+2])} } } From 3b51ded77d305b766bba8e038890ad361d2fb369 Mon Sep 17 00:00:00 2001 From: Adam Eijdenberg Date: Thu, 3 Aug 2017 17:01:18 +1000 Subject: [PATCH 2/4] Add Wildcard support --- patch/pointer.go | 9 +++++++++ patch/replace_op.go | 20 ++++++++++++++++++++ patch/replace_op_test.go | 29 +++++++++++++++++++++++++++++ patch/tokens.go | 2 ++ 4 files changed, 60 insertions(+) diff --git a/patch/pointer.go b/patch/pointer.go index a9888df..445b7eb 100644 --- a/patch/pointer.go +++ b/patch/pointer.go @@ -52,6 +52,12 @@ func NewPointerFromString(str string) (Pointer, error) { continue } + // parse wildcard + if tok == "*" { + tokens = append(tokens, WildcardToken{}) + continue + } + // parse as index idx, err := strconv.Atoi(tok) if err == nil { @@ -118,6 +124,9 @@ func (p Pointer) String() string { case IndexToken: strs = append(strs, fmt.Sprintf("%d", typedToken.Index)) + case WildcardToken: + strs = append(strs, "*") + case AfterLastIndexToken: strs = append(strs, "-") diff --git a/patch/replace_op.go b/patch/replace_op.go index 452f7bc..091b487 100644 --- a/patch/replace_op.go +++ b/patch/replace_op.go @@ -150,6 +150,8 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { switch tokens[ctx.I+2].(type) { case AfterLastIndexToken: o = []interface{}{} + case WildcardToken: + o = []interface{}{} case MatchingIndexToken: o = []interface{}{} case KeyToken: @@ -169,6 +171,24 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { }) } + case WildcardToken: + if isLast { + return nil, fmt.Errorf("Wildcard must not be the last token", NewPointer(tokens[:ctx.I+2])) + } + + typedObj, ok := ctx.Obj.([]interface{}) + if !ok { + return nil, newOpArrayMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) + } + + for idx, o := range typedObj { + ctxStack = append(ctxStack, &replaceCtx{ + PrevUpdate: func(newObj interface{}) { typedObj[idx] = newObj }, + I: ctx.I + 1, + Obj: o, + }) + } + default: return nil, opUnexpectedTokenErr{token, NewPointer(tokens[:ctx.I+2])} } diff --git a/patch/replace_op_test.go b/patch/replace_op_test.go index f50d1b6..d727511 100644 --- a/patch/replace_op_test.go +++ b/patch/replace_op_test.go @@ -8,6 +8,35 @@ import ( ) var _ = Describe("ReplaceOp.Apply", func() { + Describe("multiple replace", func() { + It("replaces many items", func() { + res, err := ReplaceOp{Path: MustNewPointerFromString("/instance_groups/*/vm_extensions?/-"), Value: "ex2"}.Apply(map[interface{}]interface{}{ + "instance_groups": []interface{}{ + map[interface{}]interface{}{ + "name": "foo", + "vm_extensions": []interface{}{"ex1"}, + }, + map[interface{}]interface{}{ + "name": "bar", + }, + }, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(res).To(Equal(map[interface{}]interface{}{ + "instance_groups": []interface{}{ + map[interface{}]interface{}{ + "name": "foo", + "vm_extensions": []interface{}{"ex1", "ex2"}, + }, + map[interface{}]interface{}{ + "name": "bar", + "vm_extensions": []interface{}{"ex2"}, + }, + }, + })) + }) + }) + It("returns error if replacement value cloning fails", func() { _, err := ReplaceOp{Path: MustNewPointerFromString(""), Value: func() {}}.Apply("a") Expect(err).To(HaveOccurred()) diff --git a/patch/tokens.go b/patch/tokens.go index 33db37d..8581cf3 100644 --- a/patch/tokens.go +++ b/patch/tokens.go @@ -8,6 +8,8 @@ type IndexToken struct { Index int } +type WildcardToken struct{} + type AfterLastIndexToken struct{} type MatchingIndexToken struct { From a8bf2110d46275efdc22a42f77a8aac3565701fa Mon Sep 17 00:00:00 2001 From: Adam Eijdenberg Date: Thu, 3 Aug 2017 17:24:15 +1000 Subject: [PATCH 3/4] Clone value at time of use to ensure that it is always cloned --- patch/replace_op.go | 35 +++++++++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/patch/replace_op.go b/patch/replace_op.go index 091b487..dbda459 100644 --- a/patch/replace_op.go +++ b/patch/replace_op.go @@ -17,16 +17,19 @@ type replaceCtx struct { Obj interface{} } -func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { - // Ensure that value is not modified by future operations - clonedValue, err := op.cloneValue(op.Value) - if err != nil { - return nil, fmt.Errorf("ReplaceOp cloning value: %s", err) - } +func replaceOpCloneValueErr(err error) error { + return fmt.Errorf("ReplaceOp cloning value: %s", err) +} +func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { tokens := op.Path.Tokens() if len(tokens) == 1 { + // Ensure that value is not modified by future operations + clonedValue, err := op.cloneValue(op.Value) + if err != nil { + return nil, replaceOpCloneValueErr(err) + } return clonedValue, nil } @@ -62,6 +65,10 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { } if isLast { + clonedValue, err := op.cloneValue(op.Value) + if err != nil { + return nil, replaceOpCloneValueErr(err) + } typedObj[idx] = clonedValue } else { ctxStack = append(ctxStack, &replaceCtx{ @@ -78,6 +85,10 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { } if isLast { + clonedValue, err := op.cloneValue(op.Value) + if err != nil { + return nil, replaceOpCloneValueErr(err) + } ctx.PrevUpdate(append(typedObj, clonedValue)) } else { return nil, fmt.Errorf("Expected after last index token to be last in path '%s'", op.Path) @@ -102,6 +113,10 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { if typedToken.Optional && len(idxs) == 0 { if isLast { + clonedValue, err := op.cloneValue(op.Value) + if err != nil { + return nil, replaceOpCloneValueErr(err) + } ctx.PrevUpdate(append(typedObj, clonedValue)) } else { o := map[interface{}]interface{}{typedToken.Key: typedToken.Value} @@ -120,6 +135,10 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { idx := idxs[0] if isLast { + clonedValue, err := op.cloneValue(op.Value) + if err != nil { + return nil, replaceOpCloneValueErr(err) + } typedObj[idx] = clonedValue } else { // no need to change prevUpdate since matching item can only be a map @@ -143,6 +162,10 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { } if isLast { + clonedValue, err := op.cloneValue(op.Value) + if err != nil { + return nil, replaceOpCloneValueErr(err) + } typedObj[typedToken.Key] = clonedValue } else { if !found { From d7e0ad0e868690c26abd99e976ab5544aa701283 Mon Sep 17 00:00:00 2001 From: Adam Eijdenberg Date: Fri, 4 Aug 2017 09:04:54 +1000 Subject: [PATCH 4/4] Use same technique for remove. --- patch/remove_op.go | 88 +++++++++++++++++++++++++++++------------ patch/remove_op_test.go | 34 ++++++++++++++++ patch/replace_op.go | 14 +++---- 3 files changed, 104 insertions(+), 32 deletions(-) diff --git a/patch/remove_op.go b/patch/remove_op.go index 9d175e3..fb03c5f 100644 --- a/patch/remove_op.go +++ b/patch/remove_op.go @@ -15,19 +15,31 @@ func (op RemoveOp) Apply(doc interface{}) (interface{}, error) { return nil, fmt.Errorf("Cannot remove entire document") } - obj := doc - prevUpdate := func(newObj interface{}) { doc = newObj } + ctxStack := []*mutationCtx{&mutationCtx{ + PrevUpdate: func(newObj interface{}) { doc = newObj }, + I: 0, + Obj: doc, + }} + for len(ctxStack) != 0 { + // Pop the next context off the stack + ctx := ctxStack[len(ctxStack)-1] + ctxStack = ctxStack[:len(ctxStack)-1] + + // Terminate if done + if ctx.I+1 >= len(tokens) { + continue + } - for i, token := range tokens[1:] { - isLast := i == len(tokens)-2 + token := tokens[ctx.I+1] + isLast := ctx.I == len(tokens)-2 switch typedToken := token.(type) { case IndexToken: idx := typedToken.Index - typedObj, ok := obj.([]interface{}) + typedObj, ok := ctx.Obj.([]interface{}) if !ok { - return nil, newOpArrayMismatchTypeErr(tokens[:i+2], obj) + return nil, newOpArrayMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) } if idx >= len(typedObj) { @@ -38,16 +50,19 @@ func (op RemoveOp) Apply(doc interface{}) (interface{}, error) { var newAry []interface{} newAry = append(newAry, typedObj[:idx]...) newAry = append(newAry, typedObj[idx+1:]...) - prevUpdate(newAry) + ctx.PrevUpdate(newAry) } else { - obj = typedObj[idx] - prevUpdate = func(newObj interface{}) { typedObj[idx] = newObj } + ctxStack = append(ctxStack, &mutationCtx{ + Obj: typedObj[idx], + PrevUpdate: func(newObj interface{}) { typedObj[idx] = newObj }, + I: ctx.I + 1, + }) } case MatchingIndexToken: - typedObj, ok := obj.([]interface{}) + typedObj, ok := ctx.Obj.([]interface{}) if !ok { - return nil, newOpArrayMismatchTypeErr(tokens[:i+2], obj) + return nil, newOpArrayMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) } var idxs []int @@ -62,11 +77,11 @@ func (op RemoveOp) Apply(doc interface{}) (interface{}, error) { } if typedToken.Optional && len(idxs) == 0 { - return doc, nil + continue // don't exit early } if len(idxs) != 1 { - return nil, opMultipleMatchingIndexErr{NewPointer(tokens[:i+2]), idxs} + return nil, opMultipleMatchingIndexErr{NewPointer(tokens[:ctx.I+2]), idxs} } idx := idxs[0] @@ -75,37 +90,60 @@ func (op RemoveOp) Apply(doc interface{}) (interface{}, error) { var newAry []interface{} newAry = append(newAry, typedObj[:idx]...) newAry = append(newAry, typedObj[idx+1:]...) - prevUpdate(newAry) + ctx.PrevUpdate(newAry) } else { - obj = typedObj[idx] - // no need to change prevUpdate since matching item can only be a map + ctxStack = append(ctxStack, &mutationCtx{ + Obj: typedObj[idx], + PrevUpdate: ctx.PrevUpdate, // no need to change prevUpdate since matching item can only be a map + I: ctx.I + 1, + }) } case KeyToken: - typedObj, ok := obj.(map[interface{}]interface{}) + typedObj, ok := ctx.Obj.(map[interface{}]interface{}) if !ok { - return nil, newOpMapMismatchTypeErr(tokens[:i+2], obj) + return nil, newOpMapMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) } - var found bool - - obj, found = typedObj[typedToken.Key] + o, found := typedObj[typedToken.Key] if !found { if typedToken.Optional { - return doc, nil + continue // don't return yet, as it may be present down alternate paths } - return nil, opMissingMapKeyErr{typedToken.Key, NewPointer(tokens[:i+2]), typedObj} + return nil, opMissingMapKeyErr{typedToken.Key, NewPointer(tokens[:ctx.I+2]), typedObj} } if isLast { delete(typedObj, typedToken.Key) } else { - prevUpdate = func(newObj interface{}) { typedObj[typedToken.Key] = newObj } + ctxStack = append(ctxStack, &mutationCtx{ + Obj: o, + PrevUpdate: func(newObj interface{}) { typedObj[typedToken.Key] = newObj }, + I: ctx.I + 1, + }) + } + + case WildcardToken: + if isLast { + return nil, fmt.Errorf("Wildcard must not be the last token", NewPointer(tokens[:ctx.I+2])) + } + + typedObj, ok := ctx.Obj.([]interface{}) + if !ok { + return nil, newOpArrayMismatchTypeErr(tokens[:ctx.I+2], ctx.Obj) + } + + for idx, o := range typedObj { + ctxStack = append(ctxStack, &mutationCtx{ + PrevUpdate: func(newObj interface{}) { typedObj[idx] = newObj }, + I: ctx.I + 1, + Obj: o, + }) } default: - return nil, opUnexpectedTokenErr{token, NewPointer(tokens[:i+2])} + return nil, opUnexpectedTokenErr{token, NewPointer(tokens[:ctx.I+2])} } } diff --git a/patch/remove_op_test.go b/patch/remove_op_test.go index 60f6b87..45e6c96 100644 --- a/patch/remove_op_test.go +++ b/patch/remove_op_test.go @@ -8,6 +8,40 @@ import ( ) var _ = Describe("RemoveOp.Apply", func() { + Describe("multiple remove", func() { + It("removes many items", func() { + res, err := RemoveOp{Path: MustNewPointerFromString("/instance_groups/*/vm_extensions?")}.Apply(map[interface{}]interface{}{ + "instance_groups": []interface{}{ + map[interface{}]interface{}{ + "name": "foo", + "vm_extensions": []interface{}{"ex1", "ex2"}, + }, + map[interface{}]interface{}{ + "name": "bar", + }, + map[interface{}]interface{}{ + "name": "baz", + "vm_extensions": []interface{}{"ex1", "ex2", "ex3"}, + }, + }, + }) + Expect(err).ToNot(HaveOccurred()) + Expect(res).To(Equal(map[interface{}]interface{}{ + "instance_groups": []interface{}{ + map[interface{}]interface{}{ + "name": "foo", + }, + map[interface{}]interface{}{ + "name": "bar", + }, + map[interface{}]interface{}{ + "name": "baz", + }, + }, + })) + }) + }) + It("returns an error if path is for the entire document", func() { _, err := RemoveOp{Path: MustNewPointerFromString("")}.Apply("a") Expect(err).To(HaveOccurred()) diff --git a/patch/replace_op.go b/patch/replace_op.go index dbda459..f50bdd5 100644 --- a/patch/replace_op.go +++ b/patch/replace_op.go @@ -11,7 +11,7 @@ type ReplaceOp struct { Value interface{} // will be cloned using yaml library } -type replaceCtx struct { +type mutationCtx struct { PrevUpdate func(interface{}) I int Obj interface{} @@ -33,7 +33,7 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { return clonedValue, nil } - ctxStack := []*replaceCtx{&replaceCtx{ + ctxStack := []*mutationCtx{&mutationCtx{ PrevUpdate: func(newObj interface{}) { doc = newObj }, I: 0, Obj: doc, @@ -71,7 +71,7 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { } typedObj[idx] = clonedValue } else { - ctxStack = append(ctxStack, &replaceCtx{ + ctxStack = append(ctxStack, &mutationCtx{ PrevUpdate: func(newObj interface{}) { typedObj[idx] = newObj }, I: ctx.I + 1, Obj: typedObj[idx], @@ -121,7 +121,7 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { } else { o := map[interface{}]interface{}{typedToken.Key: typedToken.Value} ctx.PrevUpdate(append(typedObj, o)) - ctxStack = append(ctxStack, &replaceCtx{ + ctxStack = append(ctxStack, &mutationCtx{ PrevUpdate: ctx.PrevUpdate, // no need to change prevUpdate since matching item can only be a map I: ctx.I + 1, Obj: o, @@ -142,7 +142,7 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { typedObj[idx] = clonedValue } else { // no need to change prevUpdate since matching item can only be a map - ctxStack = append(ctxStack, &replaceCtx{ + ctxStack = append(ctxStack, &mutationCtx{ PrevUpdate: ctx.PrevUpdate, // no need to change prevUpdate since matching item can only be a map I: ctx.I + 1, Obj: typedObj[idx], @@ -187,7 +187,7 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { typedObj[typedToken.Key] = o } - ctxStack = append(ctxStack, &replaceCtx{ + ctxStack = append(ctxStack, &mutationCtx{ PrevUpdate: func(newObj interface{}) { typedObj[typedToken.Key] = newObj }, I: ctx.I + 1, Obj: o, @@ -205,7 +205,7 @@ func (op ReplaceOp) Apply(doc interface{}) (interface{}, error) { } for idx, o := range typedObj { - ctxStack = append(ctxStack, &replaceCtx{ + ctxStack = append(ctxStack, &mutationCtx{ PrevUpdate: func(newObj interface{}) { typedObj[idx] = newObj }, I: ctx.I + 1, Obj: o,