Skip to content

Commit

Permalink
.Net: Include request metadata in KernelException if response cannot …
Browse files Browse the repository at this point in the history
…be serialized (#6407)

### Motivation and Context

We include additional metadata in `HttpOperationException` i.e. request
uri, payload, etc. When the response serialization fails we currently
throw a `KernelException` which doesn't include the HTTP request
metadata. This PR changes which exception is thrown to
`HttpOperationException` so we have one type of exception for clients to
catch and this includes all of the additional metadata . This is a
behaviour change but it enables clients to catch a single exception and
to get access to the request uri, payload, etc.

### Description

<!-- Describe your changes, the overall approach, the underlying design.
These notes will help understanding how your code works. Thanks! -->

### Contribution Checklist

<!-- Before submitting this PR, please make sure: -->

- [ ] The code builds clean without any errors or warnings
- [ ] The PR follows the [SK Contribution
Guidelines](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md)
and the [pre-submission formatting
script](https://github.com/microsoft/semantic-kernel/blob/main/CONTRIBUTING.md#development-scripts)
raises no violations
- [ ] All unit tests pass, and I have added new tests where possible
- [ ] I didn't break anyone 😄

---------

Co-authored-by: Dmytro Struk <[email protected]>
  • Loading branch information
markwallace-microsoft and dmytrostruk committed May 30, 2024
1 parent d6b6200 commit 5195f03
Show file tree
Hide file tree
Showing 5 changed files with 47 additions and 3 deletions.
29 changes: 29 additions & 0 deletions dotnet/src/Functions/Functions.OpenApi/RestApiOperationRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,21 @@ internal sealed class RestApiOperationRunner

private const string DefaultResponseKey = "default";

/// <summary>
/// HTTP request method.
/// </summary>
private const string HttpRequestMethod = "http.request.method";

/// <summary>
/// The HTTP request payload body.
/// </summary>
private const string HttpRequestBody = "http.request.body";

/// <summary>
/// Absolute URL describing a network resource according to RFC3986.
/// </summary>
private const string UrlFull = "url.full";

/// <summary>
/// List of payload builders/factories.
/// </summary>
Expand Down Expand Up @@ -186,9 +201,23 @@ internal sealed class RestApiOperationRunner
}
catch (HttpOperationException ex)
{
#pragma warning disable CS0618 // Type or member is obsolete
ex.RequestMethod = requestMessage.Method.Method;
ex.RequestUri = requestMessage.RequestUri;
ex.RequestPayload = payload;
#pragma warning restore CS0618 // Type or member is obsolete

ex.Data.Add(HttpRequestMethod, requestMessage.Method.Method);
ex.Data.Add(UrlFull, requestMessage.RequestUri?.ToString());
ex.Data.Add(HttpRequestBody, payload);

throw;
}
catch (KernelException ex)
{
ex.Data.Add(HttpRequestMethod, requestMessage.Method.Method);
ex.Data.Add(UrlFull, requestMessage.RequestUri?.ToString());
ex.Data.Add(HttpRequestBody, payload);

throw;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1048,7 +1048,11 @@ public async Task ItShouldThrowExceptionForUnsupportedContentTypeAsync()
var sut = new RestApiOperationRunner(this._httpClient, this._authenticationHandlerMock.Object);

// Act & Assert
await Assert.ThrowsAsync<KernelException>(() => sut.RunAsync(operation, arguments));
var kernelException = await Assert.ThrowsAsync<KernelException>(() => sut.RunAsync(operation, arguments));
Assert.Equal("The content type `fake/type` is not supported.", kernelException.Message);
Assert.Equal("POST", kernelException.Data["http.request.method"]);
Assert.Equal("https://fake-random-test-host/fake-path", kernelException.Data["url.full"]);
Assert.Equal("{\"value\":\"fake-value\"}", kernelException.Data["http.request.body"]);
}

[Fact]
Expand Down
4 changes: 2 additions & 2 deletions dotnet/src/IntegrationTests/Plugins/RepairServiceTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -101,8 +101,8 @@ public async Task HttpOperationExceptionIncludeRequestInfoAsync()
catch (HttpOperationException ex)
{
Assert.Equal("Response status code does not indicate success: 404 (Not Found).", ex.Message);
Assert.Equal("Patch", ex.RequestMethod);
Assert.Equal("https://piercerepairsapi.azurewebsites.net/repairs", ex.RequestUri!.ToString());
Assert.Equal("Patch", ex.Data["http.request.method"]);
Assert.Equal("https://piercerepairsapi.azurewebsites.net/repairs", ex.Data["url.full"]);
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ namespace Microsoft.SemanticKernel;
/// <summary>
/// Represents an exception specific to HTTP operations.
/// </summary>
/// <remarks>
/// Instances of this class optionally contain telemetry information in the Exception.Data property using keys that are consistent with the OpenTelemetry standard.
/// See https://opentelemetry.io/ for more information.
/// </remarks>
public class HttpOperationException : Exception
{
/// <summary>
Expand Down Expand Up @@ -65,6 +69,7 @@ public HttpOperationException(HttpStatusCode? statusCode, string? responseConten
/// <remarks>
/// This information is only available in limited circumstances e.g. when using Open API plugins.
/// </remarks>
[Obsolete("This property is obsolete and will be removed in a future version. Use the Exception.Data['Name'] instead.")]
public string? RequestMethod { get; set; }

/// <summary>
Expand All @@ -73,6 +78,7 @@ public HttpOperationException(HttpStatusCode? statusCode, string? responseConten
/// <remarks>
/// This information is only available in limited circumstances e.g. when using Open API plugins.
/// </remarks>
[Obsolete("This property is obsolete and will be removed in a future version. Use the Exception.Data['Url'] instead.")]
public Uri? RequestUri { get; set; }

/// <summary>
Expand All @@ -81,5 +87,6 @@ public HttpOperationException(HttpStatusCode? statusCode, string? responseConten
/// <remarks>
/// This information is only available in limited circumstances e.g. when using Open API plugins.
/// </remarks>
[Obsolete("This property is obsolete and will be removed in a future version. Use the Exception.Data['Data'] instead.")]
public object? RequestPayload { get; set; }
}
4 changes: 4 additions & 0 deletions dotnet/src/SemanticKernel.Abstractions/KernelException.cs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ namespace Microsoft.SemanticKernel;
/// <summary>
/// Represents the base exception from which all Semantic Kernel exceptions derive.
/// </summary>
/// <remarks>
/// Instances of this class optionally contain telemetry information in the Exception.Data property using keys that are consistent with the OpenTelemetry standard.
/// See https://opentelemetry.io/ for more information.
/// </remarks>
public class KernelException : Exception
{
/// <summary>
Expand Down

0 comments on commit 5195f03

Please sign in to comment.