Skip to content

Commit

Permalink
Mark the FunctionCall/ResultContent.Exception properties as [JsonIgnore]
Browse files Browse the repository at this point in the history
Given implementation details of the JSON source generator today, even with the converter applied to these properties, code is still being generated for Exception, leading to unsuppressable trimmer warnings.
  • Loading branch information
stephentoub committed Oct 9, 2024
1 parent 0c8bc3e commit bed5e4b
Show file tree
Hide file tree
Showing 8 changed files with 47 additions and 164 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,11 @@ public FunctionCallContent(string callId, string name, IDictionary<string, objec
/// Gets or sets any exception that occurred while mapping the original function call data to this class.
/// </summary>
/// <remarks>
/// When an instance of <see cref="FunctionCallContent"/> is serialized using <see cref="JsonSerializer"/>, any exception
/// stored in this property will be serialized as a string. When deserialized, the string will be converted back to an instance
/// of the base <see cref="Exception"/> type. As such, consumers shouldn't rely on the exact type of the exception stored in this property.
/// This property is for information purposes only. The <see cref="Exception"/> is not serialized as part of serializing
/// instances of this class with <see cref="JsonSerializer"/>; as such, upon deserialization, this property will be <see langword="null"/>.
/// Consumers should not rely on <see langword="null"/> indicating success.
/// </remarks>
[JsonConverter(typeof(FunctionCallExceptionConverter))]
[JsonIgnore]
public Exception? Exception { get; set; }

/// <summary>Gets a string representing this instance to display in the debugger.</summary>
Expand Down

This file was deleted.

Original file line number Diff line number Diff line change
Expand Up @@ -20,25 +20,30 @@ public sealed class FunctionResultContent : AIContent
/// </summary>
/// <param name="callId">The function call ID for which this is the result.</param>
/// <param name="name">The function name that produced the result.</param>
/// <param name="result">The function call result.</param>
/// <param name="exception">Any exception that occurred when invoking the function.</param>
/// <param name="result">
/// This may be <see langword="null"/> if the function returned <see langword="null"/>, if the function was void-returning
/// and thus had no result, or if the function call failed. Typically, however, in order to provide meaningfully representative
/// information to an AI service, a human-readable representation of those conditions should be supplied.
/// </param>
[JsonConstructor]
public FunctionResultContent(string callId, string name, object? result = null, Exception? exception = null)
public FunctionResultContent(string callId, string name, object? result)
{
CallId = Throw.IfNull(callId);
Name = Throw.IfNull(name);
Result = result;
Exception = exception;
}

/// <summary>
/// Initializes a new instance of the <see cref="FunctionResultContent"/> class.
/// </summary>
/// <param name="functionCall">The function call for which this is the result.</param>
/// <param name="result">The function call result.</param>
/// <param name="exception">Any exception that occurred when invoking the function.</param>
public FunctionResultContent(FunctionCallContent functionCall, object? result = null, Exception? exception = null)
: this(Throw.IfNull(functionCall).CallId, functionCall.Name, result, exception)
/// <param name="result">
/// This may be <see langword="null"/> if the function returned <see langword="null"/>, if the function was void-returning
/// and thus had no result, or if the function call failed. Typically, however, in order to provide meaningfully representative
/// information to an AI service, a human-readable representation of those conditions should be supplied.
/// </param>
public FunctionResultContent(FunctionCallContent functionCall, object? result)
: this(Throw.IfNull(functionCall).CallId, functionCall.Name, result)
{
}

Expand All @@ -59,17 +64,22 @@ public FunctionResultContent(FunctionCallContent functionCall, object? result =
/// <summary>
/// Gets or sets the result of the function call, or a generic error message if the function call failed.
/// </summary>
/// <remarks>
/// This may be <see langword="null"/> if the function returned <see langword="null"/>, if the function was void-returning
/// and thus had no result, or if the function call failed. Typically, however, in order to provide meaningfully representative
/// information to an AI service, a human-readable representation of those conditions should be supplied.
/// </remarks>
public object? Result { get; set; }

/// <summary>
/// Gets or sets an exception that occurred if the function call failed.
/// </summary>
/// <remarks>
/// When an instance of <see cref="FunctionResultContent"/> is serialized using <see cref="JsonSerializer"/>, any exception
/// stored in this property will be serialized as a string. When deserialized, the string will be converted back to an instance
/// of the base <see cref="Exception"/> type. As such, consumers shouldn't rely on the exact type of the exception stored in this property.
/// This property is for information purposes only. The <see cref="Exception"/> is not serialized as part of serializing
/// instances of this class with <see cref="JsonSerializer"/>; as such, upon deserialization, this property will be <see langword="null"/>.
/// Consumers should not rely on <see langword="null"/> indicating success.
/// </remarks>
[JsonConverter(typeof(FunctionCallExceptionConverter))]
[JsonIgnore]
public Exception? Exception { get; set; }

/// <summary>Gets a string representing this instance to display in the debugger.</summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -521,7 +521,7 @@ FunctionResultContent CreateFunctionResultContent(FunctionInvocationResult resul
functionResult = message;
}

return new FunctionResultContent(result.CallContent.CallId, result.CallContent.Name, functionResult, result.Exception);
return new(result.CallContent.CallId, result.CallContent.Name, functionResult) { Exception = result.Exception };
}
}

