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

GraphQL - Merge master with HTTP handlers config reworked #3646

Merged
merged 51 commits into from
May 13, 2022

Conversation

Premwoik
Copy link
Contributor

This PR adapts GraphQL to follow the latest changes in the HTTP handlers configuration. Also, use lower JID fields everywhere.

NelsonVides and others added 30 commits April 22, 2022 10:14
Doing maps:find creates the ok-tuple, which means garbage, and also then
needs to match on it. It it more efficient to pattern-match on the map key.
Pattern match the map key for more efficiency
It will be handled by mongoose_config_handler.
The goal is better separation between specs and utils
API:
- config_spec/0 returns the config section spec for all possible HTTP handlers
- get_routes/1 returns Cowboy routes for a list of configured handlers

There are two optional callbacks:
- config_spec/0 returning config specification for the handler,
  if it has any options
- routes/1 returning Cowboy routes, if different from the default

All handlers which have options should be listed
  in configurable_handler_modules/0
Use maps with defaults in the config spec
Use maps with defaults, include all handlers by default
- Use maps with defaults
- Include all handlers by default

New options:
- 'handlers' - to choose specific low-level handlers,
- 'docs' - to disable docs.
It will be placed in mongoose_http_handler

Also: refactor 'store_trails'
- Use maps with defaults
- Add new options
- Rework test layout after reworking mongoose_client_api config
- Remove the custom logic for comparing handler tuples
- Simplify examples to match the defaults in mongooseim.toml
- Document the new options: `handlers` and `docs`
- Update the list of available handlers
By now we have a pretty printer in the logger, and creating accumulators
is a hot operation, so we can skip this field when it is not needed.
Skip pretty-printing of unused accumulator field
Premwoik and others added 15 commits April 27, 2022 11:09
Rework HTTP handler configuration
This PR addresses "There is no easy way to reset cache without making new changes into the code. And sometimes cache breaks, because a committer did something wrong".

Proposed changes include:

    Use CI_CERT_KEY_VERSION env variable to change the key externally.
    Avoid caching invalid certificate data. It should make fixing bugs with certificates a bit easier.
@mongoose-im

This comment was marked as outdated.

@Premwoik Premwoik force-pushed the graphql/merge-http-handlers-config-rerwork branch from fc10607 to 06c7216 Compare May 12, 2022 12:41
@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented May 12, 2022

Codecov Report

Merging #3646 (f786e54) into feature/graphql (6b21198) will increase coverage by 0.05%.
The diff coverage is 95.55%.

@@                 Coverage Diff                 @@
##           feature/graphql    #3646      +/-   ##
===================================================
+ Coverage            81.78%   81.84%   +0.05%     
===================================================
  Files                  479      480       +1     
  Lines                33369    33342      -27     
===================================================
- Hits                 27292    27289       -3     
+ Misses                6077     6053      -24     
Impacted Files Coverage Δ
src/admin_extra/service_admin_extra_roster.erl 87.41% <ø> (ø)
src/config/mongoose_config_parser_toml.erl 99.20% <ø> (ø)
src/config/mongoose_config_validator.erl 97.05% <ø> (ø)
src/ejabberd_sup.erl 85.71% <ø> (ø)
src/jingle_sip/jingle_sip_helper.erl 70.58% <0.00%> (ø)
src/mod_bosh.erl 95.62% <ø> (ø)
src/mod_muc_log.erl 63.21% <0.00%> (ø)
src/mod_sic.erl 76.47% <ø> (ø)
...rc/mongoose_client_api/mongoose_client_api_sse.erl 57.89% <0.00%> (ø)
src/mongoose_session_api.erl 94.44% <ø> (ø)
... and 59 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b21198...f786e54. Read the comment docs.

@Premwoik Premwoik force-pushed the graphql/merge-http-handlers-config-rerwork branch from 06c7216 to f786e54 Compare May 12, 2022 14:24
@mongoose-im
Copy link
Collaborator

mongoose-im commented May 12, 2022

small_tests_24 / small_tests / f786e54
Reports root / small


small_tests_23 / small_tests / f786e54
Reports root / small


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / f786e54
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


dynamic_domains_mysql_redis_24 / mysql_redis / f786e54
Reports root/ big
OK: 3069 / Failed: 0 / User-skipped: 150 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / f786e54
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_24 / ldap_mnesia / f786e54
Reports root/ big
OK: 1689 / Failed: 0 / User-skipped: 444 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / f786e54
Reports root/ big
OK: 3086 / Failed: 0 / User-skipped: 133 / Auto-skipped: 0


ldap_mnesia_23 / ldap_mnesia / f786e54
Reports root/ big
OK: 1689 / Failed: 0 / User-skipped: 444 / Auto-skipped: 0


internal_mnesia_24 / internal_mnesia / f786e54
Reports root/ big
OK: 1735 / Failed: 0 / User-skipped: 398 / Auto-skipped: 0


elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / f786e54
Reports root/ big
OK: 2049 / Failed: 0 / User-skipped: 399 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / f786e54
Reports root/ big
OK: 3460 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mysql_redis_24 / mysql_redis / f786e54
Reports root/ big
OK: 3455 / Failed: 0 / User-skipped: 147 / Auto-skipped: 0


pgsql_mnesia_23 / pgsql_mnesia / f786e54
Reports root/ big
OK: 3460 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


mssql_mnesia_24 / odbc_mssql_mnesia / f786e54
Reports root/ big
OK: 3460 / Failed: 0 / User-skipped: 142 / Auto-skipped: 0


riak_mnesia_24 / riak_mnesia / f786e54
Reports root/ big
OK: 1896 / Failed: 0 / User-skipped: 394 / Auto-skipped: 0

@Premwoik Premwoik marked this pull request as ready for review May 12, 2022 14:55
@Premwoik Premwoik requested a review from chrzaszcz May 12, 2022 14:55
Copy link
Member

@chrzaszcz chrzaszcz 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, I only added a few comments about items that can be done in a separate PR.

config_spec() ->
#section{items = #{<<"username">> => #option{type = binary},
<<"password">> => #option{type = binary},
<<"schema_endpoint">> => #option{type = binary}},
Copy link
Member

Choose a reason for hiding this comment

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

I think there are only a few options here and we shouldn't allow an arbitrary binary, right? Using enum looks like a better option to me, because it would be validated. Another question: would it make sense to make the endpoint an atom?

%% mongoose_http_handler callbacks

-spec config_spec() -> mongoose_config_spec:config_section().
config_spec() ->
Copy link
Member

Choose a reason for hiding this comment

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

We will need a test in config_parser_SUITE for this config spec, see the listen_http_handlers_... test cases for reference.

@chrzaszcz chrzaszcz merged commit bfc1ee0 into feature/graphql May 13, 2022
@chrzaszcz chrzaszcz deleted the graphql/merge-http-handlers-config-rerwork branch May 13, 2022 06:57
@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.

5 participants