FireStore: Register converters on client

[Wackymax]

Currently the FireStore converters are stored inside of a static cache that can only be accessed internally by the library. Can we change it so that the cache is instead on a client and allow the user add converters to this client cache?

My reasoning for this is that currently the only way to add converters for FireStore is by adding attributes to the POCO objects, which I could prefer to avoid. One, it requires me to reference FireStore in a part of our project where I don't feel it is required. Secondly, it may make it difficult when trying to use POCO's from an external library where one does not have access to the code base to add FireStore attributes.

By putting the converter cache on the client and allowing the public to add converters opens up the possibility of writing converters without having to rely on the FireStore attributes.

[jskeet]

I'm not quite sure what you mean by "on a client". I'd be open to allowing the FirestoreDb itself to have a converter cache, if that's what you mean? (I think we'd probably still want the static cache for converters which would never change.)

There is a problem in terms of this plan - assuming it's what you meant - our ValueSerializer doesn't "know" which FirestoreDb any particular serialization operation is for. (Deserializing is okay.)

I may be able to work around this without taking a breaking change in the API, but I'll have to investigate. Could you confirm that this is what you were suggesting? (We'd then need to work out whether we can keep FirestoreDb itself immutable - we'd probably want a builder pattern to allow it to be created with a particular set of converters.)

[Wackymax]

Hi Jon, Yeah that is what I meant with client 😉.

…

On Tue, 23 Jul 2019, 09:58 Jon Skeet, ***@***.***> wrote: I'm not quite sure what you mean by "on a client". I'd be open to allowing the FirestoreDb itself to have a converter cache, if that's what you mean? (I think we'd probably still want the static cache for converters which would never change.) There is a problem in terms of this plan - assuming it's what you meant - our ValueSerializer doesn't "know" which FirestoreDb any particular serialization operation is for. (Deserializing is okay.) I *may* be able to work around this without taking a breaking change in the API, but I'll have to investigate. Could you confirm that this *is* what you were suggesting? (We'd then need to work out whether we can keep FirestoreDb itself immutable - we'd probably want a builder pattern to allow it to be created with a particular set of converters.) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#3255?email_source=notifications&email_token=ACYF27MGEJKULGSDWHJDF2TQA22ZNA5CNFSM4IF76K22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2SIIRQ#issuecomment-514098246>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACYF27MLJIG4XSZUKMHXHTLQA22ZNANCNFSM4IF76K2Q> .

[jskeet]

Great. Will investigate and see what we can do.

[Wackymax]

Hi @jskeet ,

Made some changes that I think could work. Maybe have a look?
https://github.com/Wackymax/google-cloud-dotnet/tree/feature/firestore-serialization

[jskeet]

I'll try to look in the coming week (although if I can't, it then won't be for another week as I'll be on vacation). I'm really hoping for solution that isn't that large though.

[jskeet]

@Wackymax: I've created a new draft PR which demonstrates the API change I'd prefer. It's quite similar to yours, but rather more limited in terms of changes. It doesn't have the converter registration part (or thus the implementation) but it's intended to capture the rest of what needs changing:

• ValueSerializer and IFirestoreInternalConverter methods gain an extra SerializationContext parameter
• FirestoreDb has a SerializationContext property
• Two new overloads on FieldValue.(ArrayRemove, ArrayUnion) accept a FirestoreDb first parameter, for the rare occasions where custom converters are required for those operations

The last bullet point is the only public API change at the moment, which appeals to me a lot. As we've released 1.0, we can't make any breaking changes on the API surface, which your commit currently does.

Could you have a look and see what you think? My hope would be that I could build on that existing commit. (I doubt that it will be within the next couple of weeks unfortunately, due to vacation and other work.)

[Wackymax]

Thanks @jskeet. Looks good to me 😃. Can I help with anything? This is currently a blocker for us on our project to actually use Firestore.

[jskeet]

I'll try implementing it tomorrow - it may prove quite simple, so could get a beta out before I go on vacation.

[Wackymax]

That would be really awesome! Thanks

[jskeet]

Have just remembered that my colleague who usually performs code reviews is on vacation, and I'm reluctant to force a change like this through without review - but if I implement it I can create a package that I could attach here. You could at least test that locally (with a local NuGet package source) to check that it works for you - I do understand it may mean you can't deploy it easily if your CI/CD pipeline doesn't allow local NuGet sources. If that's a big issue, I can try to find another reviewer... please let me know.

[Wackymax]

That would work just fine. Don't have to deploy the project at this point in time. Thank you!

[jskeet]

Okay, I've adding a couple of commits to #3334. You could have a look there, or just check out the sample usage below. If that looks okay to you, I'll create a nupkg file on Google Cloud Storage and make it public.

Sample usage:

FirestoreDb db = new FirestoreDbBuilder
{
    ProjectId = projectId, // Or leave null to autodetect
    ConverterRegistry = new ConverterRegistry { new EmailConverter(), new GuidConverter() }
}.Build();

