Skip to content

Fix: Ensure EntityConverter delegates to AIEntityConverter #303

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

Conversation

msftshlomiyosef
Copy link
Contributor

@msftshlomiyosef msftshlomiyosef commented May 21, 2025

Fixes #298
Ensure EntityConverter checks if the value is an AIEntity and delegates serialization, so @type, @context, and @id are included.

@Copilot Copilot AI review requested due to automatic review settings May 21, 2025 06:37
@msftshlomiyosef msftshlomiyosef added bug Something isn't working enhancement New feature or request labels May 21, 2025
@msftshlomiyosef msftshlomiyosef requested a review from a team as a code owner May 21, 2025 06:37
@github-actions github-actions bot added ML: Core Tags changes to core libraries ML: Tests Tags changes to tests labels May 21, 2025
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 updates the JSON serialization behavior to honor JsonIgnore and JsonPropertyName attributes by adding caching for reflection lookups and updating the property mapping logic. Key changes include:

  • Introducing GetCachedProperties and GetPropertyMap in ConnectorConverter to improve performance and respect JSON attributes.
  • Updating tests to validate derived types, proper handling of ignored properties, and thread safety.
  • Enabling JsonPropertyName attributes on AIEntity properties and removing redundant manual property handling in AIEntityConverter.

Reviewed Changes

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

File Description
src/tests/Microsoft.Agents.Model.Tests/ObjectSerializationTests.cs Added tests for derived entity serialization, thread safety, and validation using new attributes.
src/libraries/Core/Microsoft.Agents.Core/Serialization/Converters/ConnectorConverter.cs Introduced property caching and mapping to respect JsonIgnore and JsonPropertyName.
src/libraries/Core/Microsoft.Agents.Core/Serialization/Converters/AIEntityConverter.cs Removed manual extension data handling to rely on attribute-based mapping via the base.
src/libraries/Core/Microsoft.Agents.Core/Models/AIEntity.cs Uncommented and activated JsonPropertyName attributes for correct JSON property naming.

@msftshlomiyosef msftshlomiyosef self-assigned this May 21, 2025
@msftshlomiyosef msftshlomiyosef changed the title Update ConnectorConverter to honor JsonIgnore and JsonPropertyName Update ConnectorConverter to honor derived types JsonIgnore and JsonPropertyName May 21, 2025
@msftshlomiyosef msftshlomiyosef force-pushed the users/shlomiyosef/serializer-respect-name-and-ignore branch from fb0a80e to 6349bfd Compare May 22, 2025 06:24
@msftshlomiyosef msftshlomiyosef changed the title Update ConnectorConverter to honor derived types JsonIgnore and JsonPropertyName Fix: Ensure EntityConverter delegates to AIEntityConverter May 22, 2025
@tracyboehrer
Copy link
Member

Does #305 replace this PR?

@msftshlomiyosef
Copy link
Contributor Author

msftshlomiyosef commented May 23, 2025

Does #305 replace this PR?

@tracyboehrer
This PR solves a very specific problem of serializing AIEntity when the compile-time type is Entity, while preserving the approach of manually handling custom property names in the converters.
#305 solves a broader problem but changes this approach, so it can replace this PR if we agree to adopt the new model.

@tracyboehrer
Copy link
Member

@msftshlomiyosef I'm all for this route if it works. Entities in general were made to be subclassed, so obeying the attributes is of benefit.

@msftshlomiyosef
Copy link
Contributor Author

Closing this, as it will be handled here: #305

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request ML: Core Tags changes to core libraries ML: Tests Tags changes to tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AIEntityConverter Not Invoked When Serializing AIEntity Inside Activity.Entities
3 participants