Add message context to serialization interfaces

[mtmk]

Adds opt-in INatsSerializeWithContext<T> and INatsDeserializeWithContext<T> interfaces (and a combined INatsSerializerWithContext<T>) that receive a NatsMsgContext (subject, reply-to, headers) during (de)serialization. Existing serializers continue to work unchanged: the library dispatches via runtime type check and falls back to the standard interface otherwise. Built-in serializers (NatsUtf8PrimitivesSerializer, NatsRawSerializer, NatsJsonContextSerializer) also propagate the context to _next so chains keep context-awareness at the leaf. NatsMsgContext is constructed via a required-subject constructor (no init setters, no netstandard IsExternalInit shim). The read-only flag on NatsHeaders is removed so context-aware serializers can mutate headers (e.g. set Content-Type); the trade-off is that a single NatsHeaders instance is no longer safe to share across concurrent publishes, documented on the type.

[adminnz]

Shouldn't this be passing in the Headers property?

[mtmk]

fixed.

[github-actions]

🚀 Deployed on https://69e0df6ca6b18c00b244ccaa--teal-klepon-7e92d0.netlify.app

[mtmk]

Hey @adminnz, I added DIMs as well for BC. the idea is for new implementations on net8+ nothing changes. you only implement the new method. the old one is also marked as obsolete. little bit of maintenance burden on our library side but i think it's easily manageable. wdyt? if you're happy please let me know, I can merge (after fixing the tests etc.) and have it released in 3.0 previews today or tomorrow.

[adminnz]

Remove IDctionary<string,StringValues> as INatsHeaders interface is already inheriting IDictionary<string,StringValues>

[mtmk]

fixed

[adminnz]

That is a novel solution for the backwards compatibility problem.
From my experience working on very large code bases (especially in a pub/sub environment) its best to have a really tight feedback loop for the developer, and identify problems as early on in the project/product lifecycle.
Problems can occur at many points in the lifecycle.

1. Compile Time
2. Unit Tests (not all projects have tests)
3. Run Time (App Start)
4. Run Time (Specific Condition)

While the DIM allows non-breaking changes to library users with existing INatsSerialize/INatsDeserialize implementations, it does mean that a developer now has to not only run their app, they have to trigger the exact code path (stage 4) to realize they might not have implemented either of the interface methods.

It is noble to try and never introduce breaking changes, the "major" in semantic versioning is:

MAJOR version when you make incompatible API changes

Obviously that doesn't mean you have to introduce breaking changes, but it does provide the opportunity to introduce breaking changes.

In my opinion, it would be better to have 3.0 introduce a breaking change and require the header interface variants are implemented.

[mtmk]

@adminnz thanks for the feedback! I've updated the approach; the old methods no longer have DIMs so any implementor missing them gets a compile error.

However the new header overloads still keep DIMs for BC.

On the semver point agreed that 3.0 is the right place for breaking changes but the main reason we want to keep the DIM on the new overloads is transitive dependencies: third-party libraries (or e.g. company internal common libraries) implementing INatsSerialize<T> would also need to release updates before users can upgrade to 3.0. Keeping the old methods as obsolete with DIMs on the new overloads gives the ecosystem a smoother migration path without sacrificing compile-time safety. I realize netstandard would still be breaking and that's something we have to accept balancing complexity, maintenance and given its usage is slowly getting less and less. Please also see the discussion here #1061

[rickdotnet]

Your approach looks fine to me. I also prefer obvious breaking changes over random side-effects and major versions seem appropriate for that. Seems the shims are necessary.

I had started a take on this but never got it pushed. I went with johnweldon's approach and used separate interfaces to keep compatibility. Serializers can opt in if they want headers.

Here's what I had, might add some inspiration.

/// <summary>
/// Deserializer interface for NATS messages.
/// </summary>
/// <typeparam name="T">Deserialized object type</typeparam>
public interface INatsDeserialize<out T>
{
    T? Deserialize(in ReadOnlySequence<byte> buffer);
}

/// <summary>
/// Extended deserializer interface that supports receiving headers during deserialization.
/// </summary>
public interface INatsDeserializeWithHeaders<out T> : INatsDeserialize<T>
{
    /// <summary>
    /// Deserialize value from buffer with headers.
    /// </summary>
    /// <param name="buffer">Buffer with the serialized data.</param>
    /// <param name="headers">Headers from the message.</param>
    /// <returns>Deserialized object</returns>
    T? Deserialize(in ReadOnlySequence<byte> buffer, INatsHeaders headers);
}

