-
Notifications
You must be signed in to change notification settings - Fork 438
Stable stanza ID #4633
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
base: master
Are you sure you want to change the base?
Stable stanza ID #4633
Conversation
|
CircleCI results for 8ceaa0f elasticsearch_and_cassandra_latest / elasticsearch_and_cassandra_mnesia / b447414 small_tests_latest / small_tests / b447414 small_tests_legacy / small_tests / b447414 small_tests_latest_arm64 / small_tests / b447414 ldap_mnesia_legacy / ldap_mnesia / b447414 ldap_mnesia_latest / ldap_mnesia / b447414 internal_mnesia_latest / internal_mnesia / b447414 dynamic_domains_mysql_redis_latest / mysql_redis / b447414 dynamic_domains_pgsql_mnesia_latest / pgsql_mnesia / b447414 dynamic_domains_pgsql_mnesia_legacy / pgsql_mnesia / b447414 pgsql_cets_latest / pgsql_cets / b447414 pgsql_mnesia_legacy / pgsql_mnesia / b447414 cockroachdb_cets_latest / cockroachdb_cets / b447414 pgsql_mnesia_latest / pgsql_mnesia / b447414 mysql_redis_latest / mysql_redis / b447414 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4633 +/- ##
==========================================
- Coverage 86.09% 86.03% -0.06%
==========================================
Files 566 567 +1
Lines 33936 33968 +32
==========================================
+ Hits 29216 29224 +8
- Misses 4720 4744 +24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8ceaa0f to
be18199
Compare
2075e45 to
5ffeb28
Compare
bb7ae5d to
f49cb84
Compare
f49cb84 to
64cc062
Compare
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.
Pull request overview
This PR implements XEP-0359 (Unique and Stable Stanza ID) to provide stable message IDs across the XMPP system. When a user sends a message, mod_stanzaid generates a unique ID and stores it in the accumulator. This ID is then used by the message archive (MAM) and attached as a stanza-id element to outgoing messages, ensuring all recipients see the same ID.
Changes:
- Introduces mod_stanzaid module to generate and attach stable stanza IDs to messages
- Updates MAM (PM and MUC) to use stable stanza IDs when available instead of generating new ones
- Modifies MUC and MUC Light message routing to preserve and propagate stable stanza IDs through EventData
- Adds comprehensive test coverage for PM, MUC, and MUC Light scenarios with and without MAM
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| src/mod_stanzaid.erl | New module implementing XEP-0359; generates IDs on send, adds stanza-id elements to messages |
| src/muc/mod_muc_room.erl | Updates routed_message record to include stable_stanza_id; extracts from accumulator and passes through routing |
| src/muc/mod_muc.erl | Adds optional stable_stanza_id field to room_event_data type definition |
| src/muc_light/mod_muc_light_codec_modern.erl | Retrieves stable_stanza_id from accumulator and includes in EventData |
| src/muc_light/mod_muc_light_codec_legacy.erl | Retrieves stable_stanza_id from accumulator and includes in EventData |
| src/mam/mod_mam_pm.erl | Uses stable_stanza_id when available; prevents overwriting with mam_id when stable ID is used |
| src/mam/mod_mam_muc.erl | Uses stable_stanza_id from EventData when available for MUC message archiving |
| doc/modules/mod_stanzaid.md | Documentation for the new module including configuration and MAM interaction notes |
| doc/modules/mod_mam.md | Updates noting that certain settings may be overridden by mod_stanzaid |
| doc/configuration/Modules.md | Adds mod_stanzaid to the modules list |
| big_tests/tests/mod_stanzaid_SUITE.erl | Comprehensive test suite covering PM, MUC, and MUC Light with and without MAM |
| big_tests/dynamic_domains.spec | Adds mod_stanzaid_SUITE to test spec |
| big_tests/default.spec | Adds mod_stanzaid_SUITE to test spec |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| has_stanza_id(Ch); | ||
| has_stanza_id([]) -> | ||
| false; | ||
| has_stanza_id([#xmlel{name = <<"stanza-id">>, attrs = #{<<"id">> := _, |
Copilot
AI
Jan 30, 2026
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.
The has_stanza_id function checks for the presence of a stanza-id element but doesn't verify the "by" attribute. According to XEP-0359, a stanza-id element must have both "id" and "by" attributes. The function should check for the "by" attribute to ensure it's a valid stanza-id element, preventing false positives from malformed elements.
| has_stanza_id([#xmlel{name = <<"stanza-id">>, attrs = #{<<"id">> := _, | |
| has_stanza_id([#xmlel{name = <<"stanza-id">>, attrs = #{<<"id">> := _, | |
| <<"by">> := _, |
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 care?
| Activating this module implicitly sets: | ||
|
|
||
| `modules.mod_mam.pm.same_mam_id_for_peers = true` | ||
|
|
||
| `modules.mod_mam.no_stanzaid_element = false` | ||
|
|
||
| overriding any custom MAM settings. |
Copilot
AI
Jan 30, 2026
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.
The documentation states that activating mod_stanzaid implicitly sets modules.mod_mam.pm.same_mam_id_for_peers = true and modules.mod_mam.no_stanzaid_element = false, but there's no code in mod_stanzaid.erl that actually configures these settings. Either implement the configuration override mechanism or update the documentation to reflect that users must manually configure these MAM settings for proper integration with mod_stanzaid.
| Activating this module implicitly sets: | |
| `modules.mod_mam.pm.same_mam_id_for_peers = true` | |
| `modules.mod_mam.no_stanzaid_element = false` | |
| overriding any custom MAM settings. | |
| For proper integration with [mod_mam](mod_mam.md), you should configure the following options: | |
| `modules.mod_mam.pm.same_mam_id_for_peers = true` | |
| `modules.mod_mam.no_stanzaid_element = false` | |
| These options are not set automatically by `mod_stanzaid`; they must be configured explicitly in your MAM settings. |
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.
I don't think this one is a valid concern.
…addresses PR review made by AI, namely Copilot. Welcome to the future.
This PR addresses the need for stable stanza ids by implementing XEP-0359.
When a user sends a message, we generate a unique id and store it in accumulator. That id is then used by message archive. Finally we attached a
stanza-idelement to the outgoing message so that the recipients (or all of them in case of MUC) use the same id.