[Wackymax]

Thanks Jon, looks exactly like what I was looking for! 😃

…

On Tue, 30 Jul 2019, 12:18 Jon Skeet, ***@***.***> wrote: Okay, I've adding a couple of commits to #3334 <#3334>. You could have a look there, or just check out the sample usage below. If that looks okay to you, I'll create a nupkg file on Google Cloud Storage and make it public. Sample usage: FirestoreDb db = new FirestoreDbBuilder { ProjectId = projectId, // Or leave null to autodetect ConverterRegistry = new ConverterRegistry { new EmailConverter(), new GuidConverter() } }.Build(); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3255?email_source=notifications&email_token=ACYF27IMB35KOBX3GHJDNNDQCAIORA5CNFSM4IF76K22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3DQFZY#issuecomment-516358887>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACYF27KPU5OFQUMUJ2XVAMTQCAIORANCNFSM4IF76K2Q> .

[jskeet]

Okay, the two packages are here:

https://drive.google.com/open?id=1-B1ueJP573AvKHQ0g82oGTzstSQEz6CI
https://drive.google.com/open?id=1lwPog-VdYHunOK3gq_SA7a0ttVzu6023

Please let me know if you have any problems.

[jskeet]

@Wackymax: Out of interest, have you had a chance to try this yet? Any requested changes? (I'll still want to write some documentation before merging.)

[Wackymax]

Hi Jon, I have tried it out yes. I think there are a few things that we can improve on in future to make it more extensible. I will play around with some improvements that we can maybe include in a future release and let you know? For now it works as expected and is sufficient. Thanks for prioritising this for me! 😃

…

On Thu, 01 Aug 2019, 19:31 Jon Skeet, ***@***.***> wrote: @Wackymax <https://github.com/Wackymax>: Out of interest, have you had a chance to try this yet? Any requested changes? (I'll still want to write some documentation before merging.) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3255?email_source=notifications&email_token=ACYF27LOXCJUC7QYKSTT7H3QCMMWNA5CNFSM4IF76K22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD3LKSXI#issuecomment-517384541>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACYF27KJ756SMJUMGOCMCLTQCMMWNANCNFSM4IF76K2Q> .

[jskeet]

Great - glad it's working for you. Feedback as early as possible is great, to avoid things that can't be done in a non-breaking way :)

[Wackymax]

Hi @jskeet,

So I've been playing around with the changes and it works now when serializing objects. But I am hitting a snag on the deserialization and was wondering if you could offer some advice?

I created a GUID converter and registered that and it works fine. I also a generic object converter to convert some of our domain objects like this:

public class FirestoreObjectConverter<T> : IFirestoreConverter<T>
    where T : new()
    {
        public object ToFirestore(T value)
        {
            return typeof(T)
                        .GetProperties(BindingFlags.Public | BindingFlags.Instance)
                        .ToDictionary(propertyInfo => propertyInfo.Name, propertyInfo => propertyInfo.GetValue(value));
        }
        public T FromFirestore(object value)
        {
            var dictionary = value as Dictionary<string, object>;
            Debug.Assert(dictionary != null, nameof(dictionary) + " != null");
            var poco = new T();
            foreach (var propertyInfo in typeof(T)
                                         .GetProperties(BindingFlags.Public | BindingFlags.Instance)
                                         .Where(prop => dictionary.ContainsKey(prop.Name)))
            {
                propertyInfo.SetValue(poco, dictionary[propertyInfo.Name]);
            }

            return poco;
        }
    }
}

On application startup I then load up all our domain objects that will be used in Firestore in a similar manner to this:

var registry = new ConverterRegistry {new FirestoreGuidConverter()};

            foreach (var publicType in Assembly.GetExecutingAssembly()
                                               .GetTypes()
                                               .Where(type => type.IsClass && type.IsPublic &&
                                                              type.GetConstructors()
                                                                  .Any(info => info.GetParameters().Length == 0)))
            {
                var converter =
                    Activator.CreateInstance(typeof(FirestoreObjectConverter<>).MakeGenericType(publicType));
                var genericMethod = typeof(ConverterRegistry).GetMethod("Add").MakeGenericMethod(publicType);
                genericMethod.Invoke(registry, new[] {converter});
            }

            return registry;

I am doing that as the objects can get nested quite a bit and we have a lot of different domain objects to store.

This all works great for serialization with the Firestore library going though the converters to convert all the types correctly. The issue I have now is that on deserialization I get a dictionary of the top level object, but because I don't have the serialization context I can't deserialize my nested properties using the Firestore converters I registered. Any ideas on how I can go about doing this?

[jskeet]

Hi @Wackymax, I'm on vacation until next Thursday, but I'll look then. Thanks for providing detailed feedback!

[jskeet]

Okay, I'm back from holiday now. Hmm. So effectively you want to be able to deserialize recursively using custom converters - which works naturally with serialization.
I'd rather not expose the deserialization context directly - if we do that, it makes it far harder to evolve later on.

