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

Serialization API proposal cont. #177

Merged
merged 27 commits into from
Nov 7, 2023
Merged

Conversation

mtmk
Copy link
Collaborator

@mtmk mtmk commented Nov 2, 2023

https://github.com/nats-io/nats.net.v2/blob/serialization-api-change-proposal-2/docs/documentation/serialization.md

(Docs to be updated in a later PR)

Summary

  • Added serializer parameter to all publish, subscribe and consume calls alike
  • Now serializer interface is generic INatsSerializer<T>
  • New interface INatsSerializerRegistry for connection wide serialization settings
  • Removed many of the unnecessary nullables e.g. <T?> -> <T>

Details

This is the continuation of the previous proposal where simplified the serialization APIs and made them compatible with native AOT deployments.

With this change, we make it more obvious where to pass the serializer into various calls. We also made the interface generic to make it easier to implement in a type safe manner.

The main change is that the serializer option is removed from NatsSubOpts and NatsPubOpts classes and became a parameter in publish and subscribe calls (as well as in all JS / KV / Obj / Svc etc. calls):

var myJson = new NatsJsonContextSerializer(MyJsonContext.Default);

await using var sub = await nats.SubscribeAsync<MyData>(subject: "foo", serializer: myJson);

await nats.PublishAsync<MyData>(subject: "foo", data: new MyData { Id = 1, Name = "bar" }, serializer: myJson);

This makes it easier to pass in different serializers in every call, especially in native AOT deployments.

We also introduce a serializer registry concept to be used as a connection wide serialization mechanism. This was necessary with INatsSerializer<T> being generic and since we can't know the type in advance. It's very simple and can be passed in with NatsOpts.Serializers property:

public interface INatsSerializerRegistry
{
    INatsSerializer<T> GetSerializer<T>();
}

So we have the additional flexibility with INatsSerializerRegistry on top of being able to chain multiple INatsSerializer<T>s.

Also fixes #142

This is the continuation of the previous proposal where
simplified the serialization APIs and made them compatible
with native AOT deployments.

With this change, we make it more obvious where to pass
the serializer into various calls. We also made the interface
generic to make it easier to implement in certain scenarios.
@mtmk mtmk requested a review from caleblloyd November 2, 2023 14:30
@mtmk
Copy link
Collaborator Author

mtmk commented Nov 2, 2023

@jasper-d you'd be interested 💯

@jasper-d
Copy link
Contributor

jasper-d commented Nov 2, 2023

@jasper-d you'd be interested 💯

I'll take a look tomorrow this weekend.

mtmk added 11 commits November 3, 2023 11:33
# Conflicts:
#	src/NATS.Client.JetStream/NatsJSStream.cs
#	src/NATS.Client.KeyValueStore/INatsKVWatcher.cs
#	src/NATS.Client.KeyValueStore/Internal/NatsKVWatcher.cs
#	src/NATS.Client.KeyValueStore/NatsKVStore.cs
#	src/NATS.Client.ObjectStore/NatsObjStore.cs
#	src/NATS.Client.Services/NatsSvcServer.cs
Removed unused interface that slipped through during
merge conflict resolutions.
Copy link
Contributor

@jasper-d jasper-d left a comment

Choose a reason for hiding this comment

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

Thank you, I think this is a good compromise to support both, serializers passed directly and serializers attached to a connection.

string subject,
TRequest? data,
NatsHeaders? headers = default,
INatsSerializer<TRequest>? requestSerializer = default,
INatsSerializer<TReply>? replySerializer = default,
Copy link
Contributor

@jasper-d jasper-d Nov 4, 2023

Choose a reason for hiding this comment

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

Would it make sense to split INatsSerializer<T> into I(Nats)Serializer<T> and I(Nats)Deserializer<T> now? Presumably one is often not going to send and receive the same type but the current interface requires an implementation for both.

Copy link
Contributor

Choose a reason for hiding this comment

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

Which then also raises the question if it wouldn't be better (and feasible) to just use Action<IBufferWriter<byte>, T> and Func<ReadOnlySequence<byte>, T> so that clients don't have to implement the interface if they already have a suitable serializer that matches the delegates signature.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sounds ok but I'd like to see what @caleblloyd would say here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Splitting into I(Nats)Serializer<T> and I(Nats)Deserializer<T> makes sense to me

The Action / Func seems a little non-standard - it may map cleanly to some Serializer/Deseriaizers such as GRPC, but not others, such as JsonSerializer. It should be trivial to use a helper function to create a static I(Nats)Serializer<T> and I(Nats)Deserializer<T> from an Action / Func though

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree we can split the interface but I'm leaning towards interfaces too rather than action/func approach. I believe interface would be more developer friendly in general.

src/NATS.Client.Core/NatsException.cs Outdated Show resolved Hide resolved
src/NATS.Client.Core/NatsMsg.cs Outdated Show resolved Hide resolved
src/NATS.Client.KeyValueStore/NatsKVOpts.cs Outdated Show resolved Hide resolved
src/NATS.Client.Core/INatsConnection.cs Show resolved Hide resolved
mtmk added 5 commits November 6, 2023 17:09
# Conflicts:
#	sandbox/Example.JetStream.PullConsumer/Program.cs
#	src/NATS.Client.JetStream/INatsJSConsume.cs
#	src/NATS.Client.JetStream/INatsJSFetch.cs
#	src/NATS.Client.JetStream/Internal/NatsJSConsume.cs
#	src/NATS.Client.JetStream/NatsJSConsumer.cs
#	src/NATS.Client.JetStream/NatsJSContext.cs
#	src/NATS.Client.ObjectStore/NatsObjStore.cs
#	tests/NATS.Client.CheckNativeAot/Program.cs
#	tests/NATS.Client.JetStream.Tests/ConsumerConsumeTest.cs
#	tests/NATS.Client.JetStream.Tests/ConsumerFetchTest.cs
#	tests/NATS.Client.JetStream.Tests/ConsumerNextTest.cs
#	tests/NATS.Client.JetStream.Tests/JetStreamTest.cs
#	tests/NATS.Client.JetStream.Tests/PublishTest.cs
@mtmk
Copy link
Collaborator Author

mtmk commented Nov 6, 2023

I will be updating the docs in the release prep PR

@mtmk mtmk requested a review from caleblloyd November 6, 2023 19:08
@caleblloyd
Copy link
Collaborator

I like the interface splitting, but might I suggest some clarity on naming:

  • INatsSerialize- the serialize half of a serializer
  • INatsDeserialize - the deseialize half of a serializer
  • INatsSerializer: implements INatsSerialize, INatsDeserialize

Then for parameter naming what is your opinion of calling all of the params that deal with Serialize/Deseialize serializer? Even though they will be typed as INatsSerialize or INatsDeserialize. Most people will just want to provide a Serializer that can do either

Copy link
Collaborator

@caleblloyd caleblloyd left a comment

Choose a reason for hiding this comment

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

A couple docs fixes, then LGTM!

docs/documentation/serialization.md Outdated Show resolved Hide resolved
docs/documentation/serialization.md Outdated Show resolved Hide resolved
@mtmk mtmk merged commit 342c3ea into main Nov 7, 2023
8 of 9 checks passed
@mtmk mtmk deleted the serialization-api-change-proposal-2 branch November 7, 2023 03:41
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.

Request reply timeout returning null
3 participants