Johtaylo/cloudadapterstreaming #5178
Conversation
johnataylor
requested review from
DDEfromOR,
EricDahlvang,
carlosscastro,
gabog,
mrivera-ms and
stevengum
libraries/Microsoft.Bot.Connector/Authentication/ParameterizedBotFrameworkAuthentication.cs
Show resolved
Hide resolved
| var configuration = applicationBuilder.ApplicationServices.GetService<IConfiguration>(); | ||
| if (configuration != null) | ||
| { | ||
| audience = audience ?? configuration.GetSection("ToChannelFromBotOAuthScope")?.Value ?? GetBuiltinDefaultAudience(configuration); |
There was a problem hiding this comment.
This makes sense, but worried me for a second as there's a bit of logic (straightforward logic, but still a decent amount) that goes into populating the audience. @EricDahlvang and @DDEfromOR what do you think?
There was a problem hiding this comment.
So I tried to keep the logic in the CloudAdapter class itself clean and so pushed configuration and defaulting logic to this helper.
Some of the complexity comes from not breaking the current method. It would be easy to get into an ambiguous overload. So the new overload just requires everything.
Yes surprisingly complicated - despite the appropriate (and improved) use of the DI helpers.
More fundamentally starting the pipe from this extension seems awkward - and dropping the return task. But that is what the current method does. (Ultimately the protocol semantics are application logic to application logic so there isn't much that can be done at this level anyhow.)
Anyhow this is just a helper - for example the adaptive runtime does not need to use this helper at all.
libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/CloudAdapter.cs
Outdated
Show resolved
Hide resolved
tests/integration/Microsoft.Bot.Builder.Integration.AspNet.Core.Tests/CloudAdapterTests.cs
Outdated
Show resolved
Hide resolved
libraries/Microsoft.Bot.Connector/Authentication/BotFrameworkAuthentication.cs
Show resolved
Hide resolved
libraries/Microsoft.Bot.Connector/Authentication/BotFrameworkAuthenticationFactory.cs
Outdated
Show resolved
Hide resolved
|
also fixes #4774 |
|
and fixes #4835 |
libraries/integration/Microsoft.Bot.Builder.Integration.AspNet.Core/CloudAdapter.cs
Show resolved
Hide resolved
5 participants
fixes #4850 #4674
The most significant code is included in the CloudAdapter class in the integration project. Basically line 126 onwards in that class.
This codes does make some use of the existing streaming SDK integration, specifically StreamingRequestHandler. However, it keeps that dependency internal.
The most significant difference between this implementation and the existing streaming implementation is that all the outbound traffic is actually funneled through an internal HttpClient implementation that abstracts the underlying stream. There was a partial implementation of that idea but it was not wired up nor did it work. This PR does not touch that code.
The existing streaming implementation also performs a token refreshing call (per virtual "request" over the socket) in order to keep the AD registration alive, I've not yet added that.