Skip to content

Commit

Permalink
.Net: [Error handling] [Part2] Replacing a few connector exceptions b…
Browse files Browse the repository at this point in the history
…y SKException (microsoft#2298)

### Motivation and Context

This PR is one of the follow-up PRs aimed at gradually replacing SK
custom exceptions with SKException to provide a simple, consistent, and
extensible exception model. You can find more details in the
[ADR](https://github.com/microsoft/semantic-kernel/blob/main/docs/decisions/0004-error-handling.md)
discussing error handling.

### Description

This PR changes RedisMemoryStore, GrpcOperationRunner,
ProtoDocumentParser, and a few more connectors to throw SKException
instead of their dedicated exceptions and removes those exceptions.

### Contribution Checklist

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

- [x] The code builds clean without any errors or warnings
- [x] 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
- [x] All unit tests pass, and I have added new tests where possible
- [x] I didn't break anyone 😄
  • Loading branch information
SergeyMenshykh committed Aug 3, 2023
1 parent da44ac3 commit 9147003
Show file tree
Hide file tree
Showing 17 changed files with 38 additions and 236 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.SemanticKernel.AI.Embeddings;
using Microsoft.SemanticKernel.Diagnostics;
using Microsoft.SemanticKernel.Memory;
using NRedisStack;
using NRedisStack.RedisStackCommands;
Expand Down Expand Up @@ -289,7 +290,7 @@ private double GetSimilarity(Document document)

if (vectorScoreValue.IsNullOrEmpty || !vectorScoreValue.TryParse(out double vectorScore))
{
throw new RedisMemoryStoreException("Invalid or missing vector score value.");
throw new SKException("Invalid or missing vector score value.");
}

return 1 - vectorScore;
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
using Microsoft.SemanticKernel.AI.Embeddings;
using Microsoft.SemanticKernel.AI.Embeddings.VectorOperations;
using Microsoft.SemanticKernel.Connectors.Memory.Redis;
using Microsoft.SemanticKernel.Diagnostics;
using Microsoft.SemanticKernel.Memory;
using Microsoft.SemanticKernel.Memory.Collections;
using Moq;
Expand Down Expand Up @@ -745,7 +746,7 @@ public async Task GetNearestMatchAsyncThrowsExceptionOnInvalidVectorScoreAsync()
}

// Assert
RedisMemoryStoreException ex = await Assert.ThrowsAsync<RedisMemoryStoreException>(async () =>
var ex = await Assert.ThrowsAsync<SKException>(async () =>
{
// Act
await store.GetNearestMatchAsync(collection, compareEmbedding, minRelevanceScore: threshold);
Expand Down
8 changes: 4 additions & 4 deletions dotnet/src/Skills/Skills.Grpc/GrpcOperationRunner.cs
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ private string GetAddress(GrpcOperation operation, IDictionary<string, string> a

if (string.IsNullOrEmpty(address))
{
throw new GrpcOperationException($"No address provided for the '{operation.Name}' gRPC operation.");
throw new SKException($"No address provided for the '{operation.Name}' gRPC operation.");
}

return address!;
Expand Down Expand Up @@ -155,14 +155,14 @@ private object GenerateOperationRequest(GrpcOperation operation, Type type, IDic
//Getting 'payload' argument to by used as gRPC request message
if (!arguments.TryGetValue(GrpcOperation.PayloadArgumentName, out var payload))
{
throw new GrpcOperationException($"No '{GrpcOperation.PayloadArgumentName}' argument representing gRPC request message is found for the '{operation.Name}' gRPC operation.");
throw new SKException($"No '{GrpcOperation.PayloadArgumentName}' argument representing gRPC request message is found for the '{operation.Name}' gRPC operation.");
}

//Deserializing JSON payload to gRPC request message
var instance = JsonSerializer.Deserialize(payload, type, new JsonSerializerOptions { PropertyNameCaseInsensitive = true });
if (instance == null)
{
throw new GrpcOperationException($"Impossible to create gRPC request message for the '{operation.Name}' gRPC operation.");
throw new SKException($"Impossible to create gRPC request message for the '{operation.Name}' gRPC operation.");
}

return instance;
Expand Down Expand Up @@ -226,7 +226,7 @@ private static TypeInfo BuildGrpcOperationDataContractType(GrpcOperationDataCont
var type = typeBuilder.CreateTypeInfo();
if (type == null)
{
throw new GrpcOperationException($"Impossible to create type for '{dataContractMetadata.Name}' data contract.");
throw new SKException($"Impossible to create type for '{dataContractMetadata.Name}' data contract.");
}

return type;
Expand Down
35 changes: 0 additions & 35 deletions dotnet/src/Skills/Skills.Grpc/Model/GrpcOperationException.cs

This file was deleted.

6 changes: 3 additions & 3 deletions dotnet/src/Skills/Skills.Grpc/Protobuf/ProtoDocumentParser.cs
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ public IList<GrpcOperation> Parse(Stream protoDocument, string protoFileName)
var errors = descriptor.GetErrors();
if (errors != null && errors.Length != 0)
{
throw new ProtobufParsingException($"Parsing of '{protoFileName}' .proto document has failed. Details: {string.Join(";", errors.AsEnumerable())}");
throw new SKException($"Parsing of '{protoFileName}' .proto document has failed. Details: {string.Join(";", errors.AsEnumerable())}");
}

return this.GetGrpcOperations(descriptor.Files.Single());
Expand Down Expand Up @@ -91,7 +91,7 @@ private GrpcOperationDataContractType CreateDataContract(IList<DescriptorProto>
var messageType = allMessageTypes.SingleOrDefault(mt => mt.Name == fullTypeName || mt.Name == typeName);
if (messageType == null)
{
throw new ProtobufParsingException($"No '{fullTypeName}' message type is found while resolving data contracts for the '{methodName}' method.");
throw new SKException($"No '{fullTypeName}' message type is found while resolving data contracts for the '{methodName}' method.");
}

var fields = this.GetDataContractFields(messageType.Fields);
Expand Down Expand Up @@ -132,7 +132,7 @@ private static string GetProtobufDataTypeName(FieldDescriptorProto.Type type)

if (attribute == null)
{
throw new ProtobufParsingException($"Impossible to find protobuf type name corresponding to '{type}' type.");
throw new SKException($"Impossible to find protobuf type name corresponding to '{type}' type.");
}

return attribute.Name;
Expand Down
35 changes: 0 additions & 35 deletions dotnet/src/Skills/Skills.Grpc/Protobuf/ProtobufParsingException.cs

This file was deleted.

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,8 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Graph;
using Microsoft.SemanticKernel.Diagnostics;
using Microsoft.SemanticKernel.Skills.MsGraph.Connectors.Diagnostics;
using Microsoft.SemanticKernel.Skills.MsGraph.Connectors.Exceptions;
using Microsoft.SemanticKernel.Skills.MsGraph.Models;
using TaskStatus = Microsoft.Graph.TaskStatus;

Expand Down Expand Up @@ -50,7 +50,7 @@ public MicrosoftToDoConnector(GraphServiceClient graphServiceClient)

if (result == null)
{
throw new MsGraphConnectorException("Could not find default task list.");
throw new SKException("Could not find default task list.");
}

return new TaskManagementTaskList(result.Id, result.DisplayName);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,8 @@
using System.Threading;
using System.Threading.Tasks;
using Microsoft.Graph;
using Microsoft.SemanticKernel.Diagnostics;
using Microsoft.SemanticKernel.Skills.MsGraph.Connectors.Diagnostics;
using Microsoft.SemanticKernel.Skills.MsGraph.Connectors.Exceptions;

namespace Microsoft.SemanticKernel.Skills.MsGraph.Connectors;

/// <summary>
Expand Down Expand Up @@ -108,7 +107,7 @@ public async Task<string> CreateShareLinkAsync(string filePath, string type = "v
string? result = (await response.GetResponseObjectAsync().ConfigureAwait(false)).Link?.WebUrl;
if (string.IsNullOrWhiteSpace(result))
{
throw new MsGraphConnectorException("Shareable file link was null or whitespace.");
throw new SKException("Shareable file link was null or whitespace.");
}

return result!;
Expand Down
9 changes: 5 additions & 4 deletions dotnet/src/Skills/Skills.OpenAPI/Model/RestApiOperation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
using System.Net.Http;
using System.Text.RegularExpressions;
using System.Web;
using Microsoft.SemanticKernel.Diagnostics;

namespace Microsoft.SemanticKernel.Skills.OpenAPI.Model;

Expand Down Expand Up @@ -162,12 +163,12 @@ public IDictionary<string, string> RenderHeaders(IDictionary<string, string> arg

//Getting metadata for the header
var headerMetadata = this.Parameters.FirstOrDefault(p => p.Location == RestApiOperationParameterLocation.Header && p.Name == headerName)
?? throw new RestApiOperationException($"No value for the '{headerName} header is found.'");
?? throw new SKException($"No value for the '{headerName} header is found.'");

//If parameter is required it's value should always be provided.
if (headerMetadata.IsRequired)
{
throw new RestApiOperationException($"No value for the '{headerName} header is found.'");
throw new SKException($"No value for the '{headerName} header is found.'");
}

//Parameter is not required and no default value provided.
Expand Down Expand Up @@ -207,7 +208,7 @@ string ReplaceParameter(Match match)
var parameterMetadata = this.Parameters.First(p => p.Location == RestApiOperationParameterLocation.Path && p.Name == parameterName);
if (parameterMetadata?.DefaultValue == null)
{
throw new RestApiOperationException($"No argument found for parameter - '{parameterName}' for operation - '{this.Id}'");
throw new SKException($"No argument found for parameter - '{parameterName}' for operation - '{this.Id}'");
}

return parameterMetadata.DefaultValue;
Expand Down Expand Up @@ -246,7 +247,7 @@ private string AddQueryString(string path, IDictionary<string, string> arguments
//Throw an exception if the parameter is a required one but no value is provided.
if (parameter.IsRequired)
{
throw new RestApiOperationException($"No argument found for required query string parameter - '{parameter.Name}' for operation - '{this.Id}'");
throw new SKException($"No argument found for required query string parameter - '{parameter.Name}' for operation - '{this.Id}'");
}
}

Expand Down

This file was deleted.

Loading

0 comments on commit 9147003

Please sign in to comment.