Firestore: more flexible serialization

[pauloevpr]

I am having some issues trying to get Firestore API to convert between StringValue and DDD value objects which are fully convertible from/to string. Let me give an example:

[FirestoreData]
public class UserAccount
{
     [FirestoreProperty]
     public Email EmailAddress { get; set; }
}
public class Email 
{ 
     // Email has a TypeConverter implementation that allow full conversion from/to string
     // Email also has implicit operators to allow direct conversion from/to string
}

When calling snapshot.ConvertTo<UserAccount>(), ValueDeserializer fails with System.ArgumentException: 'Unable to convert value type StringValue to Email'.

It seems ValueDeserializer ignores type converters and implicit operators completely.

[pauloevpr]

The issue is the same for ValueSerializer.

[jskeet]

I definitely don't want to support TypeConverter - that would have to be for the net45 target only (as it's not in netstandard1.5), which could cause some really subtle issues. Adding a dependency on an extra package could change which version of Google.Cloud.Firestore is used, suddenly breaking code.

While we could support the implicit conversion, it does introduce extra complexity which I'd really rather not - as per our previous discussions. (If there are multiple implicit conversions, which one is used? How does inheritance play into this? What about explicit conversions?) Now that private properties are supported, it's easy to work around this though with no library change:

[FirestoreData]
public class UserAccount
{
    public Email EmailAddress { get; set; }

    // Private property to serialize the data for Firestore
    [FirestoreProperty("email")]    
    private string FirestoreEmail { get => EmailAddress; set => EmailAddress = value; }
}

[pauloevpr]

Thank you for your response.

I wasn't aware of net45 dependency of TypeConverter. I will agree with you on this. Keeping a low netstandard dependency is definitely something I support.

I will also agree that implicit conversation is not a good solution either.

Since this is clearly a feature request, I would like to propose a more generic solution to handle this and other scenarios where some sort of custom serialization is required:

Provide a generic interface like IFirestoreValueSerializer:

public interface IFirestoreValueSerializer 
{
     Value SerializeValue(object value);
     object DeserializeValue(Value value);
}

Allow FirestorePropertyAttribute to receive a Type which is an implementation of IFirestoreValueSerializer. So consumers can set a custom serialization mechanism for a specific property:

[FirestoreData]
public class UserAccount
{
    [FirestoreProperty("Email", FirestoreValueSerializerType = typeof(MyValueSerializer))]    
    public Email Email { get; set; }
}

[jskeet]

That sounds reasonable - I suspect there'll need to be a lot of warnings about not reusing Value instances etc, but it will at least be pretty flexible. I'll need to check that the Firestore team is happy with this plan as well.

I suspect the DeserializeValue method will want to take a target type as well, which would be the declared type of the property.

It's unlikely that I'll get to this for about a month I'm afraid due to other commitments - but at least the workaround should help you out for the moment.

[pauloevpr]

I am happy with the workaround for now. Thank you.

Btw, are pull requests welcome on this repository?

[jskeet]

Yes, although I may well not be able to prioritize reviewing a PR any higher than writing the code itself, I'm afraid :(

[pauloevpr]

Any updates on this case?

[jskeet]

No updates at the moment, other than that I need to write a doc to present to the Firestore team about this. (We're not developing in a vacuum here - it gives us slightly less flexibility, but will help with consistency.)

I do have that on my work backlog, but Spanner work is higher priority right now.

[pauloevpr]

Thank you for the feedback.

[jskeet]

Right, we've made some progress in internal discussions, and we'd like some feedback if possible.

We'd like the interface to use POCOs rather than protos, so that users don't have to know anything about protos at all. At first glance, that would suggest an interface like this:

public interface IFirestoreValueSerializer 
{
     object SerializeValue(object value);
     object DeserializeValue(object value);
}

However, I suspect it's actually going to be cleaner to view this as an interface that you use to decorate a type rather than a property. I've written a prototype with this interface instead:

/// <summary>
/// Converter used during serialization
/// </summary>
public interface IFirestoreConverter<T>
{
    /// <summary>
    /// Converts a value to its Firestore representation.
    /// </summary>
    /// <param name="value"></param>
    /// <returns></returns>
    object ToFirestore(T value);

    /// <summary>
    /// Converts a value from its Firestore representation.
    /// </summary>
    /// <param name="value"></param>
    /// <returns></returns>
    T FromFirestore(object value);
}

The user code might look like this:

[FirestoreData]
public sealed class CustomUser
{
    [FirestoreProperty]
    public int HighScore { get; set; }

    [FirestoreProperty]
    public string Name { get; set; }

    [FirestoreProperty]
    public Email Email { get; set; }
}

[FirestoreData(Converter = typeof(EmailConverter))]
public sealed class Email
{
    public string Address { get; }

    public Email(string address) => Address = address;
}

public class EmailConverter : IFirestoreConverter<Email>
{
    public Email FromFirestore(object value)
    {
        switch (value)
        {
            case null: return null;
            case string address: return new Email(address);
            default: throw new ArgumentException($"Unexpected data: {value.GetType()}");
        }
    }

    public object ToFirestore(Email value) => value?.Address;
}

The document would just be a map with two string values and one integer value. Types with custom converters could only be used as document "root" types if they serialized to dictionaries, but that's probably a reasonable restriction.

To be clear, this is not a promise that this is the direction we'll take, but it's an initial prototype just for feedback. Do you believe this would satisfy your requirements?

[pauloevpr]

Hi John, thank for your feedback.

I will follow you on this one. Decorating a type rather than every property of that type is much cleaner. This would completely satisfy my requirements.

Regarding IFirestoreConverter, I was wondering if T FromFirestore(object value); should have a Value parameter instead of object. It seems easier to work with Value.

[jskeet]

The intention is that users shouldn't need to know about Value at all. (The Firestore team is very keen on this.) Instead, you'd just deal with regular .NET types - the "natural" deserialization of each Value, including Dictionary<string, object> for maps etc.

It's possible that we could add an option to the interface saying "Just pass me the value, I'll do everything" but that would probably come later.

[pauloevpr]

It makes sense. I am happy with this solution.

[jskeet]

Hooray. I'll keep this issue updated with progress. (Even if the API is exactly as written, my current prototype implementation is decidedly not production-ready, or comprehensively tested!)

Thanks for all the feedback, helping to make the API better for all users.

[pauloevpr]

Thank you.

[jskeet]

PR #2734 is the first pass of the "real" version of this.

[jskeet]

Implemented in #2739 and #2734. I'll cut a new release soon (hopefully today) and update this issue accordingly.

[jskeet]

Available in version 1.0.0-beta14.

[pauloevpr]

Great! Thank you very much. I will give it a try next week.