-
Notifications
You must be signed in to change notification settings - Fork 204
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
fix(routing): add connection type on mediation grant #1147
fix(routing): add connection type on mediation grant #1147
Conversation
Signed-off-by: Ariel Gentile <[email protected]>
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.
Thanks @genaris! Could you add some tests to make sure it correctly adds instead of overwrites?
Codecov Report
@@ Coverage Diff @@
## main #1147 +/- ##
==========================================
+ Coverage 88.32% 88.35% +0.02%
==========================================
Files 706 706
Lines 16430 16437 +7
Branches 2664 2664
==========================================
+ Hits 14512 14523 +11
+ Misses 1911 1907 -4
Partials 7 7
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
Signed-off-by: Ariel Gentile <[email protected]>
Signed-off-by: Ariel Gentile <[email protected]>
@@ -642,6 +642,28 @@ export class ConnectionService { | |||
return connectionRecord | |||
} | |||
|
|||
public async addConnectionType(agentContext: AgentContext, connectionRecord: ConnectionRecord, type: string) { | |||
const tags = (connectionRecord.getTag('connectionType') as string[]) || ([] as string[]) |
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.
Now that the typing is correctly set on getTags, I think the casting is not needed anymore?
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.
If we use getTag('connectionType')
it's still needed. However I think it's more elegant to use getTags().connectionType
, where the typing is already there as you say.
} | ||
|
||
public async getConnectionTypes(connectionRecord: ConnectionRecord) { | ||
const tags = connectionRecord.getTag('connectionType') as string[] |
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.
This cast is incorrect (as it could be null), but per my comment above, not sure if the cast is needed anymore
Signed-off-by: Ariel Gentile <[email protected]>
Small update to the feature added in #994 to aggregate the connection type when a connection is related to a mediation grant.
Previously, it was setting only
ConnectionType.Mediator
, so if the connection was previously tagged with another type, it would be lost.Also fixes typing for
connectionType
inConnectionRecord
in order to be able to properly query for connections matching one or multiple types.