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

[API Proposal] Serialization Options #13

Open
stevejgordon opened this issue Nov 24, 2020 · 2 comments
Open

[API Proposal] Serialization Options #13

stevejgordon opened this issue Nov 24, 2020 · 2 comments
Labels
api suggestion Early API idea and discussion, it is NOT ready for implementation

Comments

@stevejgordon
Copy link
Collaborator

stevejgordon commented Nov 24, 2020

NOTE: This API proposal is a work in progress and open to change. This acts as a phase one suggestion which may require some prototyping to finalise.

Background and Motivation

From a usability standpoint, I would like to explore whether it would be helpful to expose configuration optionS that allow users to control serialization/deserialization of requests without having to implement the ITransportSerializer interface themselves.

Scenarios where this may be helpful include where users need to control the casing for serialized keys in the final JSON when using the low-level client. This doesn't seem to be simple with the current design.

Currently, the design allows PrettyJson to be specified on the ITransportOptions. I would propose we move options related to serialization into their own type (possibly with an interface defining the structure).

Proposed API

Our configuration options can act as a facade over common configuration options for generalised serializers. We should aim to keep this reasonably agnostic to the serializer being used and try to ensure that options are generally applicable on all serializers.

namespace Elastic.Transport
{
    // New interface
    public interface ITransportSerializerOptions
    {
        bool PrettySerialization { get; }
        bool CamelCaseKeys { get; }
        bool IgnoreNullValues { get; }
    }

    // New class (note: could we use a record?)
    public class TransportSerializerOptions : ITransportSerializerOptions
    {
        bool PrettySerialization { get; set; }
        bool CamelCaseKeys { get; set; }
        bool IgnoreNullValues { get; set;  }
    }
}

This allows us to remove serialization related properties from the ITransportConfiguration.

namespace Elastic.Transport
{
    // Existing interface
    public interface ITransportConfiguration : IDisposable
    {
        // Remove this
        bool PrettyJson { get; }
    }
}

We can also remove the fluent syntax method for the above located on TransportConfigurationBase.

Serializers may then accept the options into the ctor and apply the values as neccesary when serializing/deserializing.

namespace Elastic.Transport
{
    // Existing type
    public class LowLevelRequestResponseSerializer : ITransportSerializer
    {
        // Existing
        public LowLevelRequestResponseSerializer() : this(null) { }

        // Existing
        public LowLevelRequestResponseSerializer(IEnumerable<JsonConverter> converters)
        {
            ...
        }

        // New
        public LowLevelRequestResponseSerializer(IEnumerable<JsonConverter> converters, ITransportSerializerOptions options)
        {
            ...
        }
    }
}

I suspect we may want to provide helpers to simplify setting up the default serializer with options.

We could also enable per request control of serialization by accepting an ITransportSerializerOptions into the Serializable<T> static method. When present, these options are applied in preference to any defined on the serializer directly.

namespace Elastic.Transport
{
    public abstract partial class PostData
    {
        // Existing
        public static PostData Serializable<T>(T data) => new SerializableData<T>(data);

        // New
        public static PostData Serializable<T>(T data, ITransportSerializerOptions options) => new SerializableData<T>(data, options);
    }
}

The internal implementation could then use the provided options for the specific request. While perhaps less common, this would support codebases which interact with different indexes through a single client, where those indexes differ in their casing rules.

Usage Examples

TODO

Further Considerations

  • Might we ever see a future where other binary formats are used? If so, from a naming perspective for the options, should we avoid including the format in the name. e.g. PrettyJson could perhaps be PrettySerialization.
  • Do we require the ITransportSerializerOptions interface, or would the TransportSerializerOptions type be sufficient? Consider future additions and the impact on versioning this. Will users realistically need to mock it?
  • Do we want/need the TransportSerializerOptions type to be immutable?
@stevejgordon stevejgordon added the api suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 24, 2020
@Mpdreamz
Copy link
Member

  // Remove this
  bool PrettyJson { get; }

This one is a bit more involved. It also signals that the client needs to singal to the product to return pretty json.

This might need to be

  • bumped out of Elastic.Transport and to Elasticsearch.Net subclasses of TransportConfiguration
  • Explicitly modeled on IProductRegistraton.
  • Might we ever see a future where other binary formats are used? If so, from a naming perspective for the options, should we avoid including the format in the name. e.g. PrettyJson could perhaps be PrettySerialization.

👍 💯, there is definitely a chance we will support a product that supports something more efficient then json in the future 🤞

  • Do we require the ITransportSerializerOptions interface, or would the TransportSerializerOptions type be sufficient? Consider future additions and the impact on versioning this. Will users realistically need to mock it?

👍 💯 to not put it behind an interface. The options itself are behind interfaces to support a rather complex inheritance structure.

  • Do we want/need the TransportSerializerOptions type to be immutable

Anything stored on ITransportConfiguration should be immutable and be able to contribute to an ITransportConfiguration identity. With that in mind a record seems to be a good fit.

That said I think it be good if only LowLevelRequestResponseSerializer received TransportSerializerOptions in its constructor. It holds a an instance of JsonSerializerOptions that you want to reuse for performance reasons.

 public static PostData Serializable<T>(T data, ITransportSerializerOptions options) => new SerializableData<T>(data, options);

Would force us to create a new SerializerOptions each time and attach the global json converters from LowLevelRequestResponseSerializer and add ITransportSerializerOptions as an argument to ITransportSerializer.

For Elastic.Transport you typically configure the with good defaults for the product baked in DSL (request/responses).

Then in most cases the DSL takes user input

{ 
	"x" : 1
	"y: : {
		"data": { ... user input .. }
	}
}

In NEST this is solved by two serializer, nests low level serializer is aware of all the user defined input places and handing off to a second serializer. Something we dubbed donut hole serialization.

This forces the isolation of user configured data. It's quite rare that an API only accepts user input data where ITransportSerializerOptions would really shine without influencing the products DSL.

The current design, heavily forces folks to decorate their T's and if needed inject custom JsonConverters for them into the shared JsonSerializerOptions.

@Mpdreamz
Copy link
Member

In NEST we also allow you to influence serialization for types you don't own through:

settings.DefaultMappingFor<MyDerivedDocument>(d => d
	.PropertyName(p => p.IntProperty, nameof(MyDerivedDocument.IntProperty))
	.Ignore(p => p.StringProperty)
);

Wonder if it would be worth investigating shipping Elastic.Transport with the machinery OOTB.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api suggestion Early API idea and discussion, it is NOT ready for implementation
Projects
None yet
Development

No branches or pull requests

2 participants