fix http adapter errors #2593
fix http adapter errors #2593
Conversation
johnataylor
requested review from
cleemullins,
EricDahlvang and
Virtual-Josh
Pull Request Test Coverage Report for Build 80284
💛 - Coveralls |
|
✔️ No Binary Compatibility issues for Microsoft.Bot.Builder.dll compared against version 4.3.1 |
| @@ -62,6 +62,12 @@ public async Task ProcessAsync(HttpRequest httpRequest, HttpResponse httpRespons | |||
| // deserialize the incoming Activity | |||
| var activity = HttpHelper.ReadRequest(httpRequest); | |||
|
|
|||
| if (string.IsNullOrEmpty(activity?.Type)) | |||
There was a problem hiding this comment.
Consider a detailed comment here, as the behavior is non-obvious.
| { | ||
| activity = BotMessageSerializer.Deserialize<Activity>(bodyReader); | ||
| return null; |
There was a problem hiding this comment.
This should log, using our infrastructure so it ends up in the right place. I can see that being important to debugging sessions by the Supportability team.
There was a problem hiding this comment.
This will be logged at the HTTP level. This change uses the correct HTTP error code.
| @@ -46,6 +46,12 @@ public async Task ProcessAsync(HttpRequestMessage httpRequest, HttpResponseMessa | |||
| // deserialize the incoming Activity | |||
| var activity = await HttpHelper.ReadRequestAsync(httpRequest, cancellationToken).ConfigureAwait(false); | |||
|
|
|||
| if (string.IsNullOrEmpty(activity?.Type)) | |||
| { | |||
| httpResponse.StatusCode = HttpStatusCode.BadRequest; | |||
There was a problem hiding this comment.
I would consider logging here as well.
| if (string.IsNullOrEmpty(activity?.Type)) | ||
| { | ||
| httpResponse.StatusCode = (int)HttpStatusCode.BadRequest; | ||
| return; |
There was a problem hiding this comment.
Consider logging something. That way when support requests come in complaining "My Bot is returning Bad Request,and I'm not sure why" the truth is in the log files.
| var activity = await request.Content.ReadAsAsync<Activity>(BotMessageMediaTypeFormatters, cancellationToken).ConfigureAwait(false); | ||
| return activity; | ||
| } | ||
| catch (UnsupportedMediaTypeException) |
| } | ||
| catch (JsonException) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
Ditto point about logging - look at the HTTP traffic for the result. Bad Request is exactly that. Its not a server error - at the application level the client should log this. This makes HTTP logs more meaningful.
| @@ -142,6 +142,32 @@ public void ConstructorWithConfiguration() | |||
| Assert.Equal("botOpenIdMetadata", GovernmentChannelValidation.OpenIdMetadataUrl); | |||
| } | |||
|
|
|||
| [Fact] | |||
There was a problem hiding this comment.
Beautiful. I've got tears of joy.
This really is quite slick.
Both integration implementations return BadRequest when called with something other than an Activity.
An Activity being something that can be serialized into an Activity with a Type. All fields other than Type are optional. Technically at any one release, Type is restricted in what values it can take, however, this is not practical to constrain at this level (it is handled in the ActivityHandler layer.)