-
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
GraphQL - Implement MUC Light rooms API for admin #3538
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov Report
@@ Coverage Diff @@
## feature/graphql #3538 +/- ##
===================================================
+ Coverage 81.32% 81.37% +0.04%
===================================================
Files 449 454 +5
Lines 32854 32955 +101
===================================================
+ Hits 26719 26817 +98
- Misses 6135 6138 +3
Continue to review full report at Codecov.
|
This comment was marked as outdated.
This comment was marked as outdated.
e407f89
to
6411541
Compare
This comment was marked as outdated.
This comment was marked as outdated.
6411541
to
4b98b56
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
c72ae7a
to
cb40d77
Compare
This comment was marked as outdated.
This comment was marked as outdated.
MUC Light admin rest API required a domain to create a room and a muc light domain for other operations. Now it is unified and uses a domain everywhere.
cb40d77
to
04a5863
Compare
small_tests_24 / small_tests / 04a5863 small_tests_23 / small_tests / 04a5863 dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 04a5863 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 04a5863 dynamic_domains_mysql_redis_24 / mysql_redis / 04a5863 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 04a5863 ldap_mnesia_24 / ldap_mnesia / 04a5863 internal_mnesia_24 / internal_mnesia / 04a5863 pgsql_mnesia_23 / pgsql_mnesia / 04a5863 ldap_mnesia_23 / ldap_mnesia / 04a5863 mssql_mnesia_24 / odbc_mssql_mnesia / 04a5863 mysql_redis_24 / mysql_redis / 04a5863 pgsql_mnesia_24 / pgsql_mnesia / 04a5863 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 04a5863 riak_mnesia_24 / riak_mnesia / 04a5863 |
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.
Looks good to me in general. I think that (as we discussed) it might make more sense to use JID to identify the room in the whole API.
This comment was marked as outdated.
This comment was marked as outdated.
fd628de
to
c885d01
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Also use MUCLightDomain in commands instead of Domain. Apply review comments.
Revert `XMPPMUCHOST` for admin REST API instead of `XMPPHOST`. Apply review comments.
c885d01
to
e14f8f6
Compare
small_tests_24 / small_tests / e14f8f6 small_tests_23 / small_tests / e14f8f6 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / e14f8f6 dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / e14f8f6 dynamic_domains_mysql_redis_24 / mysql_redis / e14f8f6 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / e14f8f6 ldap_mnesia_24 / ldap_mnesia / e14f8f6 ldap_mnesia_23 / ldap_mnesia / e14f8f6 pgsql_mnesia_24 / pgsql_mnesia / e14f8f6 pgsql_mnesia_23 / pgsql_mnesia / e14f8f6 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / e14f8f6 internal_mnesia_24 / internal_mnesia / e14f8f6 mssql_mnesia_24 / odbc_mssql_mnesia / e14f8f6 mysql_redis_24 / mysql_redis / e14f8f6 riak_mnesia_24 / riak_mnesia / e14f8f6 |
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.
Looks good in general, I added a few minor comments.
|
||
-spec create_room(map()) -> {ok, map()} | {error, resolver_error()}. | ||
create_room(#{<<"id">> := null} = Args) -> | ||
create_room(Args#{<<"id">> => <<>>}); |
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.
Do we want to accept empty binary in the GraphQL query?
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.
An empty string, in this case, means that id
is not present and should be generated. It was done this way in the old commands module. I can return an error when id
is empty to eliminate the situation that someone passes an empty string by the accident. Should I do it this way?
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.
That was the point - the logic for "" is a side-effect of the way it is coded. I think it can stay as it is for now. I think that the policy for empty binaries should be similar as e.g. for empty binary for user name, jid etc.
small_tests_24 / small_tests / 28f484f small_tests_23 / small_tests / 28f484f dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 28f484f dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 28f484f dynamic_domains_mysql_redis_24 / mysql_redis / 28f484f ldap_mnesia_23 / ldap_mnesia / 28f484f dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 28f484f ldap_mnesia_24 / ldap_mnesia / 28f484f internal_mnesia_24 / internal_mnesia / 28f484f pgsql_mnesia_23 / pgsql_mnesia / 28f484f pgsql_mnesia_24 / pgsql_mnesia / 28f484f elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 28f484f mysql_redis_24 / mysql_redis / 28f484f mssql_mnesia_24 / odbc_mssql_mnesia / 28f484f riak_mnesia_24 / riak_mnesia / 28f484f |
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.
Looks good to me 👍
small_tests_24 / small_tests / 198cc90 small_tests_23 / small_tests / 198cc90 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 198cc90 dynamic_domains_mysql_redis_24 / mysql_redis / 198cc90 dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 198cc90 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 198cc90 ldap_mnesia_24 / ldap_mnesia / 198cc90 ldap_mnesia_23 / ldap_mnesia / 198cc90 internal_mnesia_24 / internal_mnesia / 198cc90 pgsql_mnesia_24 / pgsql_mnesia / 198cc90 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 198cc90 mysql_redis_24 / mysql_redis / 198cc90 mssql_mnesia_24 / odbc_mssql_mnesia / 198cc90 pgsql_mnesia_23 / pgsql_mnesia / 198cc90 riak_mnesia_24 / riak_mnesia / 198cc90 |
This PR addresses MIM-1600. It adds MUC Light admin API for GraphQL. The user part should be implemented in the next PR.
The new
mod_muc_light_api
extracted frommod_muc_light_commands
catches all errors and returns them as a{ErrCode, Message}
tuple. Previously these functions could fail in some cases (e.g. passing a non-existing domain).The old REST API now expects
MUCLightServer
in each command instead ofServer
. Previously it depended on the command, and now it is unified.Before, some commands use room name instead of room id. The room name is not unique and user can have more than one room with the same name. It is unified now and room id is used everywhere.