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

Implement XEP-0004: Data Forms in a separate module #4028

Merged
merged 16 commits into from
May 31, 2023

Conversation

chrzaszcz
Copy link
Member

@chrzaszcz chrzaszcz commented May 24, 2023

The main goal is to have a single module that implements XEP-0004 and XEP-0068 for better separation of concerns, reusability and less xmlel manipulation.

The implementation is divided into two parts:

  • Parsing of submitted forms into a parsed_form() map
  • Form construction, which builds XML from a form() map

The two data structures are different, because they serve different purposes, but they are both maps.

The form should be parsed/constructed only once. Ideally we should minimize the operations on plain XML elements, i.e.:

  • Parsing should be done as early as possible.
  • Construction should be done as late as possible.

Other changes:

  • Fix a bug in event pusher that was randomly triggered by the changes
  • Improve error messages

Excluded from this PR:

  • Improving test coverage. There are some lines that were not covered before, and can be covered by testing error conditions with missing or malformed data forms. We can do it in a later PR, but here I wanted to limit the change set.

@mongoose-im

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented May 24, 2023

Codecov Report

Patch coverage: 85.22% and project coverage change: +0.02 🎉

Comparison is base (3eff59b) 82.19% compared to head (4ff9d5a) 82.21%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4028      +/-   ##
==========================================
+ Coverage   82.19%   82.21%   +0.02%     
==========================================
  Files         535      536       +1     
  Lines       33841    33782      -59     
==========================================
- Hits        27814    27774      -40     
+ Misses       6027     6008      -19     
Impacted Files Coverage Δ
src/event_pusher/mod_event_pusher_push_plugin.erl 100.00% <ø> (ø)
src/hooks/mongoose_hooks.erl 95.68% <ø> (ø)
src/inbox/mod_inbox_utils.erl 96.59% <ø> (-0.22%) ⬇️
src/jlib.erl 90.90% <ø> (+0.73%) ⬆️
src/vcard/mod_vcard_backend.erl 100.00% <ø> (ø)
src/vcard/mod_vcard_riak.erl 0.00% <0.00%> (ø)
src/mod_muc.erl 74.05% <50.00%> (-0.19%) ⬇️
src/mod_caps.erl 78.92% <66.66%> (+1.76%) ⬆️
src/pubsub/node_push.erl 93.33% <66.66%> (-3.89%) ⬇️
src/pubsub/mod_pubsub.erl 73.74% <72.91%> (+0.24%) ⬆️
... and 19 more

... and 6 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 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.

@chrzaszcz chrzaszcz force-pushed the mongoose-data-forms branch 2 times, most recently from 01415a3 to c9624c1 Compare May 26, 2023 16:50
@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

@mongoose-im

This comment was marked as outdated.

The implementation is divided into two parts:
- Parsing of submitted forms into a parsed_form() map
- Form construction, which builds XML from a form() map

The form should be parsed/constructed only once.

Ideally we should minimize the operations on plain XML, i.e.:
- Parsing should be done as early as possible.
- Construction should be done as late as possible.
The form is now parsed to a map instead of a KV list, because it is
later converted to JSON, which is saved to RDBMS. The DB can alter the
order of JSON pairs when they are read, which makes pattern-matching
for already sent push notifications fail unpredictably, resulting in
double notifications, i.e. by using maps we eliminate an elusive bug.
@mongoose-im

This comment was marked as outdated.

- Expect map instead of list (internal format has changed)
- Remove test repetition
@mongoose-im
Copy link
Collaborator

mongoose-im commented May 30, 2023

small_tests_24 / small_tests / 3c34031
Reports root / small


small_tests_25_arm64 / small_tests / 3c34031
Reports root / small


