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

[SignalR-Playground] Ignore null fields when serializing negotiate response #8203

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

cqnguy23
Copy link
Contributor

  • Add a serializing option to ignore null fields for negotiate response as a workaround for this bug in SignalR Client .NET Library

@Copilot Copilot bot review requested due to automatic review settings March 20, 2025 07:49
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 adds a serializing option to ignore null fields in the negotiate response to work around an identified bug in the SignalR Client .NET Library.

  • Added using directives for System.Text.Json and System.Text.Json.Serialization.
  • Updated the negotiate endpoint to use Results.Json with a JsonSerializerOptions that ignores null values.

Tip: Copilot code review supports C#, Go, Java, JavaScript, Markdown, Python, Ruby and TypeScript, with more languages coming soon. Learn more

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Mar 20, 2025
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Mar 20, 2025
@IEvangelist
Copy link
Member

Hi @cqnguy23,

This bug doesn't exist in the JavaScript SignalR client; therefore, this change isn't needed—unless the playground sample code is updated to use the .NET SignalR client.

@@ -39,7 +41,10 @@
UserId = userId ?? "user1"
});

return Results.Ok(negotiateResponse);
return Results.Json(negotiateResponse, new JsonSerializerOptions(JsonSerializerDefaults.Web)
Copy link
Member

Choose a reason for hiding this comment

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

JsonSerializerOptions should not be created per request.

@cqnguy23
Copy link
Contributor Author

Hi @cqnguy23,

This bug doesn't exist in the JavaScript SignalR client; therefore, this change isn't needed—unless the playground sample code is updated to use the .NET SignalR client.

It's not necessary to run this playground sample, but I think it would prevent users from getting error in case they were looking at this sample as a reference, and they are using the .NET SignalR client instead.

using Microsoft.AspNetCore.SignalR;
using Microsoft.Azure.SignalR.Management;

var builder = WebApplication.CreateBuilder(args);

builder.Services.AddRazorPages();
builder.Services.ConfigureHttpJsonOptions(options =>
{
options.SerializerOptions.DefaultIgnoreCondition = JsonIgnoreCondition.WhenWritingNull;
Copy link
Contributor

Choose a reason for hiding this comment

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

this changes the default behavior for all the apis. var ingoreNullOption = new JsonSerializerOptions... for negotiate is enough?

@eerhardt eerhardt added area-integrations Issues pertaining to Aspire Integrations packages and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Apr 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-integrations Issues pertaining to Aspire Integrations packages community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants