-
Notifications
You must be signed in to change notification settings - Fork 17
XMPP: Support XEP-0461: Message Replies #134
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?
Changes from all commits
f83f6c4
22feac7
bd2868c
980bcfd
591e021
a7a48c4
cca872e
5deb9b2
eb86f9c
0627ef5
3d40b3c
74b0a93
6129e66
45d4f5b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -8,6 +8,7 @@ import ( | |||||||
| "sync" | ||||||||
| "time" | ||||||||
|
|
||||||||
| lru "github.com/hashicorp/golang-lru/v2" | ||||||||
| "github.com/jpillora/backoff" | ||||||||
| "github.com/matterbridge-org/matterbridge/bridge" | ||||||||
| "github.com/matterbridge-org/matterbridge/bridge/config" | ||||||||
|
|
@@ -29,10 +30,12 @@ type UploadBufferEntry struct { | |||||||
| type Bxmpp struct { | ||||||||
| *bridge.Config | ||||||||
|
|
||||||||
| startTime time.Time | ||||||||
| xc *xmpp.Client | ||||||||
| xmppMap map[string]string | ||||||||
| connected bool | ||||||||
| startTime time.Time | ||||||||
| xc *xmpp.Client | ||||||||
| xmppMap map[string]string | ||||||||
| connected bool | ||||||||
| stanzaIDs *lru.Cache[string, string] | ||||||||
| replyHeaders *lru.Cache[string, xmpp.Reply] | ||||||||
| sync.RWMutex | ||||||||
|
|
||||||||
| avatarAvailability map[string]bool | ||||||||
|
|
@@ -58,12 +61,23 @@ type Bxmpp struct { | |||||||
| } | ||||||||
|
|
||||||||
| func New(cfg *bridge.Config) bridge.Bridger { | ||||||||
| stanzaIDs, err := lru.New[string, string](5000) | ||||||||
| if err != nil { | ||||||||
| cfg.Log.Fatalf("Could not create LRU cache: %v", err) | ||||||||
| } | ||||||||
| replyHeaders, err := lru.New[string, xmpp.Reply](5000) | ||||||||
| if err != nil { | ||||||||
| cfg.Log.Fatalf("Could not create LRU cache: %v", err) | ||||||||
| } | ||||||||
|
|
||||||||
| return &Bxmpp{ | ||||||||
| Config: cfg, | ||||||||
| xmppMap: make(map[string]string), | ||||||||
| avatarAvailability: make(map[string]bool), | ||||||||
| avatarMap: make(map[string]string), | ||||||||
| httpUploadBuffer: make(map[string]*UploadBufferEntry), | ||||||||
| stanzaIDs: stanzaIDs, | ||||||||
| replyHeaders: replyHeaders, | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -142,19 +156,27 @@ func (b *Bxmpp) Send(msg config.Message) (string, error) { | |||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // XEP-0461: populate reply fields if this message is a reply. | ||||||||
| var reply *xmpp.Reply | ||||||||
| if msg.ParentValid() { | ||||||||
| if _reply, ok := b.replyHeaders.Get(msg.ParentID); ok { | ||||||||
| reply = &_reply | ||||||||
| } | ||||||||
| } | ||||||||
|
|
||||||||
| // Post normal message. | ||||||||
| b.Log.Debugf("=> Sending message %#v", msg) | ||||||||
| // Generate a dummy ID because to avoid collision with other internal messages | ||||||||
| msgID := xid.New().String() | ||||||||
| if _, err := b.xc.Send(xmpp.Chat{ | ||||||||
| Type: "groupchat", | ||||||||
| Remote: msg.Channel + "@" + b.GetString("Muc"), | ||||||||
| Text: msg.Username + msg.Text, | ||||||||
| Type: "groupchat", | ||||||||
| Remote: msg.Channel + "@" + b.GetString("Muc"), | ||||||||
| Text: msg.Username + msg.Text, | ||||||||
| OriginID: msgID, | ||||||||
| Reply: reply, | ||||||||
| }); err != nil { | ||||||||
| return "", err | ||||||||
| } | ||||||||
|
|
||||||||
| // Generate a dummy ID because to avoid collision with other internal messages | ||||||||
| // However this does not provide proper Edits/Replies integration on XMPP side. | ||||||||
| msgID := xid.New().String() | ||||||||
| return msgID, nil | ||||||||
| } | ||||||||
|
|
||||||||
|
|
@@ -300,6 +322,13 @@ func (b *Bxmpp) handleXMPP() error { | |||||||
| if v.Type == "groupchat" { | ||||||||
| b.Log.Debugf("== Receiving %#v", v) | ||||||||
|
|
||||||||
| if v.StanzaID.ID != "" { | ||||||||
| // Here the stanza-id has been set by the server and can be used to provide replies | ||||||||
| // as explained in XEP-0461 https://xmpp.org/extensions/xep-0461.html#business-id | ||||||||
| b.stanzaIDs.Add(v.StanzaID.ID, v.ID) | ||||||||
| b.replyHeaders.Add(v.ID, xmpp.Reply{ID: v.StanzaID.ID, To: v.Remote}) | ||||||||
| } | ||||||||
|
Comment on lines
+325
to
+330
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't want to be noisy in another project's repo so I'm following-up on this here, although the issue crosses both projects. Why
|
||||||||
| r := xmpp.Reply{ID: v.StanzaID.ID, To: v.Remote} | |
| b.replyHeaders.Add(v.OriginID, r) | |
| b.Log.Debugf("Caching StanzaID: %v -> %v -> %v", v.StanzaID.ID, v.OriginID, r) |
Caching StanzaID: 2026-03-06-e30b84344d0ead48 -> -> {{ } 2026-03-06-e30b84344d0ead48 bridge-test@example.im/test}
line confirms OriginID is nil.
and the result is that replies don't reply:
is rendered as:
I would love to make only minimal changes to go-xmpp but we tried and found it didn't work 🥲, so this is where we ended up.
We're talking past each other. I'm sure you have an important concern that I'm totally overlooking. Something broad maybe? What is it? Are you worried that id isn't long-term stable? Is it because you prefer to use StanzaID as the canonical ID? Are you worried about putting too much pressure for changes on go-xmpp?
Avoiding ID
I have thought about trying to do this without exposing ID in go-xmpp. It's possible but super awkward and I'm sure it will lead to confusion and bugs.
The task: we need to pick IDs that the gateway can use to refer to each message permanently as soon as the message is sent/received.
The incoming case is easy: set config.Message.ID = xmpp.Chat.StanzaID.ID (which is in fact the actual situation), and then an incoming reply will be tagged with the same StanzaID and can pass it directly up to the gateway as ParentID, so that's great ⭐ .
But the outgoing case is doubly complicated to compensate for the simplicity on that side 😭🧅🧅🧅: it needs to find out the OriginID and/or ID set by go-xmpp (always necessitating some patch to go-xmpp) -- let's say we use OriginID -- and then return that to the gateway as the stable ID. When the echoed copy comes in we need to record it in a outgoingMessageIDs lru.Cache[StanzaID, OriginID] but make sure to detect it was an echo to one of our outgoing messages; how would you do that 1? And then when a reply comes in, we need to check for OriginID , _ := outgoingMessageIDs.Get(StanzaID) and return the OriginID in that case.
So then there's two sets of IDs intermixed in the gateway's cache.
Using .ID everywhere is simpler to reason about.
Using StanzaID
Just to be clear, I am for using StanzaID as the stable ID, but until #159 is done that is really hard because we don't know it until the echo. And that's the kind of patch that can go on for months or years. Please don't let it block XMPP replies; I want to use replies now, and I want to build reactions on it soon.
Having Send() -> xmpp.Chat
mdosch: I think in general it would make sense to return xmpp.Chat as there might also be other stuff generated in the future that might not be of interest today.
I made a POC commit for the Send() function there: xmppo/go-xmpp@1089118
@kousu : I like the idea of xmppo/go-xmpp@1089118 a lot, I can definitely work with that instead
@selfhoster1312 : Please do, as it seems to be the way the go-xmpp maintainer has chosen :)
Having go-xmpp pick the outgoing IDs is cleaner and I will use that, but we still need it to expose .ID because otherwise the code is split-brained between the outgoing and incoming paths for all the reasons above.
🏴☠️ 🥇 🤝 🥇 🏴☠️
I'm sorry this is dragging on so long 😅 . I don't want to turn this into a lumbering monster. What will it take to move it forward?
Footnotes
-
Naively: keep a list
outstandingIDs []OriginID, so that's one extra data structure to juggle. Or avoid it by examiningfrom == "matterbridge@example.im"but I'm worried that's forgeable; it surely risks breaking in weird ways if multiple bridges ran simultaneously by accident or if someone logged in manually as the bridge for testing. So it's hard to see how to avoid juggling that extra list. ↩
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.
Currently matterbridge insists on knowing a message ID right after
bridge.Send. I believe this may be a wrong architecture designed around synchronous operations, but we really want to make matterbridge asynchronous by nature.I think your interpretation of the codebase is correct and "should work". I'm ok to merge this (after more review etc), but i would also appreciate if we took some time in the coming month to document the reply system entirely and make it fully asynchronous.