supabase-community / postgrest-csharp

A C# Client library for Postgrest
https://supabase-community.github.io/postgrest-csharp/api/Postgrest.html
MIT License
114 stars 22 forks source link

Not supplying value to column auto inserts 0 values #38

Closed warflash closed 2 years ago

warflash commented 2 years ago

Bug Report

Describe the bug

Not supplying a value to a column that is defined in a table results in the value being set to 0. I can see this being some kind of error prevention mechanism but columns like "created_at" become pretty hard to handle due to this behaviour. Given that both the zero value as well as column name have to be sent to the supabase instance also adds quite a bit of network overhead that could be avoided.

Reproduction

Add a column to a table, when creating the object don't supply an object to the column and upsert the entry.

System Info

acupofjose commented 2 years ago

@warflash this one is a little more complicated. Would love some feedback!

Two solutions with (what I see) are huge problems:

  1. You can set a default value to a column by specifying one in the model, for example:
class Example extends BaseModel {
   [Column("created_at")]
   public DateTime CreatedAt {get; set;} = DateTime.Now;
}

The positive, a new model instance will always have a default value, the negative, new parsed models will always have a default value and that may, at times, be incorrect.

  1. What happens if I want to explicitly null a column?

As far as determining which columns should be inserted/updated, the simplest way would be through null checks, but there are often times that one needs to insert/update a column to a null value. So that doesn't seem to be the answer either.


Potential solution?

New Attribute:

enum UseValue {
  Always,
  OnInsertAndUpsert
}

class Example extends BaseModel {
   [Column("other"), DefaultValue(15, UseValue.Always)]
   public int Other {get; set;}

   [Column("created_at"), DefaultValue(DateTime.Now, UseValue.OnInsertAndUpsert)]
   public DateTime CreatedAt {get; set;}
}

Seems like that structure would address the problems above - thoughts?

warflash commented 2 years ago

As far as determining which columns should be inserted/updated, the simplest way would be through null checks, but there are often times that one needs to insert/update a column to a null value. So that doesn't seem to be the answer either.

That is true, I did not think of that. What a rabbit hole... It's funny how easy it is to not include certain props in javascript since you can just not include the prop in the json, and set the props value to null in case you want to update the column to null. Due to C# setting a props value to null regardless if it was passed when creating or omited alltogether such a behaviour seems very difficult to model.

For my personal usecase, and generally speaking for people who don't need to update their column to null, there is the option to set NullValueHandling = NullValueHandling.Ignore in JsonSerializerSettings. I just need to figure out how to actually pass those settings to my supabase/postgrest instance .

For the issue I tried to explain I frankly don't think setting DefaultValue is the solution since even if you don't initialize a property it will still be set to the default - overwriting columns in the db. UseValue.OnNotNull might partially solve that, since that way you can decide on a per column basis on how to handle null values and you don't have to straight up omit all null properties in the serializer.

Something like the Conditional Property Serialization Newtonsoft provides sounds like it could be useful here, I just couldn't think of a way to differentiate between implicit nulls by not providing the property on construction - and explicit nulls by passing null to the property.

To kind of show exactly what I'm looking for and to maybe make things easier to reference I drew this sketch real quick, I hope it makes sense^^ image

acupofjose commented 2 years ago

@warflash tossing an answer tonight just if it’s helpful, I can look more thoroughly tomorrow.

but postgrest-sharp just wraps Newtonsoft’s json.net. So you should be able to specify serializer settings in the client (it’s in ClientOptions) or even add the NullValueHandling directly on the model and it should be respected!

acupofjose commented 2 years ago

@warflash I've set it up so that a Column on a BaseModel can specify its NullValueHandling policy.

Given:

[Table("kitchen_sink")]
public class KitchenSink : BaseModel
{
    [Column("datetime_value")]
    public DateTime? DateTimeValue { get; set; }

    [Column("datetime_value_1", NullValueHandling = NullValueHandling.Ignore)]
    public DateTime? DateTimeValue1 { get; set; }
}

The following test was written:

var client = Client.Initialize(baseUrl);
var now = DateTime.UtcNow;
var model = new KitchenSink
{
    DateTimeValue = now,
    DateTimeValue1 = now
};

var insertResponse = await client.Table<KitchenSink>().Insert(model);

Assert.AreEqual(now, insertResponse.Models[0].DateTimeValue);
Assert.AreEqual(now, insertResponse.Models[0].DateTimeValue1);

// Should update Null based on Column Attribute
insertResponse.Models[0].DateTimeValue = null;

// Should be ignored on update based on Column Attribute
insertResponse.Models[0].DateTimeValue1 = null;

var updatedResponse = await client.Table<KitchenSink>().Update(insertResponse.Models[0]);

Assert.IsNull(updatedResponse.Models[0].DateTimeValue);
Assert.IsNotNull(updatedResponse.Models[0].DateTimeValue1);
warflash commented 2 years ago

Hey @acupofjose, just got back from my holidays, hope you had a great start into 2022 :) Thanks alot for the update on the issue! The snippets do look really nice, will try to take a deeper look and test it out tomorrow👌