From fb51c28559526acbe8e985d1e8398a18a1c3eb09 Mon Sep 17 00:00:00 2001 From: getlarge Date: Mon, 30 Dec 2024 17:33:40 +0100 Subject: [PATCH 01/25] test(rabbitmq_mqtt): add wildcard tests for retained messages in ETS store --- deps/rabbitmq_mqtt/test/retainer_SUITE.erl | 49 +++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl index 13d975335614..4146678ebb63 100644 --- a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl @@ -29,7 +29,7 @@ groups() -> sub_groups() -> [ {dets, [shuffle], tests()}, - {ets, [shuffle], tests()}, + {ets, [shuffle], tests() ++ wildcard_tests()}, {noop, [shuffle], [does_not_retain]} ]. @@ -43,6 +43,12 @@ tests() -> recover_with_message_expiry_interval ]. +%% Only run wildcard tests for ETS store +wildcard_tests() -> + [retained_wildcard_single_level, + retained_wildcard_multi_level, + retained_wildcard_mixed]. + suite() -> [{timetrap, {minutes, 2}}]. @@ -230,3 +236,44 @@ recover_with_message_expiry_interval(Config) -> end, ok = emqtt:disconnect(C2). + + +%% ------------------------------------------------------------------- +%% If a client publishes a retained message to devices/sensor1/temperature and another +%% client subscribes to devices/+/temperature the client should be +%% sent retained message for the translated topic (devices/sensor1/temperature) +%% ------------------------------------------------------------------- +%% +retained_wildcard_single_level(Config) -> + Topic = <<"devices/sensor1/temperature">>, + Pattern = <<"devices/+/temperature">>, + Payload = <<"23.5">>, + C = connect(<<"wildcardClientRetainer">>, Config, [{ack_timeout, 1}]), + {ok, _} = emqtt:publish(C, Topic, #{}, Payload, [{retain, true}, {qos, 1}]), + {ok, _, _} = emqtt:subscribe(C, Pattern, qos1), + ok = expect_publishes(C, Topic, [Payload]), + ok = emqtt:disconnect(C). + +%% Test multi-level wildcard (#) +retained_wildcard_multi_level(Config) -> + Topic = <<"devices/sensor1/readings/temperature">>, + Pattern = <<"devices/#">>, + Payload = <<"23.5">>, + + C = connect(<<"wildcardClientRetainer">>, Config, [{ack_timeout, 1}]), + {ok, _} = emqtt:publish(C, Topic, #{}, Payload, [{retain, true}, {qos, 1}]), + {ok, _, _} = emqtt:subscribe(C, Pattern, qos1), + ok = expect_publishes(C, Topic, [Payload]), + ok = emqtt:disconnect(C). + +% %% Test mixed wildcards (+/#) +retained_wildcard_mixed(Config) -> + Topic = <<"devices/sensor1/readings/temperature">>, + Pattern = <<"devices/+/readings/#">>, + Payload = <<"23.5">>, + + C = connect(<<"wildcardClientRetainer">>, Config, [{ack_timeout, 1}]), + {ok, _} = emqtt:publish(C, Topic, #{}, Payload, [{retain, true}, {qos, 1}]), + {ok, _, _} = emqtt:subscribe(C, Pattern, qos1), + ok = expect_publishes(C, Topic, [Payload]), + ok = emqtt:disconnect(C). From a48599bae7e869fe774eda11a241fd45ce194d45 Mon Sep 17 00:00:00 2001 From: getlarge Date: Thu, 9 Jan 2025 15:11:43 +0100 Subject: [PATCH 02/25] feat(rabbitmq_mqtt): add has_wildcards function to check for wildcard topics --- deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl index 838104b5f9a1..59ec80d9a4d2 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl @@ -22,7 +22,7 @@ -include("rabbit_mqtt_packet.hrl"). -include_lib("kernel/include/logger.hrl"). -export([start/1, insert/3, lookup/2, delete/2, terminate/1]). --export([expire/2]). +-export([expire/2, has_wildcards/1]). -export_type([state/0, expire/0]). -define(STATE, ?MODULE). @@ -89,6 +89,11 @@ lookup(Topic, #?STATE{store_mod = Mod, Other end. +-spec has_wildcards(topic()) -> boolean(). +has_wildcards(Pattern) -> + Parts = binary:split(Pattern, <<"/">>, [global]), + lists:member(<<"#">>, Parts) orelse lists:member(<<"+">>, Parts). + -spec delete(topic(), state()) -> ok. delete(Topic, #?STATE{store_mod = Mod, store_state = StoreState}) -> From c44392fe3ef4634c90a110bf8c2293240beb78f5 Mon Sep 17 00:00:00 2001 From: getlarge Date: Thu, 9 Jan 2025 15:20:22 +0100 Subject: [PATCH 03/25] feat(rabbitmq_mqtt): add rabbit_globber module for wildcard topic matching --- deps/rabbitmq_mqtt/app.bzl | 3 + deps/rabbitmq_mqtt/include/rabbit_globber.hrl | 7 + deps/rabbitmq_mqtt/src/rabbit_globber.erl | 166 ++++++++++++++++++ 3 files changed, 176 insertions(+) create mode 100644 deps/rabbitmq_mqtt/include/rabbit_globber.hrl create mode 100644 deps/rabbitmq_mqtt/src/rabbit_globber.erl diff --git a/deps/rabbitmq_mqtt/app.bzl b/deps/rabbitmq_mqtt/app.bzl index 86830f4f9c7a..4814419bdce6 100644 --- a/deps/rabbitmq_mqtt/app.bzl +++ b/deps/rabbitmq_mqtt/app.bzl @@ -19,6 +19,7 @@ def all_beam_files(name = "all_beam_files"): srcs = [ "src/Elixir.RabbitMQ.CLI.Ctl.Commands.ListMqttConnectionsCommand.erl", "src/mc_mqtt.erl", + "src/rabbit_globber.erl", "src/rabbit_mqtt.erl", "src/rabbit_mqtt_confirms.erl", "src/rabbit_mqtt_ff.erl", @@ -65,6 +66,7 @@ def all_test_beam_files(name = "all_test_beam_files"): srcs = [ "src/Elixir.RabbitMQ.CLI.Ctl.Commands.ListMqttConnectionsCommand.erl", "src/mc_mqtt.erl", + "src/rabbit_globber.erl", "src/rabbit_mqtt.erl", "src/rabbit_mqtt_confirms.erl", "src/rabbit_mqtt_ff.erl", @@ -118,6 +120,7 @@ def all_srcs(name = "all_srcs"): srcs = [ "src/Elixir.RabbitMQ.CLI.Ctl.Commands.ListMqttConnectionsCommand.erl", "src/mc_mqtt.erl", + "src/rabbit_globber.erl", "src/rabbit_mqtt.erl", "src/rabbit_mqtt_confirms.erl", "src/rabbit_mqtt_ff.erl", diff --git a/deps/rabbitmq_mqtt/include/rabbit_globber.hrl b/deps/rabbitmq_mqtt/include/rabbit_globber.hrl new file mode 100644 index 000000000000..bc3afb89d200 --- /dev/null +++ b/deps/rabbitmq_mqtt/include/rabbit_globber.hrl @@ -0,0 +1,7 @@ +-record(globber, + {separator = <<".">>, + wildcard_one = <<"*">>, + wildcard_some = <<"#">>, + trie = maps:new()}). + +-type globber() :: #globber{}. diff --git a/deps/rabbitmq_mqtt/src/rabbit_globber.erl b/deps/rabbitmq_mqtt/src/rabbit_globber.erl new file mode 100644 index 000000000000..ebe1c9bbee16 --- /dev/null +++ b/deps/rabbitmq_mqtt/src/rabbit_globber.erl @@ -0,0 +1,166 @@ +-module(rabbit_globber). + +-export([new/0, new/3, add/2, add/3, remove/2, remove/3, test/2, match/2, match_iter/2, + clear/1]). + +-include_lib("rabbit_globber.hrl"). + +-spec new() -> globber(). +new() -> + #globber{}. + +-spec new(binary(), binary(), binary()) -> globber(). +new(Separator, WildcardOne, WildcardSome) -> + #globber{separator = Separator, + wildcard_one = WildcardOne, + wildcard_some = WildcardSome, + trie = maps:new()}. + +-spec add(globber(), binary()) -> globber(). +add(Globber, Pattern) -> + add(Globber, Pattern, <<"match">>). + +-spec add(globber(), binary(), any()) -> globber(). +add(Globber, Pattern, Val) -> + Words = split(Pattern, Globber#globber.separator), + Trie = do_add(Words, Val, Globber#globber.trie), + Globber#globber{trie = Trie}. + +-spec remove(globber(), binary()) -> globber(). +remove(Globber, Pattern) -> + remove(Globber, Pattern, <<>>). + +-spec remove(globber(), binary(), any()) -> globber(). +remove(Globber, Pattern, Val) -> + Words = split(Pattern, Globber#globber.separator), + Trie = do_remove(Words, Val, Globber#globber.trie), + Globber#globber{trie = Trie}. + +-spec match(globber(), binary()) -> list(). +match(Globber, Pattern) -> + Words = split(Pattern, Globber#globber.separator), + try do_match(Words, Globber#globber.trie, [], Globber) of + Res -> + Res + catch + % error:badmatch and all + _:_ -> + undefined + end. + +-spec test(globber(), binary()) -> boolean(). +test(Globber, Pattern) -> + case match(Globber, Pattern) of + undefined -> + false; + [] -> + false; + _ -> + true + end. + +-spec match_iter(globber(), binary()) -> list(). +match_iter(Globber, Topic) -> + Words = split(Topic, Globber#globber.separator), + do_match_iter(Words, Globber#globber.trie, Globber). + +-spec clear(globber()) -> globber(). +clear(Globber) -> + Globber#globber{trie = maps:new()}. + +split(Topic, Separator) -> + binary:split(Topic, Separator, [global]). + +do_add([], Val, Trie) -> + maps:put(<<".">>, [Val | maps:get(<<".">>, Trie, [])], Trie); +do_add([Word | Rest], Val, Trie) -> + SubTrie = maps:get(Word, Trie, maps:new()), + NewSubTrie = do_add(Rest, Val, SubTrie), + maps:put(Word, NewSubTrie, Trie). + +do_remove([], Val, Trie) -> + case maps:get(<<".">>, Trie) of + Vals when is_list(Vals) -> + NewVals = lists:delete(Val, Vals), + if NewVals =:= [] -> + maps:remove(<<".">>, Trie); + true -> + maps:put(<<".">>, NewVals, Trie) + end; + _ -> + Trie + end; +do_remove([Word | Rest], Val, Trie) -> + case maps:get(Word, Trie) of + SubTrie when is_map(SubTrie) -> + NewSubTrie = do_remove(Rest, Val, SubTrie), + case maps:size(NewSubTrie) of + 0 -> + maps:remove(Word, Trie); + _ -> + maps:put(Word, NewSubTrie, Trie) + end; + _ -> + Trie + end. + +do_match([], Trie, Acc, _Globber) -> + case maps:get(<<".">>, Trie, undefined) of + undefined -> + Acc; + Vals -> + lists:append(Vals, Acc) + end; +do_match([Word | Rest], Trie, Acc, Globber) -> + SubTrie = + case maps:get(Word, Trie, undefined) of + undefined -> + maps:new(); + Sub -> + Sub + end, + SubTrie1 = + case maps:get(Globber#globber.wildcard_one, Trie, undefined) of + undefined -> + maps:new(); + Sub1 -> + Sub1 + end, + SubTrie2 = + case maps:get(Globber#globber.wildcard_some, Trie, undefined) of + undefined -> + maps:new(); + Sub2 -> + Sub2 + end, + Acc1 = do_match(Rest, SubTrie, Acc, Globber), + Acc2 = do_match(Rest, SubTrie1, Acc1, Globber), + do_match([], SubTrie2, Acc2, Globber). + +do_match_iter([], Trie, _Globber) -> + maps:get(<<".">>, Trie, []); +do_match_iter([Word | Rest], Trie, Globber) -> + SubTrie = + case maps:get(Word, Trie, undefined) of + undefined -> + maps:new(); + Sub -> + Sub + end, + SubTrie1 = + case maps:get(Globber#globber.wildcard_one, Trie, undefined) of + undefined -> + maps:new(); + Sub1 -> + Sub1 + end, + SubTrie2 = + case maps:get(Globber#globber.wildcard_some, Trie, undefined) of + undefined -> + maps:new(); + Sub2 -> + Sub2 + end, + lists:append([do_match_iter(Rest, SubTrie, Globber), + do_match_iter(Rest, SubTrie1, Globber), + do_match_iter([], SubTrie2, Globber)]). From 9be537f62caecc3cbd0aaa0f1f879d6c82dfad54 Mon Sep 17 00:00:00 2001 From: getlarge Date: Thu, 9 Jan 2025 15:21:05 +0100 Subject: [PATCH 04/25] test(rabbitmq_mqtt): add unit tests for rabbit_globber module functionality --- deps/rabbitmq_mqtt/test/globber_SUITE.erl | 66 +++++++++++++++++++++++ 1 file changed, 66 insertions(+) create mode 100644 deps/rabbitmq_mqtt/test/globber_SUITE.erl diff --git a/deps/rabbitmq_mqtt/test/globber_SUITE.erl b/deps/rabbitmq_mqtt/test/globber_SUITE.erl new file mode 100644 index 000000000000..8b2b0b5efcc3 --- /dev/null +++ b/deps/rabbitmq_mqtt/test/globber_SUITE.erl @@ -0,0 +1,66 @@ +-module(globber_SUITE). + +-include_lib("eunit/include/eunit.hrl"). +-include_lib("rabbit_globber.hrl"). + +new_test() -> + Globber = rabbit_globber:new(), + ?assertEqual(#globber{}, Globber), + Globber2 = rabbit_globber:new(<<"/">>, <<"*">>, <<"#">>), + ?assertEqual(#globber{separator = <<"/">>, wildcard_one = <<"*">>, wildcard_some = <<"#">>}, Globber2). + +add_test() -> + Globber = rabbit_globber:new(), + Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), + ?assertMatch(#globber{trie = _}, Globber1), + Globber2 = rabbit_globber:add(Globber1, <<"test.#">>, <<"it n">>), + ?assertMatch(#globber{trie = _}, Globber2). + +remove_test() -> + Globber = rabbit_globber:new(), + Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), + Globber2 = rabbit_globber:remove(Globber1, <<"test.*">>, <<"matches">>), + ?assertEqual(Globber, Globber2). + +match_test() -> + Globber = rabbit_globber:new(), + Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"it matches">>), + Result = rabbit_globber:match(Globber1, <<"test.bar">>), + ?assertEqual([<<"it matches">>], Result), + Result2 = rabbit_globber:match(Globber1, <<"test.foo">>), + ?assertEqual([<<"it matches">>], Result2), + Result3 = rabbit_globber:match(Globber1, <<"not.foo">>), + ?assertEqual([], Result3). + +test_test() -> + Globber = rabbit_globber:new(), + Globber1 = rabbit_globber:add(Globber, <<"test.*">>), + ?assertEqual(true, rabbit_globber:test(Globber1, <<"test.bar">>)), + ?assertEqual(false, rabbit_globber:test(Globber1, <<"foo.bar">>)). + +match_iter_test() -> + Globber = rabbit_globber:new(), + Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), + Result = rabbit_globber:match_iter(Globber1, <<"test.bar">>), + ?assertEqual([<<"matches">>], Result). + +clear_test() -> + Globber = rabbit_globber:new(), + Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), + Globber2 = rabbit_globber:clear(Globber1), + ?assertEqual(Globber, Globber2). + +multiple_patterns_test() -> + Globber = rabbit_globber:new(<<".">>, <<"*">>, <<"#">>), + Globber1 = rabbit_globber:add(Globber, <<"foo.#">>, <<"catchall">>), + Globber2 = rabbit_globber:add(Globber1, <<"foo.*.bar">>, <<"single_wildcard">>), + Globber3 = rabbit_globber:add(Globber2, <<"foo.*.bar.#">>, <<"single_and_catchall">>), + + ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar">>)), + ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar.baz">>)), + + ?assertEqual([<<"catchall">>,<<"single_wildcard">>], rabbit_globber:match(Globber3, <<"foo.test.bar">>)), + ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.test.baz.bar">>)), + + ?assertEqual([<<"catchall">>,<<"single_and_catchall">>], rabbit_globber:match(Globber3, <<"foo.test.bar.baz">>)), + ?assertEqual([<<"catchall">>,<<"single_and_catchall">>], rabbit_globber:match(Globber3, <<"foo.test.bar.baz.qux">>)). From 417a3a8450548ba3ffcd5d4da0acc7a8d84e728f Mon Sep 17 00:00:00 2001 From: getlarge Date: Fri, 10 Jan 2025 10:50:17 +0100 Subject: [PATCH 05/25] refactor(rabbitmq_mqtt): rename test functions for consistency and clarity --- deps/rabbitmq_mqtt/test/globber_SUITE.erl | 54 ++++++++++++++--------- 1 file changed, 34 insertions(+), 20 deletions(-) diff --git a/deps/rabbitmq_mqtt/test/globber_SUITE.erl b/deps/rabbitmq_mqtt/test/globber_SUITE.erl index 8b2b0b5efcc3..cc9f19e42a27 100644 --- a/deps/rabbitmq_mqtt/test/globber_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/globber_SUITE.erl @@ -1,28 +1,39 @@ -module(globber_SUITE). +-compile([export_all, nowarn_export_all]). + +-include_lib("rabbitmq_mqtt/include/rabbit_globber.hrl"). -include_lib("eunit/include/eunit.hrl"). --include_lib("rabbit_globber.hrl"). -new_test() -> +all() -> + [{group, tests}]. + +groups() -> + [{tests, [shuffle], [new]}]. + +new() -> Globber = rabbit_globber:new(), ?assertEqual(#globber{}, Globber), Globber2 = rabbit_globber:new(<<"/">>, <<"*">>, <<"#">>), - ?assertEqual(#globber{separator = <<"/">>, wildcard_one = <<"*">>, wildcard_some = <<"#">>}, Globber2). + ?assertEqual(#globber{separator = <<"/">>, + wildcard_one = <<"*">>, + wildcard_some = <<"#">>}, + Globber2). -add_test() -> +add() -> Globber = rabbit_globber:new(), Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), ?assertMatch(#globber{trie = _}, Globber1), Globber2 = rabbit_globber:add(Globber1, <<"test.#">>, <<"it n">>), ?assertMatch(#globber{trie = _}, Globber2). -remove_test() -> +remove() -> Globber = rabbit_globber:new(), Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), Globber2 = rabbit_globber:remove(Globber1, <<"test.*">>, <<"matches">>), ?assertEqual(Globber, Globber2). -match_test() -> +match() -> Globber = rabbit_globber:new(), Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"it matches">>), Result = rabbit_globber:match(Globber1, <<"test.bar">>), @@ -32,35 +43,38 @@ match_test() -> Result3 = rabbit_globber:match(Globber1, <<"not.foo">>), ?assertEqual([], Result3). -test_test() -> +test() -> Globber = rabbit_globber:new(), Globber1 = rabbit_globber:add(Globber, <<"test.*">>), ?assertEqual(true, rabbit_globber:test(Globber1, <<"test.bar">>)), ?assertEqual(false, rabbit_globber:test(Globber1, <<"foo.bar">>)). -match_iter_test() -> +match_iter() -> Globber = rabbit_globber:new(), Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), Result = rabbit_globber:match_iter(Globber1, <<"test.bar">>), ?assertEqual([<<"matches">>], Result). -clear_test() -> +clear() -> Globber = rabbit_globber:new(), Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), Globber2 = rabbit_globber:clear(Globber1), ?assertEqual(Globber, Globber2). -multiple_patterns_test() -> - Globber = rabbit_globber:new(<<".">>, <<"*">>, <<"#">>), - Globber1 = rabbit_globber:add(Globber, <<"foo.#">>, <<"catchall">>), - Globber2 = rabbit_globber:add(Globber1, <<"foo.*.bar">>, <<"single_wildcard">>), - Globber3 = rabbit_globber:add(Globber2, <<"foo.*.bar.#">>, <<"single_and_catchall">>), +multiple_patterns() -> + Globber = rabbit_globber:new(<<".">>, <<"*">>, <<"#">>), + Globber1 = rabbit_globber:add(Globber, <<"foo.#">>, <<"catchall">>), + Globber2 = rabbit_globber:add(Globber1, <<"foo.*.bar">>, <<"single_wildcard">>), + Globber3 = rabbit_globber:add(Globber2, <<"foo.*.bar.#">>, <<"single_and_catchall">>), - ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar">>)), - ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar.baz">>)), + ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar">>)), + ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar.baz">>)), - ?assertEqual([<<"catchall">>,<<"single_wildcard">>], rabbit_globber:match(Globber3, <<"foo.test.bar">>)), - ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.test.baz.bar">>)), + ?assertEqual([<<"catchall">>, <<"single_wildcard">>], + rabbit_globber:match(Globber3, <<"foo.test.bar">>)), + ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.test.baz.bar">>)), - ?assertEqual([<<"catchall">>,<<"single_and_catchall">>], rabbit_globber:match(Globber3, <<"foo.test.bar.baz">>)), - ?assertEqual([<<"catchall">>,<<"single_and_catchall">>], rabbit_globber:match(Globber3, <<"foo.test.bar.baz.qux">>)). + ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], + rabbit_globber:match(Globber3, <<"foo.test.bar.baz">>)), + ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], + rabbit_globber:match(Globber3, <<"foo.test.bar.baz.qux">>)). From d6fb829112077525103faba8ae974ba0b1305112 Mon Sep 17 00:00:00 2001 From: getlarge Date: Fri, 10 Jan 2025 10:50:44 +0100 Subject: [PATCH 06/25] chore(rabbitmq_mqtt): update build config --- deps/rabbitmq_mqtt/BUILD.bazel | 5 +++++ deps/rabbitmq_mqtt/Makefile | 2 +- deps/rabbitmq_mqtt/app.bzl | 10 ++++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/deps/rabbitmq_mqtt/BUILD.bazel b/deps/rabbitmq_mqtt/BUILD.bazel index 49853b99a788..1790421bbe9b 100644 --- a/deps/rabbitmq_mqtt/BUILD.bazel +++ b/deps/rabbitmq_mqtt/BUILD.bazel @@ -301,6 +301,11 @@ rabbitmq_suite( ], ) +rabbitmq_suite( + name = "globber_SUITE", + size = "small" +) + assert_suites() alias( diff --git a/deps/rabbitmq_mqtt/Makefile b/deps/rabbitmq_mqtt/Makefile index feb46e65b5c1..c47b7c5670c0 100644 --- a/deps/rabbitmq_mqtt/Makefile +++ b/deps/rabbitmq_mqtt/Makefile @@ -94,7 +94,7 @@ define ct_master.erl halt(0) endef -PARALLEL_CT_SET_1_A = auth retainer +PARALLEL_CT_SET_1_A = auth globber retainer PARALLEL_CT_SET_1_B = cluster command config config_schema mc_mqtt packet_prop \ processor protocol_interop proxy_protocol rabbit_mqtt_confirms reader util PARALLEL_CT_SET_1_C = java v5 diff --git a/deps/rabbitmq_mqtt/app.bzl b/deps/rabbitmq_mqtt/app.bzl index 4814419bdce6..f63ac447f746 100644 --- a/deps/rabbitmq_mqtt/app.bzl +++ b/deps/rabbitmq_mqtt/app.bzl @@ -143,6 +143,7 @@ def all_srcs(name = "all_srcs"): filegroup( name = "public_hdrs", srcs = [ + "include/rabbit_globber.hrl", "include/rabbit_mqtt.hrl", "include/rabbit_mqtt_packet.hrl", ], @@ -332,3 +333,12 @@ def test_suite_beam_files(name = "test_suite_beam_files"): erlc_opts = "//:test_erlc_opts", deps = ["//deps/amqp_client:erlang_app", "//deps/rabbitmq_ct_helpers:erlang_app"], ) + erlang_bytecode( + name = "globber_SUITE_beam_files", + testonly = True, + srcs = ["test/globber_SUITE.erl"], + outs = ["test/globber_SUITE.beam"], + hdrs = ["include/rabbit_globber.hrl"], + app_name = "rabbitmq_mqtt", + erlc_opts = "//:test_erlc_opts", + ) From b01787555ddf9e5344c10f10e227bc9700637f3d Mon Sep 17 00:00:00 2001 From: getlarge Date: Mon, 13 Jan 2025 13:50:41 +0100 Subject: [PATCH 07/25] chore: rename rabbit_globber to rabbit_mqtt_topic_matcher --- deps/rabbitmq_mqtt/include/rabbit_globber.hrl | 7 -- .../include/rabbit_mqtt_topic_matcher.hrl | 13 +++ ...bber.erl => rabbit_mqtt_topic_matcher.erl} | 10 +- deps/rabbitmq_mqtt/test/globber_SUITE.erl | 80 ---------------- .../test/rabbit_mqtt_topic_matcher_SUITE.erl | 91 +++++++++++++++++++ 5 files changed, 112 insertions(+), 89 deletions(-) delete mode 100644 deps/rabbitmq_mqtt/include/rabbit_globber.hrl create mode 100644 deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl rename deps/rabbitmq_mqtt/src/{rabbit_globber.erl => rabbit_mqtt_topic_matcher.erl} (91%) delete mode 100644 deps/rabbitmq_mqtt/test/globber_SUITE.erl create mode 100644 deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl diff --git a/deps/rabbitmq_mqtt/include/rabbit_globber.hrl b/deps/rabbitmq_mqtt/include/rabbit_globber.hrl deleted file mode 100644 index bc3afb89d200..000000000000 --- a/deps/rabbitmq_mqtt/include/rabbit_globber.hrl +++ /dev/null @@ -1,7 +0,0 @@ --record(globber, - {separator = <<".">>, - wildcard_one = <<"*">>, - wildcard_some = <<"#">>, - trie = maps:new()}). - --type globber() :: #globber{}. diff --git a/deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl b/deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl new file mode 100644 index 000000000000..18619d54bcc5 --- /dev/null +++ b/deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl @@ -0,0 +1,13 @@ +%% This Source Code Form is subject to the terms of the Mozilla Public +%% License, v. 2.0. If a copy of the MPL was not distributed with this +%% file, You can obtain one at https://mozilla.org/MPL/2.0/. +%% +%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. + +-record(globber, + {separator = <<"/">>, + wildcard_one = <<"+">>, + wildcard_some = <<"#">>, + trie = maps:new()}). + +-type globber() :: #globber{}. diff --git a/deps/rabbitmq_mqtt/src/rabbit_globber.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_topic_matcher.erl similarity index 91% rename from deps/rabbitmq_mqtt/src/rabbit_globber.erl rename to deps/rabbitmq_mqtt/src/rabbit_mqtt_topic_matcher.erl index ebe1c9bbee16..e526127f6981 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_globber.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_topic_matcher.erl @@ -1,9 +1,15 @@ --module(rabbit_globber). +%% This Source Code Form is subject to the terms of the Mozilla Public +%% License, v. 2.0. If a copy of the MPL was not distributed with this +%% file, You can obtain one at https://mozilla.org/MPL/2.0/. +%% +%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. + +-module(rabbit_mqtt_topic_matcher). -export([new/0, new/3, add/2, add/3, remove/2, remove/3, test/2, match/2, match_iter/2, clear/1]). --include_lib("rabbit_globber.hrl"). +-include_lib("rabbit_mqtt_topic_matcher.hrl"). -spec new() -> globber(). new() -> diff --git a/deps/rabbitmq_mqtt/test/globber_SUITE.erl b/deps/rabbitmq_mqtt/test/globber_SUITE.erl deleted file mode 100644 index cc9f19e42a27..000000000000 --- a/deps/rabbitmq_mqtt/test/globber_SUITE.erl +++ /dev/null @@ -1,80 +0,0 @@ --module(globber_SUITE). - --compile([export_all, nowarn_export_all]). - --include_lib("rabbitmq_mqtt/include/rabbit_globber.hrl"). --include_lib("eunit/include/eunit.hrl"). - -all() -> - [{group, tests}]. - -groups() -> - [{tests, [shuffle], [new]}]. - -new() -> - Globber = rabbit_globber:new(), - ?assertEqual(#globber{}, Globber), - Globber2 = rabbit_globber:new(<<"/">>, <<"*">>, <<"#">>), - ?assertEqual(#globber{separator = <<"/">>, - wildcard_one = <<"*">>, - wildcard_some = <<"#">>}, - Globber2). - -add() -> - Globber = rabbit_globber:new(), - Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), - ?assertMatch(#globber{trie = _}, Globber1), - Globber2 = rabbit_globber:add(Globber1, <<"test.#">>, <<"it n">>), - ?assertMatch(#globber{trie = _}, Globber2). - -remove() -> - Globber = rabbit_globber:new(), - Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), - Globber2 = rabbit_globber:remove(Globber1, <<"test.*">>, <<"matches">>), - ?assertEqual(Globber, Globber2). - -match() -> - Globber = rabbit_globber:new(), - Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"it matches">>), - Result = rabbit_globber:match(Globber1, <<"test.bar">>), - ?assertEqual([<<"it matches">>], Result), - Result2 = rabbit_globber:match(Globber1, <<"test.foo">>), - ?assertEqual([<<"it matches">>], Result2), - Result3 = rabbit_globber:match(Globber1, <<"not.foo">>), - ?assertEqual([], Result3). - -test() -> - Globber = rabbit_globber:new(), - Globber1 = rabbit_globber:add(Globber, <<"test.*">>), - ?assertEqual(true, rabbit_globber:test(Globber1, <<"test.bar">>)), - ?assertEqual(false, rabbit_globber:test(Globber1, <<"foo.bar">>)). - -match_iter() -> - Globber = rabbit_globber:new(), - Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), - Result = rabbit_globber:match_iter(Globber1, <<"test.bar">>), - ?assertEqual([<<"matches">>], Result). - -clear() -> - Globber = rabbit_globber:new(), - Globber1 = rabbit_globber:add(Globber, <<"test.*">>, <<"matches">>), - Globber2 = rabbit_globber:clear(Globber1), - ?assertEqual(Globber, Globber2). - -multiple_patterns() -> - Globber = rabbit_globber:new(<<".">>, <<"*">>, <<"#">>), - Globber1 = rabbit_globber:add(Globber, <<"foo.#">>, <<"catchall">>), - Globber2 = rabbit_globber:add(Globber1, <<"foo.*.bar">>, <<"single_wildcard">>), - Globber3 = rabbit_globber:add(Globber2, <<"foo.*.bar.#">>, <<"single_and_catchall">>), - - ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar">>)), - ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.bar.baz">>)), - - ?assertEqual([<<"catchall">>, <<"single_wildcard">>], - rabbit_globber:match(Globber3, <<"foo.test.bar">>)), - ?assertEqual([<<"catchall">>], rabbit_globber:match(Globber3, <<"foo.test.baz.bar">>)), - - ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], - rabbit_globber:match(Globber3, <<"foo.test.bar.baz">>)), - ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], - rabbit_globber:match(Globber3, <<"foo.test.bar.baz.qux">>)). diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl new file mode 100644 index 000000000000..66a5b9e33172 --- /dev/null +++ b/deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl @@ -0,0 +1,91 @@ +%% This Source Code Form is subject to the terms of the Mozilla Public +%% License, v. 2.0. If a copy of the MPL was not distributed with this +%% file, You can obtain one at https://mozilla.org/MPL/2.0/. +%% +%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. +-module(rabbit_mqtt_topic_matcher_SUITE). + +-compile([export_all, nowarn_export_all]). + +-include_lib("rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl"). +-include_lib("eunit/include/eunit.hrl"). + +all() -> + [{group, tests}]. + +groups() -> + [{tests, + [parallel], + [new, add, remove, match, pattern_is_matching, match_iter, clear, multiple_patterns]}]. + +new(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(), + ?assertEqual(#globber{}, Globber), + Globber2 = rabbit_mqtt_topic_matcher:new(<<".">>, <<"+">>, <<"#">>), + ?assertEqual(#globber{separator = <<".">>, + wildcard_one = <<"+">>, + wildcard_some = <<"#">>}, + Globber2). + +add(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(), + Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), + ?assertMatch(#globber{trie = _}, Globber1), + Globber2 = rabbit_mqtt_topic_matcher:add(Globber1, <<"test/#">>, <<"it n">>), + ?assertMatch(#globber{trie = _}, Globber2). + +remove(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(), + Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), + Globber2 = rabbit_mqtt_topic_matcher:remove(Globber1, <<"test/+">>, <<"matches">>), + ?assertEqual(Globber, Globber2). + +match(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(), + Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"it matches">>), + Result = rabbit_mqtt_topic_matcher:match(Globber1, <<"test/bar">>), + ?assertEqual([<<"it matches">>], Result), + Result2 = rabbit_mqtt_topic_matcher:match(Globber1, <<"test/foo">>), + ?assertEqual([<<"it matches">>], Result2), + Result3 = rabbit_mqtt_topic_matcher:match(Globber1, <<"not/foo">>), + ?assertEqual([], Result3). + +pattern_is_matching(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(), + Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>), + ?assertEqual(true, rabbit_mqtt_topic_matcher:test(Globber1, <<"test/bar">>)), + ?assertEqual(false, rabbit_mqtt_topic_matcher:test(Globber1, <<"foo/bar">>)). + +match_iter(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(), + Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), + Result = rabbit_mqtt_topic_matcher:match_iter(Globber1, <<"test/bar">>), + ?assertEqual([<<"matches">>], Result). + +clear(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(), + Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), + Globber2 = rabbit_mqtt_topic_matcher:clear(Globber1), + ?assertEqual(Globber, Globber2). + +multiple_patterns(_Config) -> + Globber = rabbit_mqtt_topic_matcher:new(<<".">>, <<"*">>, <<"#">>), + Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"foo.#">>, <<"catchall">>), + Globber2 = + rabbit_mqtt_topic_matcher:add(Globber1, <<"foo.*.bar">>, <<"single_wildcard">>), + Globber3 = + rabbit_mqtt_topic_matcher:add(Globber2, <<"foo.*.bar.#">>, <<"single_and_catchall">>), + + ?assertEqual([<<"catchall">>], rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.bar">>)), + ?assertEqual([<<"catchall">>], + rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.bar.baz">>)), + + ?assertEqual([<<"catchall">>, <<"single_wildcard">>], + rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.bar">>)), + ?assertEqual([<<"catchall">>], + rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.baz.bar">>)), + + ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], + rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.bar.baz">>)), + ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], + rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.bar.baz.qux">>)). From e16e709ce64168e2a21f1ad2b75eaad874b81980 Mon Sep 17 00:00:00 2001 From: getlarge Date: Mon, 13 Jan 2025 13:51:56 +0100 Subject: [PATCH 08/25] chore: run `bazel run gazelle` --- deps/rabbit/BUILD.bazel | 5 +++-- deps/rabbit/app.bzl | 10 +++++----- .../BUILD.bazel | 4 ++-- deps/rabbitmq_consistent_hash_exchange/app.bzl | 8 ++++---- deps/rabbitmq_ct_client_helpers/BUILD.bazel | 1 + deps/rabbitmq_ct_client_helpers/MODULE.bazel | 6 ++++++ deps/rabbitmq_ct_helpers/BUILD.bazel | 6 ++++++ deps/rabbitmq_jms_topic_exchange/BUILD.bazel | 4 ++-- deps/rabbitmq_jms_topic_exchange/app.bzl | 8 ++++---- deps/rabbitmq_mqtt/BUILD.bazel | 2 +- deps/rabbitmq_mqtt/app.bzl | 16 ++++++++-------- deps/rabbitmq_prelaunch/BUILD.bazel | 4 ++++ .../rabbitmq_recent_history_exchange/BUILD.bazel | 4 ++-- deps/rabbitmq_recent_history_exchange/app.bzl | 8 ++++---- deps/rabbitmq_shovel/app.bzl | 2 +- deps/rabbitmq_stream_common/BUILD.bazel | 1 + moduleindex.yaml | 1 + 17 files changed, 55 insertions(+), 35 deletions(-) create mode 100644 deps/rabbitmq_ct_client_helpers/MODULE.bazel diff --git a/deps/rabbit/BUILD.bazel b/deps/rabbit/BUILD.bazel index 68d5f16da884..7a8170dbd37d 100644 --- a/deps/rabbit/BUILD.bazel +++ b/deps/rabbit/BUILD.bazel @@ -198,17 +198,18 @@ rabbitmq_app( "syntax_tools", "xmerl", "crypto", + "horus", ], license_files = [":license_files"], priv = [":priv"], deps = [ "//deps/amqp10_common:erlang_app", + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit_common:erlang_app", "//deps/rabbitmq_prelaunch:erlang_app", "@cuttlefish//:erlang_app", "@gen_batch_server//:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", "@observer_cli//:erlang_app", "@osiris//:erlang_app", "@ra//:erlang_app", diff --git a/deps/rabbit/app.bzl b/deps/rabbit/app.bzl index 1708c6af457d..eaaa32e2adbd 100644 --- a/deps/rabbit/app.bzl +++ b/deps/rabbit/app.bzl @@ -248,9 +248,9 @@ def all_beam_files(name = "all_beam_files"): erlc_opts = "//:erlc_opts", deps = [ "//deps/amqp10_common:erlang_app", + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", "@ra//:erlang_app", "@ranch//:erlang_app", "@stdout_formatter//:erlang_app", @@ -507,9 +507,9 @@ def all_test_beam_files(name = "all_test_beam_files"): erlc_opts = "//:test_erlc_opts", deps = [ "//deps/amqp10_common:erlang_app", + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", "@ra//:erlang_app", "@ranch//:erlang_app", "@stdout_formatter//:erlang_app", @@ -2021,7 +2021,7 @@ def test_suite_beam_files(name = "test_suite_beam_files"): outs = ["test/metadata_store_phase1_SUITE.beam"], app_name = "rabbit", erlc_opts = "//:test_erlc_opts", - deps = ["//deps/rabbit_common:erlang_app", "@khepri//:erlang_app"], + deps = ["//deps/khepri:erlang_app", "//deps/rabbit_common:erlang_app"], ) erlang_bytecode( name = "mc_unit_SUITE_beam_files", diff --git a/deps/rabbitmq_consistent_hash_exchange/BUILD.bazel b/deps/rabbitmq_consistent_hash_exchange/BUILD.bazel index 182b31c0656f..a0ab36fa6d9c 100644 --- a/deps/rabbitmq_consistent_hash_exchange/BUILD.bazel +++ b/deps/rabbitmq_consistent_hash_exchange/BUILD.bazel @@ -39,10 +39,10 @@ rabbitmq_app( license_files = [":license_files"], priv = [":priv"], deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) diff --git a/deps/rabbitmq_consistent_hash_exchange/app.bzl b/deps/rabbitmq_consistent_hash_exchange/app.bzl index e6a43a75079f..16aa9bc1838c 100644 --- a/deps/rabbitmq_consistent_hash_exchange/app.bzl +++ b/deps/rabbitmq_consistent_hash_exchange/app.bzl @@ -19,11 +19,11 @@ def all_beam_files(name = "all_beam_files"): dest = "ebin", erlc_opts = "//:erlc_opts", deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", "//deps/rabbitmq_cli:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) @@ -47,11 +47,11 @@ def all_test_beam_files(name = "all_test_beam_files"): dest = "test", erlc_opts = "//:test_erlc_opts", deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", "//deps/rabbitmq_cli:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) diff --git a/deps/rabbitmq_ct_client_helpers/BUILD.bazel b/deps/rabbitmq_ct_client_helpers/BUILD.bazel index 8fa9dfa34f41..1141dd990501 100644 --- a/deps/rabbitmq_ct_client_helpers/BUILD.bazel +++ b/deps/rabbitmq_ct_client_helpers/BUILD.bazel @@ -33,6 +33,7 @@ rabbitmq_app( hdrs = [":public_hdrs"], app_name = "rabbitmq_ct_client_helpers", beam_files = [":beam_files"], + extra_apps = ["rabbit_common"], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_ct_client_helpers/MODULE.bazel b/deps/rabbitmq_ct_client_helpers/MODULE.bazel new file mode 100644 index 000000000000..00bb18361f7f --- /dev/null +++ b/deps/rabbitmq_ct_client_helpers/MODULE.bazel @@ -0,0 +1,6 @@ +############################################################################### +# Bazel now uses Bzlmod by default to manage external dependencies. +# Please consider migrating your external dependencies from WORKSPACE to MODULE.bazel. +# +# For more details, please check https://github.com/bazelbuild/bazel/issues/18958 +############################################################################### diff --git a/deps/rabbitmq_ct_helpers/BUILD.bazel b/deps/rabbitmq_ct_helpers/BUILD.bazel index b5167a076972..4c90b5e887de 100644 --- a/deps/rabbitmq_ct_helpers/BUILD.bazel +++ b/deps/rabbitmq_ct_helpers/BUILD.bazel @@ -39,6 +39,12 @@ rabbitmq_app( hdrs = [":public_hdrs"], app_name = "rabbitmq_ct_helpers", beam_files = [":beam_files"], + extra_apps = [ + "common_test", + "eunit", + "inet_tcp_proxy", + "inets", + ], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_jms_topic_exchange/BUILD.bazel b/deps/rabbitmq_jms_topic_exchange/BUILD.bazel index e3e49612b060..2366725c8b7c 100644 --- a/deps/rabbitmq_jms_topic_exchange/BUILD.bazel +++ b/deps/rabbitmq_jms_topic_exchange/BUILD.bazel @@ -44,10 +44,10 @@ rabbitmq_app( license_files = [":license_files"], priv = [":priv"], deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) diff --git a/deps/rabbitmq_jms_topic_exchange/app.bzl b/deps/rabbitmq_jms_topic_exchange/app.bzl index 5c73214ef386..879a8434e170 100644 --- a/deps/rabbitmq_jms_topic_exchange/app.bzl +++ b/deps/rabbitmq_jms_topic_exchange/app.bzl @@ -19,10 +19,10 @@ def all_beam_files(name = "all_beam_files"): dest = "ebin", erlc_opts = "//:erlc_opts", deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) @@ -46,10 +46,10 @@ def all_test_beam_files(name = "all_test_beam_files"): dest = "test", erlc_opts = "//:test_erlc_opts", deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) diff --git a/deps/rabbitmq_mqtt/BUILD.bazel b/deps/rabbitmq_mqtt/BUILD.bazel index 1790421bbe9b..ec37f0556fa1 100644 --- a/deps/rabbitmq_mqtt/BUILD.bazel +++ b/deps/rabbitmq_mqtt/BUILD.bazel @@ -303,7 +303,7 @@ rabbitmq_suite( rabbitmq_suite( name = "globber_SUITE", - size = "small" + size = "small", ) assert_suites() diff --git a/deps/rabbitmq_mqtt/app.bzl b/deps/rabbitmq_mqtt/app.bzl index f63ac447f746..a6174d43a8ae 100644 --- a/deps/rabbitmq_mqtt/app.bzl +++ b/deps/rabbitmq_mqtt/app.bzl @@ -19,7 +19,6 @@ def all_beam_files(name = "all_beam_files"): srcs = [ "src/Elixir.RabbitMQ.CLI.Ctl.Commands.ListMqttConnectionsCommand.erl", "src/mc_mqtt.erl", - "src/rabbit_globber.erl", "src/rabbit_mqtt.erl", "src/rabbit_mqtt_confirms.erl", "src/rabbit_mqtt_ff.erl", @@ -35,6 +34,7 @@ def all_beam_files(name = "all_beam_files"): "src/rabbit_mqtt_retainer.erl", "src/rabbit_mqtt_retainer_sup.erl", "src/rabbit_mqtt_sup.erl", + "src/rabbit_mqtt_topic_matcher.erl", "src/rabbit_mqtt_util.erl", ], hdrs = [":public_and_private_hdrs"], @@ -66,7 +66,6 @@ def all_test_beam_files(name = "all_test_beam_files"): srcs = [ "src/Elixir.RabbitMQ.CLI.Ctl.Commands.ListMqttConnectionsCommand.erl", "src/mc_mqtt.erl", - "src/rabbit_globber.erl", "src/rabbit_mqtt.erl", "src/rabbit_mqtt_confirms.erl", "src/rabbit_mqtt_ff.erl", @@ -82,6 +81,7 @@ def all_test_beam_files(name = "all_test_beam_files"): "src/rabbit_mqtt_retainer.erl", "src/rabbit_mqtt_retainer_sup.erl", "src/rabbit_mqtt_sup.erl", + "src/rabbit_mqtt_topic_matcher.erl", "src/rabbit_mqtt_util.erl", ], hdrs = [":public_and_private_hdrs"], @@ -120,7 +120,6 @@ def all_srcs(name = "all_srcs"): srcs = [ "src/Elixir.RabbitMQ.CLI.Ctl.Commands.ListMqttConnectionsCommand.erl", "src/mc_mqtt.erl", - "src/rabbit_globber.erl", "src/rabbit_mqtt.erl", "src/rabbit_mqtt_confirms.erl", "src/rabbit_mqtt_ff.erl", @@ -137,15 +136,16 @@ def all_srcs(name = "all_srcs"): "src/rabbit_mqtt_retainer.erl", "src/rabbit_mqtt_retainer_sup.erl", "src/rabbit_mqtt_sup.erl", + "src/rabbit_mqtt_topic_matcher.erl", "src/rabbit_mqtt_util.erl", ], ) filegroup( name = "public_hdrs", srcs = [ - "include/rabbit_globber.hrl", "include/rabbit_mqtt.hrl", "include/rabbit_mqtt_packet.hrl", + "include/rabbit_mqtt_topic_matcher.hrl", ], ) filegroup( @@ -334,11 +334,11 @@ def test_suite_beam_files(name = "test_suite_beam_files"): deps = ["//deps/amqp_client:erlang_app", "//deps/rabbitmq_ct_helpers:erlang_app"], ) erlang_bytecode( - name = "globber_SUITE_beam_files", + name = "rabbit_mqtt_topic_matcher_SUITE_beam_files", testonly = True, - srcs = ["test/globber_SUITE.erl"], - outs = ["test/globber_SUITE.beam"], - hdrs = ["include/rabbit_globber.hrl"], + srcs = ["test/rabbit_mqtt_topic_matcher_SUITE.erl"], + outs = ["test/rabbit_mqtt_topic_matcher_SUITE.beam"], + hdrs = ["include/rabbit_mqtt_topic_matcher.hrl"], app_name = "rabbitmq_mqtt", erlc_opts = "//:test_erlc_opts", ) diff --git a/deps/rabbitmq_prelaunch/BUILD.bazel b/deps/rabbitmq_prelaunch/BUILD.bazel index f9cd5eda7280..bf80ecf11e63 100644 --- a/deps/rabbitmq_prelaunch/BUILD.bazel +++ b/deps/rabbitmq_prelaunch/BUILD.bazel @@ -42,6 +42,10 @@ rabbitmq_app( app_name = APP_NAME, app_version = APP_VERSION, beam_files = [":beam_files"], + extra_apps = [ + "osiris", + "systemd", + ], license_files = [":license_files"], priv = [":priv"], deps = [ diff --git a/deps/rabbitmq_recent_history_exchange/BUILD.bazel b/deps/rabbitmq_recent_history_exchange/BUILD.bazel index 73121ad44906..9954ab0e1573 100644 --- a/deps/rabbitmq_recent_history_exchange/BUILD.bazel +++ b/deps/rabbitmq_recent_history_exchange/BUILD.bazel @@ -39,10 +39,10 @@ rabbitmq_app( license_files = [":license_files"], priv = [":priv"], deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) diff --git a/deps/rabbitmq_recent_history_exchange/app.bzl b/deps/rabbitmq_recent_history_exchange/app.bzl index 3bd05fe8ae54..606dabe4b310 100644 --- a/deps/rabbitmq_recent_history_exchange/app.bzl +++ b/deps/rabbitmq_recent_history_exchange/app.bzl @@ -18,10 +18,10 @@ def all_beam_files(name = "all_beam_files"): dest = "ebin", erlc_opts = "//:erlc_opts", deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) @@ -44,10 +44,10 @@ def all_test_beam_files(name = "all_test_beam_files"): dest = "test", erlc_opts = "//:test_erlc_opts", deps = [ + "//deps/khepri:erlang_app", + "//deps/khepri_mnesia_migration:erlang_app", "//deps/rabbit:erlang_app", "//deps/rabbit_common:erlang_app", - "@khepri//:erlang_app", - "@khepri_mnesia_migration//:erlang_app", ], ) diff --git a/deps/rabbitmq_shovel/app.bzl b/deps/rabbitmq_shovel/app.bzl index 509242770a22..d3cb55095742 100644 --- a/deps/rabbitmq_shovel/app.bzl +++ b/deps/rabbitmq_shovel/app.bzl @@ -232,7 +232,7 @@ def test_suite_beam_files(name = "test_suite_beam_files"): outs = ["test/rolling_upgrade_SUITE.beam"], app_name = "rabbitmq_shovel", erlc_opts = "//:test_erlc_opts", - deps = ["//deps/amqp_client:erlang_app", "@khepri//:erlang_app"], + deps = ["//deps/amqp_client:erlang_app", "//deps/khepri:erlang_app"], ) erlang_bytecode( name = "shovel_status_command_SUITE_beam_files", diff --git a/deps/rabbitmq_stream_common/BUILD.bazel b/deps/rabbitmq_stream_common/BUILD.bazel index ec030f85a9ce..fe6d956b249e 100644 --- a/deps/rabbitmq_stream_common/BUILD.bazel +++ b/deps/rabbitmq_stream_common/BUILD.bazel @@ -35,6 +35,7 @@ rabbitmq_app( app_description = APP_DESCRIPTION, app_name = APP_NAME, beam_files = [":beam_files"], + extra_apps = ["osiris"], license_files = [":license_files"], priv = [":priv"], ) diff --git a/moduleindex.yaml b/moduleindex.yaml index eca29bb4f6eb..f0b5deed82d9 100755 --- a/moduleindex.yaml +++ b/moduleindex.yaml @@ -1056,6 +1056,7 @@ rabbitmq_mqtt: - rabbit_mqtt_retainer - rabbit_mqtt_retainer_sup - rabbit_mqtt_sup +- rabbit_mqtt_topic_matcher - rabbit_mqtt_util rabbitmq_peer_discovery_aws: - rabbit_peer_discovery_aws From 08b14604fdacb73dd8e446a9c4fc01b258b5e2af Mon Sep 17 00:00:00 2001 From: getlarge Date: Fri, 17 Jan 2025 20:03:12 +0100 Subject: [PATCH 09/25] refactor(rabbitmq_mqtt): add retained message ets store structure and functionality --- deps/rabbitmq_mqtt/app.bzl | 8 + .../rabbit_mqtt_retained_msg_store_ets.erl | 294 ++++++++++++++++-- ...bbit_mqtt_retained_msg_store_ets_SUITE.erl | 98 ++++++ 3 files changed, 366 insertions(+), 34 deletions(-) create mode 100644 deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl diff --git a/deps/rabbitmq_mqtt/app.bzl b/deps/rabbitmq_mqtt/app.bzl index a6174d43a8ae..acdc6f755537 100644 --- a/deps/rabbitmq_mqtt/app.bzl +++ b/deps/rabbitmq_mqtt/app.bzl @@ -342,3 +342,11 @@ def test_suite_beam_files(name = "test_suite_beam_files"): app_name = "rabbitmq_mqtt", erlc_opts = "//:test_erlc_opts", ) + erlang_bytecode( + name = "rabbit_mqtt_retained_msg_store_ets_SUITE_beam_files", + testonly = True, + srcs = ["test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl"], + outs = ["test/rabbit_mqtt_retained_msg_store_ets_SUITE.beam"], + app_name = "rabbitmq_mqtt", + erlc_opts = "//:test_erlc_opts", + ) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl index 70a770ff526a..d83ef359bef0 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl @@ -13,53 +13,279 @@ -export([new/2, recover/2, insert/3, lookup/2, delete/2, terminate/1]). --record(store_state, { - table :: ets:tid(), - filename :: file:filename_all() - }). +% TODO: -define(DEFAULT_MATCH_LIMIT, 1000). + +-record(store_state, + {node_table :: ets:tid(), % Stores {node_id, edge_count, is_topic} + edge_table :: ets:tid(), % Stores {{from_id, word}, to_id} + msg_table :: ets:tid(), % Stores {node_id, topic, mqtt_msg} + root_id :: binary(), % Root node ID + dir :: file:filename_all(), + vhost :: rabbit_types:vhost()}). -type store_state() :: #store_state{}. -spec new(file:name_all(), rabbit_types:vhost()) -> store_state(). new(Dir, VHost) -> - Path = rabbit_mqtt_util:path_for(Dir, VHost), - TableName = rabbit_mqtt_util:vhost_name_to_table_name(VHost), - _ = file:delete(Path), - Tid = ets:new(TableName, [set, public, {keypos, #retained_message.topic}]), - #store_state{table = Tid, filename = Path}. + BaseTableName = rabbit_mqtt_util:vhost_name_to_table_name(VHost), + + % Clean up any existing files + delete_table_files(Dir, VHost), + + % Node table - will store tuples of {node_id, edge_count, is_topic} + NodeTable = ets:new(append_suffix(BaseTableName, "_nodes"), [set, public]), + % Edge table - will store {{from_id, word}, to_id} + EdgeTable = ets:new(append_suffix(BaseTableName, "_edges"), [ordered_set, public]), + % Topic table - will store {node_id, topic, value} + % TODO: consider whether, set might still be the best option here + MsgTable = ets:new(append_suffix(BaseTableName, "_msgs"), [bag, public]), + + RootId = make_node_id(), + ets:insert(NodeTable, {RootId, 0, false}), + + #store_state{node_table = NodeTable, + edge_table = EdgeTable, + msg_table = MsgTable, + root_id = RootId, + dir = Dir, + vhost = VHost}. -spec recover(file:name_all(), rabbit_types:vhost()) -> - {ok, store_state(), rabbit_mqtt_retained_msg_store:expire()} | - {error, uninitialized}. + {ok, store_state(), rabbit_mqtt_retained_msg_store:expire()} | + {error, uninitialized}. recover(Dir, VHost) -> - Path = rabbit_mqtt_util:path_for(Dir, VHost), - case ets:file2tab(Path) of - {ok, Tid} -> - _ = file:delete(Path), - {ok, - #store_state{table = Tid, filename = Path}, - rabbit_mqtt_retained_msg_store:expire(ets, Tid)}; - {error, _} -> - {error, uninitialized} - end. + try + {ok, NodeTable} = recover_table(Dir, VHost, "nodes"), + {ok, EdgeTable} = recover_table(Dir, VHost, "edges"), + {ok, MsgTable} = recover_table(Dir, VHost, "msgs"), + + % Find root node (should be the only node with no incoming edges) + [{RootId, 0, false}] = ets:match_object(NodeTable, {'$1', 0, false}), + + State = + #store_state{node_table = NodeTable, + edge_table = EdgeTable, + msg_table = MsgTable, + root_id = RootId, + dir = Dir, + vhost = VHost}, + + delete_table_files(Dir, VHost), + + % ? should we expire all tables here? + {ok, State, rabbit_mqtt_retained_msg_store:expire(ets, MsgTable)} + catch + _:_ -> + {error, uninitialized} + end. + +-spec terminate(store_state()) -> ok. +terminate(#store_state{node_table = NodeTable, + edge_table = EdgeTable, + msg_table = MsgTable, + dir = Dir, + vhost = VHost} = + _State) -> + ok = + ets:tab2file(NodeTable, + get_table_path(Dir, VHost, "nodes"), + [{extended_info, [object_count]}]), + ok = + ets:tab2file(EdgeTable, + get_table_path(Dir, VHost, "edges"), + [{extended_info, [object_count]}]), + ok = + ets:tab2file(MsgTable, + get_table_path(Dir, VHost, "msgs"), + [{extended_info, [object_count]}]). -spec insert(topic(), mqtt_msg(), store_state()) -> ok. -insert(Topic, Msg, #store_state{table = T}) -> - true = ets:insert(T, #retained_message{topic = Topic, mqtt_msg = Msg}), +insert(Topic, Msg, #store_state{} = State) -> + Words = split_topic(Topic), + NodeId = follow_or_create_path(Words, State), + % Mark node as topic end and store message + update_node(NodeId, true, State), + % Replace any existing message for this topic + ets:delete_object(State#store_state.msg_table, {NodeId, Topic, '_'}), + ets:insert(State#store_state.msg_table, {NodeId, Topic, Msg}), ok. --spec lookup(topic(), store_state()) -> mqtt_msg() | mqtt_msg_v0() | undefined. -lookup(Topic, #store_state{table = T}) -> - case ets:lookup(T, Topic) of - [] -> undefined; - [#retained_message{mqtt_msg = Msg}] -> Msg - end. +-spec lookup(topic(), store_state()) -> [mqtt_msg()] | [mqtt_msg_v0()] | undefined. +lookup(Topic, #store_state{} = State) -> + Words = split_topic(Topic), + Matches = match_pattern_words(Words, State#store_state.root_id, State, []), + Values = + lists:flatmap(fun(NodeId) -> + case ets:lookup(State#store_state.msg_table, NodeId) of + [{_NodeId, _Topic, Value} | _] -> [Value]; + [] -> [] + end + end, + Matches), + Values. -spec delete(topic(), store_state()) -> ok. -delete(Topic, #store_state{table = T}) -> - true = ets:delete(T, Topic), +delete(Topic, State) -> + Words = split_topic(Topic), + case follow_path(Words, State) of + {ok, NodeId} -> + % Remove message + ets:delete_object(State#store_state.msg_table, {NodeId, Topic, '_'}), + % If no more messages at this node, mark as non-topic + case ets:lookup(State#store_state.msg_table, NodeId) of + [] -> + update_node(NodeId, false, State), + % Clean up unused path + maybe_clean_path(NodeId, State); + _ -> + ok + end; + error -> + ok + end, ok. --spec terminate(store_state()) -> ok. -terminate(#store_state{table = T, filename = Path}) -> - ok = ets:tab2file(T, Path, [{extended_info, [object_count]}]). +%% Internal setup/teardown functions + +append_suffix(TableName, Suffix) -> + list_to_atom(atom_to_list(TableName) ++ Suffix). + +delete_table_files(Dir, VHost) -> + Types = ["nodes", "edges", "msgs"], + lists:foreach(fun(Type) -> delete_table(Dir, VHost, Type) end, Types), + ok. + +delete_table(Dir, VHost, Type) -> + Path = get_table_path(Dir, VHost, Type), + file:delete(Path). + +recover_table(Dir, VHost, Type) -> + Path = get_table_path(Dir, VHost, Type), + ets:file2tab(Path). + +get_table_path(Dir, VHost, Type) -> + rabbit_mqtt_util:path_for(Dir, iolist_to_binary([VHost, Type])). + +% Internal trie methods +split_topic(Topic) -> + binary:split(Topic, <<"/">>, [global]). + +follow_or_create_path(Words, State) -> + follow_or_create_path(Words, State#store_state.root_id, State). + +follow_or_create_path([], NodeId, _State) -> + NodeId; +follow_or_create_path([Word | Rest], NodeId, State) -> + case find_edge(NodeId, Word, State) of + {ok, ChildId} -> + follow_or_create_path(Rest, ChildId, State); + error -> + ChildId = make_node_id(), + add_edge(NodeId, Word, ChildId, State), + follow_or_create_path(Rest, ChildId, State) + end. + +follow_path(Words, State) -> + follow_path(Words, State#store_state.root_id, State). + +follow_path([], NodeId, _State) -> + {ok, NodeId}; +follow_path([Word | Rest], NodeId, State) -> + case find_edge(NodeId, Word, State) of + {ok, ChildId} -> + follow_path(Rest, ChildId, State); + error -> + error + end. + +match_pattern_words([], NodeId, _State, Acc) -> + [NodeId | Acc]; +match_pattern_words([<<"+">> | RestWords], NodeId, State, Acc) -> + % + matches any single word + Edges = get_all_edges(NodeId, State), + lists:foldl(fun({_Key, ChildId}, EdgeAcc) -> + match_pattern_words(RestWords, ChildId, State, EdgeAcc) + end, + Acc, + Edges); +match_pattern_words([<<"#">> | _], NodeId, State, Acc) -> + % # matches zero or more words + collect_descendants(NodeId, State, [NodeId | Acc]); +match_pattern_words([Word | RestWords], NodeId, State, Acc) -> + case find_edge(NodeId, Word, State) of + {ok, ChildId} -> + match_pattern_words(RestWords, ChildId, State, Acc); + error -> + Acc + end. + +collect_descendants(NodeId, State, Acc) -> + Edges = get_all_edges(NodeId, State), + lists:foldl(fun({_Key, ChildId}, EdgeAcc) -> + collect_descendants(ChildId, State, [ChildId | EdgeAcc]) + end, + Acc, + Edges). + +find_edge(NodeId, Word, State) -> + Key = {NodeId, Word}, + case ets:lookup(State#store_state.edge_table, Key) of + [{_Key, ToNode}] -> + {ok, ToNode}; + [] -> + error + end. + +get_all_edges(NodeId, State) -> + % Match all edges from this node + Pattern = {{NodeId, '_'}, '_'}, + ets:match_object(State#store_state.edge_table, Pattern). + +make_node_id() -> + crypto:strong_rand_bytes(16). + +add_edge(FromId, Word, ToId, State) -> + Key = {FromId, Word}, + EdgeEntry = {Key, ToId}, + ets:insert(State#store_state.edge_table, EdgeEntry), + NodeEntry = {ToId, 0, false}, + ets:insert(State#store_state.node_table, NodeEntry), + update_edge_count(FromId, +1, State). + +update_edge_count(NodeId, Delta, State) -> + case ets:lookup(State#store_state.node_table, NodeId) of + [{NodeId, EdgeCount, IsTopic}] -> + NewCount = EdgeCount + Delta, + ets:insert(State#store_state.node_table, {NodeId, NewCount, IsTopic}); + [] -> + error + end. + +update_node(NodeId, IsTopic, State) -> + case ets:lookup(State#store_state.node_table, NodeId) of + [{NodeId, EdgeCount, _OldIsTopic}] -> + ets:insert(State#store_state.node_table, {NodeId, EdgeCount, IsTopic}); + [] -> + error + end. + +maybe_clean_path(NodeId, State) -> + case ets:lookup(State#store_state.node_table, NodeId) of + [{NodeId, 0, false}] -> + Pattern = {'_', '_'}, + Edges = ets:match_object(State#store_state.edge_table, {Pattern, NodeId}), + case Edges of + [{{ParentId, Word}, NodeId}] -> + remove_edge(ParentId, Word, State), + ets:delete(State#store_state.node_table, NodeId), + maybe_clean_path(ParentId, State); + [] -> + ok + end; + _ -> + ok + end. + +remove_edge(FromId, Word, State) -> + ets:delete(State#store_state.edge_table, {FromId, Word}), + update_edge_count(FromId, -1, State). diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl new file mode 100644 index 000000000000..65ac427a5586 --- /dev/null +++ b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl @@ -0,0 +1,98 @@ +-module(rabbit_mqtt_retained_msg_store_ets_SUITE). + +-compile([export_all, nowarn_export_all]). + +-include_lib("eunit/include/eunit.hrl"). +-include_lib("common_test/include/ct.hrl"). + +all() -> + [{group, tests}]. + +groups() -> + [{tests, + [parallel], + [test_add_and_match, + test_delete, + test_plus_wildcard, + test_hash_wildcard, + test_combined_wildcards]}]. + +init_per_suite(Config) -> + Config. + +end_per_suite(_Config) -> + ok. + +init_per_group(_Group, Config) -> + Config. + +end_per_group(_Group, _Config) -> + ok. + +init_per_testcase(_TestCase, Config) -> + % Create a unique directory for each test case + Dir = filename:join(["/tmp", "mqtt_test_" ++ integer_to_list(erlang:unique_integer())]), + ok = filelib:ensure_dir(Dir ++ "/"), + State = rabbit_mqtt_retained_msg_store_ets:new(Dir, <<"test">>), + [{store_state, State}, {test_dir, Dir} | Config]. + +end_per_testcase(_TestCase, Config) -> + State = ?config(store_state, Config), + Dir = ?config(test_dir, Config), + rabbit_mqtt_retained_msg_store_ets:terminate(State), + % Clean up test directory + os:cmd("rm -rf " ++ Dir), + ok. + +%%---------------------------------------------------------------------------- +%% Test Cases +%%---------------------------------------------------------------------------- + +test_add_and_match(Config) -> + State = ?config(store_state, Config), + + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), + Matches1 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/d">>, <<"msg2">>, State), + Matches2 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/d">>, State), + NoMatches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"x/y/z">>, State), + + ?_assertEqual([<<"msg1">>], Matches1), + ?_assertEqual([<<"msg2">>], Matches2), + ?_assertEqual([], NoMatches). + +test_delete(Config) -> + State = ?config(store_state, Config), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:delete(<<"a/b/c">>, State), + Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), + + ?_assertEqual([], Matches). + +test_plus_wildcard(Config) -> + State = ?config(store_state, Config), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c">>, <<"msg2">>, State), + Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/+/c">>, State), + + ?_assertEqual(lists:sort([<<"msg1">>, <<"msg2">>]), lists:sort(Matches)). + +test_hash_wildcard(Config) -> + State = ?config(store_state, Config), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, <<"msg2">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/x/y">>, <<"msg3">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/q/x/y">>, <<"msg3">>, State), + Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/#">>, State), + + ?_assertEqual([<<"msg1">>, <<"msg2">>, <<"msg3">>], lists:sort(Matches)). + +test_combined_wildcards(Config) -> + State = ?config(store_state, Config), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, <<"msg1">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c/e">>, <<"msg2">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/c/f/g">>, <<"msg3">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/d/f/g">>, <<"msg4">>, State), + Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/+/c/#">>, State), + + ?_assertEqual([<<"msg1">>, <<"msg2">>, <<"msg3">>], lists:sort(Matches)). From 055e6bc57396dde7441d3ff0888cc48e055ca315 Mon Sep 17 00:00:00 2001 From: getlarge Date: Sat, 18 Jan 2025 21:39:28 +0100 Subject: [PATCH 10/25] refactor(rabbitmq_mqtt): support sending retained messages list --- .../src/rabbit_mqtt_processor.erl | 72 ++++++++++--------- .../src/rabbit_mqtt_retained_msg_store.erl | 2 +- .../src/rabbit_mqtt_retainer.erl | 3 +- 3 files changed, 43 insertions(+), 34 deletions(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl index 499790b30bf1..19fe02b2157f 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl @@ -956,44 +956,52 @@ send_retained_messages(Subscriptions, State) -> -spec send_retained_message(topic_filter(), qos(), state()) -> state(). send_retained_message(TopicFilter0, SubscribeQos, - State0 = #state{packet_id = PacketId0, - cfg = #cfg{retainer_pid = RPid}}) -> + State0 = #state{cfg = #cfg{retainer_pid = RPid}}) -> TopicFilter = amqp_to_mqtt(TopicFilter0), case rabbit_mqtt_retainer:fetch(RPid, TopicFilter) of undefined -> State0; - #mqtt_msg{qos = MsgQos, - retain = Retain, - payload = Payload, - props = Props0} -> - Qos = effective_qos(MsgQos, SubscribeQos), - %% Wildcards are currently not supported when fetching retained - %% messages. Therefore, TopicFilter must must be a topic name. - {Topic, Props, State1} = process_topic_alias_outbound(TopicFilter, Props0, State0), - {PacketId, State} = case Qos of - ?QOS_0 -> - {undefined, State1}; - ?QOS_1 -> - {PacketId0, - State1#state{packet_id = increment_packet_id(PacketId0)}} - end, - Packet = #mqtt_packet{ - fixed = #mqtt_packet_fixed{ - type = ?PUBLISH, - qos = Qos, - dup = false, - retain = Retain - }, - variable = #mqtt_packet_publish{ - packet_id = PacketId, - topic_name = Topic, - props = Props - }, - payload = Payload}, - _ = send(Packet, State), - State + Msgs when is_list(Msgs) -> + lists:foldl( + fun(Msg, S) -> + send_retained_message_to_client(Msg, TopicFilter, SubscribeQos, S) + end, State0, Msgs); + #mqtt_msg{} = SingleMsg -> + send_retained_message_to_client(SingleMsg, TopicFilter, SubscribeQos, State0) end. +send_retained_message_to_client(#mqtt_msg{qos = MsgQos, + retain = Retain, + payload = Payload, + props = Props0}, + TopicFilter, + SubscribeQos, + State0 = #state{packet_id = PacketId0}) -> + Qos = effective_qos(MsgQos, SubscribeQos), + {Topic, Props, State1} = process_topic_alias_outbound(TopicFilter, Props0, State0), + {PacketId, State} = case Qos of + ?QOS_0 -> + {undefined, State1}; + ?QOS_1 -> + {PacketId0, + State1#state{packet_id = increment_packet_id(PacketId0)}} + end, + Packet = #mqtt_packet{ + fixed = #mqtt_packet_fixed{ + type = ?PUBLISH, + qos = Qos, + dup = false, + retain = Retain + }, + variable = #mqtt_packet_publish{ + packet_id = PacketId, + topic_name = Topic, + props = Props + }, + payload = Payload}, + _ = send(Packet, State), + State. + clear_will_msg(#state{cfg = #cfg{vhost = Vhost, client_id = ClientId}} = State) -> QNameBin = rabbit_mqtt_util:queue_name_bin(ClientId, will), diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl index 59ec80d9a4d2..b39c3454f9bf 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl @@ -45,7 +45,7 @@ ok. -callback lookup(topic(), State :: any()) -> - mqtt_msg() | mqtt_msg_v0() | undefined. + [mqtt_msg()] | mqtt_msg() | mqtt_msg_v0() | undefined. -callback delete(topic(), State :: any()) -> ok. diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl index 7ba569bb82b7..322202bda2d2 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl @@ -34,7 +34,7 @@ retain(Pid, Topic, Msg = #mqtt_msg{retain = true}) -> gen_server:cast(Pid, {retain, Topic, Msg}). -spec fetch(pid(), topic()) -> - undefined | mqtt_msg(). + undefined | mqtt_msg() | [mqtt_msg()]. fetch(Pid, Topic) -> gen_server:call(Pid, {fetch, Topic}, ?TIMEOUT). @@ -87,6 +87,7 @@ handle_cast({clear, Topic}, State = #?STATE{store_state = StoreState, ok = rabbit_mqtt_retained_msg_store:delete(Topic, StoreState), {noreply, State#?STATE{expire = Expire}}. +% TODO: handle listof messages handle_call({fetch, Topic}, _From, State = #?STATE{store_state = StoreState}) -> Reply = case rabbit_mqtt_retained_msg_store:lookup(Topic, StoreState) of #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry0} = Props, From a19e1be7b523e49002155a6bae87e27b82b59a79 Mon Sep 17 00:00:00 2001 From: getlarge Date: Sat, 18 Jan 2025 21:54:34 +0100 Subject: [PATCH 11/25] test(rabbitmq_mqtt): update publish expectations --- ...bbit_mqtt_retained_msg_store_ets_SUITE.erl | 6 + deps/rabbitmq_mqtt/test/retainer_SUITE.erl | 210 +++++++++--------- 2 files changed, 111 insertions(+), 105 deletions(-) diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl index 65ac427a5586..c712d72a54d2 100644 --- a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl @@ -1,3 +1,9 @@ +%% This Source Code Form is subject to the terms of the Mozilla Public +%% License, v. 2.0. If a copy of the MPL was not distributed with this +%% file, You can obtain one at https://mozilla.org/MPL/2.0/. +%% +%% Copyright (c) 2007-2024 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. +%% -module(rabbit_mqtt_retained_msg_store_ets_SUITE). -compile([export_all, nowarn_export_all]). diff --git a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl index 4146678ebb63..f87906c1744a 100644 --- a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl @@ -5,49 +5,37 @@ %% Copyright (c) 2007-2024 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. %% -module(retainer_SUITE). + -compile([export_all, nowarn_export_all]). -include_lib("common_test/include/ct.hrl"). -include_lib("eunit/include/eunit.hrl"). --import(util, [connect/2, connect/3, - expect_publishes/3, - assert_message_expiry_interval/2 - ]). + +-import(util, + [connect/2, connect/3, expect_publishes/3, assert_message_expiry_interval/2]). all() -> - [ - {group, v4}, - {group, v5} - ]. + [{group, v4}, {group, v5}]. groups() -> - [ - {v4, [], sub_groups()}, - {v5, [], sub_groups()} - ]. + [{v4, [], sub_groups()}, {v5, [], sub_groups()}]. sub_groups() -> - [ - {dets, [shuffle], tests()}, + [{dets, [shuffle], tests()}, {ets, [shuffle], tests() ++ wildcard_tests()}, - {noop, [shuffle], [does_not_retain]} - ]. + {noop, [shuffle], [does_not_retain]}]. tests() -> - [ - coerce_configuration_data, + [coerce_configuration_data, should_translate_amqp2mqtt_on_publish, should_translate_amqp2mqtt_on_retention, should_translate_amqp2mqtt_on_retention_search, recover, - recover_with_message_expiry_interval - ]. + recover_with_message_expiry_interval]. %% Only run wildcard tests for ETS store wildcard_tests() -> - [retained_wildcard_single_level, - retained_wildcard_multi_level, - retained_wildcard_mixed]. + [retained_wildcard_single_level, retained_wildcard_multi_level, retained_wildcard_mixed]. suite() -> [{timetrap, {minutes, 2}}]. @@ -63,37 +51,29 @@ init_per_suite(Config) -> end_per_suite(Config) -> Config. -init_per_group(G, Config) - when G =:= v4; - G =:= v5 -> +init_per_group(G, Config) when G =:= v4; G =:= v5 -> rabbit_ct_helpers:set_config(Config, {mqtt_version, G}); init_per_group(Group, Config0) -> Suffix = rabbit_ct_helpers:testcase_absname(Config0, "", "-"), - Config = rabbit_ct_helpers:set_config( - Config0, {rmq_nodename_suffix, Suffix}), + Config = rabbit_ct_helpers:set_config(Config0, {rmq_nodename_suffix, Suffix}), Mod = list_to_atom("rabbit_mqtt_retained_msg_store_" ++ atom_to_list(Group)), Env = [{rabbitmq_mqtt, [{retained_message_store, Mod}]}, - {rabbit, [ - {default_user, "guest"}, - {default_pass, "guest"}, - {default_vhost, "/"}, - {default_permissions, [".*", ".*", ".*"]} - ]}], - rabbit_ct_helpers:run_setup_steps( - Config, - [fun(Conf) -> rabbit_ct_helpers:merge_app_env(Conf, Env) end] ++ - rabbit_ct_broker_helpers:setup_steps() ++ - rabbit_ct_client_helpers:setup_steps()). - -end_per_group(G, Config) - when G =:= v4; - G =:= v5 -> + {rabbit, + [{default_user, "guest"}, + {default_pass, "guest"}, + {default_vhost, "/"}, + {default_permissions, [".*", ".*", ".*"]}]}], + rabbit_ct_helpers:run_setup_steps(Config, + [fun(Conf) -> rabbit_ct_helpers:merge_app_env(Conf, Env) end] + ++ rabbit_ct_broker_helpers:setup_steps() + ++ rabbit_ct_client_helpers:setup_steps()). + +end_per_group(G, Config) when G =:= v4; G =:= v5 -> Config; end_per_group(_, Config) -> - rabbit_ct_helpers:run_teardown_steps( - Config, - rabbit_ct_client_helpers:teardown_steps() ++ - rabbit_ct_broker_helpers:teardown_steps()). + rabbit_ct_helpers:run_teardown_steps(Config, + rabbit_ct_client_helpers:teardown_steps() + ++ rabbit_ct_broker_helpers:teardown_steps()). init_per_testcase(recover_with_message_expiry_interval = T, Config) -> case ?config(mqtt_version, Config) of @@ -108,7 +88,6 @@ init_per_testcase(Testcase, Config) -> end_per_testcase(Testcase, Config) -> rabbit_ct_helpers:testcase_finished(Config, Testcase). - %% ------------------------------------------------------------------- %% Testsuite cases %% ------------------------------------------------------------------- @@ -131,7 +110,7 @@ should_translate_amqp2mqtt_on_publish(Config) -> C = connect(<<"simpleClientRetainer">>, Config, [{ack_timeout, 1}]), %% there's an active consumer {ok, _, _} = emqtt:subscribe(C, <<"TopicA/Device.Field">>, qos1), - ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), + ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), ok = expect_publishes(C, <<"TopicA/Device/Field">>, [<<"Payload">>]), ok = emqtt:disconnect(C). @@ -143,7 +122,7 @@ should_translate_amqp2mqtt_on_publish(Config) -> should_translate_amqp2mqtt_on_retention(Config) -> C = connect(<<"simpleClientRetainer">>, Config, [{ack_timeout, 1}]), %% publish with retain = true before a consumer comes around - ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), + ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), {ok, _, _} = emqtt:subscribe(C, <<"TopicA/Device.Field">>, qos1), ok = expect_publishes(C, <<"TopicA/Device/Field">>, [<<"Payload">>]), ok = emqtt:disconnect(C). @@ -155,28 +134,27 @@ should_translate_amqp2mqtt_on_retention(Config) -> %% ------------------------------------------------------------------- should_translate_amqp2mqtt_on_retention_search(Config) -> C = connect(<<"simpleClientRetainer">>, Config, [{ack_timeout, 1}]), - ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), + ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), {ok, _, _} = emqtt:subscribe(C, <<"TopicA/Device/Field">>, qos1), ok = expect_publishes(C, <<"TopicA/Device/Field">>, [<<"Payload">>]), ok = emqtt:disconnect(C). does_not_retain(Config) -> C = connect(<<"simpleClientRetainer">>, Config, [{ack_timeout, 1}]), - ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), + ok = emqtt:publish(C, <<"TopicA/Device.Field">>, #{}, <<"Payload">>, [{retain, true}]), {ok, _, _} = emqtt:subscribe(C, <<"TopicA/Device.Field">>, qos1), receive Unexpected -> ct:fail("Unexpected message: ~p", [Unexpected]) after 1000 -> - ok + ok end, ok = emqtt:disconnect(C). recover(Config) -> Topic = Payload = ClientId = atom_to_binary(?FUNCTION_NAME), C1 = connect(ClientId, Config), - {ok, _} = emqtt:publish(C1, Topic, Payload, [{retain, true}, - {qos, 1}]), + {ok, _} = emqtt:publish(C1, Topic, Payload, [{retain, true}, {qos, 1}]), ok = emqtt:disconnect(C1), ok = rabbit_ct_broker_helpers:restart_node(Config, 0), C2 = connect(ClientId, Config), @@ -188,14 +166,25 @@ recover_with_message_expiry_interval(Config) -> ClientId = atom_to_binary(?FUNCTION_NAME), C1 = connect(ClientId, Config), Start = os:system_time(second), - {ok, _} = emqtt:publish(C1, <<"topic/1">>, - <<"m1">>, [{retain, true}, {qos, 1}]), - {ok, _} = emqtt:publish(C1, <<"topic/2">>, #{'Message-Expiry-Interval' => 100}, - <<"m2">>, [{retain, true}, {qos, 1}]), - {ok, _} = emqtt:publish(C1, <<"topic/3">>, #{'Message-Expiry-Interval' => 3}, - <<"m3">>, [{retain, true}, {qos, 1}]), - {ok, _} = emqtt:publish(C1, <<"topic/4">>, #{'Message-Expiry-Interval' => 15}, - <<"m4">>, [{retain, true}, {qos, 1}]), + {ok, _} = emqtt:publish(C1, <<"topic/1">>, <<"m1">>, [{retain, true}, {qos, 1}]), + {ok, _} = + emqtt:publish(C1, + <<"topic/2">>, + #{'Message-Expiry-Interval' => 100}, + <<"m2">>, + [{retain, true}, {qos, 1}]), + {ok, _} = + emqtt:publish(C1, + <<"topic/3">>, + #{'Message-Expiry-Interval' => 3}, + <<"m3">>, + [{retain, true}, {qos, 1}]), + {ok, _} = + emqtt:publish(C1, + <<"topic/4">>, + #{'Message-Expiry-Interval' => 15}, + <<"m4">>, + [{retain, true}, {qos, 1}]), ok = emqtt:disconnect(C1), %% Takes around 9 seconds on Linux. ok = rabbit_ct_broker_helpers:restart_node(Config, 0), @@ -209,35 +198,46 @@ recover_with_message_expiry_interval(Config) -> timer:sleep(SleepMs), ElapsedSeconds2 = os:system_time(second) - Start, - {ok, _, [1,1,1,1]} = emqtt:subscribe(C2, [{<<"topic/1">>, qos1}, - {<<"topic/2">>, qos1}, - {<<"topic/3">>, qos1}, - {<<"topic/4">>, qos1}]), - receive {publish, #{client_pid := C2, - retain := true, - topic := <<"topic/1">>, - payload := <<"m1">>, - properties := Props}} - when map_size(Props) =:= 0 -> ok - after 30_000 -> ct:fail("did not topic/1") + {ok, _, [1, 1, 1, 1]} = + emqtt:subscribe(C2, + [{<<"topic/1">>, qos1}, + {<<"topic/2">>, qos1}, + {<<"topic/3">>, qos1}, + {<<"topic/4">>, qos1}]), + receive + {publish, + #{client_pid := C2, + retain := true, + topic := <<"topic/1">>, + payload := <<"m1">>, + properties := Props}} + when map_size(Props) =:= 0 -> + ok + after 30_000 -> + ct:fail("did not topic/1") end, - receive {publish, #{client_pid := C2, - retain := true, - topic := <<"topic/2">>, - payload := <<"m2">>, - properties := #{'Message-Expiry-Interval' := MEI}}} -> - assert_message_expiry_interval(100 - ElapsedSeconds2, MEI) - after 30_000 -> ct:fail("did not topic/2") + receive + {publish, + #{client_pid := C2, + retain := true, + topic := <<"topic/2">>, + payload := <<"m2">>, + properties := #{'Message-Expiry-Interval' := MEI}}} -> + assert_message_expiry_interval(100 - ElapsedSeconds2, MEI) + after 30_000 -> + ct:fail("did not topic/2") end, - receive Unexpected -> ct:fail("Received unexpectedly: ~p", [Unexpected]) - after 0 -> ok + receive + Unexpected -> + ct:fail("Received unexpectedly: ~p", [Unexpected]) + after 0 -> + ok end, ok = emqtt:disconnect(C2). - %% ------------------------------------------------------------------- %% If a client publishes a retained message to devices/sensor1/temperature and another %% client subscribes to devices/+/temperature the client should be @@ -245,35 +245,35 @@ recover_with_message_expiry_interval(Config) -> %% ------------------------------------------------------------------- %% retained_wildcard_single_level(Config) -> - Topic = <<"devices/sensor1/temperature">>, - Pattern = <<"devices/+/temperature">>, - Payload = <<"23.5">>, C = connect(<<"wildcardClientRetainer">>, Config, [{ack_timeout, 1}]), - {ok, _} = emqtt:publish(C, Topic, #{}, Payload, [{retain, true}, {qos, 1}]), - {ok, _, _} = emqtt:subscribe(C, Pattern, qos1), - ok = expect_publishes(C, Topic, [Payload]), + ok = + emqtt:publish(C, <<"devices/sensor1/temperature">>, #{}, <<"23.5">>, [{retain, true}]), + {ok, _, _} = emqtt:subscribe(C, <<"devices/+/temperature">>, qos1), + ok = expect_publishes(C, <<"devices/+/temperature">>, [<<"23.5">>]), ok = emqtt:disconnect(C). %% Test multi-level wildcard (#) retained_wildcard_multi_level(Config) -> - Topic = <<"devices/sensor1/readings/temperature">>, - Pattern = <<"devices/#">>, - Payload = <<"23.5">>, - C = connect(<<"wildcardClientRetainer">>, Config, [{ack_timeout, 1}]), - {ok, _} = emqtt:publish(C, Topic, #{}, Payload, [{retain, true}, {qos, 1}]), - {ok, _, _} = emqtt:subscribe(C, Pattern, qos1), - ok = expect_publishes(C, Topic, [Payload]), + ok = + emqtt:publish(C, + <<"devices/sensor1/readings/temperature">>, + #{}, + <<"23.5">>, + [{retain, true}]), + {ok, _, _} = emqtt:subscribe(C, <<"devices/#">>, qos1), + ok = expect_publishes(C, <<"devices/#">>, [<<"23.5">>]), ok = emqtt:disconnect(C). -% %% Test mixed wildcards (+/#) +%% Test mixed wildcards (+/#) retained_wildcard_mixed(Config) -> - Topic = <<"devices/sensor1/readings/temperature">>, - Pattern = <<"devices/+/readings/#">>, - Payload = <<"23.5">>, - C = connect(<<"wildcardClientRetainer">>, Config, [{ack_timeout, 1}]), - {ok, _} = emqtt:publish(C, Topic, #{}, Payload, [{retain, true}, {qos, 1}]), - {ok, _, _} = emqtt:subscribe(C, Pattern, qos1), - ok = expect_publishes(C, Topic, [Payload]), + ok = + emqtt:publish(C, + <<"devices/sensor1/readings/temperature">>, + #{}, + <<"23.5">>, + [{retain, true}]), + {ok, _, _} = emqtt:subscribe(C, <<"devices/+/readings/#">>, qos1), + ok = expect_publishes(C, <<"devices/+/readings/#">>, [<<"23.5">>]), ok = emqtt:disconnect(C). From 6d9083b914376c9cb58e87a84c9835ddd6039ffb Mon Sep 17 00:00:00 2001 From: getlarge Date: Sun, 19 Jan 2025 20:29:52 +0100 Subject: [PATCH 12/25] refactor(rabbitmq_mqtt): remove rabbit_mqtt_topic_matcher and associated files --- deps/rabbitmq_mqtt/app.bzl | 15 +- .../include/rabbit_mqtt_topic_matcher.hrl | 13 -- .../src/rabbit_mqtt_topic_matcher.erl | 172 ------------------ .../test/rabbit_mqtt_topic_matcher_SUITE.erl | 91 --------- moduleindex.yaml | 1 - 5 files changed, 2 insertions(+), 290 deletions(-) delete mode 100644 deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl delete mode 100644 deps/rabbitmq_mqtt/src/rabbit_mqtt_topic_matcher.erl delete mode 100644 deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl diff --git a/deps/rabbitmq_mqtt/app.bzl b/deps/rabbitmq_mqtt/app.bzl index acdc6f755537..d10032ecb5a1 100644 --- a/deps/rabbitmq_mqtt/app.bzl +++ b/deps/rabbitmq_mqtt/app.bzl @@ -34,7 +34,6 @@ def all_beam_files(name = "all_beam_files"): "src/rabbit_mqtt_retainer.erl", "src/rabbit_mqtt_retainer_sup.erl", "src/rabbit_mqtt_sup.erl", - "src/rabbit_mqtt_topic_matcher.erl", "src/rabbit_mqtt_util.erl", ], hdrs = [":public_and_private_hdrs"], @@ -81,7 +80,6 @@ def all_test_beam_files(name = "all_test_beam_files"): "src/rabbit_mqtt_retainer.erl", "src/rabbit_mqtt_retainer_sup.erl", "src/rabbit_mqtt_sup.erl", - "src/rabbit_mqtt_topic_matcher.erl", "src/rabbit_mqtt_util.erl", ], hdrs = [":public_and_private_hdrs"], @@ -136,7 +134,6 @@ def all_srcs(name = "all_srcs"): "src/rabbit_mqtt_retainer.erl", "src/rabbit_mqtt_retainer_sup.erl", "src/rabbit_mqtt_sup.erl", - "src/rabbit_mqtt_topic_matcher.erl", "src/rabbit_mqtt_util.erl", ], ) @@ -145,7 +142,6 @@ def all_srcs(name = "all_srcs"): srcs = [ "include/rabbit_mqtt.hrl", "include/rabbit_mqtt_packet.hrl", - "include/rabbit_mqtt_topic_matcher.hrl", ], ) filegroup( @@ -333,20 +329,13 @@ def test_suite_beam_files(name = "test_suite_beam_files"): erlc_opts = "//:test_erlc_opts", deps = ["//deps/amqp_client:erlang_app", "//deps/rabbitmq_ct_helpers:erlang_app"], ) - erlang_bytecode( - name = "rabbit_mqtt_topic_matcher_SUITE_beam_files", - testonly = True, - srcs = ["test/rabbit_mqtt_topic_matcher_SUITE.erl"], - outs = ["test/rabbit_mqtt_topic_matcher_SUITE.beam"], - hdrs = ["include/rabbit_mqtt_topic_matcher.hrl"], - app_name = "rabbitmq_mqtt", - erlc_opts = "//:test_erlc_opts", - ) + erlang_bytecode( name = "rabbit_mqtt_retained_msg_store_ets_SUITE_beam_files", testonly = True, srcs = ["test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl"], outs = ["test/rabbit_mqtt_retained_msg_store_ets_SUITE.beam"], + hdrs = ["include/rabbit_mqtt_packet.hrl"], app_name = "rabbitmq_mqtt", erlc_opts = "//:test_erlc_opts", ) diff --git a/deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl b/deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl deleted file mode 100644 index 18619d54bcc5..000000000000 --- a/deps/rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl +++ /dev/null @@ -1,13 +0,0 @@ -%% This Source Code Form is subject to the terms of the Mozilla Public -%% License, v. 2.0. If a copy of the MPL was not distributed with this -%% file, You can obtain one at https://mozilla.org/MPL/2.0/. -%% -%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. - --record(globber, - {separator = <<"/">>, - wildcard_one = <<"+">>, - wildcard_some = <<"#">>, - trie = maps:new()}). - --type globber() :: #globber{}. diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_topic_matcher.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_topic_matcher.erl deleted file mode 100644 index e526127f6981..000000000000 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_topic_matcher.erl +++ /dev/null @@ -1,172 +0,0 @@ -%% This Source Code Form is subject to the terms of the Mozilla Public -%% License, v. 2.0. If a copy of the MPL was not distributed with this -%% file, You can obtain one at https://mozilla.org/MPL/2.0/. -%% -%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. - --module(rabbit_mqtt_topic_matcher). - --export([new/0, new/3, add/2, add/3, remove/2, remove/3, test/2, match/2, match_iter/2, - clear/1]). - --include_lib("rabbit_mqtt_topic_matcher.hrl"). - --spec new() -> globber(). -new() -> - #globber{}. - --spec new(binary(), binary(), binary()) -> globber(). -new(Separator, WildcardOne, WildcardSome) -> - #globber{separator = Separator, - wildcard_one = WildcardOne, - wildcard_some = WildcardSome, - trie = maps:new()}. - --spec add(globber(), binary()) -> globber(). -add(Globber, Pattern) -> - add(Globber, Pattern, <<"match">>). - --spec add(globber(), binary(), any()) -> globber(). -add(Globber, Pattern, Val) -> - Words = split(Pattern, Globber#globber.separator), - Trie = do_add(Words, Val, Globber#globber.trie), - Globber#globber{trie = Trie}. - --spec remove(globber(), binary()) -> globber(). -remove(Globber, Pattern) -> - remove(Globber, Pattern, <<>>). - --spec remove(globber(), binary(), any()) -> globber(). -remove(Globber, Pattern, Val) -> - Words = split(Pattern, Globber#globber.separator), - Trie = do_remove(Words, Val, Globber#globber.trie), - Globber#globber{trie = Trie}. - --spec match(globber(), binary()) -> list(). -match(Globber, Pattern) -> - Words = split(Pattern, Globber#globber.separator), - try do_match(Words, Globber#globber.trie, [], Globber) of - Res -> - Res - catch - % error:badmatch and all - _:_ -> - undefined - end. - --spec test(globber(), binary()) -> boolean(). -test(Globber, Pattern) -> - case match(Globber, Pattern) of - undefined -> - false; - [] -> - false; - _ -> - true - end. - --spec match_iter(globber(), binary()) -> list(). -match_iter(Globber, Topic) -> - Words = split(Topic, Globber#globber.separator), - do_match_iter(Words, Globber#globber.trie, Globber). - --spec clear(globber()) -> globber(). -clear(Globber) -> - Globber#globber{trie = maps:new()}. - -split(Topic, Separator) -> - binary:split(Topic, Separator, [global]). - -do_add([], Val, Trie) -> - maps:put(<<".">>, [Val | maps:get(<<".">>, Trie, [])], Trie); -do_add([Word | Rest], Val, Trie) -> - SubTrie = maps:get(Word, Trie, maps:new()), - NewSubTrie = do_add(Rest, Val, SubTrie), - maps:put(Word, NewSubTrie, Trie). - -do_remove([], Val, Trie) -> - case maps:get(<<".">>, Trie) of - Vals when is_list(Vals) -> - NewVals = lists:delete(Val, Vals), - if NewVals =:= [] -> - maps:remove(<<".">>, Trie); - true -> - maps:put(<<".">>, NewVals, Trie) - end; - _ -> - Trie - end; -do_remove([Word | Rest], Val, Trie) -> - case maps:get(Word, Trie) of - SubTrie when is_map(SubTrie) -> - NewSubTrie = do_remove(Rest, Val, SubTrie), - case maps:size(NewSubTrie) of - 0 -> - maps:remove(Word, Trie); - _ -> - maps:put(Word, NewSubTrie, Trie) - end; - _ -> - Trie - end. - -do_match([], Trie, Acc, _Globber) -> - case maps:get(<<".">>, Trie, undefined) of - undefined -> - Acc; - Vals -> - lists:append(Vals, Acc) - end; -do_match([Word | Rest], Trie, Acc, Globber) -> - SubTrie = - case maps:get(Word, Trie, undefined) of - undefined -> - maps:new(); - Sub -> - Sub - end, - SubTrie1 = - case maps:get(Globber#globber.wildcard_one, Trie, undefined) of - undefined -> - maps:new(); - Sub1 -> - Sub1 - end, - SubTrie2 = - case maps:get(Globber#globber.wildcard_some, Trie, undefined) of - undefined -> - maps:new(); - Sub2 -> - Sub2 - end, - Acc1 = do_match(Rest, SubTrie, Acc, Globber), - Acc2 = do_match(Rest, SubTrie1, Acc1, Globber), - do_match([], SubTrie2, Acc2, Globber). - -do_match_iter([], Trie, _Globber) -> - maps:get(<<".">>, Trie, []); -do_match_iter([Word | Rest], Trie, Globber) -> - SubTrie = - case maps:get(Word, Trie, undefined) of - undefined -> - maps:new(); - Sub -> - Sub - end, - SubTrie1 = - case maps:get(Globber#globber.wildcard_one, Trie, undefined) of - undefined -> - maps:new(); - Sub1 -> - Sub1 - end, - SubTrie2 = - case maps:get(Globber#globber.wildcard_some, Trie, undefined) of - undefined -> - maps:new(); - Sub2 -> - Sub2 - end, - lists:append([do_match_iter(Rest, SubTrie, Globber), - do_match_iter(Rest, SubTrie1, Globber), - do_match_iter([], SubTrie2, Globber)]). diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl deleted file mode 100644 index 66a5b9e33172..000000000000 --- a/deps/rabbitmq_mqtt/test/rabbit_mqtt_topic_matcher_SUITE.erl +++ /dev/null @@ -1,91 +0,0 @@ -%% This Source Code Form is subject to the terms of the Mozilla Public -%% License, v. 2.0. If a copy of the MPL was not distributed with this -%% file, You can obtain one at https://mozilla.org/MPL/2.0/. -%% -%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. --module(rabbit_mqtt_topic_matcher_SUITE). - --compile([export_all, nowarn_export_all]). - --include_lib("rabbitmq_mqtt/include/rabbit_mqtt_topic_matcher.hrl"). --include_lib("eunit/include/eunit.hrl"). - -all() -> - [{group, tests}]. - -groups() -> - [{tests, - [parallel], - [new, add, remove, match, pattern_is_matching, match_iter, clear, multiple_patterns]}]. - -new(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(), - ?assertEqual(#globber{}, Globber), - Globber2 = rabbit_mqtt_topic_matcher:new(<<".">>, <<"+">>, <<"#">>), - ?assertEqual(#globber{separator = <<".">>, - wildcard_one = <<"+">>, - wildcard_some = <<"#">>}, - Globber2). - -add(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(), - Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), - ?assertMatch(#globber{trie = _}, Globber1), - Globber2 = rabbit_mqtt_topic_matcher:add(Globber1, <<"test/#">>, <<"it n">>), - ?assertMatch(#globber{trie = _}, Globber2). - -remove(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(), - Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), - Globber2 = rabbit_mqtt_topic_matcher:remove(Globber1, <<"test/+">>, <<"matches">>), - ?assertEqual(Globber, Globber2). - -match(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(), - Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"it matches">>), - Result = rabbit_mqtt_topic_matcher:match(Globber1, <<"test/bar">>), - ?assertEqual([<<"it matches">>], Result), - Result2 = rabbit_mqtt_topic_matcher:match(Globber1, <<"test/foo">>), - ?assertEqual([<<"it matches">>], Result2), - Result3 = rabbit_mqtt_topic_matcher:match(Globber1, <<"not/foo">>), - ?assertEqual([], Result3). - -pattern_is_matching(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(), - Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>), - ?assertEqual(true, rabbit_mqtt_topic_matcher:test(Globber1, <<"test/bar">>)), - ?assertEqual(false, rabbit_mqtt_topic_matcher:test(Globber1, <<"foo/bar">>)). - -match_iter(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(), - Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), - Result = rabbit_mqtt_topic_matcher:match_iter(Globber1, <<"test/bar">>), - ?assertEqual([<<"matches">>], Result). - -clear(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(), - Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"test/+">>, <<"matches">>), - Globber2 = rabbit_mqtt_topic_matcher:clear(Globber1), - ?assertEqual(Globber, Globber2). - -multiple_patterns(_Config) -> - Globber = rabbit_mqtt_topic_matcher:new(<<".">>, <<"*">>, <<"#">>), - Globber1 = rabbit_mqtt_topic_matcher:add(Globber, <<"foo.#">>, <<"catchall">>), - Globber2 = - rabbit_mqtt_topic_matcher:add(Globber1, <<"foo.*.bar">>, <<"single_wildcard">>), - Globber3 = - rabbit_mqtt_topic_matcher:add(Globber2, <<"foo.*.bar.#">>, <<"single_and_catchall">>), - - ?assertEqual([<<"catchall">>], rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.bar">>)), - ?assertEqual([<<"catchall">>], - rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.bar.baz">>)), - - ?assertEqual([<<"catchall">>, <<"single_wildcard">>], - rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.bar">>)), - ?assertEqual([<<"catchall">>], - rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.baz.bar">>)), - - ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], - rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.bar.baz">>)), - ?assertEqual([<<"catchall">>, <<"single_and_catchall">>], - rabbit_mqtt_topic_matcher:match(Globber3, <<"foo.test.bar.baz.qux">>)). diff --git a/moduleindex.yaml b/moduleindex.yaml index f0b5deed82d9..eca29bb4f6eb 100755 --- a/moduleindex.yaml +++ b/moduleindex.yaml @@ -1056,7 +1056,6 @@ rabbitmq_mqtt: - rabbit_mqtt_retainer - rabbit_mqtt_retainer_sup - rabbit_mqtt_sup -- rabbit_mqtt_topic_matcher - rabbit_mqtt_util rabbitmq_peer_discovery_aws: - rabbit_peer_discovery_aws From 098a7c6650d3b8477b0bc39f2271fde0f712e728 Mon Sep 17 00:00:00 2001 From: getlarge Date: Sun, 19 Jan 2025 20:31:27 +0100 Subject: [PATCH 13/25] refactor(rabbitmq_mqtt): support retained message list handling --- .../src/rabbit_mqtt_retained_msg_store.erl | 61 ++++--- .../src/rabbit_mqtt_retainer.erl | 152 ++++++++++-------- 2 files changed, 120 insertions(+), 93 deletions(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl index b39c3454f9bf..5af9c3359d6c 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl @@ -22,7 +22,7 @@ -include("rabbit_mqtt_packet.hrl"). -include_lib("kernel/include/logger.hrl"). -export([start/1, insert/3, lookup/2, delete/2, terminate/1]). --export([expire/2, has_wildcards/1]). +-export([expire/2]). -export_type([state/0, expire/0]). -define(STATE, ?MODULE). @@ -78,8 +78,9 @@ insert(Topic, Msg, #?STATE{store_mod = Mod, store_state = StoreState}) -> ok = Mod:insert(Topic, Msg, StoreState). +% TODO: only return list of retained messages -spec lookup(topic(), state()) -> - mqtt_msg() | undefined. + mqtt_msg() | [mqtt_msg()] | undefined. lookup(Topic, #?STATE{store_mod = Mod, store_state = StoreState}) -> case Mod:lookup(Topic, StoreState) of @@ -89,11 +90,6 @@ lookup(Topic, #?STATE{store_mod = Mod, Other end. --spec has_wildcards(topic()) -> boolean(). -has_wildcards(Pattern) -> - Parts = binary:split(Pattern, <<"/">>, [global]), - lists:member(<<"#">>, Parts) orelse lists:member(<<"+">>, Parts). - -spec delete(topic(), state()) -> ok. delete(Topic, #?STATE{store_mod = Mod, store_state = StoreState}) -> @@ -104,24 +100,45 @@ terminate(#?STATE{store_mod = Mod, store_state = StoreState}) -> ok = Mod:terminate(StoreState). +% TODO: refactor when DETS also supports the new ETS trie structure -spec expire(ets | dets, ets:tid() | dets:tab_name()) -> expire(). expire(Mod, Tab) -> Now = os:system_time(second), - Mod:foldl( - fun(#retained_message{topic = Topic, - mqtt_msg = #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry}, - timestamp = Timestamp}}, Acc) - when is_integer(Expiry) andalso - is_integer(Timestamp) -> - if Now - Timestamp >= Expiry -> - Mod:delete(Tab, Topic), - Acc; - true -> - maps:put(Topic, {Timestamp, Expiry}, Acc) - end; - (_, Acc) -> - Acc - end, #{}, Tab). + case Mod of + dets -> + % Original code for DETS + Mod:foldl( + fun(#retained_message{topic = Topic, + mqtt_msg = #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry}, + timestamp = Timestamp}}, Acc) + when is_integer(Expiry) andalso + is_integer(Timestamp) -> + if Now - Timestamp >= Expiry -> + Mod:delete(Tab, Topic), + Acc; + true -> + maps:put(Topic, {Timestamp, Expiry}, Acc) + end; + (_, Acc) -> + Acc + end, #{}, Tab); + ets -> + % New code for ETS trie structure + Mod:foldl( + fun({_NodeId, Topic, #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry}, + timestamp = Timestamp}}, Acc) + when is_integer(Expiry) andalso + is_integer(Timestamp) -> + if Now - Timestamp >= Expiry -> + Mod:delete_object(Tab, {_NodeId, Topic, '_'}), + Acc; + true -> + maps:put(Topic, {Timestamp, Expiry}, Acc) + end; + (_, Acc) -> + Acc + end, #{}, Tab) + end. %% Retained messages written in 3.12 (or earlier) are converted when read in 3.13 (or later). -spec convert_mqtt_msg(mqtt_msg_v0()) -> mqtt_msg(). diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl index 322202bda2d2..f40bd04ec70a 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retainer.erl @@ -11,21 +11,19 @@ -behaviour(gen_server). --export([init/1, handle_call/3, handle_cast/2, handle_info/2, - terminate/2, start_link/1]). - +-export([init/1, handle_call/3, handle_cast/2, handle_info/2, terminate/2, start_link/1]). -export([retain/3, fetch/2, clear/2]). -define(TIMEOUT, 30_000). - -define(STATE, ?MODULE). --record(?STATE, {store_state :: rabbit_mqtt_retained_msg_store:state(), - expire :: #{topic() := TimerRef :: reference()} - }). + +-record(?STATE, + {store_state :: rabbit_mqtt_retained_msg_store:state(), + expire :: #{topic() := TimerRef :: reference()}}). + -type state() :: #?STATE{}. --spec start_link(rabbit_types:vhost()) -> - gen_server:start_ret(). +-spec start_link(rabbit_types:vhost()) -> gen_server:start_ret(). start_link(VHost) -> gen_server:start_link(?MODULE, VHost, []). @@ -33,8 +31,7 @@ start_link(VHost) -> retain(Pid, Topic, Msg = #mqtt_msg{retain = true}) -> gen_server:cast(Pid, {retain, Topic, Msg}). --spec fetch(pid(), topic()) -> - undefined | mqtt_msg() | [mqtt_msg()]. +-spec fetch(pid(), topic()) -> undefined | mqtt_msg() | [mqtt_msg()]. fetch(Pid, Topic) -> gen_server:call(Pid, {fetch, Topic}, ?TIMEOUT). @@ -42,75 +39,89 @@ fetch(Pid, Topic) -> clear(Pid, Topic) -> gen_server:cast(Pid, {clear, Topic}). --spec init(rabbit_types:vhost()) -> - {ok, state()}. +-spec init(rabbit_types:vhost()) -> {ok, state()}. init(VHost) -> process_flag(trap_exit, true), {StoreState, Expire0} = rabbit_mqtt_retained_msg_store:start(VHost), Now = os:system_time(second), - Expire = maps:map(fun(Topic, {Timestamp, Expiry}) -> - TimerSecs = max(0, Expiry - (Now - Timestamp)), - start_timer(TimerSecs, Topic) - end, Expire0), - {ok, #?STATE{store_state = StoreState, - expire = Expire}}. + Expire = + maps:map(fun(Topic, {Timestamp, Expiry}) -> + TimerSecs = max(0, Expiry - (Now - Timestamp)), + start_timer(TimerSecs, Topic) + end, + Expire0), + {ok, #?STATE{store_state = StoreState, expire = Expire}}. handle_cast({retain, Topic, Msg0 = #mqtt_msg{props = Props}}, - State = #?STATE{store_state = StoreState, - expire = Expire0}) -> - Expire2 = case maps:take(Topic, Expire0) of - {OldTimer, Expire1} -> - cancel_timer(OldTimer), - Expire1; - error -> - Expire0 - end, - {Msg, Expire} = case maps:find('Message-Expiry-Interval', Props) of - {ok, ExpirySeconds} -> - Timer = start_timer(ExpirySeconds, Topic), - {Msg0#mqtt_msg{timestamp = os:system_time(second)}, - maps:put(Topic, Timer, Expire2)}; - error -> - {Msg0, Expire2} - end, + State = #?STATE{store_state = StoreState, expire = Expire0}) -> + Expire2 = + case maps:take(Topic, Expire0) of + {OldTimer, Expire1} -> + cancel_timer(OldTimer), + Expire1; + error -> + Expire0 + end, + {Msg, Expire} = + case maps:find('Message-Expiry-Interval', Props) of + {ok, ExpirySeconds} -> + Timer = start_timer(ExpirySeconds, Topic), + {Msg0#mqtt_msg{timestamp = os:system_time(second)}, + maps:put(Topic, Timer, Expire2)}; + error -> + {Msg0, Expire2} + end, ok = rabbit_mqtt_retained_msg_store:insert(Topic, Msg, StoreState), {noreply, State#?STATE{expire = Expire}}; -handle_cast({clear, Topic}, State = #?STATE{store_state = StoreState, - expire = Expire0}) -> - Expire = case maps:take(Topic, Expire0) of - {OldTimer, Expire1} -> - cancel_timer(OldTimer), - Expire1; - error -> - Expire0 - end, +handle_cast({clear, Topic}, + State = #?STATE{store_state = StoreState, expire = Expire0}) -> + Expire = + case maps:take(Topic, Expire0) of + {OldTimer, Expire1} -> + cancel_timer(OldTimer), + Expire1; + error -> + Expire0 + end, ok = rabbit_mqtt_retained_msg_store:delete(Topic, StoreState), {noreply, State#?STATE{expire = Expire}}. -% TODO: handle listof messages handle_call({fetch, Topic}, _From, State = #?STATE{store_state = StoreState}) -> - Reply = case rabbit_mqtt_retained_msg_store:lookup(Topic, StoreState) of - #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry0} = Props, - timestamp = Timestamp} = MqttMsg -> - %% “The PUBLISH packet sent to a Client by the Server MUST contain a Message - %% Expiry Interval set to the received value minus the time that the - %% Application Message has been waiting in the Server [MQTT-3.3.2-6].” - Expiry = max(0, Expiry0 - (os:system_time(second) - Timestamp)), - MqttMsg#mqtt_msg{props = maps:put('Message-Expiry-Interval', Expiry, Props)}; - Other -> - Other - end, + Reply = + case rabbit_mqtt_retained_msg_store:lookup(Topic, StoreState) of + [] -> + undefined; + Messages when is_list(Messages) -> + lists:map(fun update_message_expiry/1, Messages); + SingleMessage = #mqtt_msg{} -> + update_message_expiry(SingleMessage); + Other -> + Other + end, {reply, Reply, State}. -handle_info({timeout, Timer, Topic}, State = #?STATE{store_state = StoreState, - expire = Expire0}) -> - Expire = case maps:take(Topic, Expire0) of - {Timer, Expire1} -> - ok = rabbit_mqtt_retained_msg_store:delete(Topic, StoreState), - Expire1; - _ -> - Expire0 - end, +-spec update_message_expiry(mqtt_msg()) -> mqtt_msg(). +update_message_expiry(#mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry0} = Props, + timestamp = Timestamp} = + MqttMsg) -> + %% "The PUBLISH packet sent to a Client by the Server MUST contain a Message + %% Expiry Interval set to the received value minus the time that the + %% Application Message has been waiting in the Server [MQTT-3.3.2-6]." + Expiry = max(0, Expiry0 - (os:system_time(second) - Timestamp)), + MqttMsg#mqtt_msg{props = maps:put('Message-Expiry-Interval', Expiry, Props)}; +update_message_expiry(MqttMsg) -> + MqttMsg. + +handle_info({timeout, Timer, Topic}, + State = #?STATE{store_state = StoreState, expire = Expire0}) -> + Expire = + case maps:take(Topic, Expire0) of + {Timer, Expire1} -> + ok = rabbit_mqtt_retained_msg_store:delete(Topic, StoreState), + Expire1; + _ -> + Expire0 + end, {noreply, State#?STATE{expire = Expire}}; handle_info(stop, State) -> {stop, normal, State}; @@ -121,11 +132,10 @@ terminate(_Reason, #?STATE{store_state = StoreState}) -> rabbit_mqtt_retained_msg_store:terminate(StoreState). -spec start_timer(integer(), topic()) -> reference(). -start_timer(Seconds, Topic) - when is_binary(Topic) -> - erlang:start_timer(timer:seconds(Seconds), self(), Topic). +start_timer(Seconds, Topic) when is_binary(Topic) -> + erlang:start_timer( + timer:seconds(Seconds), self(), Topic). -spec cancel_timer(reference()) -> ok. cancel_timer(TimerRef) -> - ok = erlang:cancel_timer(TimerRef, [{async, true}, - {info, false}]). + ok = erlang:cancel_timer(TimerRef, [{async, true}, {info, false}]). From cca66b31ab5147e7cb5ef8c7433fd8cd4661c8bb Mon Sep 17 00:00:00 2001 From: getlarge Date: Sun, 19 Jan 2025 20:32:19 +0100 Subject: [PATCH 14/25] fix(rabbitmq_mqtt): enhance ETS table management and recovery process --- .../rabbit_mqtt_retained_msg_store_ets.erl | 101 ++++++++++++------ 1 file changed, 70 insertions(+), 31 deletions(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl index d83ef359bef0..b79326738449 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl @@ -11,7 +11,9 @@ -include("rabbit_mqtt_packet.hrl"). --export([new/2, recover/2, insert/3, lookup/2, delete/2, terminate/1]). +-include_lib("kernel/include/logger.hrl"). + +-export([new/2, recover/2, insert/3, lookup/2, delete/2, terminate/1, get_tables/1]). % TODO: -define(DEFAULT_MATCH_LIMIT, 1000). @@ -27,18 +29,16 @@ -spec new(file:name_all(), rabbit_types:vhost()) -> store_state(). new(Dir, VHost) -> - BaseTableName = rabbit_mqtt_util:vhost_name_to_table_name(VHost), - % Clean up any existing files delete_table_files(Dir, VHost), % Node table - will store tuples of {node_id, edge_count, is_topic} - NodeTable = ets:new(append_suffix(BaseTableName, "_nodes"), [set, public]), + NodeTable = ets:new(get_table_name(VHost, <<"nodes">>), [set, public]), % Edge table - will store {{from_id, word}, to_id} - EdgeTable = ets:new(append_suffix(BaseTableName, "_edges"), [ordered_set, public]), + EdgeTable = ets:new(get_table_name(VHost, <<"edges">>), [ordered_set, public]), % Topic table - will store {node_id, topic, value} % TODO: consider whether, set might still be the best option here - MsgTable = ets:new(append_suffix(BaseTableName, "_msgs"), [bag, public]), + MsgTable = ets:new(get_table_name(VHost, <<"msgs">>), [bag, public]), RootId = make_node_id(), ets:insert(NodeTable, {RootId, 0, false}), @@ -54,13 +54,24 @@ new(Dir, VHost) -> {ok, store_state(), rabbit_mqtt_retained_msg_store:expire()} | {error, uninitialized}. recover(Dir, VHost) -> + io:format("Recovering MQTT retained message store from ~s~n", [Dir]), try - {ok, NodeTable} = recover_table(Dir, VHost, "nodes"), - {ok, EdgeTable} = recover_table(Dir, VHost, "edges"), - {ok, MsgTable} = recover_table(Dir, VHost, "msgs"), - - % Find root node (should be the only node with no incoming edges) - [{RootId, 0, false}] = ets:match_object(NodeTable, {'$1', 0, false}), + {ok, MsgTable} = recover_table(Dir, VHost, <<"msgs">>), + Expire = rabbit_mqtt_retained_msg_store:expire(ets, MsgTable), + {ok, NodeTable} = recover_table(Dir, VHost, <<"nodes">>), + {ok, EdgeTable} = recover_table(Dir, VHost, <<"edges">>), + + RootId = + case find_root_node(NodeTable, EdgeTable) of + {ok, Id} -> + io:format("Recovered existing RootId: ~p~n", [Id]), + Id; + error -> + NewId = make_node_id(), + io:format("Creating new RootId: ~p~n", [NewId]), + ets:insert(NodeTable, {NewId, 0, false}), + NewId + end, State = #store_state{node_table = NodeTable, @@ -69,13 +80,10 @@ recover(Dir, VHost) -> root_id = RootId, dir = Dir, vhost = VHost}, - - delete_table_files(Dir, VHost), - - % ? should we expire all tables here? - {ok, State, rabbit_mqtt_retained_msg_store:expire(ets, MsgTable)} + {ok, State, Expire} catch - _:_ -> + error:Reason -> + ?LOG_ERROR("~s failed to recover MQTT retained message store: ~p", [?MODULE, Reason]), {error, uninitialized} end. @@ -84,20 +92,20 @@ terminate(#store_state{node_table = NodeTable, edge_table = EdgeTable, msg_table = MsgTable, dir = Dir, - vhost = VHost} = - _State) -> + vhost = VHost}) -> ok = ets:tab2file(NodeTable, - get_table_path(Dir, VHost, "nodes"), + get_table_path(Dir, VHost, <<"nodes">>), [{extended_info, [object_count]}]), ok = ets:tab2file(EdgeTable, - get_table_path(Dir, VHost, "edges"), + get_table_path(Dir, VHost, <<"edges">>), [{extended_info, [object_count]}]), ok = ets:tab2file(MsgTable, - get_table_path(Dir, VHost, "msgs"), - [{extended_info, [object_count]}]). + get_table_path(Dir, VHost, <<"msgs">>), + [{extended_info, [object_count]}]), + ok. -spec insert(topic(), mqtt_msg(), store_state()) -> ok. insert(Topic, Msg, #store_state{} = State) -> @@ -129,7 +137,6 @@ delete(Topic, State) -> Words = split_topic(Topic), case follow_path(Words, State) of {ok, NodeId} -> - % Remove message ets:delete_object(State#store_state.msg_table, {NodeId, Topic, '_'}), % If no more messages at this node, mark as non-topic case ets:lookup(State#store_state.msg_table, NodeId) of @@ -145,31 +152,63 @@ delete(Topic, State) -> end, ok. +-spec get_tables(store_state()) -> {ets:tid(), ets:tid(), ets:tid()}. +get_tables(#store_state{node_table = NodeTable, + edge_table = EdgeTable, + msg_table = MsgTable}) -> + {NodeTable, EdgeTable, MsgTable}. + %% Internal setup/teardown functions +-spec get_table_name(rabbit_types:vhost(), binary()) -> atom(). +get_table_name(VHost, Type) -> + TableName = rabbit_mqtt_util:vhost_name_to_table_name(VHost), + Suffix = iolist_to_binary([<<"_">>, Type]), + list_to_atom(atom_to_list(TableName) ++ binary_to_list(Suffix)). -append_suffix(TableName, Suffix) -> - list_to_atom(atom_to_list(TableName) ++ Suffix). +-spec get_table_path(file:name_all(), rabbit_types:vhost(), binary()) -> file:name_all(). +get_table_path(Dir, VHost, Type) -> + rabbit_mqtt_util:path_for(Dir, iolist_to_binary([VHost, Type])). +-spec delete_table_files(file:name_all(), rabbit_types:vhost()) -> ok. delete_table_files(Dir, VHost) -> Types = ["nodes", "edges", "msgs"], lists:foreach(fun(Type) -> delete_table(Dir, VHost, Type) end, Types), ok. +-spec delete_table(file:name_all(), rabbit_types:vhost(), binary()) -> ok. delete_table(Dir, VHost, Type) -> Path = get_table_path(Dir, VHost, Type), file:delete(Path). +-spec recover_table(file:name_all(), rabbit_types:vhost(), binary()) -> {ok, ets:tid()}. recover_table(Dir, VHost, Type) -> Path = get_table_path(Dir, VHost, Type), - ets:file2tab(Path). - -get_table_path(Dir, VHost, Type) -> - rabbit_mqtt_util:path_for(Dir, iolist_to_binary([VHost, Type])). + case ets:file2tab(Path) of + {ok, Tid} -> + _ = file:delete(Path), + {ok, Tid} + end. % Internal trie methods split_topic(Topic) -> binary:split(Topic, <<"/">>, [global]). +% This might not be the most efficient way to find the root node, but the following options: +% Store root ID separately need additional storage/persistence and could get out of sync +% First node in table, requires ordered_set which could bring performance hit during lookup +find_root_node(NodeTable, EdgeTable) -> + NodeIds = ets:match(NodeTable, {'$1', '_', '_'}), + DestNodeIds = ets:match(EdgeTable, {'_', '$1'}), + % Find the node that doesn't appear as a destination in any edge + case lists:flatten(NodeIds) -- lists:flatten(DestNodeIds) of + [RootId] -> + {ok, RootId}; + [] -> + error; + _ -> + error % Multiple root nodes would indicate corruption + end. + follow_or_create_path(Words, State) -> follow_or_create_path(Words, State#store_state.root_id, State). From 7a6250e83a9177f3e18a1a8d695fbfbc488597f4 Mon Sep 17 00:00:00 2001 From: getlarge Date: Sun, 19 Jan 2025 20:35:03 +0100 Subject: [PATCH 15/25] fix(rabbitmq_mqtt): replace ets:delete_object with ets:match_delete --- deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl index b79326738449..27943eb7262b 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl @@ -137,7 +137,7 @@ delete(Topic, State) -> Words = split_topic(Topic), case follow_path(Words, State) of {ok, NodeId} -> - ets:delete_object(State#store_state.msg_table, {NodeId, Topic, '_'}), + ets:match_delete(State#store_state.msg_table, {NodeId, Topic, '_'}), % If no more messages at this node, mark as non-topic case ets:lookup(State#store_state.msg_table, NodeId) of [] -> From 54c992d06f45851db544941ca6c833e2b6b37a10 Mon Sep 17 00:00:00 2001 From: getlarge Date: Sun, 19 Jan 2025 20:37:48 +0100 Subject: [PATCH 16/25] test(rabbitmq_mqtt): update retained message tests and add recovery scenario --- ...bbit_mqtt_retained_msg_store_ets_SUITE.erl | 193 +++++++++++++++--- 1 file changed, 169 insertions(+), 24 deletions(-) diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl index c712d72a54d2..3eff47270574 100644 --- a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl @@ -8,6 +8,7 @@ -compile([export_all, nowarn_export_all]). +-include_lib("rabbitmq_mqtt/include/rabbit_mqtt_packet.hrl"). -include_lib("eunit/include/eunit.hrl"). -include_lib("common_test/include/ct.hrl"). @@ -21,7 +22,8 @@ groups() -> test_delete, test_plus_wildcard, test_hash_wildcard, - test_combined_wildcards]}]. + test_combined_wildcards, + test_recovery]}]. init_per_suite(Config) -> Config. @@ -36,7 +38,6 @@ end_per_group(_Group, _Config) -> ok. init_per_testcase(_TestCase, Config) -> - % Create a unique directory for each test case Dir = filename:join(["/tmp", "mqtt_test_" ++ integer_to_list(erlang:unique_integer())]), ok = filelib:ensure_dir(Dir ++ "/"), State = rabbit_mqtt_retained_msg_store_ets:new(Dir, <<"test">>), @@ -46,7 +47,6 @@ end_per_testcase(_TestCase, Config) -> State = ?config(store_state, Config), Dir = ?config(test_dir, Config), rabbit_mqtt_retained_msg_store_ets:terminate(State), - % Clean up test directory os:cmd("rm -rf " ++ Dir), ok. @@ -56,49 +56,194 @@ end_per_testcase(_TestCase, Config) -> test_add_and_match(Config) -> State = ?config(store_state, Config), - - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), + Msg1 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/c">>, + dup = false, + payload = <<"msg1">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg2 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/d">>, + dup = false, + payload = <<"msg2">>, + props = #{}, + timestamp = os:system_time(second)}, + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), Matches1 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/d">>, <<"msg2">>, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/d">>, Msg2, State), Matches2 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/d">>, State), NoMatches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"x/y/z">>, State), - ?_assertEqual([<<"msg1">>], Matches1), - ?_assertEqual([<<"msg2">>], Matches2), - ?_assertEqual([], NoMatches). + ?assertEqual([Msg1], Matches1), + ?assertEqual([Msg2], Matches2), + ?assertEqual([], NoMatches). test_delete(Config) -> State = ?config(store_state, Config), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), + Msg1 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/c">>, + dup = false, + payload = <<"msg1">>, + props = #{}, + timestamp = os:system_time(second)}, + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), ok = rabbit_mqtt_retained_msg_store_ets:delete(<<"a/b/c">>, State), Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), - ?_assertEqual([], Matches). + ?assertEqual([], Matches). test_plus_wildcard(Config) -> State = ?config(store_state, Config), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c">>, <<"msg2">>, State), + Msg1 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/c">>, + dup = false, + payload = <<"msg1">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg2 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/x/c">>, + dup = false, + payload = <<"msg2">>, + props = #{}, + timestamp = os:system_time(second)}, + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c">>, Msg2, State), Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/+/c">>, State), - ?_assertEqual(lists:sort([<<"msg1">>, <<"msg2">>]), lists:sort(Matches)). + ?assertEqual(lists:sort([Msg1, Msg2]), lists:sort(Matches)). test_hash_wildcard(Config) -> State = ?config(store_state, Config), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, <<"msg1">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, <<"msg2">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/x/y">>, <<"msg3">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/q/x/y">>, <<"msg3">>, State), + Msg1 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/c">>, + dup = false, + payload = <<"msg1">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg2 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/c/d">>, + dup = false, + payload = <<"msg2">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg3 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/x/y">>, + dup = false, + payload = <<"msg3">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg4 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/q/x/y">>, + dup = false, + payload = <<"msg4">>, + props = #{}, + timestamp = os:system_time(second)}, + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, Msg2, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/x/y">>, Msg3, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/q/x/y">>, Msg4, State), Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/#">>, State), - ?_assertEqual([<<"msg1">>, <<"msg2">>, <<"msg3">>], lists:sort(Matches)). + ?assertEqual([Msg1, Msg2, Msg3], lists:sort(Matches)). test_combined_wildcards(Config) -> State = ?config(store_state, Config), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, <<"msg1">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c/e">>, <<"msg2">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/c/f/g">>, <<"msg3">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/d/f/g">>, <<"msg4">>, State), + Msg1 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/c">>, + dup = false, + payload = <<"msg1">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg2 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/d">>, + dup = false, + payload = <<"msg2">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg3 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/x/c/e">>, + dup = false, + payload = <<"msg3">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg4 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/y/c/f/g">>, + dup = false, + payload = <<"msg4">>, + props = #{}, + timestamp = os:system_time(second)}, + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, Msg1, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c/e">>, Msg2, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/c/f/g">>, Msg3, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/d/f/g">>, Msg4, State), Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/+/c/#">>, State), - ?_assertEqual([<<"msg1">>, <<"msg2">>, <<"msg3">>], lists:sort(Matches)). + ?assertEqual([Msg1, Msg2, Msg3], lists:sort(Matches)). + +test_recovery(Config) -> + State = ?config(store_state, Config), + Msg1 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/c">>, + dup = false, + payload = <<"msg1">>, + props = #{}, + timestamp = os:system_time(second)}, + Msg2 = + #mqtt_msg{retain = true, + qos = 0, + topic = <<"a/b/d">>, + dup = false, + payload = <<"msg2">>, + props = #{}, + timestamp = os:system_time(second)}, + + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), + ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/d">>, Msg2, State), + Matches1 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), + Matches2 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/d">>, State), + NoMatches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"x/y/z">>, State), + ?assertEqual([Msg1], Matches1), + ?assertEqual([Msg2], Matches2), + ?assertEqual([], NoMatches), + % Recover the state + ok = rabbit_mqtt_retained_msg_store_ets:terminate(State), + {ok, Filenames} = file:list_dir(?config(test_dir, Config)), + ?assertEqual(3, length(Filenames)), + + {ok, State2, _Expire} = + rabbit_mqtt_retained_msg_store_ets:recover(?config(test_dir, Config), <<"test">>), + + Matches1 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State2), + Matches2 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/d">>, State2), + NoMatches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"x/y/z">>, State2), + ?assertEqual([Msg1], Matches1), + ?assertEqual([Msg2], Matches2), + ?assertEqual([], NoMatches). From 9bf8665641fcca5d4c4934e51363114fe745fe56 Mon Sep 17 00:00:00 2001 From: getlarge Date: Mon, 20 Jan 2025 21:24:44 +0100 Subject: [PATCH 17/25] refactor(rabbitmq_mqtt): add support for wildcard filtering for DETS store --- deps/rabbitmq_mqtt/BUILD.bazel | 5 - deps/rabbitmq_mqtt/Makefile | 2 +- .../src/rabbit_mqtt_retained_msg_store.erl | 148 ++++---- .../rabbit_mqtt_retained_msg_store_dets.erl | 320 +++++++++++++++--- .../rabbit_mqtt_retained_msg_store_ets.erl | 16 +- .../rabbit_mqtt_retained_msg_store_noop.erl | 2 +- 6 files changed, 349 insertions(+), 144 deletions(-) diff --git a/deps/rabbitmq_mqtt/BUILD.bazel b/deps/rabbitmq_mqtt/BUILD.bazel index ec37f0556fa1..49853b99a788 100644 --- a/deps/rabbitmq_mqtt/BUILD.bazel +++ b/deps/rabbitmq_mqtt/BUILD.bazel @@ -301,11 +301,6 @@ rabbitmq_suite( ], ) -rabbitmq_suite( - name = "globber_SUITE", - size = "small", -) - assert_suites() alias( diff --git a/deps/rabbitmq_mqtt/Makefile b/deps/rabbitmq_mqtt/Makefile index c47b7c5670c0..9b73227950f1 100644 --- a/deps/rabbitmq_mqtt/Makefile +++ b/deps/rabbitmq_mqtt/Makefile @@ -94,7 +94,7 @@ define ct_master.erl halt(0) endef -PARALLEL_CT_SET_1_A = auth globber retainer +PARALLEL_CT_SET_1_A = auth rabbit_mqtt_retained_msg_store retainer PARALLEL_CT_SET_1_B = cluster command config config_schema mc_mqtt packet_prop \ processor protocol_interop proxy_protocol rabbit_mqtt_confirms reader util PARALLEL_CT_SET_1_C = java v5 diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl index 5af9c3359d6c..73d0fa936d94 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl @@ -20,125 +20,101 @@ -include("rabbit_mqtt.hrl"). -include("rabbit_mqtt_packet.hrl"). + -include_lib("kernel/include/logger.hrl"). + -export([start/1, insert/3, lookup/2, delete/2, terminate/1]). -export([expire/2]). + -export_type([state/0, expire/0]). -define(STATE, ?MODULE). --record(?STATE, {store_mod :: module(), - store_state :: term()}). --opaque state() :: #?STATE{}. - --type expire() :: #{topic() := - {InsertionTimestamp :: integer(), - MessageExpiryInterval :: pos_integer()}}. --callback new(Directory :: file:name_all(), rabbit_types:vhost()) -> - State :: any(). +-record(?STATE, {store_mod :: module(), store_state :: term()}). --callback recover(Directory :: file:name_all(), rabbit_types:vhost()) -> - {ok, State :: any(), expire()} | - {error, uninitialized}. +-opaque state() :: #?STATE{}. --callback insert(topic(), mqtt_msg(), State :: any()) -> - ok. +-type expire() :: + #{topic() := {InsertionTimestamp :: integer(), MessageExpiryInterval :: pos_integer()}}. +-callback new(Directory :: file:name_all(), rabbit_types:vhost()) -> State :: any(). +-callback recover(Directory :: file:name_all(), rabbit_types:vhost()) -> + {ok, State :: any(), expire()} | {error, uninitialized}. +-callback insert(topic(), mqtt_msg(), State :: any()) -> ok. -callback lookup(topic(), State :: any()) -> - [mqtt_msg()] | mqtt_msg() | mqtt_msg_v0() | undefined. - --callback delete(topic(), State :: any()) -> - ok. - --callback terminate(State :: any()) -> - ok. + [mqtt_msg()] | [mqtt_msg_v0()] | []. +-callback delete(topic(), State :: any()) -> ok. +-callback terminate(State :: any()) -> ok. -spec start(rabbit_types:vhost()) -> {state(), expire()}. start(VHost) -> {ok, Mod} = application:get_env(?APP_NAME, retained_message_store), Dir = rabbit:data_dir(), - ?LOG_INFO("Starting MQTT retained message store ~s for vhost '~ts'", - [Mod, VHost]), - {S, Expire} = case Mod:recover(Dir, VHost) of - {ok, StoreState, Expire0} -> - ?LOG_INFO("Recovered MQTT retained message store ~s for vhost '~ts'", - [Mod, VHost]), - {StoreState, Expire0}; - {error, uninitialized} -> - StoreState = Mod:new(Dir, VHost), - ?LOG_INFO("Initialized MQTT retained message store ~s for vhost '~ts'", - [Mod, VHost]), - {StoreState, #{}} - end, - {#?STATE{store_mod = Mod, - store_state = S}, Expire}. + ?LOG_INFO("Starting MQTT retained message store ~s for vhost '~ts'", [Mod, VHost]), + {S, Expire} = + case Mod:recover(Dir, VHost) of + {ok, StoreState, Expire0} -> + ?LOG_INFO("Recovered MQTT retained message store ~s for vhost '~ts'", [Mod, VHost]), + {StoreState, Expire0}; + {error, uninitialized} -> + StoreState = Mod:new(Dir, VHost), + ?LOG_INFO("Initialized MQTT retained message store ~s for vhost '~ts'", + [Mod, VHost]), + {StoreState, #{}} + end, + {#?STATE{store_mod = Mod, store_state = S}, Expire}. -spec insert(topic(), mqtt_msg(), state()) -> ok. -insert(Topic, Msg, #?STATE{store_mod = Mod, - store_state = StoreState}) -> +insert(Topic, Msg, #?STATE{store_mod = Mod, store_state = StoreState}) -> ok = Mod:insert(Topic, Msg, StoreState). -% TODO: only return list of retained messages --spec lookup(topic(), state()) -> - mqtt_msg() | [mqtt_msg()] | undefined. -lookup(Topic, #?STATE{store_mod = Mod, - store_state = StoreState}) -> +-spec lookup(topic(), state()) -> [mqtt_msg()] | []. +lookup(Topic, #?STATE{store_mod = Mod, store_state = StoreState}) -> case Mod:lookup(Topic, StoreState) of + % Handle single old message format (legacy case) OldMsg when is_record(OldMsg, mqtt_msg, 7) -> - convert_mqtt_msg(OldMsg); - Other -> - Other + [convert_mqtt_msg(OldMsg)]; + % Handle list of messages - convert any old format ones + Messages when is_list(Messages) -> + lists:map(fun (Msg) when is_record(Msg, mqtt_msg, 7) -> + convert_mqtt_msg(Msg); + (Msg) -> + Msg + end, + Messages); + undefined -> + []; + [] -> + [] end. -spec delete(topic(), state()) -> ok. -delete(Topic, #?STATE{store_mod = Mod, - store_state = StoreState}) -> +delete(Topic, #?STATE{store_mod = Mod, store_state = StoreState}) -> ok = Mod:delete(Topic, StoreState). -spec terminate(state()) -> ok. -terminate(#?STATE{store_mod = Mod, - store_state = StoreState}) -> +terminate(#?STATE{store_mod = Mod, store_state = StoreState}) -> ok = Mod:terminate(StoreState). -% TODO: refactor when DETS also supports the new ETS trie structure -spec expire(ets | dets, ets:tid() | dets:tab_name()) -> expire(). expire(Mod, Tab) -> Now = os:system_time(second), - case Mod of - dets -> - % Original code for DETS - Mod:foldl( - fun(#retained_message{topic = Topic, - mqtt_msg = #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry}, - timestamp = Timestamp}}, Acc) - when is_integer(Expiry) andalso - is_integer(Timestamp) -> - if Now - Timestamp >= Expiry -> - Mod:delete(Tab, Topic), - Acc; - true -> - maps:put(Topic, {Timestamp, Expiry}, Acc) - end; - (_, Acc) -> - Acc - end, #{}, Tab); - ets -> - % New code for ETS trie structure - Mod:foldl( - fun({_NodeId, Topic, #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry}, - timestamp = Timestamp}}, Acc) - when is_integer(Expiry) andalso - is_integer(Timestamp) -> - if Now - Timestamp >= Expiry -> - Mod:delete_object(Tab, {_NodeId, Topic, '_'}), - Acc; - true -> - maps:put(Topic, {Timestamp, Expiry}, Acc) - end; - (_, Acc) -> - Acc - end, #{}, Tab) - end. + ExpireMsg = + fun ({NodeId, + Topic, + #mqtt_msg{props = #{'Message-Expiry-Interval' := Expiry}, timestamp = Timestamp}}, + Acc) + when is_integer(Expiry) andalso is_integer(Timestamp) -> + if Now - Timestamp >= Expiry -> + Mod:match_delete(Tab, {NodeId, Topic, '_'}), + Acc; + true -> + maps:put(Topic, {Timestamp, Expiry}, Acc) + end; + (_, Acc) -> + Acc + end, + Mod:foldl(ExpireMsg, #{}, Tab). %% Retained messages written in 3.12 (or earlier) are converted when read in 3.13 (or later). -spec convert_mqtt_msg(mqtt_msg_v0()) -> mqtt_msg(). diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl index bbae75f1829b..50da9b81c7d1 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl @@ -2,7 +2,7 @@ %% License, v. 2.0. If a copy of the MPL was not distributed with this %% file, You can obtain one at https://mozilla.org/MPL/2.0/. %% -%% Copyright (c) 2007-2024 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. +%% Copyright (c) 2007-2025 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. %% -module(rabbit_mqtt_retained_msg_store_dets). @@ -10,60 +10,300 @@ -behaviour(rabbit_mqtt_retained_msg_store). -include("rabbit_mqtt_packet.hrl"). + -include_lib("kernel/include/logger.hrl"). -export([new/2, recover/2, insert/3, lookup/2, delete/2, terminate/1]). --record(store_state, {table :: dets:tab_name()}). +-record(store_state, + {node_table :: dets:tab_name(), % Stores {node_id, edge_count, is_topic} + edge_table :: dets:tab_name(), % Stores {{from_id, word}, to_id} + msg_table :: dets:tab_name(), % Stores {node_id, topic, mqtt_msg} + root_id :: binary(), % Root node ID + dir :: file:filename_all(), + vhost :: rabbit_types:vhost()}). -type store_state() :: #store_state{}. -spec new(file:name_all(), rabbit_types:vhost()) -> store_state(). new(Dir, VHost) -> - {ok, TabName} = open_table(Dir, VHost), - #store_state{table = TabName}. + % delete_table_files(Dir, VHost), + {ok, NodeTable} = open_table(Dir, VHost, <<"nodes">>, set), + {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), + {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, bag), + RootId = make_node_id(), + ok = dets:insert(NodeTable, {RootId, 0, false}), + + #store_state{node_table = NodeTable, + edge_table = EdgeTable, + msg_table = MsgTable, + root_id = RootId, + dir = Dir, + vhost = VHost}. -spec recover(file:name_all(), rabbit_types:vhost()) -> - {ok, store_state(), rabbit_mqtt_retained_msg_store:expire()} | - {error, uninitialized}. + {ok, store_state(), rabbit_mqtt_retained_msg_store:expire()} | + {error, uninitialized}. recover(Dir, VHost) -> - case open_table(Dir, VHost) of - {ok, TabName} -> - {ok, - #store_state{table = TabName}, - rabbit_mqtt_retained_msg_store:expire(dets, TabName)}; - {error, Reason} -> - ?LOG_ERROR("~s failed to open table: ~p", [?MODULE, Reason]), - {error, uninitialized} - end. + try + {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, bag), + Expire = rabbit_mqtt_retained_msg_store:expire(dets, MsgTable), + {ok, NodeTable} = open_table(Dir, VHost, <<"nodes">>, set), + {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), --spec insert(topic(), mqtt_msg(), store_state()) -> ok. -insert(Topic, Msg, #store_state{table = T}) -> - ok = dets:insert(T, #retained_message{topic = Topic, mqtt_msg = Msg}). - --spec lookup(topic(), store_state()) -> - mqtt_msg() | mqtt_msg_v0() | undefined. -lookup(Topic, #store_state{table = T}) -> - case dets:lookup(T, Topic) of - [] -> undefined; - [#retained_message{mqtt_msg = Msg}] -> Msg + RootId = + case find_root_node(NodeTable, EdgeTable) of + {ok, Id} -> + ?LOG_INFO("Recovered existing RootId: ~p", [Id]), + Id; + error -> + NewId = make_node_id(), + ?LOG_INFO("Creating new RootId: ~p", [NewId]), + ok = dets:insert(NodeTable, {NewId, 0, false}), + NewId + end, + + State = + #store_state{node_table = NodeTable, + edge_table = EdgeTable, + msg_table = MsgTable, + root_id = RootId, + dir = Dir, + vhost = VHost}, + {ok, State, Expire} + catch + error:Reason -> + ?LOG_ERROR("Failed to recover MQTT retained message store: ~p", [Reason]), + {error, uninitialized} end. +-spec insert(topic(), mqtt_msg(), store_state()) -> ok. +insert(Topic, Msg, #store_state{} = State) -> + Words = split_topic(Topic), + Insert = + fun() -> + NodeId = follow_or_create_path(Words, State), + % Mark node as topic end and store message + update_node(NodeId, true, State), + % Replace any existing message for this topic + dets:match_delete(State#store_state.msg_table, {NodeId, Topic, '_'}), + dets:insert(State#store_state.msg_table, {NodeId, Topic, Msg}) + end, + Insert(), + %? TODO: Wrap in transaction since we're modifying multiple tables + ok. + +-spec lookup(topic(), store_state()) -> [mqtt_msg()] | [mqtt_msg_v0()] | []. +lookup(Topic, #store_state{} = State) -> + Words = split_topic(Topic), + Matches = match_pattern_words(Words, State#store_state.root_id, State, []), + Values = + lists:flatmap(fun(NodeId) -> + case dets:lookup(State#store_state.msg_table, NodeId) of + [{_NodeId, _Topic, Value} | _] -> [Value]; + [] -> [] + end + end, + Matches), + Values. + -spec delete(topic(), store_state()) -> ok. -delete(Topic, #store_state{table = T}) -> - ok = dets:delete(T, Topic). +delete(Topic, State) -> + Words = split_topic(Topic), + % Wrap in transaction since we're modifying multiple tables + Delete = + fun() -> + case follow_path(Words, State) of + {ok, NodeId} -> + dets:match_delete(State#store_state.msg_table, {NodeId, Topic, '_'}), + case dets:lookup(State#store_state.msg_table, NodeId) of + [] -> + update_node(NodeId, false, State), + maybe_clean_path(NodeId, State); + _ -> ok + end; + error -> ok + end + end, + Delete(), + %? TODO: Wrap in transaction since we're modifying multiple tables + % rabbit_mnesia:execute_mnesia_transaction(Delete), + ok. -spec terminate(store_state()) -> ok. -terminate(#store_state{table = T}) -> - ok = dets:close(T). - -open_table(Dir, VHost) -> - Tab = rabbit_mqtt_util:vhost_name_to_table_name(VHost), - Path = rabbit_mqtt_util:path_for(Dir, VHost, ".dets"), - AutoSave = rabbit_misc:get_env(rabbit_mqtt, retained_message_store_dets_sync_interval, 2000), - dets:open_file(Tab, [{type, set}, - {keypos, #retained_message.topic}, - {file, Path}, - {ram_file, true}, - {repair, true}, - {auto_save, AutoSave}]). +terminate(#store_state{node_table = NodeTable, + edge_table = EdgeTable, + msg_table = MsgTable}) -> + ok = dets:close(NodeTable), + ok = dets:close(EdgeTable), + ok = dets:close(MsgTable), + ok. + +%% Internal functions + +split_topic(Topic) -> + binary:split(Topic, <<"/">>, [global]). + +make_node_id() -> + crypto:strong_rand_bytes(16). + +get_table_name(VHost, Type) -> + TableName = rabbit_mqtt_util:vhost_name_to_table_name(VHost), + Suffix = erlang:iolist_to_binary([<<"_">>, Type]), + erlang:list_to_atom(erlang:atom_to_list(TableName) ++ erlang:binary_to_list(Suffix)). + +get_table_path(Dir, VHost, Type) -> + % rabbit_mqtt_util:path_for(Dir, VHost, Type ++ ".dets"). + % Suffix = erlang:iolist_to_binary([VHost, <<"_">>, Type]), + rabbit_mqtt_util:path_for(Dir, erlang:iolist_to_binary([VHost, Type]), ".dets"). + +find_root_node(NodeTable, EdgeTable) -> + NodeIds = dets:match(NodeTable, {'$1', '_', '_'}), + DestNodeIds = dets:match(EdgeTable, {'_', '$1'}), + case lists:flatten(NodeIds) -- lists:flatten(DestNodeIds) of + [RootId] -> + {ok, RootId}; + [] -> + error; + _ -> + error % Multiple root nodes would indicate corruption + end. + +follow_or_create_path(Words, State) -> + follow_or_create_path(Words, State#store_state.root_id, State). + +follow_or_create_path([], NodeId, _State) -> + NodeId; +follow_or_create_path([Word | Rest], NodeId, State) -> + case find_edge(NodeId, Word, State) of + {ok, ChildId} -> + follow_or_create_path(Rest, ChildId, State); + error -> + ChildId = make_node_id(), + add_edge(NodeId, Word, ChildId, State), + follow_or_create_path(Rest, ChildId, State) + end. + +follow_path(Words, State) -> + follow_path(Words, State#store_state.root_id, State). + +follow_path([], NodeId, _State) -> + {ok, NodeId}; +follow_path([Word | Rest], NodeId, State) -> + case find_edge(NodeId, Word, State) of + {ok, ChildId} -> + follow_path(Rest, ChildId, State); + error -> + error + end. + +match_pattern_words([], NodeId, _State, Acc) -> + [NodeId | Acc]; +match_pattern_words([<<"+">> | RestWords], NodeId, State, Acc) -> + % + matches any single word + Edges = get_all_edges(NodeId, State), + lists:foldl(fun({_Key, ChildId}, EdgeAcc) -> + match_pattern_words(RestWords, ChildId, State, EdgeAcc) + end, + Acc, + Edges); +match_pattern_words([<<"#">> | _], NodeId, State, Acc) -> + % # matches zero or more words + collect_descendants(NodeId, State, [NodeId | Acc]); +match_pattern_words([Word | RestWords], NodeId, State, Acc) -> + case find_edge(NodeId, Word, State) of + {ok, ChildId} -> + match_pattern_words(RestWords, ChildId, State, Acc); + error -> + Acc + end. + +collect_descendants(NodeId, State, Acc) -> + Edges = get_all_edges(NodeId, State), + lists:foldl(fun({_Key, ChildId}, EdgeAcc) -> + collect_descendants(ChildId, State, [ChildId | EdgeAcc]) + end, + Acc, + Edges). + +find_edge(NodeId, Word, State) -> + Key = {NodeId, Word}, + case dets:lookup(State#store_state.edge_table, Key) of + [{_Key, ToNode}] -> + {ok, ToNode}; + [] -> + error + end. + +get_all_edges(NodeId, State) -> + Pattern = {{NodeId, '_'}, '_'}, + dets:match_object(State#store_state.edge_table, Pattern). + +add_edge(FromId, Word, ToId, State) -> + Key = {FromId, Word}, + EdgeEntry = {Key, ToId}, + ok = dets:insert(State#store_state.edge_table, EdgeEntry), + NodeEntry = {ToId, 0, false}, + ok = dets:insert(State#store_state.node_table, NodeEntry), + update_edge_count(FromId, +1, State). + +update_edge_count(NodeId, Delta, State) -> + case dets:lookup(State#store_state.node_table, NodeId) of + [{NodeId, EdgeCount, IsTopic}] -> + NewCount = EdgeCount + Delta, + ok = dets:insert(State#store_state.node_table, {NodeId, NewCount, IsTopic}); + [] -> + error + end. + +update_node(NodeId, IsTopic, State) -> + case dets:lookup(State#store_state.node_table, NodeId) of + [{NodeId, EdgeCount, _OldIsTopic}] -> + ok = dets:insert(State#store_state.node_table, {NodeId, EdgeCount, IsTopic}); + [] -> + error + end. + +maybe_clean_path(NodeId, State) -> + case dets:lookup(State#store_state.node_table, NodeId) of + [{NodeId, 0, false}] -> + Pattern = {'_', '_'}, + Edges = dets:match_object(State#store_state.edge_table, {Pattern, NodeId}), + case Edges of + [{{ParentId, Word}, NodeId}] -> + remove_edge(ParentId, Word, State), + ok = dets:delete(State#store_state.node_table, NodeId), + maybe_clean_path(ParentId, State); + [] -> + ok + end; + _ -> + ok + end. + +remove_edge(FromId, Word, State) -> + ok = dets:delete(State#store_state.edge_table, {FromId, Word}), + update_edge_count(FromId, -1, State). + +%% Setup functions + +open_table(Dir, VHost, Type, TableType) -> + Tab = get_table_name(VHost, Type), + % Path = get_table_path(Dir, VHost, Type), + AutoSave = + rabbit_misc:get_env(rabbit_mqtt, retained_message_store_dets_sync_interval, 2000), + dets:open_file(Tab, + [{type, TableType}, + % {file, Path}, + {ram_file, true}, + {repair, true}, + {auto_save, AutoSave}]). + +delete_table_files(Dir, VHost) -> + Types = [<<"nodes">>, <<"edges">>, <<"msgs">>], + lists:foreach(fun(Type) -> + Path = get_table_path(Dir, VHost, Type), + file:delete(Path) + end, + Types), + ok. diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl index 27943eb7262b..8a4ed273d9b3 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl @@ -13,7 +13,7 @@ -include_lib("kernel/include/logger.hrl"). --export([new/2, recover/2, insert/3, lookup/2, delete/2, terminate/1, get_tables/1]). +-export([new/2, recover/2, insert/3, lookup/2, delete/2, terminate/1]). % TODO: -define(DEFAULT_MATCH_LIMIT, 1000). @@ -118,7 +118,7 @@ insert(Topic, Msg, #store_state{} = State) -> ets:insert(State#store_state.msg_table, {NodeId, Topic, Msg}), ok. --spec lookup(topic(), store_state()) -> [mqtt_msg()] | [mqtt_msg_v0()] | undefined. +-spec lookup(topic(), store_state()) -> [mqtt_msg()] | [mqtt_msg_v0()] | []. lookup(Topic, #store_state{} = State) -> Words = split_topic(Topic), Matches = match_pattern_words(Words, State#store_state.root_id, State, []), @@ -152,22 +152,16 @@ delete(Topic, State) -> end, ok. --spec get_tables(store_state()) -> {ets:tid(), ets:tid(), ets:tid()}. -get_tables(#store_state{node_table = NodeTable, - edge_table = EdgeTable, - msg_table = MsgTable}) -> - {NodeTable, EdgeTable, MsgTable}. - %% Internal setup/teardown functions -spec get_table_name(rabbit_types:vhost(), binary()) -> atom(). get_table_name(VHost, Type) -> TableName = rabbit_mqtt_util:vhost_name_to_table_name(VHost), - Suffix = iolist_to_binary([<<"_">>, Type]), - list_to_atom(atom_to_list(TableName) ++ binary_to_list(Suffix)). + Suffix = erlang:iolist_to_binary([<<"_">>, Type]), + erlang:list_to_atom(erlang:atom_to_list(TableName) ++ erlang:binary_to_list(Suffix)). -spec get_table_path(file:name_all(), rabbit_types:vhost(), binary()) -> file:name_all(). get_table_path(Dir, VHost, Type) -> - rabbit_mqtt_util:path_for(Dir, iolist_to_binary([VHost, Type])). + rabbit_mqtt_util:path_for(Dir, erlang:iolist_to_binary([VHost, Type]), ".ets"). -spec delete_table_files(file:name_all(), rabbit_types:vhost()) -> ok. delete_table_files(Dir, VHost) -> diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_noop.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_noop.erl index bd798df5e923..4091e1efbb2c 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_noop.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_noop.erl @@ -21,7 +21,7 @@ insert(_Topic, _Msg, _State) -> ok. lookup(_Topic, _State) -> - undefined. + []. delete(_Topic, _State) -> ok. From 7767e62594429a01f3879a7f619d320e42420b32 Mon Sep 17 00:00:00 2001 From: getlarge Date: Mon, 20 Jan 2025 21:25:53 +0100 Subject: [PATCH 18/25] test(rabbitmq_mqtt): add wildcard filtering tests for DETS store --- deps/rabbitmq_mqtt/app.bzl | 7 +- ... rabbit_mqtt_retained_msg_store_SUITE.erl} | 126 +++++++++++------- deps/rabbitmq_mqtt/test/retainer_SUITE.erl | 9 +- 3 files changed, 83 insertions(+), 59 deletions(-) rename deps/rabbitmq_mqtt/test/{rabbit_mqtt_retained_msg_store_ets_SUITE.erl => rabbit_mqtt_retained_msg_store_SUITE.erl} (66%) diff --git a/deps/rabbitmq_mqtt/app.bzl b/deps/rabbitmq_mqtt/app.bzl index d10032ecb5a1..925cf98c2b44 100644 --- a/deps/rabbitmq_mqtt/app.bzl +++ b/deps/rabbitmq_mqtt/app.bzl @@ -329,12 +329,11 @@ def test_suite_beam_files(name = "test_suite_beam_files"): erlc_opts = "//:test_erlc_opts", deps = ["//deps/amqp_client:erlang_app", "//deps/rabbitmq_ct_helpers:erlang_app"], ) - erlang_bytecode( - name = "rabbit_mqtt_retained_msg_store_ets_SUITE_beam_files", + name = "rabbit_mqtt_retained_msg_store_SUITE_beam_files", testonly = True, - srcs = ["test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl"], - outs = ["test/rabbit_mqtt_retained_msg_store_ets_SUITE.beam"], + srcs = ["test/rabbit_mqtt_retained_msg_store_SUITE.erl"], + outs = ["test/rabbit_mqtt_retained_msg_store_SUITE.beam"], hdrs = ["include/rabbit_mqtt_packet.hrl"], app_name = "rabbitmq_mqtt", erlc_opts = "//:test_erlc_opts", diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl similarity index 66% rename from deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl rename to deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl index 3eff47270574..492cc8cf81a5 100644 --- a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_ets_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl @@ -4,7 +4,7 @@ %% %% Copyright (c) 2007-2024 Broadcom. All Rights Reserved. The term “Broadcom” refers to Broadcom Inc. and/or its subsidiaries. All rights reserved. %% --module(rabbit_mqtt_retained_msg_store_ets_SUITE). +-module(rabbit_mqtt_retained_msg_store_SUITE). -compile([export_all, nowarn_export_all]). @@ -13,17 +13,18 @@ -include_lib("common_test/include/ct.hrl"). all() -> - [{group, tests}]. + [{group, ets}, {group, dets}]. groups() -> - [{tests, - [parallel], - [test_add_and_match, - test_delete, - test_plus_wildcard, - test_hash_wildcard, - test_combined_wildcards, - test_recovery]}]. + [{dets, [parallel], tests()}, {ets, [parallel], tests()}]. + +tests() -> + [test_add_and_match, + test_delete, + test_plus_wildcard, + test_hash_wildcard, + test_combined_wildcards, + test_recovery]. init_per_suite(Config) -> Config. @@ -31,23 +32,43 @@ init_per_suite(Config) -> end_per_suite(_Config) -> ok. -init_per_group(_Group, Config) -> - Config. +init_per_group(G, Config) -> + case G of + ets -> + Module = rabbit_mqtt_retained_msg_store_ets; + dets -> + Module = rabbit_mqtt_retained_msg_store_dets + end, + [{module, Module} | Config]. end_per_group(_Group, _Config) -> ok. init_per_testcase(_TestCase, Config) -> + Mod = ?config(module, Config), + Dir = filename:join(["/tmp", "mqtt_test_" ++ integer_to_list(erlang:unique_integer())]), ok = filelib:ensure_dir(Dir ++ "/"), - State = rabbit_mqtt_retained_msg_store_ets:new(Dir, <<"test">>), + State = Mod:new(Dir, <<"test">>), [{store_state, State}, {test_dir, Dir} | Config]. end_per_testcase(_TestCase, Config) -> State = ?config(store_state, Config), + Mod = ?config(module, Config), Dir = ?config(test_dir, Config), - rabbit_mqtt_retained_msg_store_ets:terminate(State), - os:cmd("rm -rf " ++ Dir), + Mod:terminate(State), + + timer:sleep(100), + case file:del_dir_r(Dir) of + ok -> + ok; + {error, enoent} -> + ok; % Directory already gone + {error, Reason} -> + ct:pal("Failed to delete directory ~p: ~p", [Dir, Reason]), + % Try a more aggressive cleanup + os:cmd("rm -rf " ++ Dir) + end, ok. %%---------------------------------------------------------------------------- @@ -56,6 +77,8 @@ end_per_testcase(_TestCase, Config) -> test_add_and_match(Config) -> State = ?config(store_state, Config), + Mod = ?config(module, Config), + Msg1 = #mqtt_msg{retain = true, qos = 0, @@ -72,11 +95,11 @@ test_add_and_match(Config) -> payload = <<"msg2">>, props = #{}, timestamp = os:system_time(second)}, - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), - Matches1 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/d">>, Msg2, State), - Matches2 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/d">>, State), - NoMatches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"x/y/z">>, State), + ok = Mod:insert(<<"a/b/c">>, Msg1, State), + Matches1 = Mod:lookup(<<"a/b/c">>, State), + ok = Mod:insert(<<"a/b/d">>, Msg2, State), + Matches2 = Mod:lookup(<<"a/b/d">>, State), + NoMatches = Mod:lookup(<<"x/y/z">>, State), ?assertEqual([Msg1], Matches1), ?assertEqual([Msg2], Matches2), @@ -84,6 +107,7 @@ test_add_and_match(Config) -> test_delete(Config) -> State = ?config(store_state, Config), + Mod = ?config(module, Config), Msg1 = #mqtt_msg{retain = true, qos = 0, @@ -92,14 +116,15 @@ test_delete(Config) -> payload = <<"msg1">>, props = #{}, timestamp = os:system_time(second)}, - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), - ok = rabbit_mqtt_retained_msg_store_ets:delete(<<"a/b/c">>, State), - Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), + ok = Mod:insert(<<"a/b/c">>, Msg1, State), + ok = Mod:delete(<<"a/b/c">>, State), + Matches = Mod:lookup(<<"a/b/c">>, State), ?assertEqual([], Matches). test_plus_wildcard(Config) -> State = ?config(store_state, Config), + Mod = ?config(module, Config), Msg1 = #mqtt_msg{retain = true, qos = 0, @@ -116,14 +141,15 @@ test_plus_wildcard(Config) -> payload = <<"msg2">>, props = #{}, timestamp = os:system_time(second)}, - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c">>, Msg2, State), - Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/+/c">>, State), + ok = Mod:insert(<<"a/b/c">>, Msg1, State), + ok = Mod:insert(<<"a/x/c">>, Msg2, State), + Matches = Mod:lookup(<<"a/+/c">>, State), ?assertEqual(lists:sort([Msg1, Msg2]), lists:sort(Matches)). test_hash_wildcard(Config) -> State = ?config(store_state, Config), + Mod = ?config(module, Config), Msg1 = #mqtt_msg{retain = true, qos = 0, @@ -156,16 +182,18 @@ test_hash_wildcard(Config) -> payload = <<"msg4">>, props = #{}, timestamp = os:system_time(second)}, - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, Msg2, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/x/y">>, Msg3, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/q/x/y">>, Msg4, State), - Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/#">>, State), + ok = Mod:insert(<<"a/b/c">>, Msg1, State), + ok = Mod:insert(<<"a/b/c/d">>, Msg2, State), + ok = Mod:insert(<<"a/b/x/y">>, Msg3, State), + ok = Mod:insert(<<"a/q/x/y">>, Msg4, State), + Matches = Mod:lookup(<<"a/b/#">>, State), ?assertEqual([Msg1, Msg2, Msg3], lists:sort(Matches)). test_combined_wildcards(Config) -> State = ?config(store_state, Config), + Mod = ?config(module, Config), + Msg1 = #mqtt_msg{retain = true, qos = 0, @@ -198,16 +226,18 @@ test_combined_wildcards(Config) -> payload = <<"msg4">>, props = #{}, timestamp = os:system_time(second)}, - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c/d">>, Msg1, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/x/c/e">>, Msg2, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/c/f/g">>, Msg3, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/y/d/f/g">>, Msg4, State), - Matches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/+/c/#">>, State), + ok = Mod:insert(<<"a/b/c/d">>, Msg1, State), + ok = Mod:insert(<<"a/x/c/e">>, Msg2, State), + ok = Mod:insert(<<"a/y/c/f/g">>, Msg3, State), + ok = Mod:insert(<<"a/y/d/f/g">>, Msg4, State), + Matches = Mod:lookup(<<"a/+/c/#">>, State), ?assertEqual([Msg1, Msg2, Msg3], lists:sort(Matches)). test_recovery(Config) -> State = ?config(store_state, Config), + Mod = ?config(module, Config), + Msg1 = #mqtt_msg{retain = true, qos = 0, @@ -225,25 +255,23 @@ test_recovery(Config) -> props = #{}, timestamp = os:system_time(second)}, - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/c">>, Msg1, State), - ok = rabbit_mqtt_retained_msg_store_ets:insert(<<"a/b/d">>, Msg2, State), - Matches1 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State), - Matches2 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/d">>, State), - NoMatches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"x/y/z">>, State), + ok = Mod:insert(<<"a/b/c">>, Msg1, State), + ok = Mod:insert(<<"a/b/d">>, Msg2, State), + Matches1 = Mod:lookup(<<"a/b/c">>, State), + Matches2 = Mod:lookup(<<"a/b/d">>, State), + NoMatches = Mod:lookup(<<"x/y/z">>, State), ?assertEqual([Msg1], Matches1), ?assertEqual([Msg2], Matches2), ?assertEqual([], NoMatches), % Recover the state - ok = rabbit_mqtt_retained_msg_store_ets:terminate(State), - {ok, Filenames} = file:list_dir(?config(test_dir, Config)), - ?assertEqual(3, length(Filenames)), + ok = Mod:terminate(State), + {ok, _Filenames} = file:list_dir(?config(test_dir, Config)), - {ok, State2, _Expire} = - rabbit_mqtt_retained_msg_store_ets:recover(?config(test_dir, Config), <<"test">>), + {ok, State2, _Expire} = Mod:recover(?config(test_dir, Config), <<"test">>), - Matches1 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/c">>, State2), - Matches2 = rabbit_mqtt_retained_msg_store_ets:lookup(<<"a/b/d">>, State2), - NoMatches = rabbit_mqtt_retained_msg_store_ets:lookup(<<"x/y/z">>, State2), + Matches1 = Mod:lookup(<<"a/b/c">>, State2), + Matches2 = Mod:lookup(<<"a/b/d">>, State2), + NoMatches = Mod:lookup(<<"x/y/z">>, State2), ?assertEqual([Msg1], Matches1), ?assertEqual([Msg2], Matches2), ?assertEqual([], NoMatches). diff --git a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl index f87906c1744a..35511f29c8dc 100644 --- a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl @@ -22,7 +22,7 @@ groups() -> sub_groups() -> [{dets, [shuffle], tests()}, - {ets, [shuffle], tests() ++ wildcard_tests()}, + {ets, [shuffle], tests()}, {noop, [shuffle], [does_not_retain]}]. tests() -> @@ -31,11 +31,8 @@ tests() -> should_translate_amqp2mqtt_on_retention, should_translate_amqp2mqtt_on_retention_search, recover, - recover_with_message_expiry_interval]. - -%% Only run wildcard tests for ETS store -wildcard_tests() -> - [retained_wildcard_single_level, retained_wildcard_multi_level, retained_wildcard_mixed]. + recover_with_message_expiry_interval, + retained_wildcard_single_level, retained_wildcard_multi_level, retained_wildcard_mixed]. suite() -> [{timetrap, {minutes, 2}}]. From 30c9a4206442520f1b3a0134e76e5b72273b5ce3 Mon Sep 17 00:00:00 2001 From: getlarge Date: Mon, 20 Jan 2025 22:09:45 +0100 Subject: [PATCH 19/25] fix(rabbitmq_mqtt): improve topics deletion efficiency --- deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl | 5 ++--- .../src/rabbit_mqtt_retained_msg_store_dets.erl | 6 ++---- .../src/rabbit_mqtt_retained_msg_store_ets.erl | 5 +---- 3 files changed, 5 insertions(+), 11 deletions(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl index 73d0fa936d94..64a46bc0a3ac 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl @@ -41,8 +41,7 @@ -callback recover(Directory :: file:name_all(), rabbit_types:vhost()) -> {ok, State :: any(), expire()} | {error, uninitialized}. -callback insert(topic(), mqtt_msg(), State :: any()) -> ok. --callback lookup(topic(), State :: any()) -> - [mqtt_msg()] | [mqtt_msg_v0()] | []. +-callback lookup(topic(), State :: any()) -> [mqtt_msg()] | [mqtt_msg_v0()] | []. -callback delete(topic(), State :: any()) -> ok. -callback terminate(State :: any()) -> ok. @@ -106,7 +105,7 @@ expire(Mod, Tab) -> Acc) when is_integer(Expiry) andalso is_integer(Timestamp) -> if Now - Timestamp >= Expiry -> - Mod:match_delete(Tab, {NodeId, Topic, '_'}), + Mod:delete(Tab, NodeId), Acc; true -> maps:put(Topic, {Timestamp, Expiry}, Acc) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl index 50da9b81c7d1..547f3728ad33 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl @@ -30,7 +30,7 @@ new(Dir, VHost) -> % delete_table_files(Dir, VHost), {ok, NodeTable} = open_table(Dir, VHost, <<"nodes">>, set), {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), - {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, bag), + {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, set), RootId = make_node_id(), ok = dets:insert(NodeTable, {RootId, 0, false}), @@ -46,7 +46,7 @@ new(Dir, VHost) -> {error, uninitialized}. recover(Dir, VHost) -> try - {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, bag), + {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, set), Expire = rabbit_mqtt_retained_msg_store:expire(dets, MsgTable), {ok, NodeTable} = open_table(Dir, VHost, <<"nodes">>, set), {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), @@ -85,8 +85,6 @@ insert(Topic, Msg, #store_state{} = State) -> NodeId = follow_or_create_path(Words, State), % Mark node as topic end and store message update_node(NodeId, true, State), - % Replace any existing message for this topic - dets:match_delete(State#store_state.msg_table, {NodeId, Topic, '_'}), dets:insert(State#store_state.msg_table, {NodeId, Topic, Msg}) end, Insert(), diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl index 8a4ed273d9b3..e1cca9e67d0f 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl @@ -37,8 +37,7 @@ new(Dir, VHost) -> % Edge table - will store {{from_id, word}, to_id} EdgeTable = ets:new(get_table_name(VHost, <<"edges">>), [ordered_set, public]), % Topic table - will store {node_id, topic, value} - % TODO: consider whether, set might still be the best option here - MsgTable = ets:new(get_table_name(VHost, <<"msgs">>), [bag, public]), + MsgTable = ets:new(get_table_name(VHost, <<"msgs">>), [set, public]), RootId = make_node_id(), ets:insert(NodeTable, {RootId, 0, false}), @@ -113,8 +112,6 @@ insert(Topic, Msg, #store_state{} = State) -> NodeId = follow_or_create_path(Words, State), % Mark node as topic end and store message update_node(NodeId, true, State), - % Replace any existing message for this topic - ets:delete_object(State#store_state.msg_table, {NodeId, Topic, '_'}), ets:insert(State#store_state.msg_table, {NodeId, Topic, Msg}), ok. From 092b10d76791fd8a9c9b8116e9b6aefdae53aa0f Mon Sep 17 00:00:00 2001 From: getlarge Date: Wed, 22 Jan 2025 18:10:49 +0100 Subject: [PATCH 20/25] refactor(rabbitmq_mqtt): improve root node handling and add custom file path in DETS store --- .../src/rabbit_mqtt_retained_msg_store.erl | 3 - .../rabbit_mqtt_retained_msg_store_dets.erl | 89 ++++++++----------- .../rabbit_mqtt_retained_msg_store_ets.erl | 5 +- 3 files changed, 39 insertions(+), 58 deletions(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl index 64a46bc0a3ac..4dd6daea7e2d 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl @@ -70,9 +70,6 @@ insert(Topic, Msg, #?STATE{store_mod = Mod, store_state = StoreState}) -> -spec lookup(topic(), state()) -> [mqtt_msg()] | []. lookup(Topic, #?STATE{store_mod = Mod, store_state = StoreState}) -> case Mod:lookup(Topic, StoreState) of - % Handle single old message format (legacy case) - OldMsg when is_record(OldMsg, mqtt_msg, 7) -> - [convert_mqtt_msg(OldMsg)]; % Handle list of messages - convert any old format ones Messages when is_list(Messages) -> lists:map(fun (Msg) when is_record(Msg, mqtt_msg, 7) -> diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl index 547f3728ad33..554ab4c3597f 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl @@ -27,12 +27,10 @@ -spec new(file:name_all(), rabbit_types:vhost()) -> store_state(). new(Dir, VHost) -> - % delete_table_files(Dir, VHost), {ok, NodeTable} = open_table(Dir, VHost, <<"nodes">>, set), {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, set), - RootId = make_node_id(), - ok = dets:insert(NodeTable, {RootId, 0, false}), + {ok, RootId} = find_or_insert_root_node(NodeTable, EdgeTable), #store_state{node_table = NodeTable, edge_table = EdgeTable, @@ -50,18 +48,7 @@ recover(Dir, VHost) -> Expire = rabbit_mqtt_retained_msg_store:expire(dets, MsgTable), {ok, NodeTable} = open_table(Dir, VHost, <<"nodes">>, set), {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), - - RootId = - case find_root_node(NodeTable, EdgeTable) of - {ok, Id} -> - ?LOG_INFO("Recovered existing RootId: ~p", [Id]), - Id; - error -> - NewId = make_node_id(), - ?LOG_INFO("Creating new RootId: ~p", [NewId]), - ok = dets:insert(NodeTable, {NewId, 0, false}), - NewId - end, + {ok, RootId} = find_or_insert_root_node(NodeTable, EdgeTable), State = #store_state{node_table = NodeTable, @@ -80,15 +67,10 @@ recover(Dir, VHost) -> -spec insert(topic(), mqtt_msg(), store_state()) -> ok. insert(Topic, Msg, #store_state{} = State) -> Words = split_topic(Topic), - Insert = - fun() -> - NodeId = follow_or_create_path(Words, State), - % Mark node as topic end and store message - update_node(NodeId, true, State), - dets:insert(State#store_state.msg_table, {NodeId, Topic, Msg}) - end, - Insert(), - %? TODO: Wrap in transaction since we're modifying multiple tables + NodeId = follow_or_create_path(Words, State), + % Mark node as topic end and store message + update_node(NodeId, true, State), + dets:insert(State#store_state.msg_table, {NodeId, Topic, Msg}), ok. -spec lookup(topic(), store_state()) -> [mqtt_msg()] | [mqtt_msg_v0()] | []. @@ -98,8 +80,11 @@ lookup(Topic, #store_state{} = State) -> Values = lists:flatmap(fun(NodeId) -> case dets:lookup(State#store_state.msg_table, NodeId) of + [] -> []; [{_NodeId, _Topic, Value} | _] -> [Value]; - [] -> [] + {error, _Reason} -> + ?LOG_ERROR("Failed to lookup MQTT retained message for node ~p", [NodeId]), + [] end end, Matches), @@ -108,24 +93,19 @@ lookup(Topic, #store_state{} = State) -> -spec delete(topic(), store_state()) -> ok. delete(Topic, State) -> Words = split_topic(Topic), - % Wrap in transaction since we're modifying multiple tables - Delete = - fun() -> - case follow_path(Words, State) of - {ok, NodeId} -> - dets:match_delete(State#store_state.msg_table, {NodeId, Topic, '_'}), - case dets:lookup(State#store_state.msg_table, NodeId) of - [] -> - update_node(NodeId, false, State), - maybe_clean_path(NodeId, State); - _ -> ok - end; - error -> ok - end - end, - Delete(), - %? TODO: Wrap in transaction since we're modifying multiple tables - % rabbit_mnesia:execute_mnesia_transaction(Delete), + case follow_path(Words, State) of + {ok, NodeId} -> + dets:match_delete(State#store_state.msg_table, {NodeId, Topic, '_'}), + case dets:lookup(State#store_state.msg_table, NodeId) of + [] -> + update_node(NodeId, false, State), + maybe_clean_path(NodeId, State); + _ -> + ok + end; + error -> + ok + end, ok. -spec terminate(store_state()) -> ok. @@ -167,6 +147,16 @@ find_root_node(NodeTable, EdgeTable) -> error % Multiple root nodes would indicate corruption end. +find_or_insert_root_node(NodeTable, EdgeTable) -> + case find_root_node(NodeTable, EdgeTable) of + {ok, Id} -> + {ok, Id}; + error -> + NewId = make_node_id(), + ok = dets:insert(NodeTable, {NewId, 0, false}), + {ok, NewId} + end. + follow_or_create_path(Words, State) -> follow_or_create_path(Words, State#store_state.root_id, State). @@ -287,21 +277,12 @@ remove_edge(FromId, Word, State) -> open_table(Dir, VHost, Type, TableType) -> Tab = get_table_name(VHost, Type), - % Path = get_table_path(Dir, VHost, Type), + Path = get_table_path(Dir, VHost, Type), AutoSave = rabbit_misc:get_env(rabbit_mqtt, retained_message_store_dets_sync_interval, 2000), dets:open_file(Tab, [{type, TableType}, - % {file, Path}, + {file, Path}, {ram_file, true}, {repair, true}, {auto_save, AutoSave}]). - -delete_table_files(Dir, VHost) -> - Types = [<<"nodes">>, <<"edges">>, <<"msgs">>], - lists:foreach(fun(Type) -> - Path = get_table_path(Dir, VHost, Type), - file:delete(Path) - end, - Types), - ok. diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl index e1cca9e67d0f..62a572c87f7c 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl @@ -122,8 +122,11 @@ lookup(Topic, #store_state{} = State) -> Values = lists:flatmap(fun(NodeId) -> case ets:lookup(State#store_state.msg_table, NodeId) of + [] -> []; [{_NodeId, _Topic, Value} | _] -> [Value]; - [] -> [] + {error, _Reason} -> + ?LOG_ERROR("Failed to lookup MQTT retained message for node ~p", [NodeId]), + [] end end, Matches), From 0cdf063634d56e07704debb2c40f3660d71578dd Mon Sep 17 00:00:00 2001 From: getlarge Date: Wed, 22 Jan 2025 18:11:17 +0100 Subject: [PATCH 21/25] test(rabbitmq_mqtt): ensure DETS tables are unique --- .../rabbit_mqtt_retained_msg_store_SUITE.erl | 16 +++++++--------- 1 file changed, 7 insertions(+), 9 deletions(-) diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl index 492cc8cf81a5..a1f4910442e1 100644 --- a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl @@ -46,11 +46,12 @@ end_per_group(_Group, _Config) -> init_per_testcase(_TestCase, Config) -> Mod = ?config(module, Config), - - Dir = filename:join(["/tmp", "mqtt_test_" ++ integer_to_list(erlang:unique_integer())]), + Dir = filename:join(["/tmp", "mqtt_test_" ++ erlang:integer_to_list(erlang:unique_integer())]), ok = filelib:ensure_dir(Dir ++ "/"), - State = Mod:new(Dir, <<"test">>), - [{store_state, State}, {test_dir, Dir} | Config]. + % VHost needs to be different for each test case to create a new table + VHost = erlang:iolist_to_binary([<<"test">>, erlang:integer_to_list(erlang:unique_integer())]), + State = Mod:new(Dir, VHost), + [{store_state, State}, {test_dir, Dir}, {vhost, VHost} | Config]. end_per_testcase(_TestCase, Config) -> State = ?config(store_state, Config), @@ -63,10 +64,9 @@ end_per_testcase(_TestCase, Config) -> ok -> ok; {error, enoent} -> - ok; % Directory already gone + ok; {error, Reason} -> ct:pal("Failed to delete directory ~p: ~p", [Dir, Reason]), - % Try a more aggressive cleanup os:cmd("rm -rf " ++ Dir) end, ok. @@ -265,9 +265,7 @@ test_recovery(Config) -> ?assertEqual([], NoMatches), % Recover the state ok = Mod:terminate(State), - {ok, _Filenames} = file:list_dir(?config(test_dir, Config)), - - {ok, State2, _Expire} = Mod:recover(?config(test_dir, Config), <<"test">>), + {ok, State2, _Expire} = Mod:recover(?config(test_dir, Config), ?config(vhost, Config)), Matches1 = Mod:lookup(<<"a/b/c">>, State2), Matches2 = Mod:lookup(<<"a/b/d">>, State2), From 690f9157c54934c7b078467b0ff8d1662e97020c Mon Sep 17 00:00:00 2001 From: getlarge Date: Wed, 22 Jan 2025 19:42:49 +0100 Subject: [PATCH 22/25] feat(rabbitmq_mqtt): add max retained messages count configuration for MQTT plugin --- deps/rabbitmq_mqtt/BUILD.bazel | 1 + deps/rabbitmq_mqtt/Makefile | 1 + .../priv/schema/rabbitmq_mqtt.schema | 4 ++ .../src/rabbit_mqtt_retained_msg_store.erl | 8 ++- .../rabbit_mqtt_retained_msg_store_dets.erl | 25 +++++++--- .../rabbit_mqtt_retained_msg_store_ets.erl | 49 +++++++++++-------- 6 files changed, 58 insertions(+), 30 deletions(-) diff --git a/deps/rabbitmq_mqtt/BUILD.bazel b/deps/rabbitmq_mqtt/BUILD.bazel index 49853b99a788..978f9f35db05 100644 --- a/deps/rabbitmq_mqtt/BUILD.bazel +++ b/deps/rabbitmq_mqtt/BUILD.bazel @@ -34,6 +34,7 @@ APP_ENV = """[ {retained_message_store, rabbit_mqtt_retained_msg_store_dets}, %% only used by DETS store {retained_message_store_dets_sync_interval, 2000}, + {retained_message_store_max_retained_messages_count, 2000}, {prefetch, 10}, {ssl_listeners, []}, {tcp_listeners, [1883]}, diff --git a/deps/rabbitmq_mqtt/Makefile b/deps/rabbitmq_mqtt/Makefile index 9b73227950f1..3605333cb5d1 100644 --- a/deps/rabbitmq_mqtt/Makefile +++ b/deps/rabbitmq_mqtt/Makefile @@ -12,6 +12,7 @@ define PROJECT_ENV {retained_message_store, rabbit_mqtt_retained_msg_store_dets}, %% only used by DETS store {retained_message_store_dets_sync_interval, 2000}, + {retained_message_store_max_retained_messages_count, 2000}, {prefetch, 10}, {ssl_listeners, []}, {tcp_listeners, [1883]}, diff --git a/deps/rabbitmq_mqtt/priv/schema/rabbitmq_mqtt.schema b/deps/rabbitmq_mqtt/priv/schema/rabbitmq_mqtt.schema index b69e2b06075c..b28608be1068 100644 --- a/deps/rabbitmq_mqtt/priv/schema/rabbitmq_mqtt.schema +++ b/deps/rabbitmq_mqtt/priv/schema/rabbitmq_mqtt.schema @@ -87,6 +87,10 @@ end}. {mapping, "mqtt.retained_message_store_dets_sync_interval", "rabbitmq_mqtt.retained_message_store_dets_sync_interval", [{datatype, integer}]}. +%% Limit how many messages are returned by MQTT plugin retained messages store +{mapping, "mqtt.retained_message_store_max_retained_messages_count", "rabbitmq_mqtt.retained_message_store_max_retained_messages_count", + [{datatype, integer}]}. + %% Whether or not to enable proxy protocol support. %% %% {proxy_protocol, false} diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl index 4dd6daea7e2d..8a5178a41d52 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store.erl @@ -24,7 +24,7 @@ -include_lib("kernel/include/logger.hrl"). -export([start/1, insert/3, lookup/2, delete/2, terminate/1]). --export([expire/2]). +-export([expire/2, get_max_retained_messages_count/0]). -export_type([state/0, expire/0]). @@ -122,3 +122,9 @@ convert_mqtt_msg({mqtt_msg, Retain, Qos, Topic, Dup, PacketId, Payload}) -> packet_id = PacketId, payload = Payload, props = #{}}. + +-spec get_max_retained_messages_count() -> pos_integer(). +get_max_retained_messages_count() -> + rabbit_misc:get_env(rabbit_mqtt, + retained_message_store_max_retained_messages_count, + 2000). diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl index 554ab4c3597f..9f4b69ffa7be 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_dets.erl @@ -21,7 +21,8 @@ msg_table :: dets:tab_name(), % Stores {node_id, topic, mqtt_msg} root_id :: binary(), % Root node ID dir :: file:filename_all(), - vhost :: rabbit_types:vhost()}). + vhost :: rabbit_types:vhost(), + max_retained_messages_count :: pos_integer()}). -type store_state() :: #store_state{}. @@ -31,13 +32,15 @@ new(Dir, VHost) -> {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), {ok, MsgTable} = open_table(Dir, VHost, <<"msgs">>, set), {ok, RootId} = find_or_insert_root_node(NodeTable, EdgeTable), - + MaxRetainedMessagesCount = + rabbit_mqtt_retained_msg_store:get_max_retained_messages_count(), #store_state{node_table = NodeTable, edge_table = EdgeTable, msg_table = MsgTable, root_id = RootId, dir = Dir, - vhost = VHost}. + vhost = VHost, + max_retained_messages_count = MaxRetainedMessagesCount}. -spec recover(file:name_all(), rabbit_types:vhost()) -> {ok, store_state(), rabbit_mqtt_retained_msg_store:expire()} | @@ -49,14 +52,16 @@ recover(Dir, VHost) -> {ok, NodeTable} = open_table(Dir, VHost, <<"nodes">>, set), {ok, EdgeTable} = open_table(Dir, VHost, <<"edges">>, set), {ok, RootId} = find_or_insert_root_node(NodeTable, EdgeTable), - + MaxRetainedMessagesCount = + rabbit_mqtt_retained_msg_store:get_max_retained_messages_count(), State = #store_state{node_table = NodeTable, edge_table = EdgeTable, msg_table = MsgTable, root_id = RootId, dir = Dir, - vhost = VHost}, + vhost = VHost, + max_retained_messages_count = MaxRetainedMessagesCount}, {ok, State, Expire} catch error:Reason -> @@ -74,12 +79,16 @@ insert(Topic, Msg, #store_state{} = State) -> ok. -spec lookup(topic(), store_state()) -> [mqtt_msg()] | [mqtt_msg_v0()] | []. -lookup(Topic, #store_state{} = State) -> +lookup(Topic, + #store_state{root_id = RootId, + msg_table = MsgTable, + max_retained_messages_count = Limit} = + State) -> Words = split_topic(Topic), - Matches = match_pattern_words(Words, State#store_state.root_id, State, []), + Matches = lists:sublist(match_pattern_words(Words, RootId, State, []), Limit), Values = lists:flatmap(fun(NodeId) -> - case dets:lookup(State#store_state.msg_table, NodeId) of + case dets:lookup(MsgTable, NodeId) of [] -> []; [{_NodeId, _Topic, Value} | _] -> [Value]; {error, _Reason} -> diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl index 62a572c87f7c..cea642015044 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_retained_msg_store_ets.erl @@ -15,21 +15,19 @@ -export([new/2, recover/2, insert/3, lookup/2, delete/2, terminate/1]). -% TODO: -define(DEFAULT_MATCH_LIMIT, 1000). - -record(store_state, {node_table :: ets:tid(), % Stores {node_id, edge_count, is_topic} edge_table :: ets:tid(), % Stores {{from_id, word}, to_id} msg_table :: ets:tid(), % Stores {node_id, topic, mqtt_msg} root_id :: binary(), % Root node ID dir :: file:filename_all(), - vhost :: rabbit_types:vhost()}). + vhost :: rabbit_types:vhost(), + max_retained_messages_count :: pos_integer()}). -type store_state() :: #store_state{}. -spec new(file:name_all(), rabbit_types:vhost()) -> store_state(). new(Dir, VHost) -> - % Clean up any existing files delete_table_files(Dir, VHost), % Node table - will store tuples of {node_id, edge_count, is_topic} @@ -42,12 +40,15 @@ new(Dir, VHost) -> RootId = make_node_id(), ets:insert(NodeTable, {RootId, 0, false}), + MaxRetainedMessagesCount = + rabbit_mqtt_retained_msg_store:get_max_retained_messages_count(), #store_state{node_table = NodeTable, edge_table = EdgeTable, msg_table = MsgTable, root_id = RootId, dir = Dir, - vhost = VHost}. + vhost = VHost, + max_retained_messages_count = MaxRetainedMessagesCount}. -spec recover(file:name_all(), rabbit_types:vhost()) -> {ok, store_state(), rabbit_mqtt_retained_msg_store:expire()} | @@ -71,14 +72,16 @@ recover(Dir, VHost) -> ets:insert(NodeTable, {NewId, 0, false}), NewId end, - + MaxRetainedMessagesCount = + rabbit_mqtt_retained_msg_store:get_max_retained_messages_count(), State = #store_state{node_table = NodeTable, edge_table = EdgeTable, msg_table = MsgTable, root_id = RootId, dir = Dir, - vhost = VHost}, + vhost = VHost, + max_retained_messages_count = MaxRetainedMessagesCount}, {ok, State, Expire} catch error:Reason -> @@ -116,21 +119,25 @@ insert(Topic, Msg, #store_state{} = State) -> ok. -spec lookup(topic(), store_state()) -> [mqtt_msg()] | [mqtt_msg_v0()] | []. -lookup(Topic, #store_state{} = State) -> +lookup(Topic, + #store_state{max_retained_messages_count = Limit, + msg_table = MsgTable, + root_id = RootId} = + State) -> Words = split_topic(Topic), - Matches = match_pattern_words(Words, State#store_state.root_id, State, []), - Values = - lists:flatmap(fun(NodeId) -> - case ets:lookup(State#store_state.msg_table, NodeId) of - [] -> []; - [{_NodeId, _Topic, Value} | _] -> [Value]; - {error, _Reason} -> - ?LOG_ERROR("Failed to lookup MQTT retained message for node ~p", [NodeId]), - [] - end - end, - Matches), - Values. + % limiting the length of the list of matches to avoid performance issues + % it is simpler and in most cases more efficient to use sublist after the fact than to try to limit the number of matches in the match_pattern_words function + Matches = lists:sublist(match_pattern_words(Words, RootId, State, []), Limit), + lists:flatmap(fun(NodeId) -> + case ets:lookup(MsgTable, NodeId) of + [] -> []; + [{_NodeId, _Topic, Value} | _] -> [Value]; + {error, _Reason} -> + ?LOG_ERROR("Failed to lookup MQTT retained message for node ~p", [NodeId]), + [] + end + end, + Matches). -spec delete(topic(), store_state()) -> ok. delete(Topic, State) -> From b86de4ec72e27fab5161ef7df8765575ba39ab75 Mon Sep 17 00:00:00 2001 From: getlarge Date: Mon, 3 Feb 2025 08:44:12 +0100 Subject: [PATCH 23/25] ci: trigger From 63666b25ce659dcb0d3ed36e1a4b5b8178d87f70 Mon Sep 17 00:00:00 2001 From: getlarge Date: Sat, 8 Feb 2025 14:31:51 +0100 Subject: [PATCH 24/25] fix(mqtt): update topic handling in retained message processing and tests --- deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl | 9 +++++---- .../test/rabbit_mqtt_retained_msg_store_SUITE.erl | 8 ++++++-- deps/rabbitmq_mqtt/test/retainer_SUITE.erl | 6 +++--- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl b/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl index f584bd4f68a4..ec791ea8bfe9 100644 --- a/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl +++ b/deps/rabbitmq_mqtt/src/rabbit_mqtt_processor.erl @@ -999,21 +999,22 @@ send_retained_message(TopicFilter0, SubscribeQos, Msgs when is_list(Msgs) -> lists:foldl( fun(Msg, S) -> - send_retained_message_to_client(Msg, TopicFilter, SubscribeQos, S) + send_retained_message_to_client(Msg, SubscribeQos, S) end, State0, Msgs); #mqtt_msg{} = SingleMsg -> - send_retained_message_to_client(SingleMsg, TopicFilter, SubscribeQos, State0) + send_retained_message_to_client(SingleMsg, SubscribeQos, State0) end. send_retained_message_to_client(#mqtt_msg{qos = MsgQos, retain = Retain, + topic = Topic0, payload = Payload, props = Props0}, - TopicFilter, SubscribeQos, State0 = #state{packet_id = PacketId0}) -> Qos = effective_qos(MsgQos, SubscribeQos), - {Topic, Props, State1} = process_topic_alias_outbound(TopicFilter, Props0, State0), + Topic1 = amqp_to_mqtt(Topic0), + {Topic, Props, State1} = process_topic_alias_outbound(Topic1, Props0, State0), {PacketId, State} = case Qos of ?QOS_0 -> {undefined, State1}; diff --git a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl index a1f4910442e1..f8a3a1c53014 100644 --- a/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/rabbit_mqtt_retained_msg_store_SUITE.erl @@ -118,9 +118,13 @@ test_delete(Config) -> timestamp = os:system_time(second)}, ok = Mod:insert(<<"a/b/c">>, Msg1, State), ok = Mod:delete(<<"a/b/c">>, State), - Matches = Mod:lookup(<<"a/b/c">>, State), + Matches1 = Mod:lookup(<<"a/b/c">>, State), + Matches2 = Mod:lookup(<<"a/b/+">>, State), + Matches3 = Mod:lookup(<<"a/#">>, State), - ?assertEqual([], Matches). + ?assertEqual([], Matches1), + ?assertEqual([], Matches2), + ?assertEqual([], Matches3). test_plus_wildcard(Config) -> State = ?config(store_state, Config), diff --git a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl index 9b68fcfdb747..494c2dcf3b0e 100644 --- a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl @@ -246,7 +246,7 @@ retained_wildcard_single_level(Config) -> ok = emqtt:publish(C, <<"devices/sensor1/temperature">>, #{}, <<"23.5">>, [{retain, true}]), {ok, _, _} = emqtt:subscribe(C, <<"devices/+/temperature">>, qos1), - ok = expect_publishes(C, <<"devices/+/temperature">>, [<<"23.5">>]), + ok = expect_publishes(C, <<"devices/sensor1/temperature">>, [<<"23.5">>]), ok = emqtt:disconnect(C). %% Test multi-level wildcard (#) @@ -259,7 +259,7 @@ retained_wildcard_multi_level(Config) -> <<"23.5">>, [{retain, true}]), {ok, _, _} = emqtt:subscribe(C, <<"devices/#">>, qos1), - ok = expect_publishes(C, <<"devices/#">>, [<<"23.5">>]), + ok = expect_publishes(C, <<"devices/sensor1/readings/temperature">>, [<<"23.5">>]), ok = emqtt:disconnect(C). %% Test mixed wildcards (+/#) @@ -272,5 +272,5 @@ retained_wildcard_mixed(Config) -> <<"23.5">>, [{retain, true}]), {ok, _, _} = emqtt:subscribe(C, <<"devices/+/readings/#">>, qos1), - ok = expect_publishes(C, <<"devices/+/readings/#">>, [<<"23.5">>]), + ok = expect_publishes(C, <<"devices/sensor1/readings/temperature">>, [<<"23.5">>]), ok = emqtt:disconnect(C). From 5ea15b20d159577baa2e96b7b720b10e4b8b11cb Mon Sep 17 00:00:00 2001 From: getlarge Date: Sat, 8 Feb 2025 15:14:12 +0100 Subject: [PATCH 25/25] test(mqtt): add should_remove_from_retained test for message retention behavior --- deps/rabbitmq_mqtt/test/retainer_SUITE.erl | 24 +++++++++++++++++++++- 1 file changed, 23 insertions(+), 1 deletion(-) diff --git a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl index 494c2dcf3b0e..859c09b8532f 100644 --- a/deps/rabbitmq_mqtt/test/retainer_SUITE.erl +++ b/deps/rabbitmq_mqtt/test/retainer_SUITE.erl @@ -32,7 +32,10 @@ tests() -> should_translate_amqp2mqtt_on_retention_search, recover, recover_with_message_expiry_interval, - retained_wildcard_single_level, retained_wildcard_multi_level, retained_wildcard_mixed]. + retained_wildcard_single_level, + retained_wildcard_multi_level, + retained_wildcard_mixed, + should_remove_from_retained]. suite() -> [{timetrap, {minutes, 2}}]. @@ -274,3 +277,22 @@ retained_wildcard_mixed(Config) -> {ok, _, _} = emqtt:subscribe(C, <<"devices/+/readings/#">>, qos1), ok = expect_publishes(C, <<"devices/sensor1/readings/temperature">>, [<<"23.5">>]), ok = emqtt:disconnect(C). + +should_remove_from_retained(Config) -> + C = connect(<<"wildcardClientRetainer">>, Config, [{ack_timeout, 1}]), + ok = + emqtt:publish(C, + <<"devices/sensor1/readings/temperature">>, + #{}, + <<"23.5">>, + [{retain, true}]), + ok = + emqtt:publish(C, <<"devices/sensor1/readings/temperature">>, #{}, <<>>, [{retain, true}]), + {ok, _, _} = emqtt:subscribe(C, <<"devices/sensor1/readings/temperature">>, qos1), + receive + Unexpected -> + ct:fail("Unexpected message: ~p", [Unexpected]) + after 1000 -> + ok + end, + ok = emqtt:disconnect(C).