From 4055b2025981857b5ac00728375e265015128ccb Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Tue, 16 Aug 2022 21:32:24 +0800 Subject: [PATCH 1/4] fix: sql compare to undefined values --- .../src/emqx_rule_runtime.erl | 14 +- .../test/emqx_rule_engine_SUITE.erl | 208 ++++++++++++++++++ 2 files changed, 216 insertions(+), 6 deletions(-) diff --git a/apps/emqx_rule_engine/src/emqx_rule_runtime.erl b/apps/emqx_rule_engine/src/emqx_rule_runtime.erl index 88b2c9143..145e30eec 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_runtime.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_runtime.erl @@ -216,10 +216,8 @@ match_conditions({}, _Data) -> true. %% comparing numbers against strings -compare(Op, undefined, undefined) -> - do_compare(Op, undefined, undefined); -compare(_Op, L, R) when L == undefined; R == undefined -> - false; +compare(Op, L, R) when L == undefined; R == undefined -> + do_compare(Op, L, R); 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) -> @@ -232,10 +230,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) -> L >= R; +do_compare('<=', 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). diff --git a/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl b/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl index 9031c3df8..e1851b76c 100644 --- a/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl +++ b/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl @@ -126,6 +126,10 @@ groups() -> t_sqlparse_array_range_1, t_sqlparse_array_range_2, t_sqlparse_true_false, + 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 +2490,210 @@ 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_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 " From 582ead1d77cba4bda669b931c22c63e2ca9be4a4 Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Tue, 16 Aug 2022 21:38:57 +0800 Subject: [PATCH 2/4] fix: update appup for rule engine --- .../src/emqx_rule_engine.appup.src | 22 +++++++++++++++---- 1 file changed, 18 insertions(+), 4 deletions(-) diff --git a/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src b/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src index 1c5fb0e7b..792f99310 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src +++ b/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src @@ -1,10 +1,17 @@ %% -*- mode: erlang -*- %% Unless you know what you are doing, DO NOT edit manually!! {VSN, - [{"4.3.13",[{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]}, - {"4.3.12",[{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]}, + [{"4.3.13",[ + {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}, + {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]} + ]}, + {"4.3.12",[ + {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}, + {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]} + ]}, {"4.3.11", [{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}, + {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}, {load_module,emqx_rule_validator,brutal_purge,soft_purge,[]}, {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}]}, {"4.3.10", @@ -166,11 +173,18 @@ {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}, {load_module,emqx_rule_engine_api,brutal_purge,soft_purge,[]}]}, {<<".*">>,[]}], - [{"4.3.13",[{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]}, - {"4.3.12",[{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}]}, + [{"4.3.13",[ + {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}, + {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]} + ]}, + {"4.3.12",[ + {load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}, + {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]} + ]}, {"4.3.11", [{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,[]}, From e1615e86df2741141ca41d86018252da34643fd1 Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Wed, 17 Aug 2022 18:14:29 +0800 Subject: [PATCH 3/4] chore: update the change log --- CHANGES-4.3.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGES-4.3.md b/CHANGES-4.3.md index ab74b877a..6023772a3 100644 --- a/CHANGES-4.3.md +++ b/CHANGES-4.3.md @@ -13,7 +13,12 @@ File format: ## v4.3.19 ### Bug fixes + - Fix GET `/auth_clientid` and `/auth_username` counts. [#8655](https://github.com/emqx/emqx/pull/8655) +- 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. ## v4.3.19 From 413612a69de1a8ad8bdcee773775c388b103269c Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Thu, 18 Aug 2022 09:18:41 +0800 Subject: [PATCH 4/4] fix: duplicate appup instructions --- .../src/emqx_rule_engine.appup.src | 1 - .../test/emqx_rule_engine_SUITE.erl | 28 ++++++++++++++++++- 2 files changed, 27 insertions(+), 2 deletions(-) diff --git a/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src b/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src index ff2ddc4c9..f50c1a368 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src +++ b/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src @@ -190,7 +190,6 @@ {"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,[]}, diff --git a/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl b/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl index e1851b76c..d1e6b26a0 100644 --- a/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl +++ b/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl @@ -126,6 +126,7 @@ 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, @@ -2493,8 +2494,33 @@ t_sqlparse_true_false(_Config) -> -define(TEST_SQL(SQL), emqx_rule_sqltester:test( #{<<"rawsql">> => SQL, - <<"ctx">> => #{<<"payload">> => <<"">>, + <<"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 "