Feature/full text search #1136

rslota · 2016-12-28T11:20:15Z

This PR introduces MVP for MAM full text search. Currently it works only with ODBC and Riak backends and its very limited when comes to text matching. This is NOT an extension to the standard. Current MAM XEP-0313 allows for queries using "forms" that may contain any server-defined fields, therefore simple field named 'full-text-search' was introduced in order to provide this feature.

The following stanza shows a example MAM stanza that uses full text search:

<iq type='set' id='query1'>
  <query xmlns='urn:xmpp:mam:1'>
    <x xmlns='jabber:x:data' type='submit'>
      <field type='hidden' var='FORM_TYPE'>
        <value>urn:xmpp:mam:1</value>
      </field>
      <field type='text-single' var='full-text-search'>
        <value>cat and dog</value>
      </field>
    </x>
  </query>
</iq>

Full Text Search is an optional server-side feature. To find out whether its supported, client may query server about active form fields for MAM stanza:

<iq type='get' id='form1'>
  <query xmlns='urn:xmpp:mam:1'/>
</iq>

The response shows which form fields may be used to filter MAM messages (that includes required fields i.e. with, start and end):

<iq type='result' id='form1'>
  <query xmlns='urn:xmpp:mam:1'>
    <x xmlns='jabber:x:data' type='form'>
      <field type='hidden' var='FORM_TYPE'>
        <value>urn:xmpp:mam:1</value>
      </field>
      <field type='jid-single' var='with'/>
      <field type='text-single' var='start'/>
      <field type='text-single' var='end'/>
      <field type='text-single' var='full-text-search'/>
    </x>
  </query>
</iq>

While reading PR you may want to skip "Make Elvis happy" commit to get rid of code quality changes.

michalwski

This is very good job, thanks! I really appreciate you also added the search feature to Riak's backend. Here are my general comments also:

The PR description is not up-to-date (doesn't say about Riak backend)
It would be very valuable if you could add an example stanza in the PR desc so it's easy for client devs to see and understand how to use this feature.
It would be perfect if the search feature were optional. There maybe some installations where the packet format is in fact sth encrypted (that's why the packet's format is configurable). In such situation we don't want to keep the body in plain text. Also for setups where the search feature is not needed, disabling it will safe some storage space.

michalwski · 2017-01-02T08:55:10Z

apps/ejabberd/src/mod_mam_cassandra_arch.erl

+                _Start, _End, _Now, _WithJID, <<_SearchText/binary>>,
+                _PageSize, _LimitPassed, _MaxResultLimit,
+                _IsSimple) ->
+    omg;


This is really interesting return value :) however I think {error, omg} is better and can still be improved ;)

Yea, sorry, that was a placeholder :)

michalwski · 2017-01-02T08:56:17Z

apps/ejabberd/src/mod_mam_muc_cassandra_arch.erl

+                _Start, _End, _Now, _WithJID, <<_SearchText/binary>>,
+                _PageSize, _LimitPassed, _MaxResultLimit,
+                _IsSimple) ->
+    {error, 'not-supported'};


This one looks far more better then omg from prev comment :)

michalwski · 2017-01-02T09:12:42Z

apps/ejabberd/src/mod_mam_odbc_arch.erl

@@ -133,7 +143,7 @@ stop_pm(Host) ->
 %% ----------------------------------------------------------------------
 %% Add hooks for mod_mam_muc

-spec start_muc(ejabberd:server(),_) -> 'ok'.
+-spec start_muc(ejabberd:server(), _) -> 'ok'.
 start_muc(Host, _Opts) ->
    case gen_mod:get_module_opt(Host, ?MODULE, no_writer, false) of


This is probably not needed here, am I right?

michalwski · 2017-01-02T09:14:44Z

apps/ejabberd/src/mod_mam_utils.erl

