Fix IntoActiveValue for Option

[YoshieraHuang]

Related to #322

[tyt2y3]

@billy1624 thoughts?

[billy1624]

Given the original model

#[derive(Clone, Debug, PartialEq, DeriveEntityModel)]
#[sea_orm(table_name = "fruit")]
pub struct Model {
    #[sea_orm(primary_key)]
    pub id: i32,
    pub name: String,
    pub cake_id: Option<i32>,
}

Some of the possible mappings are...

1. Mapping T to T

   • Handled by

     impl IntoActiveValue<T> for T {
         fn into_active_value(self) -> ActiveValue<T> {
             Set(self)
         }
     }

   • Well handled!

2. Mapping Option<T> to T

   • Handled by

     impl IntoActiveValue<T> for Option<T> {
         fn into_active_value(self) -> ActiveValue<T> {
             match self {
                 Some(value) => Set(value),
                 None => Unset(None),
             }
         }
     }

   • Well handled!

3. Mapping Option<Option<T>> to Option<T>

   • Handled by

     impl IntoActiveValue<Option<T>> for Option<Option<T>> {
         fn into_active_value(self) -> ActiveValue<Option<T>> {
             match self {
                 Some(value) => Set(value),
                 None => Unset(None),
             }
         }
     }

   • Well handled!

4. Mapping Option<T> to Option<T>

   • Handled by

     impl IntoActiveValue<T> for Option<T> {
         fn into_active_value(self) -> ActiveValue<T> {
             match self {
                 Some(value) => Set(value),
                 None => Unset(None),
             }
         }
     }

   • Wait! Seems wrong...
     See 7cfd010 and CI log

     #[derive(DeriveIntoActiveModel)]
     pub struct NewFruit {
         pub cake_id: Option<i32>,
     }
     
     // This is fine
     assert_eq!(
         my_fruit::NewFruit {
             cake_id: Some(1),
         }
         .into_active_model(),
         fruit::ActiveModel {
             id: Unset(None),
             name: Unset(None),
             cake_id: Set(Some(1)),
         }
     );
     
     assert_eq!(
         my_fruit::NewFruit {
             cake_id: None,
         }
         .into_active_model(),
         fruit::ActiveModel {
             id: Unset(None),
             name: Unset(None),
             // Isn't this `Set(None)`?
             cake_id: Unset(None),
         }
     );

   • This could be resolved by adding... but it confused the compiler
     See 962b2fa and CI log

     impl IntoActiveValue<Option<T>> for Option<T> {
         fn into_active_value(self) -> ActiveValue<Option<T>> {
             match self {
                 Some(value) => Set(Some(value)),
                 None => Set(None),
             }
         }
     }

[billy1624]

May I ask @acidic9 for help? Since he is the original author of it

[tyt2y3]

Thanks @billy1624 for the investigation. So there are different possibilities. In a nutshell,

1. ActiveValue<Option<T>> -> Option<Option<T>>, can be Set(T), Set(NULL) or Unset
2. ActiveValue<T> -> T: always map to Set(T)
3. ActiveValue<T> -> Option<T>: can be Set(T) or Unset, but not Set(NULL)
4. Option<T> -> Option<T>: Set(T) or Set(NULL) (this does not currently exist, and might be what the issue is asking for)

So I think there is a semantic ambiguity, but probably not a bug.

[tqwewe]

#[derive(Clone, Debug, PartialEq, DeriveEntityModel)]
#[sea_orm(table_name = "fruit")]
pub struct Model {
    #[sea_orm(primary_key)]
    pub id: i32,
    pub foo: String,
    pub bar: Option<i32>,
}

#[derive(DeriveIntoActiveModel)]
pub struct UpdateFruit {
    pub name: String,
    pub price: f64, // Sets bar to `Set(Some(price))`
}

#[derive(DeriveIntoActiveModel)]
pub struct UpdateFruit {
    pub name: String,
    pub price: Option<f64>, // Sets bar to `Set(price)`
}

#[derive(DeriveIntoActiveModel)]
pub struct UpdateFruit2 {
    pub name: String,
    pub price: Option<Option<f64>>, // Sets bar to `Set(Some(price))` or `Set(None`
}

I think this is the ended behaviour of the API I had in mind when writing this.. they are not all working correctly?
I haven't had time to clone this PR and test it, but I thought I'd share what I was expecting my implementation to do.

[YoshieraHuang]

Hi, thanks for your concerns. After reading comments, I agree that it is not a bug, it is just an ambiguity. I think the ambiguity originates from the nullability of fields in Model. For example:

#[derive(Clone, Debug, PartialEq, DeriveEntityModel)]
#[sea_orm(table_name = "fruit")]
pub struct Model {
    #[sea_orm(primary_key)]
    pub id: i32,
    pub non_nullable: String,
    pub nullable: Option<String>
}

The current code is reasonable for nullable field:

1. Some(Some(String)) -> Set(Some(String))
2. Some(None) -> Set(None)
3. Some(String) -> Set(Some(String))
4. None -> Unset(None)

It works fine now .1 and 2 are achieved by impl IntoActiveValue<Option<T>> for Option<Option<T>>, whiile 3 and 4 are achieved by impl IntoActiveValue<Option<T>> for Option<T>.

There are some possible desired mapping from UpdateXXX to ActiveModel for non_nullable field:

1. String -> Set(String), the string is required and should be set to field. Ok, works fine for now.
2. None -> Unset(None). the string is not required. Let the field untouched. Ok, also fine.
3. Some(String) -> Set(String). the string is not required but it is given, the field has to be set. Uhh, seems wrong. The type mismatches.

The 3rd of the mapping of non_nullable field conflicts with the 3rd of nullable field. However, now there is no tools to tell from each other.

[tyt2y3]

Yes, it appears our type system cannot properly express the idea of required but nullable.

Say, we have a ActiveValue and ActiveValueNullable instead of relying on ActiveValue<Option<T>>, then there would not be such ambiguity. But it seems to require a major overhaul. And it might be a bad idea. There could be smarter ways.

[tyt2y3]

Oh, there is the Deserialize macro.

So, from a serde_json standpoint, can we actually enforce the schema of required but nullable?

Seems not! Because it maps both undefined and null to None.

This thread might be helpful serde-rs/serde#984

There may not be a good derive macro solution to handle this automatically for now.

[billy1624]

I will revisit this after

• Rework ActiveValue #340

[tyt2y3]

Close it for now, we need to look for a better solution

[kun1988]

Close it for now, we need to look for a better solution

Have a better solution now? Thanks. @tyt2y3

[franklx]

I managed to implement partial updates using a custom simple Option-like type:
franklx@2585894
After testing and refinements I'll open a PR.

[LockedThread]

I wish there was a better solution for this