fix DialogId logic

adamchol · adamchol · commit 939a3c3eb94f · 2026-01-17T15:04:36.000+01:00
diff --git a/examples/client/main.rs b/examples/client/main.rs
@@ -7,6 +7,7 @@ use rsipstack::dialog::dialog_layer::DialogLayer;
 use rsipstack::dialog::invitation::InviteOption;
 use rsipstack::dialog::server_dialog::ServerInviteDialog;
 use rsipstack::transaction::endpoint::EndpointInnerRef;
+use rsipstack::transaction::key::TransactionRole;
 use rsipstack::Result;
 use rsipstack::{
     dialog::{authenticate::Credential, registration::Registration},
@@ -306,7 +307,7 @@ async fn process_incoming_request(
         info!("Received transaction: {:?}", tx.key);
 
         match tx.original.to_header()?.tag()?.as_ref() {
-            Some(_) => match dialog_layer.match_dialog(&tx.original) {
+            Some(_) => match dialog_layer.match_dialog(&tx) {
                 Some(mut d) => {
                     tokio::spawn(async move {
                         d.handle(&mut tx).await?;
diff --git a/examples/proxy.rs b/examples/proxy.rs
@@ -257,7 +257,7 @@ async fn process_incoming_request(
             }
             rsip::Method::Ack => {
                 info!(method = ?tx.original.method, "received out of transaction ack");
-                let dialog_id = DialogId::try_from(&tx.original)?;
+                let dialog_id = DialogId::try_from(&tx)?;
                 if !state_ref.inner.sessions.lock().await.contains(&dialog_id) {
                     tx.reply(rsip::StatusCode::NotAcceptable).await?;
                     continue;
@@ -458,7 +458,7 @@ async fn handle_invite(state: AppState, mut tx: Transaction) -> Result<()> {
                             header_pop!(resp.headers, rsip::Header::Via);
                             resp.headers.push_front(record_route.clone().into());
                             if resp.status_code.kind() == rsip::StatusCodeKind::Successful {
-                                let dialog_id = DialogId::try_from(&resp)?;
+                                let dialog_id = DialogId::try_from((&resp, TransactionRole::Client))?;
                                 info!(id = %dialog_id, "add session");
                                 state.inner.sessions.lock().await.insert(dialog_id);
                             }
@@ -523,7 +523,7 @@ async fn handle_bye(state: AppState, mut tx: Transaction) -> Result<()> {
 
     bye_tx.send().await?;
 
-    let dialog_id = DialogId::try_from(&bye_tx.original)?;
+    let dialog_id = DialogId::try_from(&bye_tx)?;
     if state.inner.sessions.lock().await.remove(&dialog_id) {
         info!(id = %dialog_id, "removed session");
     }
@@ -857,8 +857,8 @@ mod tests {
         // Create a test dialog ID
         let dialog_id = DialogId {
             call_id: "test-call-id".to_string(),
-            from_tag: "local-tag".to_string(),
-            to_tag: "remote-tag".to_string(),
+            local_tag: "local-tag".to_string(),
+            remote_tag: "remote-tag".to_string(),
         };
 
         state.inner.sessions.lock().await.insert(dialog_id.clone());
diff --git a/src/bin/bench_ua.rs b/src/bin/bench_ua.rs
@@ -107,7 +107,7 @@ async fn run_server(
                     });
                 }
                 rsip::Method::Bye => {
-                    if let Ok(dialog_id) = DialogId::try_from(&tx.original) {
+                    if let Ok(dialog_id) = DialogId::try_from(&tx) {
                         stats.active_calls.lock().unwrap().remove(&dialog_id);
                         tx.reply(rsip::StatusCode::OK).await.ok();
                         dialog_layer.remove_dialog(&dialog_id);
diff --git a/src/dialog/authenticate.rs b/src/dialog/authenticate.rs
@@ -197,7 +197,7 @@ pub async fn handle_client_authenticate(
             let proxy_header = rsip::header_opt!(resp.headers().iter(), Header::ProxyAuthenticate);
             let proxy_header = proxy_header.ok_or(crate::Error::DialogError(
                 "missing proxy/www authenticate".to_string(),
-                DialogId::try_from(&tx.original)?,
+                DialogId::try_from(tx)?,
                 code,
             ))?;
             Header::ProxyAuthenticate(proxy_header.clone())
diff --git a/src/dialog/client_dialog.rs b/src/dialog/client_dialog.rs
@@ -1,13 +1,16 @@
 use super::dialog::DialogInnerRef;
 use super::DialogId;
-use crate::dialog::{
-    authenticate::handle_client_authenticate,
-    dialog::{DialogState, TerminatedReason, TransactionHandle},
-    subscription::ClientSubscriptionDialog,
-};
 use crate::rsip_ext::RsipResponseExt;
 use crate::transaction::transaction::Transaction;
 use crate::Result;
+use crate::{
+    dialog::{
+        authenticate::handle_client_authenticate,
+        dialog::{DialogState, TerminatedReason, TransactionHandle},
+        subscription::ClientSubscriptionDialog,
+    },
+    transaction::key::TransactionRole,
+};
 use rsip::prelude::HasHeaders;
 use rsip::{prelude::HeadersExt, Header};
 use rsip::{Response, SipMessage, StatusCode};
@@ -702,7 +705,7 @@ impl ClientInviteDialog {
                         None => {}
                     }
 
-                    if let Ok(id) = DialogId::try_from(&resp) {
+                    if let Ok(id) = DialogId::try_from((&resp, TransactionRole::Client)) {
                         dialog_id = id;
                     }
                     match resp.status_code {
diff --git a/src/dialog/dialog.rs b/src/dialog/dialog.rs
@@ -403,7 +403,14 @@ impl DialogInner {
         let from = initial_request.from_header()?.typed()?;
         let mut to = initial_request.to_header()?.typed()?;
         if !to.params.iter().any(|p| matches!(p, Param::Tag(_))) {
-            to.params.push(rsip::Param::Tag(id.to_tag.clone().into()));
+            match role {
+                TransactionRole::Client => to
+                    .params
+                    .push(rsip::Param::Tag(id.remote_tag.clone().into())),
+                TransactionRole::Server => to
+                    .params
+                    .push(rsip::Param::Tag(id.local_tag.clone().into())),
+            }
         }
 
         let mut route_set = vec![];
@@ -461,7 +468,7 @@ impl DialogInner {
     }
 
     pub fn update_remote_tag(&self, tag: &str) -> Result<()> {
-        self.id.lock().unwrap().to_tag = tag.to_string();
+        self.id.lock().unwrap().remote_tag = tag.to_string();
         let mut to = self.to.lock().unwrap();
         *to = to.clone().with_tag(tag.into());
         Ok(())
@@ -795,7 +802,7 @@ impl DialogInner {
                         && !to.params.iter().any(|p| matches!(p, Param::Tag(_)))
                     {
                         to.params.push(rsip::Param::Tag(
-                            self.id.lock().unwrap().to_tag.clone().into(),
+                            self.id.lock().unwrap().local_tag.clone().into(),
                         ));
                     }
                     resp_headers.push(Header::To(to.into()));
diff --git a/src/dialog/dialog_layer.rs b/src/dialog/dialog_layer.rs
@@ -10,7 +10,6 @@ use crate::transaction::make_tag;
 use crate::transaction::{endpoint::EndpointInnerRef, transaction::Transaction};
 use crate::Result;
 use rsip::prelude::HeadersExt;
-use rsip::Request;
 use std::sync::atomic::{AtomicU32, Ordering};
 use std::{
     collections::HashMap,
@@ -155,8 +154,8 @@ impl DialogLayer {
         credential: Option<Credential>,
         local_contact: Option<rsip::Uri>,
     ) -> Result<ServerInviteDialog> {
-        let mut id = DialogId::try_from(&tx.original)?;
-        if !id.to_tag.is_empty() {
+        let mut id = DialogId::try_from(tx)?;
+        if !id.local_tag.is_empty() {
             let dlg = self
                 .inner
                 .dialogs
@@ -175,7 +174,7 @@ impl DialogLayer {
                 }
             }
         }
-        id.to_tag = make_tag().to_string(); // generate to tag
+        id.local_tag = make_tag().to_string(); // generate to tag
 
         let mut local_contact = local_contact;
         if local_contact.is_none() {
@@ -216,8 +215,8 @@ impl DialogLayer {
         credential: Option<Credential>,
         local_contact: Option<rsip::Uri>,
     ) -> Result<ServerSubscriptionDialog> {
-        let mut id = DialogId::try_from(&tx.original)?;
-        if !id.to_tag.is_empty() {
+        let mut id = DialogId::try_from(tx)?;
+        if !id.local_tag.is_empty() {
             let dlg = self
                 .inner
                 .dialogs
@@ -236,7 +235,7 @@ impl DialogLayer {
                 }
             }
         }
-        id.to_tag = make_tag().to_string(); // generate to tag
+        id.local_tag = make_tag().to_string(); // generate to tag
 
         let mut local_contact = local_contact;
         if local_contact.is_none() {
@@ -277,8 +276,8 @@ impl DialogLayer {
         credential: Option<Credential>,
         local_contact: Option<rsip::Uri>,
     ) -> Result<ServerPublicationDialog> {
-        let mut id = DialogId::try_from(&tx.original)?;
-        if !id.to_tag.is_empty() {
+        let mut id = DialogId::try_from(tx)?;
+        if !id.local_tag.is_empty() {
             let dlg = self
                 .inner
                 .dialogs
@@ -297,7 +296,7 @@ impl DialogLayer {
                 }
             }
         }
-        id.to_tag = make_tag().to_string(); // generate to tag
+        id.local_tag = make_tag().to_string(); // generate to tag
 
         let mut local_contact = local_contact;
         if local_contact.is_none() {
@@ -341,8 +340,8 @@ impl DialogLayer {
     ) -> Result<ClientPublicationDialog> {
         let id = DialogId {
             call_id,
-            from_tag,
-            to_tag,
+            local_tag: from_tag,
+            remote_tag: to_tag,
         };
 
         if let Some(Dialog::ClientPublication(dlg)) = self.get_dialog(&id) {
@@ -391,8 +390,8 @@ impl DialogLayer {
     ) -> Result<ClientSubscriptionDialog> {
         let id = DialogId {
             call_id,
-            from_tag,
-            to_tag,
+            local_tag: from_tag,
+            remote_tag: to_tag,
         };
 
         if let Some(Dialog::ClientSubscription(dlg)) = self.get_dialog(&id) {
@@ -501,8 +500,8 @@ impl DialogLayer {
             .map(|d| d.on_remove());
     }
 
-    pub fn match_dialog(&self, req: &Request) -> Option<Dialog> {
-        let id = DialogId::try_from(req).ok()?;
+    pub fn match_dialog(&self, tx: &Transaction) -> Option<Dialog> {
+        let id = DialogId::try_from(tx).ok()?;
         self.get_dialog(&id)
     }
 
diff --git a/src/dialog/invitation.rs b/src/dialog/invitation.rs
@@ -548,7 +548,7 @@ impl DialogLayer {
             }
         }
 
-        let id = DialogId::try_from(&request)?;
+        let id = DialogId::try_from(&tx)?;
         let dlg_inner = DialogInner::new(
             TransactionRole::Client,
             id.clone(),
diff --git a/src/dialog/mod.rs b/src/dialog/mod.rs
@@ -1,4 +1,7 @@
-use crate::{Error, Result};
+use crate::{
+    transaction::{key::TransactionRole, transaction::Transaction},
+    Error, Result,
+};
 use rsip::{
     prelude::{HeadersExt, UntypedHeader},
     Request, Response,
@@ -47,46 +50,17 @@ mod tests;
 /// - During early dialog establishment, `to_tag` may be an empty string
 /// - Dialog ID remains constant throughout the dialog lifetime
 /// - Used for managing and routing SIP messages at the dialog layer
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, Hash, PartialEq, Eq)]
 pub struct DialogId {
     pub call_id: String,
-    pub from_tag: String,
-    pub to_tag: String,
-}
-
-impl PartialEq for DialogId {
-    fn eq(&self, other: &DialogId) -> bool {
-        if self.call_id != other.call_id {
-            return false;
-        }
-        if self.from_tag == other.from_tag && self.to_tag == other.to_tag {
-            return true;
-        }
-        if self.from_tag == other.to_tag && self.to_tag == other.from_tag {
-            return true;
-        }
-        false
-    }
+    pub local_tag: String,
+    pub remote_tag: String,
 }
 
-impl Eq for DialogId {}
-impl std::hash::Hash for DialogId {
-    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
-        self.call_id.hash(state);
-        if self.from_tag > self.to_tag {
-            self.from_tag.hash(state);
-            self.to_tag.hash(state);
-        } else {
-            self.to_tag.hash(state);
-            self.from_tag.hash(state);
-        }
-    }
-}
-
-impl TryFrom<&Request> for DialogId {
+impl TryFrom<(&Request, TransactionRole)> for DialogId {
     type Error = crate::Error;
 
-    fn try_from(request: &Request) -> Result<Self> {
+    fn try_from((request, direction): (&Request, TransactionRole)) -> Result<Self> {
         let call_id = request.call_id_header()?.value().to_string();
 
         let from_tag = match request.from_header()?.tag()? {
@@ -99,18 +73,25 @@ impl TryFrom<&Request> for DialogId {
             None => "".to_string(),
         };
 
-        Ok(DialogId {
-            call_id,
-            from_tag,
-            to_tag,
-        })
+        match direction {
+            TransactionRole::Client => Ok(DialogId {
+                call_id,
+                local_tag: from_tag,
+                remote_tag: to_tag,
+            }),
+            TransactionRole::Server => Ok(DialogId {
+                call_id,
+                local_tag: to_tag,
+                remote_tag: from_tag,
+            }),
+        }
     }
 }
 
-impl TryFrom<&Response> for DialogId {
+impl TryFrom<(&Response, TransactionRole)> for DialogId {
     type Error = crate::Error;
 
-    fn try_from(resp: &Response) -> Result<Self> {
+    fn try_from((resp, direction): (&Response, TransactionRole)) -> Result<Self> {
         let call_id = resp.call_id_header()?.value().to_string();
 
         let from_tag = match resp.from_header()?.tag()? {
@@ -123,24 +104,31 @@ impl TryFrom<&Response> for DialogId {
             None => return Err(Error::Error("to tag not found".to_string())),
         };
 
-        Ok(DialogId {
-            call_id,
-            from_tag,
-            to_tag,
-        })
+        match direction {
+            TransactionRole::Client => Ok(DialogId {
+                call_id,
+                local_tag: from_tag,
+                remote_tag: to_tag,
+            }),
+            TransactionRole::Server => Ok(DialogId {
+                call_id,
+                local_tag: to_tag,
+                remote_tag: from_tag,
+            }),
+        }
+    }
+}
+
+impl TryFrom<&Transaction> for DialogId {
+    type Error = crate::Error;
+
+    fn try_from(value: &Transaction) -> std::result::Result<Self, Self::Error> {
+        DialogId::try_from((&value.original, value.role()))
     }
 }
 
 impl std::fmt::Display for DialogId {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        if self.from_tag > self.to_tag {
-            if self.to_tag.is_empty() {
-                write!(f, "{}-{}", self.call_id, self.from_tag)
-            } else {
-                write!(f, "{}-{}-{}", self.call_id, self.from_tag, self.to_tag)
-            }
-        } else {
-            write!(f, "{}-{}-{}", self.call_id, self.to_tag, self.from_tag)
-        }
+        write!(f, "{}-{}-{}", self.call_id, self.local_tag, self.remote_tag)
     }
 }
diff --git a/src/dialog/registration.rs b/src/dialog/registration.rs
@@ -495,7 +495,7 @@ impl Registration {
         }
         return Err(crate::Error::DialogError(
             "registration transaction is already terminated".to_string(),
-            DialogId::try_from(&tx.original)?,
+            DialogId::try_from(&tx)?,
             StatusCode::BadRequest,
         ));
     }
diff --git a/src/transaction/endpoint.rs b/src/transaction/endpoint.rs
diff --git a/src/transaction/key.rs b/src/transaction/key.rs
diff --git a/src/transaction/transaction.rs b/src/transaction/transaction.rs

Original file line number	Diff line number	Diff line change
`@@ -107,7 +107,7 @@ async fn run_server(`
`107`	`107`	`});`
`108`	`108`	`}`
`109`	`109`	`rsip::Method::Bye => {`
`110`		`- if let Ok(dialog_id) = DialogId::try_from(&tx.original) {`
	`110`	`+ if let Ok(dialog_id) = DialogId::try_from(&tx) {`
`111`	`111`	`stats.active_calls.lock().unwrap().remove(&dialog_id);`
`112`	`112`	`tx.reply(rsip::StatusCode::OK).await.ok();`
`113`	`113`	`dialog_layer.remove_dialog(&dialog_id);`
Original file line number	Diff line number	Diff line change
`@@ -548,7 +548,7 @@ impl DialogLayer {`
`548`	`548`	`}`
`549`	`549`	`}`
`550`	`550`
`551`		`- let id = DialogId::try_from(&request)?;`
	`551`	`+ let id = DialogId::try_from(&tx)?;`
`552`	`552`	`let dlg_inner = DialogInner::new(`
`553`	`553`	`TransactionRole::Client,`
`554`	`554`	`id.clone(),`
Original file line number	Diff line number	Diff line change
`@@ -495,7 +495,7 @@ impl Registration {`
`495`	`495`	`}`
`496`	`496`	`return Err(crate::Error::DialogError(`
`497`	`497`	`"registration transaction is already terminated".to_string(),`
`498`		`- DialogId::try_from(&tx.original)?,`
	`498`	`+ DialogId::try_from(&tx)?,`
`499`	`499`	`StatusCode::BadRequest,`
`500`	`500`	`));`
`501`	`501`	`}`