Qualify did exch connection lookup by role #1670
Conversation
Signed-off-by: Ian Costanzo <[email protected]>
Signed-off-by: Ian Costanzo <[email protected]>
| try: | ||
| conn_rec = await ConnRecord.retrieve_by_request_id( | ||
| session, response._thread_id, | ||
| their_role=ConnRecord.Role.RESPONDER.rfc160, |
There was a problem hiding this comment.
It looks like the rfc23 roles should be used, but the created connection records are using the rfc160 roles ... so I included checks for both
Signed-off-by: Ian Costanzo <[email protected]>
Codecov Report
@@ Coverage Diff @@
## main #1670 +/- ##
==========================================
- Coverage 95.50% 95.50% -0.01%
==========================================
Files 528 528
Lines 32752 32766 +14
==========================================
+ Hits 31280 31293 +13
- Misses 1472 1473 +1 |
There was a problem hiding this comment.
Great! Thanks @ianco
Adding their_role to the TAG_NAMES means this will prevent old exchanges from finishing after PR has been released. This is not an issue for completed or new exchanges so I guest it's not really an issue?
Good to keep track somehow of later added tags, as now it may look you can query record by the their_role property, but this is only the case for records saved after this PR has been merged.
|
We do have the "migrate" mechanism to update stored data on deployment of a new version, but given the tight scope of this change, I don't think we need to use it. Agreed? We used the mechanism with 0.73 to update connection records. |
|
I don't think we need to "migrate" for this function because it's only used during the process of establishing a connection. Once the connection is established we don't use the "request_id" to lookup the connection. |
Signed-off-by: Ian Costanzo [email protected]
Fixes issue #1541