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

Unify API for stanzas #3814

Merged
merged 7 commits into from
Oct 25, 2022
Merged

Unify API for stanzas #3814

merged 7 commits into from
Oct 25, 2022

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented Oct 17, 2022

The main goal is to make mongoose_stanza_api:

  • Contain all API calls needed by the GraphQL and REST API for stanzas.
  • Return {ok, Result} or {ErrorReasonAtom, ErrorMsg} like the remaining API modules.
  • Handle common errors for REST and GraphQL.

Functional changes

  • Allow stanza without 'from' when 'user' is known, because 'from' is set to the user JID later in the routing process anyway.
  • Return Stanza ID for GraphQL and REST. MAM ID was used for GraphQL before. We can add MAM ID as a separate GraphQL field when needed.
  • Set Stanza ID if the stanza does not already have one - this is similar to the behaviour for messages.
  • Do not treat 0 and <<>> values as undefined - this was a bit unexpected, and now in the unified API the user can always omit them when needed.
  • Set correct max_result_limit - previously it was set to 1.
  • Check for mod_mam_pm before querying the last messages with GraphQL. This is not done for REST, as we don't have a simple way to do it. We are not developing REST further anyway.

Refactoring notes

  • The concept of folding over several steps is used in mongoose_stanza_api. We could reuse it in other modules as well (by moving to a separate module later on), but I don't want to force it. Anyway, the fold function, which has all the logic, is very simple. The advantages are steps reusability (even between API's in the future), and readability (the happy path is easy to follow).

@codecov
Copy link

codecov bot commented Oct 19, 2022

Codecov Report

Base: 83.03% // Head: 83.01% // Decreases project coverage by -0.02% ⚠️

Coverage data is based on head (f78be93) compared to base (187cb40).
Patch coverage: 98.98% of modified lines in pull request are covered.

❗ Current head f78be93 differs from pull request most recent head 4c81efb. Consider uploading reports for the commit 4c81efb to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3814      +/-   ##
==========================================
- Coverage   83.03%   83.01%   -0.03%     
==========================================
  Files         529      528       -1     
  Lines       33886    33822      -64     
==========================================
- Hits        28138    28076      -62     
+ Misses       5748     5746       -2     
Impacted Files Coverage Δ
src/mam/mod_mam_utils.erl 89.63% <ø> (-0.40%) ⬇️
src/mongoose_stanza_api.erl 98.38% <98.36%> (+8.38%) ⬆️
...l/admin/mongoose_graphql_stanza_admin_mutation.erl 100.00% <100.00%> (ø)
...phql/admin/mongoose_graphql_stanza_admin_query.erl 100.00% <100.00%> (ø)
src/graphql/mongoose_graphql_helper.erl 100.00% <100.00%> (ø)
src/graphql/mongoose_graphql_stanza_helper.erl 100.00% <100.00%> (ø)
...hql/user/mongoose_graphql_stanza_user_mutation.erl 100.00% <100.00%> (+4.76%) ⬆️
...raphql/user/mongoose_graphql_stanza_user_query.erl 100.00% <100.00%> (ø)
...mongoose_admin_api/mongoose_admin_api_messages.erl 100.00% <100.00%> (ø)
.../mongoose_admin_api/mongoose_admin_api_stanzas.erl 100.00% <100.00%> (ø)
... and 26 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.

@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.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

- Move functions from mongoose_stanza_helper, because there is no need
  for two separate modules here.
- Perform a fold over individual steps to organize error handling.
  The 'fold' function can be later reused, and moved e.g. to
  mongoose_api_common.
- Handle more error cases, that were caught only by graphql-specific
  code. Now the REST api handles them as well.
- Allow stanza without 'from' when 'user' is known.
  'from' is set to the user JID later in the routing process anyway.
- Use Stanza ID not only for REST. MAM ID was used for GraphQL.
  We can add MAM ID as a separate GraphQL field when needed.
- Set Stanza ID if the stanza does not already have one - this is
  similar to the behaviour for messages.
- Do not treat 0 and <<>> values as undefined - this is a bit
  unexpected, and now in the unified API the user can always omit
  them when needed.
- Set correct max_result_limit - previously it was set to 1.
- Call the new API, which makes it possible to ged rid of some checks.
- Remove unused helper functions.
- Make format_error/2 more flexible.
- The new API makes it possible to avoid some checks
@mongoose-im

This comment was marked as outdated.

- The Id is a stanza Id now.
- It is possible to send a stanza with predefined Id and/or without 'from'.
- Update error messages.
- Update error messages
- Test getting messages for a non-existent user
@mongoose-im

This comment was marked as outdated.

@chrzaszcz chrzaszcz marked this pull request as ready for review October 24, 2022 07:11
get_id(Stanza) ->
exml_query:attr(Stanza, <<"id">>, undefined).

fold({_, _} = Result, _) ->
Copy link
Contributor

Choose a reason for hiding this comment

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

I really enjoyed reading this fragment of code 😄

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 75c9692 into master Oct 25, 2022
@JanuszJakubiec JanuszJakubiec deleted the api-module-rework branch October 25, 2022 07:23
@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.

3 participants