Expand Down
5 changes: 3 additions & 2 deletions src/Libraries/Microsoft.Extensions.AI/JsonDefaults.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Text.Json;
using System.Text.Json.Serialization;
using System.Text.Json.Serialization.Metadata;
Expand All @@ -16,6 +17,8 @@ internal static partial class JsonDefaults
public static JsonSerializerOptions Options { get; } = CreateDefaultOptions();

/// <summary>Creates the default <see cref="JsonSerializerOptions"/> to use for serialization-related operations.</summary>
[UnconditionalSuppressMessage("AotAnalysis", "IL3050", Justification = "DefaultJsonTypeInfoResolver is only used when reflection-based serialization is enabled")]
[UnconditionalSuppressMessage("ReflectionAnalysis", "IL2026", Justification = "DefaultJsonTypeInfoResolver is only used when reflection-based serialization is enabled")]
private static JsonSerializerOptions CreateDefaultOptions()
{
// If reflection-based serialization is enabled by default, use it, as it's the most permissive in terms of what it can serialize,
Expand All @@ -28,9 +31,7 @@ private static JsonSerializerOptions CreateDefaultOptions()
var options = new JsonSerializerOptions(JsonSerializerDefaults.Web)
{
DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull,
#pragma warning disable IL3050, IL2026 // only used when reflection-based serialization is enabled
TypeInfoResolver = new DefaultJsonTypeInfoResolver(),
#pragma warning restore IL3050, IL2026
};

options.MakeReadOnly();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,11 +18,6 @@
<TreatWarningsAsErrors>true</TreatWarningsAsErrors>
</PropertyGroup>

<PropertyGroup>
<!-- TEMPORARY UNTIL JSON SOURCE GENERATOR ISSUE CAN BE SORTED OUT -->
<NoWarn>$(NoWarn);IL2026</NoWarn>
</PropertyGroup>

<PropertyGroup>
<InjectSharedCollectionExtensions>true</InjectSharedCollectionExtensions>
<InjectSharedEmptyCollections>true</InjectSharedEmptyCollections>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,6 @@
using System.Collections.Generic;
using System.Collections.ObjectModel;
using System.Linq;
#if NET
using System.Runtime.ExceptionServices;
#endif
using System.Text.Json;
using System.Text.Json.Nodes;
using System.Threading;
Expand Down Expand Up @@ -89,41 +86,19 @@ public void ItShouldBeSerializableAndDeserializableWithException()
{
// Arrange
var ex = new InvalidOperationException("hello", new NullReferenceException("bye"));
#if NET
ExceptionDispatchInfo.SetRemoteStackTrace(ex, "stack trace");
#endif
var sut = new FunctionCallContent("callId1", "functionName") { Exception = ex };
var sut = new FunctionCallContent("callId1", "functionName", new Dictionary<string, object?> { ["key"] = "value" }) { Exception = ex };

// Act
var json = JsonSerializer.SerializeToNode(sut, TestJsonSerializerContext.Default.Options);
var deserializedSut = JsonSerializer.Deserialize<FunctionCallContent>(json, TestJsonSerializerContext.Default.Options);

// Assert
JsonObject jsonEx = Assert.IsType<JsonObject>(json!["exception"]);
Assert.Equal(4, jsonEx.Count);
Assert.Equal("System.InvalidOperationException", (string?)jsonEx["className"]);
Assert.Equal("hello", (string?)jsonEx["message"]);
#if NET
Assert.StartsWith("stack trace", (string?)jsonEx["stackTraceString"]);
#endif
JsonObject jsonExInner = Assert.IsType<JsonObject>(jsonEx["innerException"]);
Assert.Equal(4, jsonExInner.Count);
Assert.Equal("System.NullReferenceException", (string?)jsonExInner["className"]);
Assert.Equal("bye", (string?)jsonExInner["message"]);
Assert.Null(jsonExInner["innerException"]);
Assert.Null(jsonExInner["stackTraceString"]);

Assert.NotNull(deserializedSut);
Assert.IsType<Exception>(deserializedSut.Exception);
Assert.Equal("hello", deserializedSut.Exception.Message);
#if NET
Assert.StartsWith("stack trace", deserializedSut.Exception.StackTrace);
#endif

Assert.IsType<Exception>(deserializedSut.Exception.InnerException);
Assert.Equal("bye", deserializedSut.Exception.InnerException.Message);
Assert.Null(deserializedSut.Exception.InnerException.StackTrace);
Assert.Null(deserializedSut.Exception.InnerException.InnerException);
Assert.Equal("callId1", deserializedSut.CallId);
Assert.Equal("functionName", deserializedSut.Name);
Assert.NotNull(deserializedSut.Arguments);
Assert.Single(deserializedSut.Arguments);
Assert.Null(deserializedSut.Exception);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ public class FunctionResultContentTests
[Fact]
public void Constructor_PropsDefault()
{
FunctionResultContent c = new("callId1", "functionName");
FunctionResultContent c = new("callId1", "functionName", null);
Assert.Equal("callId1", c.CallId);
Assert.Equal("functionName", c.Name);
Assert.Null(c.RawRepresentation);
Expand All @@ -25,36 +25,32 @@ public void Constructor_PropsDefault()
[Fact]
public void Constructor_String_PropsRoundtrip()
{
Exception e = new();

FunctionResultContent c = new("id", "name", "result", e);
FunctionResultContent c = new("id", "name", "result");
Assert.Null(c.RawRepresentation);
Assert.Null(c.ModelId);
Assert.Null(c.AdditionalProperties);
Assert.Equal("name", c.Name);
Assert.Equal("id", c.CallId);
Assert.Equal("result", c.Result);
Assert.Same(e, c.Exception);
Assert.Null(c.Exception);
}

[Fact]
public void Constructor_FunctionCallContent_PropsRoundtrip()
{
Exception e = new();

FunctionResultContent c = new(new FunctionCallContent("id", "name"), "result", e);
FunctionResultContent c = new(new FunctionCallContent("id", "name"), "result");
Assert.Null(c.RawRepresentation);
Assert.Null(c.ModelId);
Assert.Null(c.AdditionalProperties);
Assert.Equal("id", c.CallId);
Assert.Equal("result", c.Result);
Assert.Same(e, c.Exception);
Assert.Null(c.Exception);
}

[Fact]
public void Constructor_PropsRoundtrip()
{
FunctionResultContent c = new("callId1", "functionName");
FunctionResultContent c = new("callId1", "functionName", null);

Assert.Null(c.RawRepresentation);
object raw = new();
Expand Down Expand Up @@ -106,15 +102,17 @@ public void ItShouldBeSerializableAndDeserializable()
public void ItShouldBeSerializableAndDeserializableWithException()
{
// Arrange
var sut = new FunctionResultContent("callId1", "functionName") { Exception = new InvalidOperationException("hello") };
var sut = new FunctionResultContent("callId1", "functionName", null) { Exception = new InvalidOperationException("hello") };

// Act
var json = JsonSerializer.Serialize(sut, TestJsonSerializerContext.Default.Options);
var deserializedSut = JsonSerializer.Deserialize<FunctionResultContent>(json, TestJsonSerializerContext.Default.Options);

// Assert
Assert.NotNull(deserializedSut);
Assert.IsType<Exception>(deserializedSut.Exception);
Assert.Contains("hello", deserializedSut.Exception.Message);
Assert.Equal(sut.Name, deserializedSut.Name);
Assert.Equal(sut.CallId, deserializedSut.CallId);
Assert.Equal(sut.Result, deserializedSut.Result?.ToString());
Assert.Null(deserializedSut.Exception);
}
}

0 comments on commit bed5e4b

Please sign in to comment.