Extension methods to prevent type-check spam.

public static class NatsSerializationExtensions
{
    /// <summary>
    /// Serializes the value with header support, falling back to standard serialization if headers are not supported.
    /// </summary>
    public static void Serialize<T>(this INatsSerialize<T> serializer, IBufferWriter<byte> bufferWriter, T value, INatsHeaders? headers)
    {
        if (serializer is INatsSerializeWithHeaders<T> withHeaders && headers != null)
        {
            withHeaders.Serialize(bufferWriter, value, headers);
            return;
        }

        // Fallback to standard serialization
        serializer.Serialize(bufferWriter, value);
    }

    /// <summary>
    /// Deserializes the value with header support, falling back to standard deserialization if headers are not supported.
    /// </summary>
    public static T? Deserialize<T>(this INatsDeserialize<T> deserializer, in ReadOnlySequence<byte> buffer, INatsHeaders? headers)
    {
        if (deserializer is INatsDeserializeWithHeaders<T> withHeaders)
            return withHeaders.Deserialize(buffer, headers ?? new NatsHeaders());

        return deserializer.Deserialize(buffer);
    }
}

[mtmk]

@rickdotnet

INatsDeserializeWithHeaders

nice. this means we also cover netstandard as another first class target without breaking backward compatibility. cc @johnweldon

[adminnz]

Yes that looks like a good solution.

[rickdotnet]

Looks good to me.

Should serializers be able to produce/add headers during serialization? Looks like this supports header-aware deserialization, not header-producing serialization. Seems appropriate to me unless there is a use-case worth tackling now.

@adminnz re: typed vs bytes - valid architectural discussion for a future layer, but this PR doesn't block that path.

Would something like this work?

public class CustomClient
{
    private readonly INatsConnection _conn;
    public INatsSendMsgFactory Factory { get; set; }
    
    public async Task PublishAsync(object payload, NatsHeaders? headers = null)
    {
        var msg = Factory.Create(payload, headers);  // Subject from convention
        await _conn.PublishAsync(msg.Subject, msg.Payload, msg.Headers);
    }
}

[scottf]

LGTM

[adminnz]

Looks good to me.

Should serializers be able to produce/add headers during serialization? Looks like this supports header-aware deserialization, not header-producing serialization. Seems appropriate to me unless there is a use-case worth tackling now.

@adminnz re: typed vs bytes - valid architectural discussion for a future layer, but this PR doesn't block that path.

Would something like this work?

public class CustomClient
{
    private readonly INatsConnection _conn;
    public INatsSendMsgFactory Factory { get; set; }
    
    public async Task PublishAsync(object payload, NatsHeaders? headers = null)
    {
        var msg = Factory.Create(payload, headers);  // Subject from convention
        await _conn.PublishAsync(msg.Subject, msg.Payload, msg.Headers);
    }
}

Serialization (producing) should definitely be able to add to headers, otherwise sending is seriously gimped.

And yes a higher level client can the provide very easy to use apis

[mtmk]

@adminnz re separating typed serialization from core: I agree a higher-level client with a factory pattern is a useful abstraction and nothing in here prevents building that on top of INatsConnection. The reason serialization is baked into core is zero-copy performance. Serializers write directly into a pooled IBufferWriter<byte> that goes straight onto the wire, no intermediate allocation. Your example materializes the payload as a string/byte array before handing it to NatsMsg which is an extra allocation + copy. you could change the factory to use an IBufferWriter, but then we're basically back to what current serializers does. Happy to explore a higher-level typed client in a separate issue if you want to open one.

Re header mutating serialization: good catch, it doesn't actually work yet. headers get locked to read-only before it reaches the serializer. the fix is to move SetReadOnly() after serialization. That said, the read-only flag was always a weak guarantee since headers are copied to a byte buffer synchronously regardless. Worth considering removing it entirely. @rickdotnet wdyt?

[mtmk]

Went ahead and removed the read-only flag entirely in 3a54d03 rather than just moving SetReadOnly(), since it was protecting against mutations that can't reach the wire anyway (headers get copied to a byte buffer synchronously inside CommandWriter). Added a test in SerializerTest.cs to verify serializers can mutate context.Headers (e.g. set Content-Type) during serialization. @rickdotnet please yell if you see any issue with this.