Skip to content
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

Refactor Client REST API handlers #3801

Merged
merged 11 commits into from
Oct 14, 2022
Merged

Refactor Client REST API handlers #3801

merged 11 commits into from
Oct 14, 2022

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Oct 7, 2022

Rework all mongoose_client_api_* modules to follow the design patterns of mongoose_admin_api_*.

  • mongoose_client_api has generic helpers for authentication, error handling, argument parsing etc.
  • mongoose_client_api_* modules use the helpers consistently, and call the API helper modules (e.g. mod_muc_light_api) similarly to the GraphQL handlers.

Other changes:

  • Fixed minor issues with error handling for MUC Light.
  • Improved the test coverage by adding tests and fixing already existing ones.

Notes:

  • Some API functionality exceeds the GraphQL equivalent, e.g. for MUC Light there is a support for chat markers, and stanzas are given UUID's. I haven't added these options to GraphQL, but we might want to do it later.

@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Oct 7, 2022

Codecov Report

Base: 82.89% // Head: 82.95% // Increases project coverage by +0.06% 🎉

Coverage data is based on head (13f4c0c) compared to base (2aa0aa7).
Patch coverage: 99.63% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3801      +/-   ##
==========================================
+ Coverage   82.89%   82.95%   +0.06%     
==========================================
  Files         529      529              
  Lines       33840    33802      -38     
==========================================
- Hits        28051    28042       -9     
+ Misses       5789     5760      -29     
Impacted Files Coverage Δ
.../event_pusher/mod_event_pusher_hook_translator.erl 88.88% <ø> (ø)
src/mam/mod_mam_pm.erl 89.64% <ø> (ø)
src/mongoose_hooks.erl 96.32% <ø> (-0.08%) ⬇️
src/muc_light/mod_muc_light_utils.erl 92.78% <ø> (+0.78%) ⬆️
.../mongoose_client_api/mongoose_client_api_rooms.erl 98.48% <98.27%> (+8.07%) ⬆️
src/mongoose_client_api/mongoose_client_api.erl 83.87% <100.00%> (+12.96%) ⬆️
...ngoose_client_api/mongoose_client_api_contacts.erl 100.00% <100.00%> (+17.34%) ⬆️
...ngoose_client_api/mongoose_client_api_messages.erl 100.00% <100.00%> (+7.81%) ⬆️
...se_client_api/mongoose_client_api_rooms_config.erl 100.00% <100.00%> (+13.63%) ⬆️
..._client_api/mongoose_client_api_rooms_messages.erl 100.00% <100.00%> (+4.05%) ⬆️
... and 17 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz force-pushed the refactor-client-api branch 2 times, most recently from 402af77 to 51e9299 Compare October 12, 2022 16:25
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

This functionality will be needed to support the Client REST API.
GraphQL should support sending chat markers as well, but this is left
for a future PR. The support for markers could be placed in this
module, and shared by both API's.

Also:
  - export user affiliation getter for use in the API modules
  - fix a minor mistake (double nesting of 'error')
@chrzaszcz chrzaszcz force-pushed the refactor-client-api branch 2 times, most recently from a4c4bde to 8cda768 Compare October 13, 2022 12:42
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

Both modules expose the generic helpers now.
The admin code is not simply reused because of slight differences,
e.g. with authentication and error handling.
Use mongoose_stanza_* helper modules.
Remove the logic using 'resource_exists' for consistency with the
other API modules, and for avoiding duplicate calls to MUC Light modules.
Rework the permission-related code
- Catch the case when another type is passed instead of binary
- Respond with a validation error (as expected) instead of crashing
- Update error messages and codes
- Fix typos and dead test cases
- Improve coverage by handling more error cases
@mongoose-im
Copy link
Collaborator

mongoose-im commented Oct 14, 2022

small_tests_24 / small_tests / 13f4c0c
Reports root / small


small_tests_25 / small_tests / 13f4c0c
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 13f4c0c
Reports root/ big
OK: 2096 / Failed: 0 / User-skipped: 775 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 13f4c0c
Reports root/ big
OK: 3978 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 13f4c0c
Reports root/ big
OK: 3978 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 13f4c0c
Reports root/ big
OK: 2096 / Failed: 0 / User-skipped: 775 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 13f4c0c
Reports root/ big
OK: 3952 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 13f4c0c
Reports root/ big
OK: 4352 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 13f4c0c
Reports root/ big
OK: 2232 / Failed: 0 / User-skipped: 639 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 13f4c0c
Reports root/ big
OK: 3978 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 13f4c0c
Reports root/ big
OK: 2572 / Failed: 0 / User-skipped: 634 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 13f4c0c
Reports root/ big
OK: 4352 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 13f4c0c
Reports root/ big
OK: 4338 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / 13f4c0c
Reports root/ big
OK: 2418 / Failed: 0 / User-skipped: 620 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 13f4c0c
Reports root/ big
OK: 4352 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review October 14, 2022 07:38
Copy link
Contributor

@JanuszJakubiec JanuszJakubiec left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@JanuszJakubiec JanuszJakubiec merged commit a9ee17f into master Oct 14, 2022
@JanuszJakubiec JanuszJakubiec deleted the refactor-client-api branch October 14, 2022 10:28
@chrzaszcz chrzaszcz added this to the 6.0.0 milestone Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants