Skip to content

Commit

Permalink
Mark the FunctionCall/ResultContent.Exception properties as [JsonIgno…
Browse files Browse the repository at this point in the history
…re] (#5492)

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 authored Oct 9, 2024
1 parent a51631e commit 3970109
Show file tree
Hide file tree
Showing 7 changed files with 59 additions and 152 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,10 +20,31 @@ 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;
}

/// <summary>
/// Initializes a new instance of the <see cref="FunctionResultContent"/> class.
/// </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">
/// 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>
/// <param name="exception">Any exception that occurred when invoking the function.</param>
public FunctionResultContent(string callId, string name, object? result, Exception? exception)
{
CallId = Throw.IfNull(callId);
Name = Throw.IfNull(name);
Expand All @@ -35,9 +56,13 @@ public FunctionResultContent(string callId, string name, object? result = null,
/// 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="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>
/// <param name="exception">Any exception that occurred when invoking the function.</param>
public FunctionResultContent(FunctionCallContent functionCall, object? result = null, Exception? exception = null)
public FunctionResultContent(FunctionCallContent functionCall, object? result, Exception? exception = null)
: this(Throw.IfNull(functionCall).CallId, functionCall.Name, result, exception)
{
}
Expand All @@ -59,17 +84,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
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 Down Expand Up @@ -54,7 +54,7 @@ public void Constructor_FunctionCallContent_PropsRoundtrip()
[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 +106,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, 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 3970109

Please sign in to comment.