small_tests_25 / small_tests / 3c34031
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 3c34031
Reports root/ big
OK: 2222 / Failed: 0 / User-skipped: 834 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 3c34031
Reports root/ big
OK: 4194 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 3c34031
Reports root/ big
OK: 2222 / Failed: 0 / User-skipped: 834 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 3c34031
Reports root/ big
OK: 4194 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 3c34031
Reports root/ big
OK: 4567 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 3c34031
Reports root/ big
OK: 4191 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 3c34031
Reports root/ big
OK: 4168 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 3c34031
Reports root/ big
OK: 2728 / Failed: 0 / User-skipped: 667 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 3c34031
Reports root/ big
OK: 2368 / Failed: 0 / User-skipped: 688 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 3c34031
Reports root/ big
OK: 4553 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 3c34031
Reports root/ big
OK: 4564 / Failed: 0 / User-skipped: 100 / Auto-skipped: 0

@mongoose-im
Copy link
Collaborator

mongoose-im commented May 30, 2023

small_tests_24 / small_tests / 4ff9d5a
Reports root / small


small_tests_25_arm64 / small_tests / 4ff9d5a
Reports root / small


small_tests_25 / small_tests / 4ff9d5a
Reports root / small


ldap_mnesia_24 / ldap_mnesia / 4ff9d5a
Reports root/ big
OK: 2222 / Failed: 0 / User-skipped: 834 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 4ff9d5a
Reports root/ big
OK: 4194 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 4ff9d5a
Reports root/ big
OK: 4194 / Failed: 0 / User-skipped: 88 / Auto-skipped: 0


ldap_mnesia_25 / ldap_mnesia / 4ff9d5a
Reports root/ big
OK: 2222 / Failed: 0 / User-skipped: 834 / Auto-skipped: 0


dynamic_domains_mysql_redis_25 / mysql_redis / 4ff9d5a
Reports root/ big
OK: 4168 / Failed: 0 / User-skipped: 114 / Auto-skipped: 0


dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 4ff9d5a
Reports root/ big
OK: 4191 / Failed: 0 / User-skipped: 91 / Auto-skipped: 0


internal_mnesia_25 / internal_mnesia / 4ff9d5a
Reports root/ big
OK: 2368 / Failed: 0 / User-skipped: 688 / Auto-skipped: 0


pgsql_mnesia_24 / pgsql_mnesia / 4ff9d5a
Reports root/ big
OK: 4567 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 4ff9d5a
Reports root/ big
OK: 2728 / Failed: 0 / User-skipped: 667 / Auto-skipped: 0


pgsql_mnesia_25 / pgsql_mnesia / 4ff9d5a
Reports root/ big
OK: 4567 / Failed: 0 / User-skipped: 97 / Auto-skipped: 0


mysql_redis_25 / mysql_redis / 4ff9d5a
Reports root/ big
OK: 4553 / Failed: 0 / User-skipped: 111 / Auto-skipped: 0


mssql_mnesia_25 / odbc_mssql_mnesia / 4ff9d5a
Reports root/ big
OK: 4564 / Failed: 0 / User-skipped: 100 / Auto-skipped: 0

@chrzaszcz chrzaszcz marked this pull request as ready for review May 30, 2023 16:00
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, I've added minor formatting comments, but in my opinion they are too minor to be relevant.

jlib:parse_xdata_submit(XEl)) of
-spec is_allowed_log_change([{binary(), [binary()]}], state(), jid:jid()) -> boolean().
is_allowed_log_change(XData, StateData, From) ->
case lists:keymember(<<"muc#roomconfig_enablelogging">>, 1, XData) of
false ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong indentation, minor

jlib:parse_xdata_submit(XEl)) of
-spec is_allowed_persistent_change([{binary(), [binary()]}], state(), jid:jid()) -> boolean().
is_allowed_persistent_change(XData, StateData, From) ->
case lists:keymember(<<"muc#roomconfig_persistentroom">>, 1, XData) of
false ->
Copy link
Contributor

Choose a reason for hiding this comment

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

Wrong indentation, minor

@JanuszJakubiec JanuszJakubiec merged commit 0bd9702 into master May 31, 2023
4 checks passed
@JanuszJakubiec JanuszJakubiec deleted the mongoose-data-forms branch May 31, 2023 09:06
@chrzaszcz chrzaszcz added this to the 6.2.0 milestone Dec 11, 2023
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