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
Dynamic lookup of GDPR-related modules for retrieval #2297
Conversation
This comment has been minimized.
This comment has been minimized.
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.
PR is slightly in Erlang hacker style, but I don't see better way to handle the issue.
bin_to_int(T, (X*10)+(H-$0)); | ||
bin_to_int(Bin, X) -> | ||
{X, Bin}. | ||
%% WARNING! For simplicity, this function searches only MongooseIM code dir |
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.
Would it work for MIM installed from binaries?
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'm 99.9% certain it will because we're asking for a path for mongooseim
lib and beam files are always in ebin
subdir.
src/mongoose_lib.erl
Outdated
-spec find_behaviour_implementations(Behaviour :: module()) -> [module()]. | ||
find_behaviour_implementations(Behaviour) -> | ||
{ok, EbinFiles} = file:list_dir(code:lib_dir(mongooseim, ebin)), | ||
Mods = [ list_to_existing_atom(filename:rootname(File)) |
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 line may return badarg
if there does not already exist an atom whose text representation is String. See: http://erlang.org/doc/man/erlang.html#list_to_existing_atom-1
find_behaviour_implementations/1
is used in mod_vcard:get_personal_data/2
without try catch
an than there is top try catch
in src/admin_extra/service_admin_extra_gdpr.erl
but this will not work for following case:
Let's assume:
- module
mod_m
with backendsmod_m_a
,mod_m_b
,mod_m_c
mod_m_a
is loaded but without datamod_m_b
is not loaded and there is no data in this backendmod_m_c
is loaded and there is some data in this backend
Than:
Generating list Mods will crush onmod_m_b
and will exit with reasonbadarg
so no further code will be executed and the data from backend provided bymod_m_c
will never be retrieved.
Correct me if I am wrong.
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.
Another case would be to start MIM and BEAM with some module and after saving some data in the backend for this module, stop BEAM and when restarting it do it without the module data is in, so there the module is not is not in atoms list but there is data in the backend.
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 am pretty sure there are some other cases that will need attention, but I cannot think of any more right now.
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 is correct, I tried to be overcautious.
src/mongoose_lib.erl
Outdated
|| File <- EbinFiles, filename:extension(File) == ".beam" ], | ||
lists:filter(fun(M) -> | ||
try lists:keyfind([Behaviour], 2, M:module_info(attributes)) of | ||
{Key, _} when Key == behaviour; Key == behavior -> true; |
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.
{behaviour, _} -> true;
{behavior, _} -> true;
??
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 wanted to save 1 line :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.
The code is cool, mine is no better, I just showed different version for you to pick which one you like more ;)
Codecov Report
@@ Coverage Diff @@
## gdpr-retrieve-clean #2297 +/- ##
======================================================
- Coverage 78.95% 78.9% -0.06%
======================================================
Files 334 334
Lines 29038 28918 -120
======================================================
- Hits 22928 22817 -111
+ Misses 6110 6101 -9
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
c0083fb
to
2f3ccee
Compare
This comment has been minimized.
This comment has been minimized.
6444.1 / Erlang 19.3 / small_tests / d55510e 6444.6 / Erlang 19.3 / elasticsearch_and_cassandra_mnesia / d55510e 6444.2 / Erlang 19.3 / internal_mnesia / d55510e pubsub_SUITE:tree+node_config:disable_notifications_test{error,{{badmatch,false},
[{pubsub_tools,check_response,2,
[{file,"pubsub_tools.erl"},{line,468}]},
{pubsub_tools,receive_response,3,
[{file,"pubsub_tools.erl"},{line,458}]},
{pubsub_tools,receive_and_check_response,4,
[{file,"pubsub_tools.erl"},{line,448}]},
{escalus_story,story,4,
[{file,"/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1529}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1045}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,977}]}]}} 6444.3 / Erlang 19.3 / mysql_redis / d55510e 6444.4 / Erlang 19.3 / odbc_mssql_mnesia / d55510e 6444.5 / Erlang 19.3 / ldap_mnesia / d55510e sm_SUITE:parallel:subscription_requests_are_buffered_properly{error,{{badmatch,false},
[{escalus_session,stream_management,2,
[{file,"/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_session.erl"},
{line,227}]},
{escalus_connection,connection_step,2,
[{file,"/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_connection.erl"},
{line,134}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1263}]},
{escalus_connection,start,2,
[{file,"/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_connection.erl"},
{line,118}]},
{sm_SUITE,'-subscription_requests_are_buffered_properly/1-fun-3-',6,
[{file,"sm_SUITE.erl"},{line,848}]},
{escalus_story,story,4,
[{file,"/home/travis/build/esl/MongooseIM/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1529}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1045}]}]}} 6444.8 / Erlang 20.0 / pgsql_mnesia / d55510e 6444.9 / Erlang 21.0 / riak_mnesia / d55510e |
[{Jid, SerializedRecords}]; | ||
_ -> [] | ||
catch | ||
_:_ -> |
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.
Silent catch-all error - is there really a reason not to log it? This will be a nightmare to debug.
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 thought about the same thing after merging and decided to fix it after all other PRs are merged.
Result when is_list(Result) -> Result; | ||
_ -> [] | ||
catch | ||
_:_ -> [] |
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.
FYI, a similar silent catch-all
* Find GDPR modules dynamically * Find module backends for GDPR retrieval dynamically * Fix behaviours' impls lookup * Add test case for GDPR retrieval with disabled modules * Fix GDPR suite
This PR replaces hardcoded GDPR modules list with a dynamic lookup. It also changes the retrieval implementations to query all module backends.