-
Notifications
You must be signed in to change notification settings - Fork 428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Simple MAM queries with complete result flag #3734
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Report
@@ Coverage Diff @@
## master #3734 +/- ##
==========================================
+ Coverage 82.28% 82.32% +0.04%
==========================================
Files 526 526
Lines 33844 33862 +18
==========================================
+ Hits 27847 27876 +29
+ Misses 5997 5986 -11
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
small_tests_24 / small_tests / f6ae3c3 small_tests_25 / small_tests / f6ae3c3 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / f6ae3c3 ldap_mnesia_24 / ldap_mnesia / f6ae3c3 dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / f6ae3c3 ldap_mnesia_25 / ldap_mnesia / f6ae3c3 dynamic_domains_mysql_redis_25 / mysql_redis / f6ae3c3 internal_mnesia_25 / internal_mnesia / f6ae3c3 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / f6ae3c3 pgsql_mnesia_24 / pgsql_mnesia / f6ae3c3 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / f6ae3c3 pgsql_mnesia_25 / pgsql_mnesia / f6ae3c3 mysql_redis_25 / mysql_redis / f6ae3c3 muc_SUITE:hibernation:hibernated_room_can_be_queried_for_archive{error,{{assertion_failed,assert,is_groupchat_message,
[<<"Restorable message">>],
undefined,"undefined"},
[{escalus_new_assert,assert_true,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{muc_SUITE,wait_for_mam_result,3,
[{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
{line,4394}]},
{muc_SUITE,'-hibernated_room_can_be_queried_for_archive/1-fun-0-',3,
[{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
{line,4130}]},
{escalus_story,story,4,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{muc_SUITE,hibernated_room_can_be_queried_for_archive,1,
[{file,"/home/circleci/project/big_tests/tests/muc_SUITE.erl"},
{line,4126}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1782}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1291}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1223}]}]}} mssql_mnesia_25 / odbc_mssql_mnesia / f6ae3c3 riak_mnesia_24 / riak_mnesia / f6ae3c3 pubsub_SUITE:tree+basic:subscribe_options_deliver_option_test{error,
{{badmatch,
[{xmlel,<<"message">>,
[{<<"from">>,<<"pubsub.localhost">>},
{<<"to">>,
<<"bob_subscribe_options_deliver_option_test_1531@localhost/res1">>},
{<<"type">>,<<"headline">>}],
[{xmlel,<<"event">>,
[{<<"xmlns">>,
<<"http://jabber.org/protocol/pubsub#event">>}],
[{xmlel,<<"items">>,
[{<<"node">>,<<"princely_musings_9JahZBKl9mA=">>}],
[{xmlel,<<"item">>,
[{<<"id">>,<<"item1">>}],
[{xmlel,<<"entry">>,
[{<<"xmlns">>,
<<"http://www.w3.org/2005/Atom">>}],
[]}]}]}]},
{xmlel,<<"headers">>,
[{<<"xmlns">>,<<"http://jabber.org/protocol/shim">>}],
[]}]}]},
[{pubsub_SUITE,'-subscribe_options_deliver_option_test/1-fun-0-',3,
[{file,"/home/circleci/project/big_tests/tests/pubsub_SUITE.erl"},
{line,479}]},
{escalus_story,story,4,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}} mysql_redis_25 / mysql_redis / f6ae3c3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good to merge, but just some small comments.
end. | ||
|
||
remove_extra_message(Params, Messages) -> | ||
case maps:get(ordering_direction, Params, forward) of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be pattern-matched on the function clause, it'll be even faster (see maps performance guide if curious).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, in case key exists - yea
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And in the case it does not, a second catch-all clause just throws the default.
Other | ||
end. | ||
|
||
patch_fun_to_make_result_as_map(F) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh god, this is patching extreme dynamic typing 😱 anyway, for future major refactorings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I knew you will love it!
Yea, lookup_result_map should replace the tuple. Bad news is that it is like 50 places we use the tuple :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah I imagined 😱
@@ -1155,19 +1156,75 @@ is_policy_violation(TotalCount, Offset, MaxResultLimit, LimitPassed) -> | |||
LookupResult :: mod_mam:lookup_result(), | |||
R :: {ok, mod_mam:lookup_result()} | {error, item_not_found}. | |||
check_for_item_not_found(#rsm_in{direction = before, id = ID}, | |||
PageSize, {TotalCount, Offset, MessageRows}) -> | |||
_PageSize, {TotalCount, Offset, MessageRows}) -> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this parameter is not needed it can be removed, maybe?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
of course xd
Remained for compat/lazy.
Will get refactored with the tuple. And we probably wanna pass Params, instead of just RSM.
This PR addresses "complete flag is not always set".
Proposed changes include:
complete
flag: