Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mark the FunctionCall/ResultContent.Exception properties as [JsonIgnore] #5492

Merged
merged 1 commit into from
Oct 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need to expose a ctor overload accepting Exception given that the property is settable?

Copy link
Member Author

@stephentoub stephentoub Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trying to avoid breaking binaru changes right now if they're not necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but if we can't make them now it will be even more difficult to make them later.

Copy link
Member Author

@stephentoub stephentoub Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather take any such changes in batch when we have a meaningful set, do a more thorough review based on received feedback, etc. Force people to rev fewer times.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will we remember/have enough context to remove this constructor when such a review is going to be made? Unless we release new bits to customers on a very frequent basis I don't see how postponing such a decision would impact churn substantially.

Copy link
Member Author

@stephentoub stephentoub Oct 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each one of these is a binary breaking change. If every single release makes such a change but is binary breaking, it effectively means that the ecosystem gets reset with every single release. At a minimum these are shipping monthly now along with the rest of dotnet/extensions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand, but at the same time this seems fair game or even expected when evolving previews.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing this constructor is not a meaningful fix. It doesn't improve what someone is able to do. All it does is force someone to use a different syntax to construct it. Why would we want to reset the ecosystem for such a change? When there are other binary breaking changes we decide to make in preview, then we can make this at the same time, but I don't see the value in just making this one. And shipping will be frequent enough that it very well could be the only one.

{
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);
}
}
Loading