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

[PROPOSAL] Make ad-hoc JSON Serialization the Default with INatsClient and Make INatsConnection a low-level API #503

Open
mtmk opened this issue May 29, 2024 · 8 comments

Comments

@mtmk
Copy link
Collaborator

mtmk commented May 29, 2024

Proposed change

(1) Add NATS.Client.Serializers.Json to NATS.Net main package and NATS.Extensions.Microsoft.DependencyInjection package then add the ad-hoc serialization to the default serialization registry without breaking the AOT compilation (i.e. compile without trimming warnings) for the rest of the packages. Applications requiring AOT compilation will have to reference packages individually, which is usually the case in the examples I saw out there in the wild.

(2) To achieve this, we need to provide an abstraction over NatsConnection. Changing the existing NatsConnection and default NatsOpts would make the NATS.Client.Core project not AOT-friendly. Hence, we have to introduce a new thin abstraction over NatsConnection, which can be implemented in the NATS.Net package:

// wire-up Raw + Primitive + ad-hoc JSON serializers
// in a new serializer registry
await using var client = NatsClient();

// Only expose high level APIs but provide full functionality
await client.PublishAsync();
await foreach(var msg in client.SubscribeAsync()) {}

INatsConnection connection = client.Connection;

(3) With this we make INatsConnection a low level API giving us a chance to open up some of the calls which were marked as internal. This will give us a chance to write the mistakes of having JetStream, KV, Object store and services packages depending on core internals. We should aim to remove all internal access to core, including the tests.

Use case

Most applications does not require highly optimized generated serialization suitable for AOT compilations. Developers new to NATS .NET frequently get confused about serialization options when they first start using it. Having ad-hoc JSON serialization also provides a more progressive pathway to gradually increased feature set and complexity as the application requirements evolve.

  • Enable new developers pick up NATS .NET quickly.
  • Constructing the client with new NatsClient() is arguably more intuitive and similar to how other APIs work e.g. HttpClient
  • Serialization does the expected thing for most commonly used types such as byte[], Memory<byte>, string, int, etc. then all the custom types applications use as DTOs, falling back to JSON serialization automatically.
  • For applications using AOT, developers can still use individual packages.
  • When an application evolves to need lower level APIs, NatsClient can be swapped out with NatsConnection with little to no code change since NatsClient would only have a subset of NatsConnection interface.
@rickdotnet
Copy link
Collaborator

rickdotnet commented May 30, 2024

If I'm tracking right, would this be better addressed in the DI package? It could reference NATS.Client.Serializers.Json and we could add it by default, then provide an opt-out mechanism for those needing different serialization methods or AOT compatibility.

Rough example:

private INatsSerializerRegistry? _registry = null;

// opt out
public NatsBuilder AddSerializerRegistry(INatsSerializerRegistry registry)
{
    _registry = registry;
    return this;
}

internal IServiceCollection Build()
{
    if (_registry == null)
    {
        // default to JSON
    }

    ConfigureOptions(opts => opts with { SerializerRegistry = _registry });
    
    // ...

    return _services;
}

While we're here, if this method doesn't get removed we probably want to be friendlier and add params to the signature. Looks like an oversight on my part when I added that.

public NatsBuilder AddJsonSerialization(params JsonSerializerContext[] contexts)
    => ConfigureOptions(opts =>
    {
        var jsonRegistry = new NatsJsonContextSerializerRegistry(contexts);
        return opts with { SerializerRegistry = jsonRegistry };
    });

@mtmk
Copy link
Collaborator Author

mtmk commented May 30, 2024

It could reference NATS.Client.Serializers.Json and we could add it by default, then provide an opt-out mechanism for those needing different serialization methods or AOT compatibility.

How can we make it optionally AOT compatible without trimming warnings? Or do we need to have a separate package complementing the AOT one with ad-hoc JSON serializer?

@mtmk mtmk changed the title [PROPOSAL] Make ad-hoc JSON Serialization the Default [PROPOSAL] Make ad-hoc JSON Serialization the Default with INatsClient May 30, 2024
@rickdotnet
Copy link
Collaborator

rickdotnet commented May 30, 2024

how can we make it optionally AOT compatible

Maybe we can't. If not, I would recommend those folks not use the DI package and bootstrap it themselves or provide an separate package as you mention. My thought there is if someone is needing AOT compatibility, they should be comfortable bootstrapping the client themselves. Also, full disclosure I don't have a lot of experience in the AOT world so I'm not familiar enough to speak to the issues there.

The main reason I'd put default serialization in the DI package is to help with the concern around new devs coming to NATS and provide them with default behavior; then, let the outliers figure it out. For example, I prefer NO serializer. I want the bytes[] only. I'm fine with handling my own DI registration if it means I get more control of the behavior.

@mtmk
Copy link
Collaborator Author

mtmk commented May 30, 2024

The main reason I'd put default serialization in the DI package is to help with the concern around new devs coming to NATS

yes, I agree we should add it to DI, and that's great for people who are using DI but not everyone will be using the it. Hence also the proposal of NatsClient which would come with NATS.Net package. (just edited the description above) edit: just added a not for the DI as well.

@rickdotnet
Copy link
Collaborator

I see. The trade-off IMO is between a JSON dependency in the core package, which may not sit well with everyone, and offering a user-friendly path with alternative packages. I'm fine with either approach, personally. I happen to have more bandwidth at the moment so I'll be on standby and help if I can.

@mtmk mtmk changed the title [PROPOSAL] Make ad-hoc JSON Serialization the Default with INatsClient [PROPOSAL] Make ad-hoc JSON Serialization the Default with INatsClient and Make INatsConnection a low-level API Jun 1, 2024
@to11mtm
Copy link
Collaborator

to11mtm commented Jun 17, 2024

Most applications does not require highly optimized generated serialization suitable for AOT compilations.

Agreed, I have historically fallen into the MessagePack (non AOT but semi-customized) case for NATS usage and know it's the edge case. That said It has become my default for most NATS projects for various preferential/composition reasons.

Developers new to NATS .NET frequently get confused about serialization options when they first start using it.

I guess I wonder if this is a doc issue? AFAIK it's normal serialization/etc.

Having ad-hoc JSON serialization also provides a more progressive pathway to gradually increased feature set and complexity as the application requirements evolve.

I will admit I have a very 'particular' leaning here, however from a defense in depth and general partitioning standpoint I tend to keep my peanut-butter and jelly separate; The serializers configured for ASPNETCORE controllers are intentionally different from the serializers my backend (be it NATS, MassTransit, Akka, misused Redis) because oftentimes the security rules are different (unsafe semi-dynamic polymorphic serialization comes to mind, if a backend is firewalled/partitioned/etc restrictions can sometimes be relaxed,)

What would the API difference be between NatsClient and NatsConnection from today, if any?

How would we help users 'drop down' where needed? Asking this because we want to avoid having too much of a barrier between the easy case and the more esoteric cases, the NATS api is thankfully pretty simple as is, we mostly want to avoid cases where to go low level is so difficult everyone sticks to the 'simple' use even when it is detrimental.

I think this is a great thing and look forward to seeing what comes out of it!

@mtmk
Copy link
Collaborator Author

mtmk commented Jun 30, 2024

Just a few ideas to discuss:

@mtmk
Copy link
Collaborator Author

mtmk commented Oct 28, 2024

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

No branches or pull requests

3 participants