schotime / NPoco

Simple microORM that maps the results of a query onto a POCO object. Project based on Schotime's branch of PetaPoco
Apache License 2.0
848 stars 302 forks source link

Snapshotter does not return changes to properties with generic types #613

Closed Coyne closed 3 years ago

Coyne commented 4 years ago

During our upgrade from NPoco 2.10.11 to NPoco 4.0.3 we noticed the saving of certain properties were not done upon updating. After further investigation we came to the following conclusion:

We were using the snapshot mechanism to only update columns that have actually changed to the database, passing the changes the snapshot gives back as the second parameter of our UpdateAsync. Our object we were trying to save to the database looked like this:

public class SomeObject
{
    public int Id { get; set; }
    public string Name { get; set; }
    public OtherObjectReference OtherObject { get; set; }
}

And OtherObjectReference was mapped correctly, and looked like this:

public class OtherObjectReference
{
    public OtherObjectReference(int id)
    {
        Id = id;
    }
    public int Id { get; }
}

What we noticed is, that in NPoco 2.10.11, the OtherObject property would be a change according to the snapshot, and thus be updated in the database. In NPoco 4.0.3 however, the snapshot no longer returned OtherObject property as a changed property, even though the Id property of the OtherObjectReference actually changed from 3 to 5.

We narrowed this problem down to be in the FastJsonColumnSerializer, where apparently the default value for ShowReadOnlyProperties is set to false, causing the serialization of OtherObjectReference to like like an empty object: {}.

We have now fixed this by just setting it to true manually during startup, but the way this is hidden from the end user through the DatabaseFactory made troubleshooting the problem really hard.

My suggestion would be to be able to pas a serializer to the DatabaseFactory as we do with the config and so on, and then end users will actually need to be aware about these options, instead of them being hidden away...

schotime commented 4 years ago

You're gonna have to provide more info that unfortunately.

Coyne commented 4 years ago

You're gonna have to provide more info that unfortunately.

Hey Adam, I'm sorry, we had a family emergency and I had to run off in a hurry. Everything is OK again, and I've updated the item so you have a little more context as to what the problem is ;-)

schotime commented 3 years ago

This has been add in the latest 5.1.2. thx