Fix DialogId structure and logic

[adamchol]

This is my proposition of the DialogId fix described in #78.

[shenjinti]

Thanks for the feedback.

I really like your idea of binding the ID creation to the TransactionRole.

In fact, I'm thinking of talking it a step further:

1. DialogId::try_from will primarily support &Transaction. Since the Transaction already knows its role and contains the orignal request, the eliminates any ambiguity and makes the API much cleaner.
2. match_dialog will also updated to accept a &Transaction instead of a raw Request.

This seems to combine the safety of your PR #80 with better encapsulation. I also agree with your Display implementation suggestion.

What do you think about this Transaction-first approach?

[adamchol]

You can say that it's already Transaction-first but you got to have some ability to make a DialogId from a pure Request/Response as well. Making DialogId out of Transaction like in TryFrom<&Transaction> is great but this means constructing the ID from the original request from the transaction, which is not always what's intended. The original request doesn't include all of the tags so we have to have a way to make the ID from arbitrary response/request as well I think.

This is especially true for UAC where the dialog ID is kinda dynamic - first it comes partly from the original request but then you could retrieve the full ID from the final response.

And here I'm obviously talking about DialogId as a standalone module which should be capable of things I described.

[adamchol]

Hey @shenjinti, let me know if you guys plan on merging this. Idk if I should fix the tests or leave it if you are not intereseted.

[yeoleobun]

Hey @shenjinti, let me know if you guys plan on merging this. Idk if I should fix the tests or leave it if you are not intereseted.

This pr have some conflict with the previous merged commit, would you mind update it?

And here is my opinion:

According RFC 3261: https://datatracker.ietf.org/doc/html/rfc3261#section-6:

Dialog: A dialog is a peer-to-peer SIP relationship between two
UAs that persists for some time. A dialog is established by
SIP messages, such as a 2xx response to an INVITE request. A
dialog is identified by a call identifier, local tag, and a
remote tag. A dialog was formerly known as a call leg in RFC
2543.

so call id should consist of call id, local tag and remote tag instead of from tag and to tag.

and it can distinguishing in your loop back test, because they are different role.

[adamchol]

Great, I'll resolve conflicts and update the tests ASAP

[adamchol]

@yeoleobun done. Tests fixes and conflicts resolved. Also tested with some of my load tests. Let me know if you got any comments after review :D

[adamchol]

Hey, can this be merged now?

[yeoleobun]

Hey, can this be merged now?

Apologies for the delay. I forgot to submit the review; I thought you could already see my comments of the pending review.

We should keep this empty check because the client dialog is created BEFORE a response with a To tag is received. This will results in a To header like: <sip:user@domain:port>;tag=.
While this behavior is not strictly RFC 3261 compliant, we are maintaining it for backward compatibility.

So, we want keep the empty check in DialogInner::new in src/dialog/dialog.rs#L435-440

[adamchol]

Alright, done!

[yeoleobun]

We appreciate your valuable contribution and your patience during this process.