From 174ede1fe355bcf84a78b6bdff124319dec43ed2 Mon Sep 17 00:00:00 2001 From: Dominik Richter Date: Fri, 1 Dec 2023 02:12:42 -0800 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20fix=20contains=20on=20nil=20dict?= =?UTF-8?q?s=20(#2720)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Any dict entry can be `null`, which means that subsequent calls to its fields can lead to unexpected behavior. Such is the case of `contains`. This call makes a lot of sense on top of e.g. a string array: ```coffee ["a", "b"].contains("b") ``` But if the structure doesn't exist, then the call fails in unexpected ways: ``` left side of operation is null ``` This approach still uses an old way of addressing the problem, which looks quite a bit ugly in the implementation. We have to check for non-nil and for non-zero length. Once v10 comes around, we can rely on the `empty` keyword and all this is reduced to just the internal non-empty check. Signed-off-by: Dominik Richter --- .github/actions/spelling/expect.txt | 1 + mql/mql_test.go | 22 +++++++++++ mqlc/builtin_map.go | 37 +++++++++++++++++-- .../testutils/mockprovider/resources/all.go | 12 ++++++ .../mockprovider/resources/mockprovider.lr | 2 + .../mockprovider/resources/mockprovider.lr.go | 28 ++++++++++++++ providers/os/resources/mql_test.go | 33 ++++++++++------- 7 files changed, 118 insertions(+), 17 deletions(-) diff --git a/.github/actions/spelling/expect.txt b/.github/actions/spelling/expect.txt index f1681e6d30..219bb54ea9 100644 --- a/.github/actions/spelling/expect.txt +++ b/.github/actions/spelling/expect.txt @@ -42,6 +42,7 @@ mgroup Mpim nodepool nullgroup +nullstring opcplc Pids postgre diff --git a/mql/mql_test.go b/mql/mql_test.go index da8a84343a..ec31064860 100644 --- a/mql/mql_test.go +++ b/mql/mql_test.go @@ -219,3 +219,25 @@ func TestNullResources(t *testing.T) { }, }) } + +func TestNullString(t *testing.T) { + x := testutils.InitTester(testutils.LinuxMock()) + x.TestSimple(t, []testutils.SimpleTest{ + { + Code: "muser.nullstring.contains('123')", + ResultIndex: 0, + Expectation: false, + }, + }) +} + +func TestDictMethods(t *testing.T) { + x := testutils.InitTester(testutils.LinuxMock()) + x.TestSimple(t, []testutils.SimpleTest{ + { + Code: "muser.dict.nonexisting.contains('abc')", + ResultIndex: 1, + Expectation: false, + }, + }) +} diff --git a/mqlc/builtin_map.go b/mqlc/builtin_map.go index 4d07f064ea..e01c1cc1d8 100644 --- a/mqlc/builtin_map.go +++ b/mqlc/builtin_map.go @@ -137,19 +137,50 @@ func compileDictContains(c *compiler, typ types.Type, ref uint64, id string, cal Binding: c.tailRef(), }, }) + lengthRef := c.tailRef() - // > 0 + // FIXME: DEPRECATED, replace in v10.0 wit the use of != empty vv + // != 0 c.addChunk(&llx.Chunk{ Call: llx.Chunk_FUNCTION, - Id: string(">" + types.Int), + Id: string("!=" + types.Int), Function: &llx.Function{ Type: string(types.Bool), - Binding: c.tailRef(), + Binding: lengthRef, Args: []*llx.Primitive{ llx.IntPrimitive(0), }, }, }) + neq0Ref := c.tailRef() + + // != null + c.addChunk(&llx.Chunk{ + Call: llx.Chunk_FUNCTION, + Id: string("!=" + types.Nil), + Function: &llx.Function{ + Type: string(types.Bool), + Binding: ref, + Args: []*llx.Primitive{ + // Note: we need this for backwards compatibility because labels + // require 1 argument on < v9.1 + llx.NilPrimitive, + }, + }, + }) + neqNullRef := c.tailRef() + + c.addChunk(&llx.Chunk{ + Call: llx.Chunk_FUNCTION, + Id: string("&&" + types.Bool), + Function: &llx.Function{ + Type: string(types.Bool), + Binding: neqNullRef, + Args: []*llx.Primitive{ + llx.RefPrimitiveV2(neq0Ref), + }, + }, + }) checksum := c.Result.CodeV2.Checksums[c.tailRef()] c.Result.Labels.Labels[checksum] = "[].contains()" diff --git a/providers-sdk/v1/testutils/mockprovider/resources/all.go b/providers-sdk/v1/testutils/mockprovider/resources/all.go index aa19965c5d..fb8dc8a166 100644 --- a/providers-sdk/v1/testutils/mockprovider/resources/all.go +++ b/providers-sdk/v1/testutils/mockprovider/resources/all.go @@ -27,6 +27,11 @@ func (c *mqlMuser) nullgroup() (*mqlMgroup, error) { return nil, nil } +func (c *mqlMuser) nullstring() (string, error) { + c.Nullstring.State = plugin.StateIsSet | plugin.StateIsNull + return "", nil +} + func (c *mqlMuser) groups() ([]interface{}, error) { one, err := CreateResource(c.MqlRuntime, "mgroup", map[string]*llx.RawData{ "name": llx.StringData("group one"), @@ -43,3 +48,10 @@ func (c *mqlMuser) groups() ([]interface{}, error) { func (c *mqlMgroup) id() (string, error) { return c.Name.Data, nil } + +func (c *mqlMuser) dict() (any, error) { + return map[string]any{ + "a1": []any{int64(1), int64(2), int64(3)}, + "s1": "hello world", + }, nil +} diff --git a/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr b/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr index 2801333ce8..391f7a1df8 100644 --- a/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr +++ b/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr @@ -8,7 +8,9 @@ muser { name string group() mgroup nullgroup() mgroup + nullstring() string groups() []mgroup + dict() dict } mgroup { diff --git a/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr.go b/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr.go index c669e4f8bf..d6c4e96d54 100644 --- a/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr.go +++ b/providers-sdk/v1/testutils/mockprovider/resources/mockprovider.lr.go @@ -102,9 +102,15 @@ var getDataFields = map[string]func(r plugin.Resource) *plugin.DataRes{ "muser.nullgroup": func(r plugin.Resource) *plugin.DataRes { return (r.(*mqlMuser).GetNullgroup()).ToDataRes(types.Resource("mgroup")) }, + "muser.nullstring": func(r plugin.Resource) *plugin.DataRes { + return (r.(*mqlMuser).GetNullstring()).ToDataRes(types.String) + }, "muser.groups": func(r plugin.Resource) *plugin.DataRes { return (r.(*mqlMuser).GetGroups()).ToDataRes(types.Array(types.Resource("mgroup"))) }, + "muser.dict": func(r plugin.Resource) *plugin.DataRes { + return (r.(*mqlMuser).GetDict()).ToDataRes(types.Dict) + }, "mgroup.name": func(r plugin.Resource) *plugin.DataRes { return (r.(*mqlMgroup).GetName()).ToDataRes(types.String) }, @@ -136,10 +142,18 @@ var setDataFields = map[string]func(r plugin.Resource, v *llx.RawData) bool { r.(*mqlMuser).Nullgroup, ok = plugin.RawToTValue[*mqlMgroup](v.Value, v.Error) return }, + "muser.nullstring": func(r plugin.Resource, v *llx.RawData) (ok bool) { + r.(*mqlMuser).Nullstring, ok = plugin.RawToTValue[string](v.Value, v.Error) + return + }, "muser.groups": func(r plugin.Resource, v *llx.RawData) (ok bool) { r.(*mqlMuser).Groups, ok = plugin.RawToTValue[[]interface{}](v.Value, v.Error) return }, + "muser.dict": func(r plugin.Resource, v *llx.RawData) (ok bool) { + r.(*mqlMuser).Dict, ok = plugin.RawToTValue[interface{}](v.Value, v.Error) + return + }, "mgroup.__id": func(r plugin.Resource, v *llx.RawData) (ok bool) { r.(*mqlMgroup).__id, ok = v.Value.(string) return @@ -180,7 +194,9 @@ type mqlMuser struct { Name plugin.TValue[string] Group plugin.TValue[*mqlMgroup] Nullgroup plugin.TValue[*mqlMgroup] + Nullstring plugin.TValue[string] Groups plugin.TValue[[]interface{}] + Dict plugin.TValue[interface{}] } // createMuser creates a new instance of this resource @@ -256,6 +272,12 @@ func (c *mqlMuser) GetNullgroup() *plugin.TValue[*mqlMgroup] { }) } +func (c *mqlMuser) GetNullstring() *plugin.TValue[string] { + return plugin.GetOrCompute[string](&c.Nullstring, func() (string, error) { + return c.nullstring() + }) +} + func (c *mqlMuser) GetGroups() *plugin.TValue[[]interface{}] { return plugin.GetOrCompute[[]interface{}](&c.Groups, func() ([]interface{}, error) { if c.MqlRuntime.HasRecording { @@ -272,6 +294,12 @@ func (c *mqlMuser) GetGroups() *plugin.TValue[[]interface{}] { }) } +func (c *mqlMuser) GetDict() *plugin.TValue[interface{}] { + return plugin.GetOrCompute[interface{}](&c.Dict, func() (interface{}, error) { + return c.dict() + }) +} + // mqlMgroup for the mgroup resource type mqlMgroup struct { MqlRuntime *plugin.Runtime diff --git a/providers/os/resources/mql_test.go b/providers/os/resources/mql_test.go index 1413ead750..9e9ba167c2 100644 --- a/providers/os/resources/mql_test.go +++ b/providers/os/resources/mql_test.go @@ -261,37 +261,37 @@ func TestDict_Methods_Contains(t *testing.T) { x.TestSimple(t, []testutils.SimpleTest{ { Code: p + "params['hello'].contains('ll')", - ResultIndex: 1, + ResultIndex: 2, Expectation: true, }, { Code: p + "params['hello'].contains('lloo')", - ResultIndex: 1, + ResultIndex: 2, Expectation: false, }, { Code: p + "params['hello'].contains(['xx','he'])", - ResultIndex: 1, + ResultIndex: 2, Expectation: true, }, { Code: p + "params['hello'].contains(['xx'])", - ResultIndex: 1, + ResultIndex: 2, Expectation: false, }, { Code: p + "params['string-array'].contains('a')", - ResultIndex: 1, + ResultIndex: 2, Expectation: true, }, { Code: p + "params['string-array'].containsOnly(['c', 'a', 'b'])", - ResultIndex: 1, + ResultIndex: 2, Expectation: true, }, { Code: p + "params['string-array'].containsOnly(['a', 'b'])", - ResultIndex: 1, + ResultIndex: 2, Expectation: false, }, // { @@ -300,37 +300,37 @@ func TestDict_Methods_Contains(t *testing.T) { // }, { Code: p + "params['string-array'].containsNone(['d','e'])", - ResultIndex: 1, + ResultIndex: 2, Expectation: true, }, { Code: p + "params['string-array'].containsNone(['a', 'e'])", - ResultIndex: 1, + ResultIndex: 2, Expectation: false, }, { Code: p + "params['string-array'].none('a')", - ResultIndex: 1, + ResultIndex: 2, Expectation: false, }, { Code: p + "params['string-array'].contains(_ == 'a')", - ResultIndex: 1, + ResultIndex: 2, Expectation: true, }, { Code: p + "params['string-array'].none(_ == /a/)", - ResultIndex: 1, + ResultIndex: 2, Expectation: false, }, { Code: p + "params['string-array'].contains(value == 'a')", - ResultIndex: 1, + ResultIndex: 2, Expectation: true, }, { Code: p + "params['string-array'].none(value == 'a')", - ResultIndex: 1, + ResultIndex: 2, Expectation: false, }, }) @@ -346,6 +346,11 @@ func TestDict_Methods_Map(t *testing.T) { x := testutils.InitTester(testutils.LinuxMock()) x.TestSimple(t, []testutils.SimpleTest{ + { + Code: p + "params.nonexistent.contains('sth')", + Expectation: false, + ResultIndex: 1, + }, { Code: p + "params['string-array'].where(_ == 'a')", Expectation: []interface{}{"a"},