[api] PostObjectConfigUpdate unexpected removal of DEFAULT section

Bug example: $ ox foo unset --kw fs#1.disable' => removal of DEFAULT section What happens: POST /.../foo/config/update?delete=&unset=fs%231.disable => deletes is []string{""} SectionsByName("") returns "DEFAULT" the DEFAULT section is removed - PostObjectConfigUpdate filters "" from params.Delete - keyop.ParseOps filters out any invalid operations (based on IsZero) - key.ParseStrings filters out any invalid keyword (based on IsZero) - key.Parse returns zero key.T on invalid string Now Post object update with empty changes returns 400: No valid update requested
opensvc · Oct 24, 2024 · 8e19482 · 8e19482
1 parent be40bf3
commit 8e19482
Show file tree

Hide file tree

Showing 5 changed files with 157 additions and 54 deletions.
diff --git a/core/keyop/keyop.go b/core/keyop/keyop.go
@@ -108,11 +108,17 @@ func New(key key.T, op Op, value string, idx int) *T {
 	}
 }
 
+// ParseOps function processes a list of strings, parses them into operations,
+// filters out any invalid operations (based on the IsZero check),
+// and returns a list of valid operations.
 func ParseOps(kws []string) L {
-	l := make(L, len(kws))
-	for i, kw := range kws {
+	l := make(L, 0, len(kws))
+	for _, kw := range kws {
 		op := Parse(kw)
-		l[i] = *op
+		if op.IsZero() {
+			continue
+		}
+		l = append(l, *op)
 	}
 	return l
 }

diff --git a/core/keyop/keyop_test.go b/core/keyop/keyop_test.go
@@ -8,7 +8,7 @@ import (
 	"github.com/opensvc/om3/util/key"
 )
 
-func TestKeyopsDrop(t *testing.T) {
+func TestDrop(t *testing.T) {
 	op1 := T{
 		Key:   key.Parse("topology"),
 		Op:    Set,
@@ -29,7 +29,7 @@ func TestKeyopsDrop(t *testing.T) {
 	assert.Len(t, ops, 1)
 }
 
-func TestKeyopParse(t *testing.T) {
+func TestParse(t *testing.T) {
 	tests := []struct {
 		expr  string
 		key   key.T
@@ -126,3 +126,45 @@ func TestKeyopParse(t *testing.T) {
 		})
 	}
 }
+
+func TestParseOps(t *testing.T) {
+	cases := []struct {
+		l        []string
+		expected L
+	}{
+		{
+			l: []string{"foo=bar"},
+			expected: L{
+				T{Key: key.T{Section: "DEFAULT", Option: "foo"}, Op: 1, Value: "bar", Index: 0},
+			},
+		},
+		{
+			l: []string{"foo1=bar1", "must_be_dropped", "foo2=bar2"},
+			expected: L{
+				T{Key: key.T{Section: "DEFAULT", Option: "foo1"}, Op: 1, Value: "bar1", Index: 0},
+				T{Key: key.T{Section: "DEFAULT", Option: "foo2"}, Op: 1, Value: "bar2", Index: 0},
+			},
+		},
+		{
+			l:        []string{"must_be_dropped"},
+			expected: L{},
+		},
+		{
+			l:        []string{""},
+			expected: L{},
+		},
+		{
+			l:        []string{},
+			expected: L{},
+		},
+		{
+			l:        nil,
+			expected: L{},
+		},
+	}
+	for _, tc := range cases {
+		t.Logf("ParseOps(%q)", tc.l)
+		ops := ParseOps(tc.l)
+		assert.Equal(t, tc.expected, ops)
+	}
+}
diff --git a/daemon/daemonapi/post_object_config_update.go b/daemon/daemonapi/post_object_config_update.go
@@ -36,7 +36,14 @@ func (a *DaemonAPI) PostObjectConfigUpdate(ctx echo.Context, namespace string, k
 		unsets = key.ParseStrings(*params.Unset)
 	}
 	if params.Delete != nil {
-		deletes = *params.Delete
+		for _, section := range *params.Delete {
+			if section == "" {
+				// Prevents from accidental remove DEFAULT section. SectionsByName("")
+				// return "DEFAULT". Use explicit section="DEFAULT" to remove DEFAULT section.
+				continue
+			}
+			deletes = append(deletes, section)
+		}
 	}
 	if len(sets)+len(unsets)+len(deletes) == 0 {
 		return JSONProblemf(ctx, http.StatusBadRequest, "No valid update requested", "")

diff --git a/util/key/key.go b/util/key/key.go
@@ -20,15 +20,27 @@ func New(section, option string) T {
 	}
 }
 
+// ParseStrings function processes a list of strings, parses them into keyword,
+// filters out any invalid keyword (based on the IsZero check),
+// and returns a list of valid keywords.
 func ParseStrings(l []string) L {
 	kws := make(L, 0)
 	for _, s := range l {
+		kw := Parse(s)
+		if kw.IsZero() {
+			continue
+		}
 		kws = append(kws, Parse(s))
 	}
 	return kws
 }
 
+// Parse function construct key T from the parsed string s.
+// On invalid string s the zero key is returned.
 func Parse(s string) T {
+	if s == "" || strings.ContainsAny(s, " \t") {
+		return T{}
+	}
 	l := strings.SplitN(s, ".", 2)
 	switch len(l) {
 	case 1:
@@ -67,3 +79,7 @@ func (t T) String() string {
 	}
 	return t.Section + "." + t.Option
 }
+
+func (t T) IsZero() bool {
+	return (t.Option + t.Section) == ""
+}
diff --git a/util/key/key_test.go b/util/key/key_test.go
@@ -7,71 +7,103 @@ import (
 )
 
 func TestKey(t *testing.T) {
-	tests := []struct {
-		s       string
-		section string
-		option  string
-		render  string
-		scope   string
+	cases := []struct {
+		s      string
+		t      T
+		render string
+		scope  string
+		isZero bool
 	}{
 		{
-			s:       "topology",
-			section: "DEFAULT",
-			option:  "topology",
-			render:  "topology",
-			scope:   "",
+			s:      "topology",
+			t:      T{"DEFAULT", "topology"},
+			render: "topology",
 		},
 		{
-			s:       "DEFAULT.topology",
-			section: "DEFAULT",
-			option:  "topology",
-			render:  "topology",
-			scope:   "",
+			s:      "DEFAULT.topology",
+			t:      T{"DEFAULT", "topology"},
+			render: "topology",
 		},
 		{
-			s:       "topology@nodes",
-			section: "DEFAULT",
-			option:  "topology@nodes",
-			render:  "topology@nodes",
-			scope:   "nodes",
+			s:      "topology@nodes",
+			t:      T{"DEFAULT", "topology@nodes"},
+			render: "topology@nodes",
+			scope:  "nodes",
 		},
 		{
-			s:       "DEFAULT.topology@nodes",
-			section: "DEFAULT",
-			option:  "topology@nodes",
-			render:  "topology@nodes",
-			scope:   "nodes",
+			s:      "DEFAULT.topology@nodes",
+			t:      T{"DEFAULT", "topology@nodes"},
+			render: "topology@nodes",
+			scope:  "nodes",
 		},
 		{
-			s:       "fs#1.dev",
-			section: "fs#1",
-			option:  "dev",
-			render:  "fs#1.dev",
-			scope:   "",
+			s:      "fs#1.dev",
+			t:      T{"fs#1", "dev"},
+			render: "fs#1.dev",
 		},
 		{
-			s:       "data.a.b/C.D",
-			section: "data",
-			option:  "a.b/C.D",
-			render:  "data.a.b/C.D",
-			scope:   "",
+			s:      "data.a.b/C.D",
+			t:      T{"data", "a.b/C.D"},
+			render: "data.a.b/C.D",
 		},
 		{
-			s:       "container#1",
-			section: "container#1",
-			option:  "",
-			render:  "container#1",
-			scope:   "",
+			s:      "container#1",
+			t:      T{"container#1", ""},
+			render: "container#1",
+		},
+		{
+			s:      ".foo",
+			t:      T{Option: "foo"},
+			render: ".foo",
+		},
+		// test cases where Parse must return zero T
+		{
+			s:      "",
+			isZero: true,
+		},
+		{
+			s:      ".",
+			isZero: true,
+		},
+		{
+			s:      " ",
+			isZero: true,
+		},
+		{
+			s:      "a ",
+			isZero: true,
+		},
+		{
+			s:      " b",
+			isZero: true,
+		},
+		{
+			s:      " .foo",
+			isZero: true,
+		},
+		{
+			s:      " foo.bar",
+			isZero: true,
+		},
+		{
+			s:      "foo.bar ",
+			isZero: true,
+		},
+		{
+			s:      "\tfoo.bar",
+			isZero: true,
 		},
 	}
-	for _, test := range tests {
-		t.Logf("%s", test.s)
-		k := Parse(test.s)
+	for _, tc := range cases {
+		t.Logf("verify after k := Parse(%q) if k == %#v, k.String() == %q, k.Scope() == %q and k.IsZero() == %v",
+			tc.s, tc.t, tc.render, tc.scope, tc.isZero)
+		k := Parse(tc.s)
 		render := k.String()
-		assert.Equal(t, test.render, render)
-		assert.Equal(t, test.section, k.Section)
-		assert.Equal(t, test.option, k.Option)
-		assert.Equal(t, test.scope, k.Scope())
+		assert.Equal(t, tc.render, render, "k.String()")
+		assert.Equal(t, tc.t.Section, k.Section, "k.Section")
+		assert.Equal(t, tc.t.Option, k.Option, "k.Option")
+		assert.Equal(t, tc.scope, k.Scope(), "k.Scope()")
+		assert.Equal(t, tc.isZero, k.IsZero(), "k.IsZero()")
 	}
 
 }