From f0cc75d1446582b9e2829e390eadb84d5b42f150 Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Thu, 8 Sep 2022 16:37:38 +0800 Subject: [PATCH 1/3] fix(sql): topic matching to null values should return false --- CHANGES-4.3.md | 6 +- .../src/emqx_rule_runtime.erl | 5 +- .../test/emqx_rule_engine_SUITE.erl | 142 ++++++++++++++++-- 3 files changed, 138 insertions(+), 15 deletions(-) diff --git a/CHANGES-4.3.md b/CHANGES-4.3.md index aa250b7ad..9be7de63f 100644 --- a/CHANGES-4.3.md +++ b/CHANGES-4.3.md @@ -17,7 +17,11 @@ File format: - Fix rule-engine update behaviour which may initialize actions for disabled rules. [#8849](https://github.com/emqx/emqx/pull/8849) - Fix JWT plugin don't support non-integer timestamp claims. [#8862](https://github.com/emqx/emqx/pull/8862) - Fix dashboard binding IP address not working. [#8916](https://github.com/emqx/emqx/pull/8916) - +- Fix rule SQL topic matching to null values failed. [#?](https://github.com/emqx/emqx/pull/?) + The following SQL should not fail (crash) but return `{"r": false}`: + `SELECT topic =~ 't' as r FROM "$events/client_connected"`. + The topic is a null value as there's no such field in event `$events/client_connected`, so it + should return false if mathch it to a topic. ## v4.3.19 diff --git a/apps/emqx_rule_engine/src/emqx_rule_runtime.erl b/apps/emqx_rule_engine/src/emqx_rule_runtime.erl index 794c70dfd..71775f6ee 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_runtime.erl +++ b/apps/emqx_rule_engine/src/emqx_rule_runtime.erl @@ -242,7 +242,10 @@ 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). +do_compare('=~', undefined, undefined) -> true; +do_compare('=~', T, F) when T == undefined; F == undefined -> false; +do_compare('=~', T, F) -> + emqx_topic:match(T, F). number(Bin) -> try binary_to_integer(Bin) 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 d1e6b26a0..c8f66ea00 100644 --- a/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl +++ b/apps/emqx_rule_engine/test/emqx_rule_engine_SUITE.erl @@ -2505,6 +2505,13 @@ t_sqlparse_compare_undefined(_Config) -> %% no match ?assertMatch({error, nomatch}, ?TEST_SQL(Sql00)), + Sql00_1 = "select " + " * " + "from \"t/#\" " + "where dev <> undefined ", + %% no match + ?assertMatch({error, nomatch}, ?TEST_SQL(Sql00_1)), + Sql01 = "select " " 'd' as dev " "from \"t/#\" " @@ -2513,13 +2520,29 @@ t_sqlparse_compare_undefined(_Config) -> %% pass ?assertMatch(#{}, Res01), + Sql01_1 = "select " + " 'd' as dev " + "from \"t/#\" " + "where dev <> undefined ", + {ok, Res01_1} = ?TEST_SQL(Sql01_1), + %% pass + ?assertMatch(#{}, Res01_1), + Sql02 = "select " " * " "from \"t/#\" " "where dev != 'undefined' ", {ok, Res02} = ?TEST_SQL(Sql02), %% pass - ?assertMatch(#{}, Res02). + ?assertMatch(#{}, Res02), + + Sql03 = "select " + " * " + "from \"t/#\" " + "where dev =~ 'undefined' ", + Res03 = ?TEST_SQL(Sql03), + %% no match + ?assertMatch({error, nomatch}, Res03). t_sqlparse_compare_null_null(_Config) -> %% test undefined == undefined @@ -2538,6 +2561,14 @@ t_sqlparse_compare_null_null(_Config) -> ?assertMatch(#{<<"c">> := false }, Res01), + %% test undefined <> undefined + Sql01_1 = "select " + " a <> b as c " + "from \"t/#\" ", + {ok, Res01_1} = ?TEST_SQL(Sql01_1), + ?assertMatch(#{<<"c">> := false + }, Res01_1), + %% test undefined > undefined Sql02 = "select " " a > b as c " @@ -2568,10 +2599,18 @@ t_sqlparse_compare_null_null(_Config) -> "from \"t/#\" ", {ok, Res05} = ?TEST_SQL(Sql05), ?assertMatch(#{<<"c">> := true - }, Res05). + }, Res05), + + %% test undefined =~ undefined + Sql06 = "select " + " a =~ b as c " + "from \"t/#\" ", + {ok, Res06} = ?TEST_SQL(Sql06), + ?assertMatch(#{<<"c">> := true + }, Res06). t_sqlparse_compare_null_notnull(_Config) -> - %% test undefined == b + %% test undefined == 'b' Sql00 = "select " " 'b' as b, a = b as c " "from \"t/#\" ", @@ -2579,7 +2618,7 @@ t_sqlparse_compare_null_notnull(_Config) -> ?assertMatch(#{<<"c">> := false }, Res00), - %% test undefined != b + %% test undefined != 'b' Sql01 = "select " " 'b' as b, a != b as c " "from \"t/#\" ", @@ -2587,7 +2626,15 @@ t_sqlparse_compare_null_notnull(_Config) -> ?assertMatch(#{<<"c">> := true }, Res01), - %% test undefined > b + %% test undefined <> 'b' + Sql01_1 = "select " + " 'b' as b, a <> b as c " + "from \"t/#\" ", + {ok, Res01_1} = ?TEST_SQL(Sql01_1), + ?assertMatch(#{<<"c">> := true + }, Res01_1), + + %% test undefined > 'b' Sql02 = "select " " 'b' as b, a > b as c " "from \"t/#\" ", @@ -2595,7 +2642,7 @@ t_sqlparse_compare_null_notnull(_Config) -> ?assertMatch(#{<<"c">> := false }, Res02), - %% test undefined < b + %% test undefined < 'b' Sql03 = "select " " 'b' as b, a < b as c " "from \"t/#\" ", @@ -2603,7 +2650,7 @@ t_sqlparse_compare_null_notnull(_Config) -> ?assertMatch(#{<<"c">> := false }, Res03), - %% test undefined <= b + %% test undefined <= 'b' Sql04 = "select " " 'b' as b, a <= b as c " "from \"t/#\" ", @@ -2611,13 +2658,21 @@ t_sqlparse_compare_null_notnull(_Config) -> ?assertMatch(#{<<"c">> := false }, Res04), - %% test undefined >= b + %% test undefined >= 'b' Sql05 = "select " " 'b' as b, a >= b as c " "from \"t/#\" ", {ok, Res05} = ?TEST_SQL(Sql05), ?assertMatch(#{<<"c">> := false - }, Res05). + }, Res05), + + %% test undefined =~ 'b' + Sql06 = "select " + " 'b' as b, a =~ b as c " + "from \"t/#\" ", + {ok, Res06} = ?TEST_SQL(Sql06), + ?assertMatch(#{<<"c">> := false + }, Res06). t_sqlparse_compare_notnull_null(_Config) -> %% test 'a' == undefined @@ -2636,6 +2691,14 @@ t_sqlparse_compare_notnull_null(_Config) -> ?assertMatch(#{<<"c">> := true }, Res01), + %% test 'a' <> undefined + Sql01_1 = "select " + " 'a' as a, a <> b as c " + "from \"t/#\" ", + {ok, Res01_1} = ?TEST_SQL(Sql01_1), + ?assertMatch(#{<<"c">> := true + }, Res01_1), + %% test 'a' > undefined Sql02 = "select " " 'a' as a, a > b as c " @@ -2666,7 +2729,15 @@ t_sqlparse_compare_notnull_null(_Config) -> "from \"t/#\" ", {ok, Res05} = ?TEST_SQL(Sql05), ?assertMatch(#{<<"c">> := false - }, Res05). + }, Res05), + + %% test 'a' =~ undefined + Sql06 = "select " + " 'a' as a, a =~ b as c " + "from \"t/#\" ", + {ok, Res06} = ?TEST_SQL(Sql06), + ?assertMatch(#{<<"c">> := false + }, Res06). t_sqlparse_compare(_Config) -> Sql00 = "select " @@ -2676,6 +2747,13 @@ t_sqlparse_compare(_Config) -> ?assertMatch(#{<<"c">> := true }, Res00), + Sql00_1 = "select " + " 'true' as a, true as b, a = b as c " + "from \"t/#\" ", + {ok, Res00_1} = ?TEST_SQL(Sql00_1), + ?assertMatch(#{<<"c">> := true + }, Res00_1), + Sql01 = "select " " is_null(a) as c " "from \"t/#\" ", @@ -2704,7 +2782,21 @@ t_sqlparse_compare(_Config) -> ?assertMatch(#{<<"c">> := false }, Res04), - %% test 'a' >= undefined + Sql04_0 = "select " + " 1 as a, 1 as b, a = b as c " + "from \"t/#\" ", + {ok, Res04_0} = ?TEST_SQL(Sql04_0), + ?assertMatch(#{<<"c">> := true + }, Res04_0), + + Sql04_1 = "select " + " 1 as a, '1' as b, a = b as c " + "from \"t/#\" ", + {ok, Res04_1} = ?TEST_SQL(Sql04_1), + ?assertMatch(#{<<"c">> := true + }, Res04_1), + + %% test 1 >= 2 Sql05 = "select " " 1 as a, 2 as b, a >= b as c " "from \"t/#\" ", @@ -2712,13 +2804,37 @@ t_sqlparse_compare(_Config) -> ?assertMatch(#{<<"c">> := false }, Res05), - %% test 'a' >= undefined + %% test 1 <= 2 Sql06 = "select " " 1 as a, 2 as b, a <= b as c " "from \"t/#\" ", {ok, Res06} = ?TEST_SQL(Sql06), ?assertMatch(#{<<"c">> := true - }, Res06). + }, Res06), + + %% test 1 != 2 + Sql07 = "select " + " 1 as a, 2 as b, a != b as c " + "from \"t/#\" ", + {ok, Res07} = ?TEST_SQL(Sql07), + ?assertMatch(#{<<"c">> := true + }, Res07), + + %% test 1 <> 2 + Sql07_1 = "select " + " 1 as a, 2 as b, a <> b as c " + "from \"t/#\" ", + {ok, Res07_1} = ?TEST_SQL(Sql07_1), + ?assertMatch(#{<<"c">> := true + }, Res07_1), + + %% test 't' =~ 't' + Sql08 = "select " + " 't' as a, 't' as b, a =~ b as c " + "from \"t/#\" ", + {ok, Res08} = ?TEST_SQL(Sql08), + ?assertMatch(#{<<"c">> := true + }, Res08). t_sqlparse_new_map(_Config) -> %% construct a range without 'as' From b9d75181e5f74c0ecaae65776757a71052737e5c Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Thu, 8 Sep 2022 16:38:34 +0800 Subject: [PATCH 2/3] chore: update emqx_rule_engine.appup.src --- apps/emqx_rule_engine/src/emqx_rule_engine.appup.src | 2 ++ 1 file changed, 2 insertions(+) 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 b976a92bd..c24bead6e 100644 --- a/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src +++ b/apps/emqx_rule_engine/src/emqx_rule_engine.appup.src @@ -3,6 +3,7 @@ {VSN, [{"4.3.14", [{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}, + {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}, {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}]}, {"4.3.13", [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}, @@ -185,6 +186,7 @@ {<<".*">>,[]}], [{"4.3.14", [{load_module,emqx_rule_registry,brutal_purge,soft_purge,[]}, + {load_module,emqx_rule_runtime,brutal_purge,soft_purge,[]}, {load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}]}, {"4.3.13", [{load_module,emqx_rule_engine,brutal_purge,soft_purge,[]}, From 93d10e63b04ff3c1ada172997cb0ef519052f882 Mon Sep 17 00:00:00 2001 From: Shawn <506895667@qq.com> Date: Thu, 8 Sep 2022 16:53:24 +0800 Subject: [PATCH 3/3] chore: update CHANGES-4.3.md for #8927 --- CHANGES-4.3.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/CHANGES-4.3.md b/CHANGES-4.3.md index 9be7de63f..796efcfff 100644 --- a/CHANGES-4.3.md +++ b/CHANGES-4.3.md @@ -17,11 +17,11 @@ File format: - Fix rule-engine update behaviour which may initialize actions for disabled rules. [#8849](https://github.com/emqx/emqx/pull/8849) - Fix JWT plugin don't support non-integer timestamp claims. [#8862](https://github.com/emqx/emqx/pull/8862) - Fix dashboard binding IP address not working. [#8916](https://github.com/emqx/emqx/pull/8916) -- Fix rule SQL topic matching to null values failed. [#?](https://github.com/emqx/emqx/pull/?) +- Fix rule SQL topic matching to null values failed. [#8927](https://github.com/emqx/emqx/pull/8927) The following SQL should not fail (crash) but return `{"r": false}`: `SELECT topic =~ 't' as r FROM "$events/client_connected"`. The topic is a null value as there's no such field in event `$events/client_connected`, so it - should return false if mathch it to a topic. + should return false if match it to a topic. ## v4.3.19