Rather than do involving all this reflection when trying to reason about this, would it make sense for me to put together an example of what I think you're trying to do, just with two custom converters (an "outer" one and an "inner" one)? I can then try to work out how to get that working, and we can take it from there.

[Wackymax]

Hi @jskeet,
That essentially that is what I am trying to do yes. Currently it seems like the Firestore serialization is very rigid and not open to be extensible and customization. Which is what I would like to do. I think Json.Net has a really good implementation of serialization/deserialization so we can probably draw some inspiration from there.

Here is the best example I can give of what our entities look like more or less

public class OuterClass
{
	public Guid SomeId {get;set;}
	public string SomeProperty {get;set;}
	public InnerClass SomeInnerClass {get;set;}
}

public class InnerClass
{
	public string SomeProperty {get;set;}
	public AnotherInnerClass AnotherInnerClass {get;set;}
}

public class AnotherInnerClass
{
	public IInnerInterface IInnerInterface {get;set;} //Can be either Implementation1 or Implementation2
}

public interface IInnerInterface
{
	public IInnerInterfaceType Type {get;}
}

public enum InnerInterfaceType
{
	Implementation1,
	Implementation2
}

public class Implementation1 : IInnerInterface
{
	public InnerInterfaceType Type => Implementation1;
	public string MyString {get;set;}
}

public class Implementation2 : IInnerInterface
{
	public InnerInterfaceType Type => Implementation2;
	public int MyInt {get;set;}
}

Unfortunately the product we are building is multi-cloud, with us using SaaS services by the various cloud providers, like Firebase. For that reason I don't want to for instance attribute Firebase specific classes if we are running our product on Azure, and vice versa. Hope this makes sense?

[jskeet]

Compared with other platforms, the .NET support for serialization is rather more customizable than most, between support for anonymous types, attributed types, attribute-on-type converters, attribute-on-property converters, and now the custom converter registries. I don't think Json.NET is a good comparison here: that's a library designed solely for serialization and deserialization. That's all it tries to do. The Firestore library is a library that includes serialization as part of its job, but I don't want the serialization aspect to drown everything else (which it's already in danger of doing).

One thing you might want to do is have a completely separate "model to Firestore" conversion layer that doesn't involve the Firestore library at all, beyond attributes. In other words, separate out your logical model from your storage model. There'll be duplication that way, for sure, but you would keep the storage model "clean" in terms of being just data with no behavior at all.

I'll have a look at whether we can cleanly recurse somehow, but I really don't want to make this much more complex. As I say, this is a Firestore client library that has serialization aspects, not a serialization library that happens to talk to Firestore as well.

[Wackymax]

So I can agree on that. I also don't think it should try to solve all the potential serialization/deserialization scenarios. I do think however it should have the support for someone to add that functionality if they want to, which is what I am looking for. Currently the mantra seems to be use our serialization or don't use our SDK at all.

I also looked at creating separate Firestore models for this, but there is simply to many of those for it to be practical here. On that though, we have decided to not use as much of Firestore anymore due the serialization limitations and are now exploring other means of storing our information. Given that I don't know if you should be spending that much time on this anymore?

But thanks for the awesome support so far! It is a really great experience having such open communication!

[jskeet]

I do think however it should have the support for someone to add that functionality if they want to, which is what I am looking for.

Making serialization/deserialization completely open-ended is just asking for problems in terms of trying to maintain a stable API in the long term. If you want to perform your own serialization completely, you can absolutely do that - pass in a Dictionary<string, Value> as your document, and we'll do pretty much nothing. But what you're really looking for isn't doing all the serialization - it's doing some of the serialization, and that's where the interface between the library and the application becomes very complicated very quickly. While we may expose more complexity over time, we've only recently released 1.0, and moving to a more complex system that is less flexible in terms of changes we may want to make later, based on just a few customers, feels like a bad way to go. I would rather be a lot more cautious about progress - which is why I don't mind the converter registry approach, as it's relatively limited.

I can understand the frustration, but I hope you understand that the more we try to make it perfect for your specific use case, the more we may be harming future customers who have slightly different requirements - or our ability to add new features later on. (For example, any time we expose a public interface, we can never add a new member to it without a new major version. Whereas if it's internal, we can do what we like to support more functionality like additional sentinel attributes.)

I'll still look into this scenario to see whether there's a way of supporting it in a relatively painless way, but thanks for letting me know you won't be using Firestore anyway, so I can timebox my efforts on this front.

[Wackymax]

Makes sense from your point of view as well, and I can understand and appreciate that. I do hope that the library can evolve to support more advance scenarios in the future, but this is a specific use case on our side at the moment. Thanks for taking the time to look at it.

We are still using Firebase, just in a very limited sense and for a much smaller use case. And we are doing the mapping to Firebase specific models for those cases. So at the end of the day this isn't a train smash.