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

All calls to JsonSerializer.Serialize should use JsonSerialisationOptions.Options #3419

Open
SteveBush opened this issue Dec 13, 2024 · 1 comment
Labels
0 - Backlog Bug good first issue .NET Pull requests that update .net code v9 Required for v9 release v10 Allocal to a v10 release

Comments

@SteveBush
Copy link
Contributor

Describe the bug

In constructing the Brighter service host, I add my JsonConverters to JsonSerialisationOptions.Options.Converters. I use these converters to serialize/deserialize Brighter messages. Unfortunately, there are instances of JsonSerializer.Serialize in the code (e.g., RequestLoggingHandler and RequestLoggingHandlerAsync) that doesn't include JsonSerialisationOptions.Options in calls to JsonSerializer.Serialize and JsonSerializer.Deserialize. When logging the request, the serialization fails. Similar behavior for BrighterTracer.

I'm happy to make these changes in a PR.

To Reproduce

Steps to reproduce.

  1. Create a Request message with an IPAddress as a public field.
  2. Post the Request to an Inbox with request logging;
  3. RequestLoggingHandler.LogCommand fails to serialize the request.

Fix

Update all calls to JsonSerializer.Serialize and JsonSerializer.Deserialize to include JsonSerialisationOptions.Options.

  • RequestLoggingHandler
  • RequestLoggingHandlerAsync
  • BrighterTracer
  • MQTTMessageConsumer
  • MQTTMessagePublisher

Exceptions (if any)

Further technical details

  • Brighter version: 10.
@SteveBush SteveBush changed the title All calls to JsonSerializer.Serialize use JsonSerialisationOptions.Options All calls to JsonSerializer.Serialize should use JsonSerialisationOptions.Options Dec 13, 2024
@iancooper iancooper added 0 - Backlog Bug good first issue v10 Allocal to a v10 release v9 Required for v9 release .NET Pull requests that update .net code labels Dec 18, 2024
@iancooper
Copy link
Member

@SteveBush Thanks for this, we will pick up this bug for V10 as requested. Feel free to raise a PR if you want.

lillo42 added a commit to lillo42/Brighter that referenced this issue Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
0 - Backlog Bug good first issue .NET Pull requests that update .net code v9 Required for v9 release v10 Allocal to a v10 release
Projects
None yet
Development

No branches or pull requests

2 participants