Fix ODBC API support for MongooseIM #1816
Conversation
|
I suggest that we drop Travis job odbc_pgsql because:
|
|
We can disable the travis job, but keep preset. Also, be aware, that we use a forked version of odbc. It would have more tests like in rdbms_SUITE. But for now I think this PR is already big enough. |
Codecov Report
@@ Coverage Diff @@
## master #1816 +/- ##
==========================================
- Coverage 74.77% 74.42% -0.35%
==========================================
Files 297 298 +1
Lines 27737 27910 +173
==========================================
+ Hits 20740 20772 +32
- Misses 6997 7138 +141
Continue to review full report at Codecov.
|
src/mod_mam_odbc_user.erl
Outdated
| "(server, user_name) VALUES ('", SServer, "', '", SUserName, "')"]), | ||
| "(server, user_name) VALUES (", | ||
| mongoose_rdbms:use_escaped_string(SServer), ", ", | ||
| mongoose_rdbms:use_escaped_string(SUserName), ")"]), | ||
| case Res of | ||
| {updated, 1} -> | ||
| ok; | ||
| %% Ignore the race condition Duplicate entry ... for key 'uc_mam_server_user_name' | ||
| {error, duplicate_key} -> |
There was a problem hiding this comment.
I don't like the error checking here.
In a next PR we can use something like.
mongoose_rdbms:is_duplicate_key_error(duplicate_key) -> true;
mongoose_rdbms:is_duplicate_key_error("[FreeTDS.....) -> true;
...
There was a problem hiding this comment.
That's a huge effort, thanks a lot @arcusfelis.
There are some comments/questions to the code which I'd like to see addressed.
Also I think it'd be good if you spent some time and sent PR to Erlang/OTP with the changes you made to extracted eodbc application.
Also we need to add to the doc what needs to be installed on the system in order to compile MongooseIM with eodbc. It didn't compile on my Mac because currently I don't have sql.h file.
.travis.yml
Outdated
| @@ -65,6 +67,7 @@ env: | |||
| - PRESET=internal_mnesia DB=mnesia REL_CONFIG=with-all TLS_DIST=yes | |||
| - PRESET=mysql_redis DB=mysql REL_CONFIG="with-mysql with-redis" | |||
| - PRESET=odbc_pgsql_mnesia DB=pgsql REL_CONFIG=with-odbc | |||
| - PRESET=odbc_mssql_mnesia DB=mssql REL_CONFIG=with-odbc | |||
There was a problem hiding this comment.
Could you remove the above odbc_pgsql_mnesia job? There is no need to have it if we'll test ODBC layer with this newly added job.
rel/files/app.config
Outdated
| @@ -6,7 +6,7 @@ | |||
| {log_root, "{{mongooseim_log_dir}}"}, | |||
| {crash_log, "crash.log"}, | |||
| {handlers, [ | |||
| {lager_console_backend, [info, {lager_default_formatter,[{eol, "\r\n"}]}]}, | |||
| {lager_console_backend, [{level, info}]}, | |||
There was a problem hiding this comment.
Why this change? I'm not saying it's wrong, I just want to know the reason.
There was a problem hiding this comment.
because currently lager_console_backend crashes just after start and we don't have logs.
2018-04-30 11:45:07.329 [error] <0.205.0>@lager_handler_watcher:123
Lager fatally failed to install handler lager_console_backend into lager_event,
NOT retrying: {bad_console_config, info}There was a problem hiding this comment.
will put in a separate PR.
| @@ -65,7 +65,8 @@ stop_pool(Pool) -> | |||
| mongoose_rdbms_sup:remove_pool(Pool). | |||
|
|
|||
| compile_odbc_type_helper() -> | |||
| Key = {odbc_server_type, ?MYNAME}, | |||
| %% TODO This parameter should not be global, but pool-name parameterized | |||
There was a problem hiding this comment.
Is the TODO still valid?
There was a problem hiding this comment.
It would require some pool refactoring, so can be a separate task.
There was a problem hiding this comment.
#1833 reported separately
src/mod_mam_odbc_user.erl
Outdated
| case Res of | ||
| {updated, 1} -> | ||
| ok; | ||
| %% Ignore the race condition Duplicate entry ... for key 'uc_mam_server_user_name' | ||
| {error, duplicate_key} -> | ||
| ok; | ||
| {error, "[FreeTDS][SQL Server]Violation of UNIQUE KEY constraint" ++ _} -> |
There was a problem hiding this comment.
Hmm, I wonder if it is possible that sth else then FreeTDS is used?
There was a problem hiding this comment.
Than we would have the current hook failed.
Overall, we need to refactor this part in the module separately.
For example, by always retrying to select key after any error here.
But I feel it's a separate PR...
src/mod_roster.erl
Outdated
| @@ -282,22 +282,19 @@ write_roster_version(LUser, LServer, InTransaction) -> | |||
| %% - roster versioning is used by server and client, | |||
| %% BUT the server isn't storing versions on db OR | |||
| %% - the roster version from client don't match current version. | |||
| process_iq_get(From, To, #iq{sub_el = SubEl} = IQ) -> | |||
| process_iq_get(From, To, IQ) -> | |||
| mongoose_error:try_to_handle_iq(From, To, IQ, fun do_process_iq_get/3). | |||
There was a problem hiding this comment.
I'm not sure if mongoose_error is the best place for this function... I don't have suggestion for other place ATM.
There was a problem hiding this comment.
maybe mim_iq module :)
src/mod_roster.erl
Outdated
| IQ#iq{type = error, | ||
| sub_el = [SubEl, mongoose_xmpp_errors:internal_server_error()]} | ||
| end. | ||
| AttrVer = xml:get_tag_attr(<<"ver">>, SubEl), |
There was a problem hiding this comment.
Since you touched this line, could you also change it to use exml_query?
There was a problem hiding this comment.
Not really good PR for refactoring this, but OK, I did it.
src/mongoose_error.erl
Outdated
| %% @doc Generic error handling code | ||
| %% | ||
| %% Use these functions, instead of `try ... catch _:_ -> ok end'. | ||
| -module(mongoose_error). |
There was a problem hiding this comment.
Maybe renaming this module to mongoose_iq would be better? We could later move here other IQ related functions.
Enable mssql travis build Disable odbc_mssql_mnesia travis job Use arcusfelis/eodbc version of odbc application eodbc returns types of fields eodbc correctly handles long binaries/texts in pgsql eodbc prints data when wrong data is written into port Use eodbc in mongoose_rdbms_SUITE Use return_types in eodbc to handle utf16 Use utf8mb4 encoding in mysql Make start_services optional Allow to enable preset on just some nodes Update travis-setup-db.sh with license reference Use shared odbc.ini for both mssql and pgsql Fix mongoose_rdbms_type:get() function Add search_body into priv/mssql2012.sql schema Use varbinary for message field in MSSQL Use nvarchar in mssql schema Reduce length of some fields in mssql schema Correcly escape varchar in mssql Add is_integer(ArcID) check into mod_mam_odbc_async_pool_writer Add is_integer(UserID) check to mod_mam_odbc_prefs:get_behaviour/5 Change a primary key for mam_muc_message from id to (room_id,id) Handle duplicate error from MSSQL in mod_mam_odbc_user Log an error with good description when sql_execute fails Add unicode_messages_can_be_extracted test Use several like statements when searching for multiple words Add muclight tables Add test_types table to postgres and mysql schemas Add a ssl tip comment into /tools/travis-setup-db.sh Add rdbms_SUITE in big_tests Refactor string escaping API Add escape_boolean Add a comment into privacy_SUITE about kicking (non mssql) Introduce ejabberd_c2s:process_incoming_stanza_with_conflict_check Wait in mam_SUITE:long_text_search_request Fix unexpected presences in last_SUITE Fix a race condition in rest_client_SUITE Correctly decode ID in mod_privacy_odbc:get_privacy_list Remove application:start from rdbms_odbc:connect/2 Introduce choose_get_user_roster_strategy into mod_roster Add mongoose_iq:try_to_handle_iq/4 helper function
arcusfelis
force-pushed
the
mssql-travis
branch
from
9ef5661
to
d9ddfd9
Compare
|
Comments from @michalwski were addressed. |
big_tests/tests/last_SUITE.erl
Outdated
| last_server(Config) -> | ||
| escalus:story(Config, [{alice, 1}], | ||
| escalus:fresh_story(Config, [{alice, 1}], |
There was a problem hiding this comment.
Why do we switch to fresh_story only for this particular test case?
There was a problem hiding this comment.
because other stories want Alice and Bob to be friends
big_tests/tests/mam_SUITE.erl
Outdated
| @@ -1370,6 +1373,13 @@ long_text_search_request(Config) -> | |||
| timer:sleep(50) | |||
| end, Msgs), | |||
|
|
|||
| %% Just check than Bob receives them. | |||
big_tests/tests/mam_SUITE.erl
Outdated
| @@ -1370,6 +1373,13 @@ long_text_search_request(Config) -> | |||
| timer:sleep(50) | |||
| end, Msgs), | |||
|
|
|||
| %% Just check than Bob receives them. | |||
| %% It should help, when the server is overloaded. | |||
There was a problem hiding this comment.
the CI server, because currently it may refer to MongooseIM server as well. :)
| @@ -366,6 +371,8 @@ simple_story(Config, Fun) -> | |||
| ). | |||
|
|
|||
| clear_list_relogin(Config) -> | |||
| %% unexprected presence unavalable | |||
| @@ -26,11 +26,10 @@ all_tests() -> | |||
| groupchat_message_is_not_stored, | |||
| headline_message_is_not_stored, | |||
| expired_messages_are_not_delivered, | |||
| max_offline_messages_reached | |||
| ]. | |||
| max_offline_messages_reached]. | |||
There was a problem hiding this comment.
Newline here helps in quick cases disable. ;)
There was a problem hiding this comment.
not really, there are still commas.
src/rdbms/mongoose_rdbms.erl
Outdated
| {aborted, #{reason := Reason, sql_query := SqlQuery}} | ||
| when NRestarts =:= 0 -> | ||
| %% Too many retries of outer transaction. | ||
| ?ERROR_MSG("SQL transaction restarts exceeded~n" |
src/rdbms/mongoose_rdbms.erl
Outdated
| erlang:get_stacktrace(), State]), | ||
| sql_query_internal([<<"rollback;">>], State), | ||
| {{aborted, Reason}, State}; | ||
| {aborted, Reason} when NRestarts =:= 0 -> %% old format for abort | ||
| %% Too many retries of outer transaction. | ||
| ?ERROR_MSG("SQL transaction restarts exceeded~n" |
| @@ -87,11 +87,9 @@ | |||
| add_privacy_list/2, | |||
| set_privacy_list/2, | |||
| del_privacy_lists/3, | |||
| set_vcard/26, | |||
| get_vcard/2, | |||
| set_vcard/27, | |||
There was a problem hiding this comment.
F.... these REALLY should be rewritten with maps. Not in this PR though. :)
src/mod_vcard_odbc.erl
Outdated
| SUsername = mongoose_rdbms:escape_string(User), | ||
| SLUsername = mongoose_rdbms:escape_string(LUser), | ||
| SLServer = mongoose_rdbms:escape_string(VHost), | ||
| SVCARD = mongoose_rdbms:escape_string( exml:to_binary(VCard)), |
| %%% Some backends allow ArcID can be not set (riak?). | ||
| %%% This means, that there are no "jid => integer" mapping. | ||
| %%% | ||
| %%% But this module requires ArcID not be an integer. |
There was a problem hiding this comment.
"requires ArcID not be an integer" but several lines below you mention that ArcID is required to be integer?
There was a problem hiding this comment.
to be an integer, typo
| @@ -34,6 +34,13 @@ $ xcode-select --install # install compilation tools | |||
| $ brew install git erlang openssl | |||
| ``` | |||
|
|
|||
| If you are planning to use ODBC to connect to MSSQL: | |||
There was a problem hiding this comment.
This is not if. Below libs are need to compile current MongooseIM on MacOS X even if user is not going to use ODBC and MSSQL.
There was a problem hiding this comment.
Correction, unixodbc is needed to compile, freetds is needed only to connect to MSSQL
There was a problem hiding this comment.
But even though, it doesn't work on MacOS X High Sierra :( that's really sad we make our own life harder... Not everyone is using El Capitan or Sierra :(
There was a problem hiding this comment.
socket = connect_to_erlang(port);
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:344:12: error: non-object type 'int (int, int, int)' is not assignable
socket = connect_to_erlang(port);
~~~~~~ ^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:345:11: warning: implicit declaration of function 'receive_msg' is invalid in C99 [-Wimplicit-function-declaration]
msg = receive_msg(socket);
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:345:9: warning: incompatible integer to pointer conversion assigning to 'byte *' (aka 'char *') from 'int' [-Wint-conversion]
msg = receive_msg(socket);
^ ~~~~~~~~~~~~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:354:5: warning: implicit declaration of function 'close_socket' is invalid in C99 [-Wimplicit-function-declaration]
close_socket(socket);
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:378:12: error: non-object type 'int (int, int, int)' is not assignable
socket = connect_to_erlang(port);
~~~~~~ ^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:381:17: warning: incompatible integer to pointer conversion assigning to 'byte *' (aka 'char *') from 'int' [-Wint-conversion]
request_buffer = receive_msg(socket);
^ ~~~~~~~~~~~~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:386:2: warning: implicit declaration of function 'send_msg' is invalid in C99 [-Wimplicit-function-declaration]
send_msg(&msg, socket); /* Send answer to erlang */
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:400:14: warning: incompatible pointer to integer conversion passing 'int (int, int, int)' to parameter of type 'int' [-Wint-conversion]
shutdown(socket, 2);
^~~~~~
/usr/include/sys/socket.h:700:17: note: passing argument to parameter here
int shutdown(int, int);
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:1510:15: warning: unused variable 'result' [-Wunused-variable]
SQLRETURN result;
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:1918:1: error: expected identifier or '('
{
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2721:14: warning: incompatible pointer types passing 'SQLLEN **' (aka 'long **') to parameter of type 'SQLLEN *' (aka 'long *'); remove & [-Wincompatible-pointer-types]
blocklen, &StrLen_or_IndPtr);
^~~~~~~~~~~~~~~~~
/usr/local/include/sql.h:706:43: note: passing argument to parameter 'StrLen_or_Ind' here
SQLLEN *StrLen_or_Ind);
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2724:41: warning: comparison between pointer and integer ('SQLLEN *' (aka 'long *') and 'int')
fetched_bytes = (((StrLen_or_IndPtr == SQL_NO_TOTAL) || (StrLen_or_IndPtr > blocklen))
~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2724:79: warning: ordered comparison between pointer and integer ('SQLLEN *' (aka 'long *') and 'int')
fetched_bytes = (((StrLen_or_IndPtr == SQL_NO_TOTAL) || (StrLen_or_IndPtr > blocklen))
~~~~~~~~~~~~~~~~ ^ ~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2725:13: warning: pointer/integer type mismatch in conditional expression ('int' and 'SQLLEN *' (aka 'long *')) [-Wconditional-type-mismatch]
? (blocklen-1) : StrLen_or_IndPtr);
^ ~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2724:19: warning: incompatible pointer to integer conversion assigning to 'int' from 'SQLLEN *' (aka 'long *') [-Wint-conversion]
fetched_bytes = (((StrLen_or_IndPtr == SQL_NO_TOTAL) || (StrLen_or_IndPtr > blocklen))
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2743:5: warning: incompatible pointer types passing 'SQLLEN **' (aka 'long **') to parameter of type 'SQLLEN *' (aka 'long *'); remove & [-Wincompatible-pointer-types]
&StrLen_or_IndPtr);
^~~~~~~~~~~~~~~~~
/usr/local/include/sql.h:706:43: note: passing argument to parameter 'StrLen_or_Ind' here
SQLLEN *StrLen_or_Ind);
^
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2745:45: warning: comparison between pointer and integer ('SQLLEN *' (aka 'long *') and 'int')
fetched_bytes = (((StrLen_or_IndPtr == SQL_NO_TOTAL) || (StrLen_or_IndPtr > blocklen))
~~~~~~~~~~~~~~~~ ^ ~~~~~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2745:83: warning: ordered comparison between pointer and integer ('SQLLEN *' (aka 'long *') and 'int')
fetched_bytes = (((StrLen_or_IndPtr == SQL_NO_TOTAL) || (StrLen_or_IndPtr > blocklen))
~~~~~~~~~~~~~~~~ ^ ~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2746:17: warning: pointer/integer type mismatch in conditional expression ('int' and 'SQLLEN *' (aka 'long *')) [-Wconditional-type-mismatch]
? (blocklen-1) : StrLen_or_IndPtr);
^ ~~~~~~~~~~~~ ~~~~~~~~~~~~~~~~
/Users/michalpiotrowski/projects/MongooseIM/repo/_build/default/lib/eodbc/c_src/odbcserver.c:2745:23: warning: incompatible pointer to integer conversion assigning to 'int' from 'SQLLEN *' (aka 'long *') [-Wint-conversion]
fetched_bytes = (((StrLen_or_IndPtr == SQL_NO_TOTAL) || (StrLen_or_IndPtr > blocklen))
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
18 warnings and 3 errors generated.```
There was a problem hiding this comment.
Also noticed by @kzemek. We're going to assign a ticket for this in upcoming sprint.
Fix ODBC API support for MongooseIM Enable mssql travis build Disable odbc_mssql_mnesia travis job Use arcusfelis/eodbc version of odbc application eodbc returns types of fields eodbc correctly handles long binaries/texts in pgsql eodbc prints data when wrong data is written into port Use eodbc in mongoose_rdbms_SUITE Use return_types in eodbc to handle utf16 Use utf8mb4 encoding in mysql Make start_services optional Allow to enable preset on just some nodes Update travis-setup-db.sh with license reference Use shared odbc.ini for both mssql and pgsql Fix mongoose_rdbms_type:get() function Add search_body into priv/mssql2012.sql schema Use varbinary for message field in MSSQL Use nvarchar in mssql schema Reduce length of some fields in mssql schema Correcly escape varchar in mssql Add is_integer(ArcID) check into mod_mam_odbc_async_pool_writer Add is_integer(UserID) check to mod_mam_odbc_prefs:get_behaviour/5 Change a primary key for mam_muc_message from id to (room_id,id) Handle duplicate error from MSSQL in mod_mam_odbc_user Log an error with good description when sql_execute fails Add unicode_messages_can_be_extracted test Use several like statements when searching for multiple words Add muclight tables Add test_types table to postgres and mysql schemas Add a ssl tip comment into /tools/travis-setup-db.sh Add rdbms_SUITE in big_tests Refactor string escaping API Add escape_boolean Add a comment into privacy_SUITE about kicking (non mssql) Introduce ejabberd_c2s:process_incoming_stanza_with_conflict_check Wait in mam_SUITE:long_text_search_request Fix unexpected presences in last_SUITE Fix a race condition in rest_client_SUITE Correctly decode ID in mod_privacy_odbc:get_privacy_list Remove application:start from rdbms_odbc:connect/2 Introduce choose_get_user_roster_strategy into mod_roster Add mongoose_iq:try_to_handle_iq/4 helper function Add docs about how mod_mam_odbc_arch full-text search works Retry to query ArcID in mod_mam_odbc_user after any error Use version of eodbc that can compile on OSX
Proposed changes include:
Enable mssql travis build
Use arcusfelis/eodbc version of odbc application
eodbc returns types of fields
eodbc correctly handles long binaries/texts in pgsql
eodbc prints data when wrong data is written into port
Use eodbc in mongoose_rdbms_SUITE
Use return_types in eodbc to handle utf16
Use utf8mb4 encoding in mysql
Make start_services optional
Allow to enable preset on just some nodes
Update travis-setup-db.sh with license reference
Use shared odbc.ini for both mssql and pgsql
Fix mongoose_rdbms_type:get() function
Add search_body into priv/mssql2012.sql schema
Use varbinary for message field in MSSQL
Use nvarchar in mssql schema
Reduce length of some fields in mssql schema
Correcly escape varchar in mssql
Add is_integer(ArcID) check into mod_mam_odbc_async_pool_writer
Add is_integer(UserID) check to mod_mam_odbc_prefs:get_behaviour/5
Change a primary key for mam_muc_message from id to (room_id,id)
Handle duplicate error from MSSQL in mod_mam_odbc_user
Log an error with good description when sql_execute fails
Add unicode_messages_can_be_extracted test
Use several like statements when searching for multiple words
Add muclight tables
Add test_types table to postgres and mysql schemas
Add a ssl tip comment into /tools/travis-setup-db.sh
Add rdbms_SUITE in big_tests
Refactor string escaping API
Add escape_boolean
Add a comment into privacy_SUITE about kicking (non mssql)
Introduce ejabberd_c2s:process_incoming_stanza_with_conflict_check
Wait in mam_SUITE:long_text_search_request
Fix unexpected presences in last_SUITE
Fix a race condition in rest_client_SUITE
Correctly decode ID in mod_privacy_odbc:get_privacy_list
Remove application:start from rdbms_odbc:connect/2