@@ -141,13 +149,13 @@ microseconds_to_now(MicroSeconds) when is_integer(MicroSeconds) ->

 %% @doc Returns time in `now()' format.
 -spec iso8601_datetime_binary_to_timestamp(iso8601_datetime_binary())
-        -> erlang:timestamp() | undefined.
+                                          -> erlang:timestamp() | undefined.
 iso8601_datetime_binary_to_timestamp(DateTime) when is_binary(DateTime) ->
    jlib:datetime_binary_to_timestamp(DateTime).


A word (or more) of an explanation will makes things easier to understand. A simple example may also help.

I'm not sure what this refers to, considering that i didn't even touch this function ;)

Your IDE seems to have touched it.

michalwski · 2017-01-02T09:15:21Z

apps/ejabberd/src/mod_mam_utils.erl

 replace_x_user_element(FromJID, Role, Affiliation, Packet) ->
    append_x_user_element(FromJID, Role, Affiliation,
-    delete_x_user_element(Packet)).


That's clever :) I like it!

michalwski · 2017-01-02T09:18:46Z

elvis.config

@@ -27,7 +27,7 @@
                 %% Allow usage of ?assertMatch and similar macros, since after expanding they
                 %% result in use of variables like __X and __V that wouldn't be permitted by
                 %% default regex.
-                 {elvis_style, variable_naming_convention, #{regex => "^(_?[A-Z][0-9a-zA-Z_]*)$"}}
+                 {elvis_style, variable_naming_convention, #{regex => "^(_?[A-Z][0-9a-zA-Z]*)$"}}


I think we do need _ in this regexp. Otherwise Elvis may complain about macros like ?A_MACRO_WITH_WORDS, am I right? We have such macros in tests.

I'm gonna revert it. That was an merge issue.

michalwski · 2017-01-10T11:03:30Z

apps/ejabberd/src/mod_mam_odbc_arch.erl

@@ -228,29 +239,32 @@ do_archive_message(_Result, Host, MessID, UserID,
    SDir = encode_direction(Dir),
    SRemLResource = ejabberd_odbc:escape(RemLResource),
    Data = packet_to_stored_binary(Packet),
+    TextBody = packet_to_search_body(Host, Packet),
+    STextBody = ejabberd_odbc:escape(TextBody),
+    string:to_lower(STextBody),


This is probably not needed here, am I right?

Yes, it's redundant. I'm gonna remove it.

michalwski · 2017-01-10T11:05:51Z

apps/ejabberd/src/mod_mam_utils.erl

+-spec normalize_search_text(binary() | string() | undefined, string()) -> string() | undefined.
+normalize_search_text(undefined, _WordSeparator) ->
+    undefined;
+normalize_search_text(Text, WordSeparator) ->


A word (or more) of an explanation will makes things easier to understand. A simple example may also help.

michalwski · 2017-01-10T11:07:15Z

apps/ejabberd/src/mod_mam_utils.erl

-        true ->
-            false %% full page but not the last one in the result set
-    end;
+    TotalCount =:= PagedCount; %% false means full page but not the last one in the result set


That's clever :) I like it!

gadgetci · 2017-01-16T16:12:53Z

test/ejabberd_tests/tests/mam_SUITE.erl

@@ -917,6 +978,153 @@ simple_archive_request(Config) ->
        end,
    escalus_fresh:story(Config, [{alice, 1}, {bob, 1}], F).

+text_search_is_not_available(Config) ->


According to Elvis:

The code in the following (LINE, COL) locations has the same structure: (981, 1), (1008, 1).

kzemek

There's still documentation missing, with focus on what happens when full-text search is turned on (and a note that turning it on won't make old messages searchable).

kzemek · 2017-02-03T12:36:41Z

apps/ejabberd/src/ejabberd_gen_mam_archive.erl

-        ArchiveID :: mod_mam:archive_id(), ArchiveJID :: ejabberd:jid())
-            -> integer().
+                       ArchiveID :: mod_mam:archive_id(), ArchiveJID :: ejabberd:jid())
+-> integer().


Looks like the formatter doesn't like lines beginning with an arrow.

kzemek · 2017-02-03T12:36:55Z

apps/ejabberd/src/ejabberd_gen_mam_archive.erl

-            -> purge_single_message_result().
+                               MessID :: mod_mam:message_id(), ArchiveID :: mod_mam:archive_id(),
+                               ArchiveJID :: ejabberd:jid(), Now :: mod_mam:unix_timestamp())
+-> purge_single_message_result().


One more mis-formatted arrow.

kzemek · 2017-02-03T12:38:00Z

apps/ejabberd/src/mod_commands.erl

@@ -186,7 +186,7 @@ send_message(From, To, Body) ->
    ejabberd_hooks:run(user_send_packet,
                       F#jid.lserver,
                       [F, T, Packet]),
-    % privacy check is missing, but is it needed?
+                                                % privacy check is missing, but is it needed?


Misformatted comment.

kzemek · 2017-02-03T12:39:46Z

apps/ejabberd/src/mod_mam.erl

@@ -65,7 +67,8 @@

 -import(mod_mam_utils,
        [maybe_microseconds/1,
-         microseconds_to_now/1]).
+         microseconds_to_now/1,
+         normalize_search_text/2]).


Do you think it would make sense to avoid importing new things into this module and call them fully qualified with mod_mam_utils:? The end goal would be to eliminate those imports alltogether.

kzemek · 2017-02-03T12:41:10Z

apps/ejabberd/src/mod_mam.erl

@@ -112,7 +116,8 @@
 -include_lib("ejabberd/include/amp.hrl").
 -include_lib("exml/include/exml.hrl").

-%% ----------------------------------------------------------------------
+%% ---------------------------------------------------
+%% -------------------


What happened :<

kzemek · 2017-02-03T13:46:31Z

apps/ejabberd/src/mod_mam_utils.erl

-        children = [ResultSetEl]}.
+       name = <<"fin">>,
+       attrs = [{<<"xmlns">>, MamNs}]
+       ++ [{<<"complete">>, <<"true">>} || IsComplete]


I feel like formatting in this case makes the code less readable.

kzemek · 2017-02-03T13:46:35Z

apps/ejabberd/src/mod_mam_utils.erl

-        children = [ResultSetEl]}.
+       name = <<"fin">>,
+       attrs = [{<<"xmlns">>, MamNs}]
+       ++ [{<<"complete">>, <<"true">>} || IsComplete]


I feel like formatting in this case makes the code less readable.

kzemek · 2017-02-03T13:47:03Z

apps/ejabberd/src/mod_mam_utils.erl

    [form_type_field(MamNs),
     form_field(<<"jid-single">>, <<"with">>),
     form_field(<<"text-single">>, <<"start">>),
-     form_field(<<"text-single">>, <<"end">>)].
+     form_field(<<"text-single">>, <<"end">>)] ++ TextSearch.


| TextSearch]? :)

That's not the same :) I want lists concatenation, not insertion.

I know it's not the same in general, but isn't it more idiomatic (and marginally faster) in this case?

kzemek · 2017-02-03T13:53:02Z

apps/ejabberd/src/mod_mam_utils.erl

+    LowerBody = string:to_lower(BodyString),
+    ReOpts = [{return, list}, global, unicode, ucp],
+    Re0 = re:replace(LowerBody, "[, .:;-?!]+", " ", ReOpts),
+    Re1 = re:replace(Re0, "([^\\w\\d ]+)|(^\\s+)|(\\s+$)", "", ReOpts),


The \d is redundant here as \w group already includes digits.

kzemek · 2017-02-03T13:56:00Z

apps/ejabberd/src/mod_mam_utils.erl

+-spec normalize_search_text(binary() | string() | undefined, string()) -> string() | undefined.
+normalize_search_text(undefined, _WordSeparator) ->
+    undefined;
+normalize_search_text(Text, WordSeparator) ->


This function has to do some heavy lifting on texts, have you measured the performance impact of ?

It adds like 100 μs to processing time per row on my slow CPU. Simple string:to_lower is only 10 times faster then this whole function. Also, I've checked that precompiling regexes only speed up this function by ~5%. Removing only one of those 3 regexes could give also no more then 5-10%, but I'm not sure if its easy to do.

Fix not-supported error

rslota force-pushed the feature/free-text-search branch 5 times, most recently from c15389d to 251d09c Compare December 29, 2016 14:51

michalwski reviewed Jan 2, 2017

View reviewed changes

rslota changed the title ~~Feature/free text search~~ Feature/full text search Jan 9, 2017

rslota force-pushed the feature/free-text-search branch 2 times, most recently from 8643221 to 7548e5f Compare January 10, 2017 08:10

michalwski reviewed Jan 10, 2017

View reviewed changes

gadgetci reviewed Jan 16, 2017

View reviewed changes

rslota force-pushed the feature/free-text-search branch 3 times, most recently from 509a6b4 to 89c8cea Compare January 19, 2017 17:43

rslota added MAM ready labels Jan 24, 2017

kzemek suggested changes Feb 3, 2017

View reviewed changes

rslota and others added 11 commits February 16, 2017 14:02

Enable full text search in ODBC based MAM

a82b961

Enable full text search in ODBC based MUC MAM

a421e42

Make Elvis happy

900475b

Fix Mongoose API requests to MAM

cf28648

Change search body column for MAM full text search

c8a837b

Enable full text search in Riak MAM backend

9102d9f

Fix typo

529131e

Fix not-supported error

Make full text search in MAM optional

d138a79

Add some docs

ad429dd

Test disabling full text search

20f9d87

Add tests for disabling full text search

16a7506

rslota force-pushed the feature/free-text-search branch 3 times, most recently from fbc1746 to 456e7e3 Compare February 16, 2017 14:28

Fix code after rebase

456e7e3

rslota force-pushed the feature/free-text-search branch 2 times, most recently from 9a698c7 to 30dbec3 Compare February 17, 2017 13:53

Add full text search doc

9569e6d

rslota force-pushed the feature/free-text-search branch from 30dbec3 to 9569e6d Compare February 17, 2017 13:59

kzemek approved these changes Feb 17, 2017

View reviewed changes

michalwski merged commit 0ddc72a into master Feb 20, 2017

michalwski deleted the feature/free-text-search branch February 20, 2017 08:55

michalwski mentioned this pull request Mar 28, 2017

MongooseIM 2.1.0beta1 #1244

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/full text search #1136

Feature/full text search #1136

rslota commented Dec 28, 2016 •

edited

Loading

michalwski left a comment

michalwski Jan 2, 2017

rslota Jan 3, 2017

michalwski Jan 2, 2017

michalwski Jan 2, 2017

michalwski Jan 2, 2017

rslota Jan 10, 2017

bartekgorny Jan 10, 2017

michalwski Jan 2, 2017

michalwski Jan 2, 2017

rslota Jan 3, 2017

michalwski Jan 10, 2017

rslota Jan 10, 2017

michalwski Jan 10, 2017

michalwski Jan 10, 2017

gadgetci Jan 16, 2017

kzemek left a comment

kzemek Feb 3, 2017

kzemek Feb 3, 2017

kzemek Feb 3, 2017

kzemek Feb 3, 2017

kzemek Feb 3, 2017

kzemek Feb 3, 2017

kzemek Feb 3, 2017

kzemek Feb 3, 2017

rslota Feb 16, 2017

kzemek Feb 16, 2017

kzemek Feb 3, 2017

kzemek Feb 3, 2017

rslota Feb 16, 2017

Feature/full text search #1136

Feature/full text search #1136

Conversation

rslota commented Dec 28, 2016 • edited Loading

michalwski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kzemek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rslota commented Dec 28, 2016 •

edited

Loading