pact-net icon indicating copy to clipboard operation
pact-net copied to clipboard

Support Alternative JSON Serialisers, Like System.Text.Json

Open tl-facundo-aita opened this issue 3 years ago • 2 comments
trafficstars

When using .WithJsonBody, would be nice to be able to pass in a System.Text.Json.JsonSerializerOptions body. Another solution would be not to serialize the when the body is already a string (maybe related to this issue #391 )

This following line is the one I want to avoid:

https://github.com/pact-foundation/pact-net/blob/701cb50e6a1e307ee6965079e4880d910572d7bb/src/PactNet/RequestBuilder.cs#L376

tl-facundo-aita avatar May 26 '22 20:05 tl-facundo-aita

This is something we need to support but it would be a really big breaking change.

The correct solution is to allow the user to choose the serialiser via a plug-in mechanism, in the same way that ASP.Net Core defaults to STJ but plugins can use different JSON libraries instead.

The problem is that the internals of PactNet rely on Newtonsoft also (e.g. for serialising matchers), so there'd have to be internal refactoring to abstract this.

Both of those together would result in a breaking change of the public API, so not something that's going to happen soon. We can put this in a bucket for PactNet 5.0 though

adamrodger avatar May 27 '22 13:05 adamrodger

So, to demonstrate the problem, the PactNet.Abstractions library currently has a reference to Newtonsoft because matchers need to be serialised in a special format recognised by the FFI:

https://github.com/pact-foundation/pact-net/blob/085ce4eb59470d4697708dfb7285dbc50f868274/src/PactNet.Abstractions/Matchers/IMatcher.cs

PactConfig also has default JSON settings so that you don't need to specify settings per-interaction:

https://github.com/pact-foundation/pact-net/blob/085ce4eb59470d4697708dfb7285dbc50f868274/src/PactNet.Abstractions/PactConfig.cs

That means we'd either need to:

  • Continue using Newtonsoft internally but allow non-Newtonsoft at the same time. This creates an awkward duality in the API, e.g. the default settings on PactConfig wouldn't be used for any interaction that isn't using Newtonsoft. It would relegate non-Newtonsoft serialisers to second class citizens, which I don't like.
  • Remove the references to Newtonsoft inside PactNet.Abstractions, and create a plugin-like system to add your desired serialiser. This is awkward because matchers need special serialisation settings which can't be represented in normal C# (because the property names contain colons) and there needs to be a way to specify default JSON settings per serialiser used.

None of those problems are insurmountable, but you end up with a breaking change either way. The latter option is my preference and the API could look something like:

// Newtonsoft
IPactBuilderV3 pact = Pact.V3("My Consumer", "My Provider")
                          .WithNewtonsoftSerialiser(new JsonSerializerSettings
                          {
                              ContractResolver = new CamelCasePropertyNamesContractResolver()
                          })
                          .UsingNativeBackend();

// System.Text.Json
IPactBuilderV3 pact = Pact.V3("My Consumer", "My Provider")
                          .WithSystemTextJsonSerialiser(new JsonSerializerOptions
                          {
                              // options...
                          })
                          .UsingNativeBackend();

We'd use the typestate pattern to make sure you have to specify a serialiser before you can finish initialising the pact builder. That allows you to specify default JSON settings upfront as well, and those are specific to that builder/serialiser. I don't think we should support using multiple serialisers in the same pact builder. That just sounds like it's asking for compatibility troubles and inconsistency. I think you should specify the serialiser once per builder and then you have to use that for the current builder.

Each serialiser would have to internally supply a method which could serialise matchers properly somehow. Obviously it can't use attributes on IMatcher so instead it'd need a custom serialiser per implementation.

Each serialiser would also have to supply its own implementation of WithJsonBody (which would probably need removing from PactNet.Abstractions entirely) so that it could be passed its own variant of custom JSON settings per interaction. I wonder if we can use generics here so that the main code can stay centralised whilst still delegating the actual serialisation to the configured serialiser. As in PactNet.Abstractions would define something like:

public IJsonRequestBodyBuilderV3<T>
{
    IResponseBuilderV3 WithJsonBody(dynamic body);
    IResponseBuilderV3 WithJsonBody(dynamic body, T settings);
}

public IJsonResponseBodyBuilderV3<T>
{
    void WithJsonBody(dynamic body);
    void WithJsonBody(dynamic body, T settings);
}

That could get very awkward very quick though, because everything in the call chain would need to become generic so that the settings type could be passed all the way down. It would at least enforce that you can only use one serialiser type per builder though.

adamrodger avatar May 27 '22 15:05 adamrodger

Is there any known workarounds for this? I'm currently working on implementing contract testing within our microservices and have ended up having to re-introduce newtonsoft based attributes to get it to function. I tried looking at ways in which I can provide the json string but it doesn't seem to go through the same logic / process compared to when you specify it via WithJsonBody

cropper1991 avatar Apr 26 '23 18:04 cropper1991

PactNet has to pick and use one serialiser internally because there are internal implementation details which require control of how things like matchers are serialised.

That means that currently Newtonsoft is the only supported serialiser. You can pass arbitrary strings to WithBody though if you want finer control.

adamrodger avatar Apr 29 '23 07:04 adamrodger

Replaced with RFC: https://github.com/pact-foundation/pact-net/issues/458

adamrodger avatar May 05 '23 13:05 adamrodger