Skip to content

Comments

fix: Use custom JsonConverter for A2AEvent and A2AResponse instead of [JsonPolymorphic] to fix discriminator deserialization issues#158

Closed
Blackhex wants to merge 1 commit intoa2aproject:mainfrom
Blackhex:fix/131-custom-polymorfism
Closed

fix: Use custom JsonConverter for A2AEvent and A2AResponse instead of [JsonPolymorphic] to fix discriminator deserialization issues#158
Blackhex wants to merge 1 commit intoa2aproject:mainfrom
Blackhex:fix/131-custom-polymorfism

Conversation

@Blackhex
Copy link
Collaborator

@Blackhex Blackhex commented Aug 21, 2025

Summary

  • Replace [JsonPolymorphic] with custom JsonConverter implementations for A2AEvent and A2AResponse to fix discriminator-based deserialization.
  • Ensure consistent round-trip serialization using source-generated JsonContext and explicit kind discriminators.

Problem

  • [JsonPolymorphic] produced inconsistent discriminator deserialization across contexts/AOT, causing events to fail to materialize to concrete types or lose the kind field.

Changes

  • Add A2AEventConverter and A2AResponseConverter with strict kind handling:
    • Read: parse once, route by kindmessage, task, status-update, artifact-update; throw on unknown/missing.
    • Write: serialize via A2AJsonUtilities.JsonContext to concrete metadata.
  • Add explicit Kind overrides:
    • Messagemessage, AgentTasktask, TaskStatusUpdateEventstatus-update, TaskArtifactUpdateEventartifact-update.
  • Expand source-gen coverage in A2AJsonUtilities.JsonContext for affected types.
  • Add comprehensive unit tests in tests/A2A.UnitTests/Models/A2AResponseTests.cs.

Testing

  • Deserialization as A2AEvent: succeeds for all four kinds; throws for unknown/missing kind.
  • Deserialization as A2AResponse: succeeds for message/task; throws for status-update/artifact-update.
  • Serialization as A2AEvent/A2AResponse: kind is always present and correct; selected exact-shape assertions for known cases.
  • dotnet test A2A.slnx -v m passes on net8.0/net9.0.

Impact

  • Public model shape remains stable; behavior is stricter and more predictable.
  • Potentially breaking only if external consumers subclass A2AEvent/TaskUpdateEvent: they must implement Kind, and custom kind values are not yet routed by the new converters.

Migration Notes

  • If you have custom event types, add public override string Kind => "<your-kind>"; and handle custom routing via an app-level converter or open an issue to extend built-in routing.

Files

  • Updated: src/A2A/Models/A2AResponse.cs, src/A2A/Models/Message.cs, src/A2A/Models/AgentTask.cs, src/A2A/Models/TaskStatusUpdateEvent.cs, src/A2A/Models/TaskArtifactUpdateEvent.cs, src/A2A/A2AJsonUtilities.cs
  • Added tests: tests/A2A.UnitTests/Models/A2AResponseTests.cs

Related

@Blackhex Blackhex force-pushed the fix/131-custom-polymorfism branch 2 times, most recently from 8165caa to e2e3e06 Compare August 22, 2025 11:03
@Blackhex Blackhex requested a review from Copilot August 22, 2025 11:04

This comment was marked as outdated.

@Blackhex Blackhex force-pushed the fix/131-custom-polymorfism branch from e2e3e06 to 5190265 Compare August 22, 2025 11:15
@Blackhex Blackhex requested a review from Copilot August 22, 2025 11:16

This comment was marked as outdated.

@Blackhex Blackhex force-pushed the fix/131-custom-polymorfism branch from 5190265 to 77bf0ed Compare August 22, 2025 11:23
@Blackhex Blackhex requested a review from Copilot August 22, 2025 11:26

This comment was marked as outdated.

@Blackhex Blackhex force-pushed the fix/131-custom-polymorfism branch from 77bf0ed to efc8a0f Compare August 22, 2025 11:32
@Blackhex Blackhex requested a review from Copilot August 22, 2025 11:36

This comment was marked as outdated.

@Blackhex Blackhex force-pushed the fix/131-custom-polymorfism branch from efc8a0f to 4b3fe25 Compare August 22, 2025 11:40
@Blackhex Blackhex requested a review from Copilot August 22, 2025 11:41
@Blackhex Blackhex self-assigned this Aug 22, 2025
Copy link
Contributor

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 replaces the [JsonPolymorphic] attribute with custom JsonConverter implementations for A2AEvent and A2AResponse to fix discriminator-based deserialization issues in AOT scenarios.

Key changes:

  • Custom converters with explicit kind discriminator routing for consistent serialization/deserialization
  • Added explicit Kind property overrides to all concrete event types
  • Enhanced source-generated JSON context coverage for affected types

Reviewed Changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/A2A/Models/A2AResponse.cs Replaced [JsonPolymorphic] with custom converters and added abstract Kind property
src/A2A/Models/Message.cs Added explicit Kind property override returning "message"
src/A2A/Models/AgentTask.cs Added explicit Kind property override returning "task"
src/A2A/Models/TaskStatusUpdateEvent.cs Added explicit Kind property override returning "status-update"
src/A2A/Models/TaskArtifactUpdateEvent.cs Added explicit Kind property override returning "artifact-update"
src/A2A/A2AJsonUtilities.cs Added missing type registrations for source-generated JSON context
tests/A2A.UnitTests/Models/A2AResponseTests.cs Added comprehensive test coverage for new converter behavior

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

{
return a2aResponse;
}
throw new JsonException("JSON did not represent an A2AResponse instance.");
Copy link

Copilot AI Aug 22, 2025

Choose a reason for hiding this comment

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

The Read method is being called on a ref Utf8JsonReader that has already been consumed by the previous call. This will cause the reader to be positioned incorrectly. You need to reset the reader position or pass a copy of the reader state.

Suggested change
throw new JsonException("JSON did not represent an A2AResponse instance.");
if (reader.TokenType != JsonTokenType.StartObject)
{
throw new JsonException("Expected StartObject token");
}
using var document = JsonDocument.ParseValue(ref reader);
var a2aResponse = document.RootElement.Deserialize(A2AJsonUtilities.JsonContext.Default.A2AResponse);
if (a2aResponse is null)
{
throw new JsonException("JSON did not represent an A2AResponse instance.");
}
return a2aResponse;

Copilot uses AI. Check for mistakes.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Correct me if I am wrong but this comment seems false-positive to me.

@Blackhex Blackhex force-pushed the fix/131-custom-polymorfism branch from 4b3fe25 to 17ac04c Compare August 22, 2025 11:48
@Blackhex Blackhex marked this pull request as ready for review August 22, 2025 11:49
@Blackhex Blackhex requested a review from darrelmiller August 22, 2025 11:51
…ead of `[JsonPolymorphic]` to fix discriminator deserialization issues
@Blackhex Blackhex force-pushed the fix/131-custom-polymorfism branch from 17ac04c to 1dbc533 Compare August 25, 2025 14:56
@brandonh-msft brandonh-msft marked this pull request as draft August 26, 2025 15:25
@Blackhex
Copy link
Collaborator Author

Closing in favor of #163

@Blackhex Blackhex closed this Aug 26, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Serializing Message property of MessageSendParameters does not include "kind"

2 participants