Skip to content

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Jul 10, 2025

Addresses modelcontextprotocol/csharp-sdk#601

Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR ensures that functions with Nullable<T> parameters generate correct JSON schemas (including null types and defaults) and updates related tests and utilities.

  • Added a new test for nullable int? and DateTime? parameters in AIFunctionFactoryTest.
  • Refactored AssertExtensions to centralize JSON schema/value comparisons.
  • Extended the schema generator in AIJsonUtilities to include null in type arrays for Nullable<T> and skip uninitialized objects for Nullable<T> defaults.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
test/Libraries/Microsoft.Extensions.AI.Tests/Functions/AIFunctionFactoryTest.cs New AIFunctionFactory_NullableParameters test and added JsonSerializable for int?/DateTime?.
test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/AssertExtensions.cs Introduced EqualJsonValues and updated helper for JSON comparisons.
src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs Updated schema exporter to handle Nullable<T> types and default values correctly.
Comments suppressed due to low confidence (3)

src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs:292

  • There’s logic to handle nullable enum types but no corresponding unit test. Consider adding a test for a Nullable<YourEnum> parameter to verify enum schemas include ["string","null"].
                if (Nullable.GetUnderlyingType(ctx.TypeInfo.Type) is Type nullableElement)

test/Libraries/Microsoft.Extensions.AI.Tests/Functions/AIFunctionFactoryTest.cs:861

  • [nitpick] This assertion about StructWithDefaultCtor isn’t related to validating nullable-parameter schemas and may be confusing. Consider removing or moving it to a dedicated test for struct default behavior.
        Assert.NotEqual(new StructWithDefaultCtor().Value, default(StructWithDefaultCtor).Value);

src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs:300

  • The typeValue comes from a non-nullable GetValue<string>()! call, so this null check is redundant and can be removed to simplify the logic.
                        if (typeValue is not null)

…IJsonUtilities.Schema.Create.cs

Co-authored-by: Copilot <[email protected]>
@eiriktsarpalis eiriktsarpalis requested a review from a team as a code owner July 11, 2025 13:12
@eiriktsarpalis
Copy link
Member Author

@jeffhandley we need a second approval from dotnet/dotnet-extensions-fundamentals because this PR is touching "shared" components.

@jeffhandley
Copy link
Member

I connected with @rosebyte to request a review from them or someone else on @dotnet/dotnet-extensions-fundamentals.

Copy link
Member

@evgenyfedorov2 evgenyfedorov2 left a comment

Choose a reason for hiding this comment

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

Approve for Fundamentals

@eiriktsarpalis eiriktsarpalis merged commit 7874d8f into dotnet:main Jul 15, 2025
6 checks passed
@eiriktsarpalis eiriktsarpalis deleted the fix/nullable-parameters branch July 15, 2025 08:52
jeffhandley pushed a commit to jeffhandley/extensions that referenced this pull request Jul 15, 2025
* Fix schema generation for Nullable<T> function parameters.

* Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs

Co-authored-by: Copilot <[email protected]>

* Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs

* Incorporate fix from dotnet/runtime#117493.

* Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs

* Extend fix to include AllowReadingFromString.

---------

Co-authored-by: Copilot <[email protected]>
jeffhandley pushed a commit that referenced this pull request Jul 15, 2025
* Fix schema generation for Nullable<T> function parameters.

* Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs

Co-authored-by: Copilot <[email protected]>

* Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs

* Incorporate fix from dotnet/runtime#117493.

* Update src/Libraries/Microsoft.Extensions.AI.Abstractions/Utilities/AIJsonUtilities.Schema.Create.cs

* Extend fix to include AllowReadingFromString.

---------

Co-authored-by: Copilot <[email protected]>
@github-actions github-actions bot locked and limited conversation to collaborators Aug 14, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-ai Microsoft.Extensions.AI libraries
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants