Merge pull request #8743 from terry-xiaoyu/fix_sql_compare

Fix sql compare to undefined values

CHANGES-4.3.md

@@ -22,6 +22,10 @@ File format:
 
 ### Bug fixes
 
+- Fix rule SQL compare to null values always returns false. [#8743](https://github.com/emqx/emqx/pull/8743)
+  Before this change, the following SQL failed to match on the WHERE clause (`clientid != foo` returns false):
+  `SELECT 'some_var' as clientid FROM "t" WHERE clientid != foo`.
+  The `foo` variable is a null value, so `clientid != foo` should be evaluated as true.
 - Fix GET `/auth_clientid` and `/auth_username` counts. [#8655](https://github.com/emqx/emqx/pull/8655)
 - Add an idle timer for ExProto UDP client to avoid client leaking [#8628](https://github.com/emqx/emqx/pull/8628)
 - Fix GET `/listeners/` crashes when listener is not ready. [#8752](https://github.com/emqx/emqx/pull/8752)

apps/emqx_rule_engine/src/emqx_rule_engine.appup.src

@@ -190,9 +190,9 @@
    {"4.3.11",
     [{load_module,emqx_rule_actions,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_utils,brutal_purge,soft_purge,[]},
-     {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_validator,brutal_purge,soft_purge,[]},
+     {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]},
      {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}]},
    {"4.3.10",
     [{load_module,emqx_rule_validator,brutal_purge,soft_purge,[]},

apps/emqx_rule_engine/src/emqx_rule_runtime.erl

@@ -218,10 +218,8 @@ match_conditions({}, _Data) ->
     true.
 
 %% comparing numbers against strings
-compare(Op, undefined, undefined) ->
+compare(Op, L, R) when L == undefined; R == undefined ->
-    do_compare(Op, undefined, undefined);
+    do_compare(Op, L, R);
-compare(_Op, L, R) when L == undefined; R == undefined ->
-    false;
 compare(Op, L, R) when is_number(L), is_binary(R) ->
     do_compare(Op, L, number(R));
 compare(Op, L, R) when is_binary(L), is_number(R) ->
@@ -234,10 +232,14 @@ compare(Op, L, R) ->
     do_compare(Op, L, R).
 
 do_compare('=', L, R) -> L == R;
+do_compare('>', L, R) when L == undefined; R == undefined -> false;
 do_compare('>', L, R) -> L > R;
+do_compare('<', L, R) when L == undefined; R == undefined -> false;
 do_compare('<', L, R) -> L < R;
-do_compare('<=', L, R) -> L =< R;
+do_compare('<=', L, R) ->
-do_compare('>=', L, R) -> L >= R;
+    do_compare('=', L, R) orelse do_compare('<', L, R);
+do_compare('>=', L, R) ->
+    do_compare('=', L, R) orelse do_compare('>', L, R);
 do_compare('<>', L, R) -> L /= R;
 do_compare('!=', L, R) -> L /= R;
 do_compare('=~', T, F) -> emqx_topic:match(T, F).

apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl

@@ -126,6 +126,11 @@ groups() ->
        t_sqlparse_array_range_1,
        t_sqlparse_array_range_2,
        t_sqlparse_true_false,
+       t_sqlparse_compare_undefined,
+       t_sqlparse_compare_null_null,
+       t_sqlparse_compare_null_notnull,
+       t_sqlparse_compare_notnull_null,
+       t_sqlparse_compare,
        t_sqlparse_new_map,
        t_sqlparse_invalid_json
       ]},
@@ -2486,6 +2491,235 @@ t_sqlparse_true_false(_Config) ->
                    <<"c">> := [true]
                    }, Res00).
 
+-define(TEST_SQL(SQL),
+    emqx_rule_sqltester:test(
+        #{<<"rawsql">> => SQL,
+          <<"ctx">> => #{<<"payload">> => <<"{}">>,
+                         <<"topic">> => <<"t/a">>}})).
+
+t_sqlparse_compare_undefined(_Config) ->
+    Sql00 = "select "
+            " * "
+            "from \"t/#\" "
+            "where dev != undefined ",
+    %% no match
+    ?assertMatch({error, nomatch}, ?TEST_SQL(Sql00)),
+
+    Sql01 = "select "
+            " 'd' as dev "
+            "from \"t/#\" "
+            "where dev != undefined ",
+    {ok, Res01} = ?TEST_SQL(Sql01),
+    %% pass
+    ?assertMatch(#{}, Res01),
+
+    Sql02 = "select "
+            " * "
+            "from \"t/#\" "
+            "where dev != 'undefined' ",
+    {ok, Res02} = ?TEST_SQL(Sql02),
+    %% pass
+    ?assertMatch(#{}, Res02).
+
+t_sqlparse_compare_null_null(_Config) ->
+    %% test undefined == undefined
+    Sql00 = "select "
+            " a = b as c "
+            "from \"t/#\" ",
+    {ok, Res00} = ?TEST_SQL(Sql00),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res00),
+
+    %% test undefined != undefined
+    Sql01 = "select "
+            " a != b as c "
+            "from \"t/#\" ",
+    {ok, Res01} = ?TEST_SQL(Sql01),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res01),
+
+    %% test undefined > undefined
+    Sql02 = "select "
+            " a > b as c "
+            "from \"t/#\" ",
+    {ok, Res02} = ?TEST_SQL(Sql02),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res02),
+
+    %% test undefined < undefined
+    Sql03 = "select "
+            " a < b as c "
+            "from \"t/#\" ",
+    {ok, Res03} = ?TEST_SQL(Sql03),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res03),
+
+    %% test undefined <= undefined
+    Sql04 = "select "
+            " a <= b as c "
+            "from \"t/#\" ",
+    {ok, Res04} = ?TEST_SQL(Sql04),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res04),
+
+    %% test undefined >= undefined
+    Sql05 = "select "
+            " a >= b as c "
+            "from \"t/#\" ",
+    {ok, Res05} = ?TEST_SQL(Sql05),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res05).
+
+t_sqlparse_compare_null_notnull(_Config) ->
+    %% test undefined == b
+    Sql00 = "select "
+            " 'b' as b, a = b as c "
+            "from \"t/#\" ",
+    {ok, Res00} = ?TEST_SQL(Sql00),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res00),
+
+    %% test undefined != b
+    Sql01 = "select "
+            " 'b' as b, a != b as c "
+            "from \"t/#\" ",
+    {ok, Res01} = ?TEST_SQL(Sql01),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res01),
+
+    %% test undefined > b
+    Sql02 = "select "
+            " 'b' as b, a > b as c "
+            "from \"t/#\" ",
+    {ok, Res02} = ?TEST_SQL(Sql02),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res02),
+
+    %% test undefined < b
+    Sql03 = "select "
+            " 'b' as b, a < b as c "
+            "from \"t/#\" ",
+    {ok, Res03} = ?TEST_SQL(Sql03),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res03),
+
+    %% test undefined <= b
+    Sql04 = "select "
+            " 'b' as b, a <= b as c "
+            "from \"t/#\" ",
+    {ok, Res04} = ?TEST_SQL(Sql04),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res04),
+
+    %% test undefined >= b
+    Sql05 = "select "
+            " 'b' as b, a >= b as c "
+            "from \"t/#\" ",
+    {ok, Res05} = ?TEST_SQL(Sql05),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res05).
+
+t_sqlparse_compare_notnull_null(_Config) ->
+    %% test 'a' == undefined
+    Sql00 = "select "
+            " 'a' as a, a = b as c "
+            "from \"t/#\" ",
+    {ok, Res00} = ?TEST_SQL(Sql00),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res00),
+
+    %% test 'a' != undefined
+    Sql01 = "select "
+            " 'a' as a, a != b as c "
+            "from \"t/#\" ",
+    {ok, Res01} = ?TEST_SQL(Sql01),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res01),
+
+    %% test 'a' > undefined
+    Sql02 = "select "
+            " 'a' as a, a > b as c "
+            "from \"t/#\" ",
+    {ok, Res02} = ?TEST_SQL(Sql02),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res02),
+
+    %% test 'a' < undefined
+    Sql03 = "select "
+            " 'a' as a, a < b as c "
+            "from \"t/#\" ",
+    {ok, Res03} = ?TEST_SQL(Sql03),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res03),
+
+    %% test 'a' <= undefined
+    Sql04 = "select "
+            " 'a' as a, a <= b as c "
+            "from \"t/#\" ",
+    {ok, Res04} = ?TEST_SQL(Sql04),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res04),
+
+    %% test 'a' >= undefined
+    Sql05 = "select "
+            " 'a' as a, a >= b as c "
+            "from \"t/#\" ",
+    {ok, Res05} = ?TEST_SQL(Sql05),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res05).
+
+t_sqlparse_compare(_Config) ->
+    Sql00 = "select "
+            " 'a' as a, 'a' as b, a = b as c "
+            "from \"t/#\" ",
+    {ok, Res00} = ?TEST_SQL(Sql00),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res00),
+
+    Sql01 = "select "
+            " is_null(a) as c "
+            "from \"t/#\" ",
+    {ok, Res01} = ?TEST_SQL(Sql01),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res01),
+
+    Sql02 = "select "
+            " 1 as a, 2 as b, a < b as c "
+            "from \"t/#\" ",
+    {ok, Res02} = ?TEST_SQL(Sql02),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res02),
+
+    Sql03 = "select "
+            " 1 as a, 2 as b, a > b as c "
+            "from \"t/#\" ",
+    {ok, Res03} = ?TEST_SQL(Sql03),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res03),
+
+    Sql04 = "select "
+            " 1 as a, 2 as b, a = b as c "
+            "from \"t/#\" ",
+    {ok, Res04} = ?TEST_SQL(Sql04),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res04),
+
+    %% test 'a' >= undefined
+    Sql05 = "select "
+            " 1 as a, 2 as b, a >= b as c "
+            "from \"t/#\" ",
+    {ok, Res05} = ?TEST_SQL(Sql05),
+    ?assertMatch(#{<<"c">> := false
+                   }, Res05),
+
+    %% test 'a' >= undefined
+    Sql06 = "select "
+            " 1 as a, 2 as b, a <= b as c "
+            "from \"t/#\" ",
+    {ok, Res06} = ?TEST_SQL(Sql06),
+    ?assertMatch(#{<<"c">> := true
+                   }, Res06).
+
 t_sqlparse_new_map(_Config) ->
     %% construct a range without 'as'
     Sql00 = "select "