Isolated Content Serialization #571

stevewgh · 2018-10-18T14:46:07Z

My first effort at adding support for custom serializers #566 . I'm interested in hearing what you think, getting feedback etc. Thanks.

Overview of Changes

Isolated all content serialization to the IContentSerializer interface
Added JsonContentSerializer and XmlContentSerializer implmentations
RefitSettings allows the IContentSerializer to be specified but still defaults to JsonContentSerializer for back-compat reasons
.NET Standard 1.4 has a new package dependency on System.Xml.XmlSerializer

Tests

Tests modified to exercise Json and Xml serialization
XmlContentSerializer unit test added
Some existing tests have been modified to use both JsonContentSerializer and XmlContentSerializer using the an xunit theory

How to use

// 1. uses Json serialization - no change
var service = RestService.For<IService>("https://localhost");

// 2. uses Json serialization and passes the JsonSerializerSettings to the JsonContentSerializer
var service = RestService.For<IService>("https://localhost",
    new RefitSettings
    {
        JsonSerializerSettings = new JsonSerializerSettings {Formatting = Formatting.Indented}
    });

// 3. equivalent of 2. 
var service = RestService.For<IService>("https://localhost",
    new RefitSettings
    {
        ContentSerializer = new JsonContentSerializer(new JsonSerializerSettings {Formatting = Formatting.Indented})
    });

// 4. uses Xml serialization
var service = RestService.For<IService>("https://localhost",
    new RefitSettings
    {
        ContentSerializer = new XmlContentSerializer()
    });

// 5. uses Xml serialization with additional configuration
var service = RestService.For<IService>("https://localhost",
    new RefitSettings
    {
        ContentSerializer = new XmlContentSerializer(
            new XmlContentSerializerSettings
            {
                XmlReaderWriterSettings = new XmlReaderWriterSettings(
                    new XmlReaderSettings
                    {
                        ConformanceLevel = ConformanceLevel.Document
                    })
            })
    });

Possible further changes

Should the RefitSettings.JsonSerializerSettings property as marked as [Obsolete]? It should only be used with the JsonContentSerializer.
Simplify how the ContentSerializer property is set. One idea is to add a RefitSettings extension class - something like this:

public static class RefitSettingsJsonContentExtensions
{
    public static RefitSettings UseJsonSerialization(this RefitSettings settings, JsonSerializerSettings jsonSerializerSettings)
    {
        settings.ContentSerializer = new JsonContentSerializer(jsonSerializerSettings);
        return settings;
    }
}

The extension class would be used like this:

var service = RestService.For<IService>("https://localhost", new RefitSettings().UseJsonSerialization(new JsonSerializerSettings {Formatting = Formatting.Indented}));

dnfclas · 2018-10-18T14:46:20Z

All CLA requirements met.

clairernovotny · 2018-11-09T21:46:26Z

Thanks for this!

Should the RefitSettings.JsonSerializerSettings property as marked as [Obsolete]? It should only be used with the JsonContentSerializer.

Yes, I would say so, but with a warning, not an error, and the message should point to the new type and docs.

I'm not keen on using an extension method to configure a new instance of the settings object -- @paulcbetts any thoughts?

- Added IContentSerializer, JsonContentSerializer, XmlContentSerializer - RefitSettings allows the IContentSerializer to be specified - Refit defaults to JsonContentSerializer for back-compat - Tests modified to exercise Json and Xml Serialization - .NET Standard 1.4 has package dep on System.Xml.XmlSerializer

- Added Obsolete attributes - Added Serialized enum as a generic replacement of JSON enum - Updated tests to use ContentSerializer property - Updated README with XML example

stevewgh · 2018-11-16T23:22:14Z

Glad to help!

I've rebased my branch on master and made some further changes.

Marked RefitSettings.JsonSerializerSettings as obsolete
Marked the BodySerializationMethod.Json enum as obsolete, suggest using the newly added BodySerializationMethod.Serialized instead
Updated the README document with examples of how to use Refit with XML

