Skip to content

replace BinaryFormatter with System.Text.Json.JsonSerializer#960

Merged
yang-xiaodong merged 3 commits intodotnetcore:masterfrom
ChangYinHan:master
Jul 28, 2021
Merged

replace BinaryFormatter with System.Text.Json.JsonSerializer#960
yang-xiaodong merged 3 commits intodotnetcore:masterfrom
ChangYinHan:master

Conversation

@ChangYinHan
Copy link
Contributor

@ChangYinHan ChangYinHan commented Jul 23, 2021

No description provided.

@ChangYinHan
Copy link
Contributor Author

ChangYinHan commented Jul 27, 2021

Hi brother,

The BinaryFormatter has beed disabled by default at .NET 5, and an exception will be throw when running CAP with NATS at .NET 5.

fail: DotNetCore.CAP.Processor.Dispatcher[0]
      An exception occured while publishing a message, reason:Failed : . message id:1419838550676574208
      DotNetCore.CAP.Internal.PublisherSentFailedException: BinaryFormatter serialization and deserialization are disabled within this application. See https://aka.ms/binaryformatter for more information.
       ---> System.NotSupportedException: BinaryFormatter serialization and deserialization are disabled within this application. See https://aka.ms/binaryformatter for more information.
         at System.Runtime.Serialization.Formatters.Binary.BinaryFormatter.Serialize(Stream serializationStream, Object graph)
         at DotNetCore.CAP.NATS.NATSTransport.SendAsync(TransportMessage message)
         --- End of inner exception stack trace ---

And this pull request will fix this bug.

@yang-xiaodong yang-xiaodong requested a review from xiangxiren July 27, 2021 03:43
yang-xiaodong
yang-xiaodong reviewed Jul 27, 2021
View reviewed changes
Copy link
Member

@yang-xiaodong yang-xiaodong left a comment

Choose a reason for hiding this comment

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

Hello, Thanks for your PR, These are some problems that need improvement

src/DotNetCore.CAP.NATS/ITransport.NATS.cs Outdated
//return Task.FromResult(OperateResult.Success);

var reply = connection.Request(message.GetName(), mStream.ToArray(), 2000);
var json= JsonSerializer.Serialize(message);
Copy link
Member

@yang-xiaodong yang-xiaodong Jul 27, 2021

Choose a reason for hiding this comment

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

We have the serialization interface, need to use ISerializer instead here

Copy link
Contributor Author

@ChangYinHan ChangYinHan Jul 27, 2021

Choose a reason for hiding this comment

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

Do you mean DotNetCore.CAP.Serialization.ISerializer?

Copy link
Contributor Author

@ChangYinHan ChangYinHan Jul 27, 2021

Choose a reason for hiding this comment

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

I have checked the DotNetCore.CAP.Serialization.ISerializer and JsonUtf8Serializer, they are based on Message not TransportMessage, may I add another method for TransportMessage?

Copy link
Member

@yang-xiaodong yang-xiaodong Jul 27, 2021

Choose a reason for hiding this comment

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

Consider using the version 0.14.0-pre1, which provides support for headers

https://github.com/nats-io/nats.net/releases/tag/0.14.0-pre1

Copy link
Contributor Author

@ChangYinHan ChangYinHan Jul 28, 2021

Choose a reason for hiding this comment

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

Thanks, I have updated to 0.14.0-pre1 now, and the interface ISerializer is no longer need, so I removed it.

src/DotNetCore.CAP.NATS/NATSConsumerClient.cs Outdated

var message = (TransportMessage)binFormatter.Deserialize(mStream);
var json = UTF8Encoding.Default.GetString(e.Message.Data);
var message = JsonSerializer.Deserialize<TransportMessage>(json);
Copy link
Member

@yang-xiaodong yang-xiaodong Jul 27, 2021

Choose a reason for hiding this comment

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

We have the serialization interface, need to use ISerializer instead here

src/DotNetCore.CAP.NATS/NATSConsumerClient.cs Outdated
@@ -5,6 +5,8 @@
using System.Collections.Generic;
using System.IO;
using System.Runtime.Serialization.Formatters.Binary;
Copy link
Member

@yang-xiaodong yang-xiaodong Jul 27, 2021

Choose a reason for hiding this comment

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

Unused namespace

test/DotNetCore.CAP.Test/DotNetCore.CAP.Test.csproj Outdated

<ItemGroup>
<ProjectReference Include="..\..\src\DotNetCore.CAP.InMemoryStorage\DotNetCore.CAP.InMemoryStorage.csproj" />
<ProjectReference Include="..\..\src\DotNetCore.CAP.NATS\DotNetCore.CAP.NATS.csproj" />
Copy link
Member

@yang-xiaodong yang-xiaodong Jul 27, 2021

Choose a reason for hiding this comment

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

This is a unit test project, the integration test of NATS cannot be reference

yang-xiaodong
yang-xiaodong reviewed Jul 27, 2021
View reviewed changes
test/DotNetCore.CAP.Test/Nats/NatsTransportTest.cs Outdated
@@ -0,0 +1,57 @@
using System;
Copy link
Member

@yang-xiaodong yang-xiaodong Jul 27, 2021

Choose a reason for hiding this comment

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

There is no need to add integration tests for each Transport, so it can be removed

yang-xiaodong
yang-xiaodong approved these changes Jul 28, 2021
View reviewed changes
Copy link
Member

@yang-xiaodong yang-xiaodong left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

@yang-xiaodong yang-xiaodong merged commit 4c3a363 into dotnetcore:master Jul 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yang-xiaodong yang-xiaodong yang-xiaodong approved these changes

@xiangxiren xiangxiren Awaiting requested review from xiangxiren

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ChangYinHan @yang-xiaodong