From fa0a712bac285cb7ef9d812fd36c41b0de79b32e Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Fri, 16 Jun 2023 14:17:45 +0200 Subject: [PATCH 1/2] Flatten empty custom vars of type `array` & `map` correctly --- pkg/flatten/flatten.go | 23 ++++++++++++++++++++--- pkg/icingadb/v1/customvar.go | 15 ++++++--------- schema/mysql/schema.sql | 2 +- schema/mysql/upgrades/1.2.0.sql | 1 + schema/pgsql/schema.sql | 2 +- schema/pgsql/upgrades/1.2.0.sql | 1 + 6 files changed, 30 insertions(+), 14 deletions(-) create mode 100644 schema/mysql/upgrades/1.2.0.sql create mode 100644 schema/pgsql/upgrades/1.2.0.sql diff --git a/pkg/flatten/flatten.go b/pkg/flatten/flatten.go index a31705be6..94a6e7ebc 100644 --- a/pkg/flatten/flatten.go +++ b/pkg/flatten/flatten.go @@ -1,26 +1,43 @@ package flatten import ( + "database/sql" + "fmt" + "github.com/icinga/icingadb/pkg/types" "strconv" ) // Flatten creates flat, one-dimensional maps from arbitrarily nested values, e.g. JSON. -func Flatten(value interface{}, prefix string) map[string]interface{} { +func Flatten(value interface{}, prefix string) map[string]types.String { var flatten func(string, interface{}) - flattened := make(map[string]interface{}) + flattened := make(map[string]types.String) flatten = func(key string, value interface{}) { switch value := value.(type) { case map[string]interface{}: + if len(value) == 0 { + flattened[key] = types.String{} + break + } + for k, v := range value { flatten(key+"."+k, v) } case []interface{}: + if len(value) == 0 { + flattened[key] = types.String{} + break + } + for i, v := range value { flatten(key+"["+strconv.Itoa(i)+"]", v) } default: - flattened[key] = value + val := "null" + if value != nil { + val = fmt.Sprintf("%v", value) + } + flattened[key] = types.String{NullString: sql.NullString{String: val, Valid: true}} } } diff --git a/pkg/icingadb/v1/customvar.go b/pkg/icingadb/v1/customvar.go index 0e85cc07e..462b87c4e 100644 --- a/pkg/icingadb/v1/customvar.go +++ b/pkg/icingadb/v1/customvar.go @@ -2,7 +2,6 @@ package v1 import ( "context" - "fmt" "github.com/icinga/icingadb/internal" "github.com/icinga/icingadb/pkg/com" "github.com/icinga/icingadb/pkg/contracts" @@ -25,7 +24,7 @@ type CustomvarFlat struct { CustomvarMeta `json:",inline"` Flatname string `json:"flatname"` FlatnameChecksum types.Binary `json:"flatname_checksum"` - Flatvalue string `json:"flatvalue"` + Flatvalue types.String `json:"flatvalue"` } func NewCustomvar() contracts.Entity { @@ -117,11 +116,9 @@ func flattenCustomvars(ctx context.Context, g *errgroup.Group, cvs <-chan contra flattened := flatten.Flatten(value, customvar.Name) for flatname, flatvalue := range flattened { - var fv string - if flatvalue == nil { - fv = "null" - } else { - fv = fmt.Sprintf("%v", flatvalue) + var fv interface{} + if flatvalue.Valid { + fv = flatvalue.String } select { @@ -131,7 +128,7 @@ func flattenCustomvars(ctx context.Context, g *errgroup.Group, cvs <-chan contra IdMeta: IdMeta{ // TODO(el): Schema comment is wrong. // Without customvar.Id we would produce duplicate keys here. - Id: utils.Checksum(objectpacker.MustPackSlice(customvar.EnvironmentId, customvar.Id, flatname, flatvalue)), + Id: utils.Checksum(objectpacker.MustPackSlice(customvar.EnvironmentId, customvar.Id, flatname, fv)), }, }, EnvironmentMeta: EnvironmentMeta{ @@ -141,7 +138,7 @@ func flattenCustomvars(ctx context.Context, g *errgroup.Group, cvs <-chan contra }, Flatname: flatname, FlatnameChecksum: utils.Checksum(flatname), - Flatvalue: fv, + Flatvalue: flatvalue, }: case <-ctx.Done(): return ctx.Err() diff --git a/schema/mysql/schema.sql b/schema/mysql/schema.sql index 36135c29f..467b810de 100644 --- a/schema/mysql/schema.sql +++ b/schema/mysql/schema.sql @@ -983,7 +983,7 @@ CREATE TABLE customvar_flat ( flatname_checksum binary(20) NOT NULL COMMENT 'sha1(flatname after conversion)', flatname varchar(512) COLLATE utf8mb4_unicode_ci NOT NULL COMMENT 'Path converted with `.` and `[ ]`', - flatvalue text NOT NULL, + flatvalue text DEFAULT NULL, PRIMARY KEY (id), diff --git a/schema/mysql/upgrades/1.2.0.sql b/schema/mysql/upgrades/1.2.0.sql new file mode 100644 index 000000000..b91f9adec --- /dev/null +++ b/schema/mysql/upgrades/1.2.0.sql @@ -0,0 +1 @@ +ALTER TABLE customvar_flat MODIFY COLUMN flatvalue text DEFAULT NULL; diff --git a/schema/pgsql/schema.sql b/schema/pgsql/schema.sql index af103a547..96f938f85 100644 --- a/schema/pgsql/schema.sql +++ b/schema/pgsql/schema.sql @@ -1553,7 +1553,7 @@ CREATE TABLE customvar_flat ( flatname_checksum bytea20 NOT NULL, flatname citext NOT NULL, - flatvalue text NOT NULL, + flatvalue text DEFAULT NULL, CONSTRAINT pk_customvar_flat PRIMARY KEY (id) ); diff --git a/schema/pgsql/upgrades/1.2.0.sql b/schema/pgsql/upgrades/1.2.0.sql new file mode 100644 index 000000000..abda26117 --- /dev/null +++ b/schema/pgsql/upgrades/1.2.0.sql @@ -0,0 +1 @@ +ALTER TABLE customvar_flat ALTER COLUMN flatvalue DROP NOT NULL; From 23ac5914aa7e21ae462b7c521cd32916e4413351 Mon Sep 17 00:00:00 2001 From: Yonas Habteab Date: Mon, 19 Jun 2023 12:59:05 +0200 Subject: [PATCH 2/2] Add empty custom vars test cases --- tests/object_sync_test.go | 145 +++++++++++++++++++++++--------------- 1 file changed, 88 insertions(+), 57 deletions(-) diff --git a/tests/object_sync_test.go b/tests/object_sync_test.go index 27d3c9551..e2455ec47 100644 --- a/tests/object_sync_test.go +++ b/tests/object_sync_test.go @@ -3,6 +3,7 @@ package icingadb_test import ( "bytes" "context" + "database/sql" _ "embed" "fmt" "github.com/go-redis/redis/v8" @@ -1194,9 +1195,9 @@ func newString(s string) *string { } type CustomVarTestData struct { - Value interface{} // Value to put into config or API - Vars map[string]string // Expected values in customvar table - VarsFlat map[string]string // Expected values in customvar_flat table + Value interface{} // Value to put into config or API + Vars map[string]string // Expected values in customvar table + VarsFlat map[string]sql.NullString // Expected values in customvar_flat table } func (c *CustomVarTestData) Icinga2ConfigValue() string { @@ -1246,20 +1247,22 @@ func (c *CustomVarTestData) verify(t require.TestingT, logger *zap.Logger, db *s require.NoError(t, err, "querying customvars") defer func() { _ = rows.Close() }() - expectedSrc := c.Vars - if flat { - expectedSrc = c.VarsFlat - } - // copy map to remove items while reading from the database - expected := make(map[string]string) - for k, v := range expectedSrc { - expected[k] = v + expected := make(map[string]sql.NullString) + if flat { + for k, v := range c.VarsFlat { + expected[k] = v + } + } else { + for k, v := range c.Vars { + expected[k] = toDBString(v) + } } for rows.Next() { - var cvName, cvValue string - err := rows.Scan(&cvName, &cvValue) + var cvName string + var cvValue sql.NullString + err = rows.Scan(&cvName, &cvValue) require.NoError(t, err, "scanning query row") logger.Debug("custom var from database", @@ -1267,13 +1270,13 @@ func (c *CustomVarTestData) verify(t require.TestingT, logger *zap.Logger, db *s zap.String("object-type", typeName), zap.Any("object-name", name), zap.String("custom-var-name", cvName), - zap.String("custom-var-value", cvValue)) + zap.String("custom-var-value", cvValue.String)) if cvExpected, ok := expected[cvName]; ok { assert.Equalf(t, cvExpected, cvValue, "custom var %q", cvName) delete(expected, cvName) } else if !ok { - assert.Failf(t, "unexpected custom var", "%q: %q", cvName, cvValue) + assert.Failf(t, "unexpected custom var", "%q: %q", cvName, cvValue.String) } } @@ -1299,9 +1302,33 @@ func makeCustomVarTestData(t *testing.T) []*CustomVarTestData { id + "-hello": `"` + id + ` world"`, id + "-foo": `"` + id + ` bar"`, }, - VarsFlat: map[string]string{ - id + "-hello": id + " world", - id + "-foo": id + " bar", + VarsFlat: map[string]sql.NullString{ + id + "-hello": toDBString(id + " world"), + id + "-foo": toDBString(id + " bar"), + }, + }) + + // empty custom vars of type array and dictionaries + data = append(data, &CustomVarTestData{ + Value: map[string]interface{}{ + id + "-empty-list": []interface{}{}, + id + "-empty-nested-list": []interface{}{[]interface{}{}}, + id + "-empty-dict": map[string]interface{}{}, + id + "-empty-nested-dict": map[string]interface{}{ + "some-key": map[string]interface{}{}, + }, + }, + Vars: map[string]string{ + id + "-empty-list": `[]`, + id + "-empty-nested-list": `[[]]`, + id + "-empty-dict": `{}`, + id + "-empty-nested-dict": `{"some-key":{}}`, + }, + VarsFlat: map[string]sql.NullString{ + id + "-empty-list": {}, + id + "-empty-nested-list[0]": {}, + id + "-empty-dict": {}, + id + "-empty-nested-dict.some-key": {}, }, }) @@ -1342,29 +1369,29 @@ func makeCustomVarTestData(t *testing.T) []*CustomVarTestData { id + "-nested-dict": `{"array":["answer?",42],"dict":{"another-key":"another-value","yet-another-key":4711},"top-level-entry":"good morning"}`, id + "-nested-array": `[[1,2,3],{"contains-a-map":"yes","really?":true},-42]`, }, - VarsFlat: map[string]string{ - id + "-array[0]": `foo`, - id + "-array[1]": `23`, - id + "-array[2]": `bar`, - id + "-dict.some-key": `some-value`, - id + "-dict.other-key": `other-value`, - id + "-string": `hello icinga`, - id + "-int": `-1`, - id + "-float": `13.37`, - id + "-true": `true`, - id + "-false": `false`, - id + "-null": `null`, - id + "-nested-dict.dict.another-key": `another-value`, - id + "-nested-dict.dict.yet-another-key": `4711`, - id + "-nested-dict.array[0]": `answer?`, - id + "-nested-dict.array[1]": `42`, - id + "-nested-dict.top-level-entry": `good morning`, - id + "-nested-array[0][0]": `1`, - id + "-nested-array[0][1]": `2`, - id + "-nested-array[0][2]": `3`, - id + "-nested-array[1].contains-a-map": `yes`, - id + "-nested-array[1].really?": `true`, - id + "-nested-array[2]": `-42`, + VarsFlat: map[string]sql.NullString{ + id + "-array[0]": toDBString(`foo`), + id + "-array[1]": toDBString(`23`), + id + "-array[2]": toDBString(`bar`), + id + "-dict.some-key": toDBString(`some-value`), + id + "-dict.other-key": toDBString(`other-value`), + id + "-string": toDBString(`hello icinga`), + id + "-int": toDBString(`-1`), + id + "-float": toDBString(`13.37`), + id + "-true": toDBString(`true`), + id + "-false": toDBString(`false`), + id + "-null": toDBString(`null`), + id + "-nested-dict.dict.another-key": toDBString(`another-value`), + id + "-nested-dict.dict.yet-another-key": toDBString(`4711`), + id + "-nested-dict.array[0]": toDBString(`answer?`), + id + "-nested-dict.array[1]": toDBString(`42`), + id + "-nested-dict.top-level-entry": toDBString(`good morning`), + id + "-nested-array[0][0]": toDBString(`1`), + id + "-nested-array[0][1]": toDBString(`2`), + id + "-nested-array[0][2]": toDBString(`3`), + id + "-nested-array[1].contains-a-map": toDBString(`yes`), + id + "-nested-array[1].really?": toDBString(`true`), + id + "-nested-array[2]": toDBString(`-42`), }, }) @@ -1380,14 +1407,14 @@ func makeCustomVarTestData(t *testing.T) []*CustomVarTestData { "b": `["bar",42,-13.37]`, "c": `{"a":true,"b":false,"c":null}`, }, - VarsFlat: map[string]string{ - "a": "foo", - "b[0]": `bar`, - "b[1]": `42`, - "b[2]": `-13.37`, - "c.a": `true`, - "c.b": `false`, - "c.c": `null`, + VarsFlat: map[string]sql.NullString{ + "a": toDBString("foo"), + "b[0]": toDBString(`bar`), + "b[1]": toDBString(`42`), + "b[2]": toDBString(`-13.37`), + "c.a": toDBString(`true`), + "c.b": toDBString(`false`), + "c.c": toDBString(`null`), }, }, &CustomVarTestData{ Value: map[string]interface{}{ @@ -1400,16 +1427,20 @@ func makeCustomVarTestData(t *testing.T) []*CustomVarTestData { "b": `[true,false,null]`, "c": `{"a":"foo","b":"bar","c":42}`, }, - VarsFlat: map[string]string{ - "a": `-13.37`, - "b[0]": `true`, - "b[1]": `false`, - "b[2]": `null`, - "c.a": "foo", - "c.b": `bar`, - "c.c": `42`, + VarsFlat: map[string]sql.NullString{ + "a": toDBString(`-13.37`), + "b[0]": toDBString(`true`), + "b[1]": toDBString(`false`), + "b[2]": toDBString(`null`), + "c.a": toDBString("foo"), + "c.b": toDBString(`bar`), + "c.c": toDBString(`42`), }, }) return data } + +func toDBString(str string) sql.NullString { + return sql.NullString{String: str, Valid: true} +}