I hope this is OK?

Thanks.

Refit/RefitSettings.cs

IContentSeriaizer - all methods made async - removed 2nd deserialize method - type safe operation ApiException - Added GetContentAsAsync<T>() method - Marked GetContentAs<T>() as obsolete

Refit/ValidationApiException.cs

Refit/RequestBuilderImplementation.cs

stevewgh · 2018-11-26T21:46:56Z

Good spot with the the missing ConfigureAwait's, I'll add them in.

The ValidationApiException.Content property is a bit trickier. If I change this factory method to async Task<ValidationApiException>

refit/Refit/ValidationApiException.cs

Lines 22 to 25 in c14b2c2

    
           public static ValidationApiException Create(ApiException exception) 
        
           { 
        
               return new ValidationApiException(exception); 
        
           }

I could then set the Content property from within the Create method and the property would avoid deadlocks.

We already have an async context where the ValidationApiException.Create is called, so I think this could work.

…Exception.Content

Refit/ValidationApiException.cs

Refit/RefitSettings.cs

clairernovotny · 2018-11-28T20:35:04Z

Looks good! One last thing and then I think it can be merged.

stevewgh · 2018-12-03T08:59:23Z

Hi @onovotny are the changes that I made in cc05c3a ok?

richbryant · 2019-01-11T16:31:02Z

This still doesn't make it easy to use one's own ContentSerializer, since the IContentSerializer interface is buried in RefitSettings. I'm using Blazor, Json.NET is not supported under WASM, I can easily do my own deserialization based on returning HttpResponseMessage but posting is proving more than a little evil.

If I could simply throw in something that implements IContentSerializer, it'd be easy. But I can't. Why not?

stevewgh · 2019-01-12T13:44:53Z

Hi @richbryant if you want to implement your own IContentSerializer then you can. Here's one which sends everything as Base64 strings and uses a non-existent (I think!) media type of text/base64

public class Base64ContentSerializer : IContentSerializer
{
    BinaryFormatter formatter = new BinaryFormatter();

    public async Task<T> DeserializeAsync<T>(HttpContent content)
    {
        var bytes = Convert.FromBase64String(await content.ReadAsStringAsync());

        using (var ms = new MemoryStream(bytes))
        {
            return (T)formatter.Deserialize(ms);
        }
    }

    public Task<HttpContent> SerializeAsync<T>(T item)
    {
        using (var stream = new MemoryStream())
        {
            formatter.Serialize(stream, item);
            var content = (HttpContent)new StringContent(Convert.ToBase64String(stream.ToArray()), System.Text.Encoding.UTF8, "text/base64");
            return Task.FromResult(content);
        }
    }
}

// use it like this

var client = RestService.For<IApi>("http://localhost:5000",
    new RefitSettings
    {
        ContentSerializer = new Base64ContentSerializer()
    });

The IContentSerializer interface is available in versions 4.6.68 onwards which is only available on the 'MyGet' feed for now.

@onovotny do you have an ETA for when 4.6.68 be published to NuGet?

stevewgh mentioned this pull request Oct 19, 2018

Add support for custom serializers #566

Closed

stevewgh added 2 commits November 14, 2018 20:28

Isolated Content Serialization continued

741b7b6

- Added Obsolete attributes - Added Serialized enum as a generic replacement of JSON enum - Updated tests to use ContentSerializer property - Updated README with XML example

Merge branch 'master' into custom-serializers

89a2c34

clairernovotny reviewed Nov 23, 2018

View reviewed changes

Refit/RefitSettings.cs Outdated Show resolved Hide resolved

Changes made in response to PR feedback

7a387ed

IContentSeriaizer - all methods made async - removed 2nd deserialize method - type safe operation ApiException - Added GetContentAsAsync<T>() method - Marked GetContentAs<T>() as obsolete