sea-orm icon indicating copy to clipboard operation
sea-orm copied to clipboard

Blanket IntoActiveValue implementations so custom db types are supported

Open wdcocq opened this issue 3 years ago • 8 comments

PR Info

Currently it doesn't seem possible to use DeriveIntoActiveModel with a field with Option<Option<CustomDbType>> or Option<CustomDbType> because the IntoActiveValue<Option<CustomDbType>> trait is missing.

error[E0277]: the trait bound `std::option::Option<std::option::Option<Language>>: IntoActiveValue<_>` is not satisfied
   |
10 |     Clone, Default, Debug, Hash, PartialEq, Eq, DeriveIntoActiveModel, Serialize, Deserialize,
   |                                                 ^^^^^^^^^^^^^^^^^^^^^ the trait `IntoActiveValue<_>` is not implemented for `std::option::Option<std::option::Option<Language>>

Because of the orphan rule you can't implement IntoActiveValue<Option<CustomDbType>> for Option<CustomDbType>. You can for just CustomDbType of course.

error[E0117]: only traits defined in the current crate can be implemented for types defined outside of the crate
   |
17 | impl IntoActiveValue<Option<Language>> for Option<Option<Language>> {
   | ^^^^^----------------------------------^^^^^-------------------------
   | |    |                                      |
   | |    |                                      `std::option::Option` is not defined in the current crate
   | |    `std::option::Option` is not defined in the current crate
   | impl doesn't use only types from inside the current crate

Changes

By changing the Option<T> and Option<Option<T>> from being implemented by the impl_into_active_value macro, to blanket implementations this will work for every type that implements IntoActiveValue<T>


impl IntoActiveValue<Language> for Language {
    fn into_active_value(self) -> ActiveValue<Language> {
        ActiveValue::set(self)
    }
}

Will make it possible to use DeriveIntoActiveModel on structs with a Option<Option<CustomDbType>> field

wdcocq avatar Jul 02 '22 09:07 wdcocq

I agree that we should support Option<CustomDbType> but what's the usecase for Option<Option<<CustomDbType>>> ?

tyt2y3 avatar Jul 02 '22 10:07 tyt2y3

see the discussion here: https://github.com/SeaQL/sea-orm/pull/323 It's to be able to set a field to NULL in the database

wdcocq avatar Jul 02 '22 10:07 wdcocq

Ah thank you for the context

tyt2y3 avatar Jul 02 '22 10:07 tyt2y3

Sorry but I need more information on exactly what you are trying to do that does not compile, and how this change fixes it now.

It'd be great if you can add a test case somewhere under tests/features. It will also ensure that it will be well maintained in the future

tyt2y3 avatar Jul 02 '22 10:07 tyt2y3

Sorry i linked the wrong issue, i changed it in my previous comment

wdcocq avatar Jul 02 '22 10:07 wdcocq

Ah I see, well it's a complicated topic - type system! Definitely needed some test cases then

tyt2y3 avatar Jul 02 '22 10:07 tyt2y3

There doesn't seem to be any current test cases around DeriveIntoActiveModel where i can plug in..

For more context, this currently doesn't work:

#[derive(Debug, Clone, DeriveIntoActiveModel)]
pub struct User {
    pub name: Option<String>,
    pub email: Option<Option<String>>,
    pub language: Option<Option<Language>>,
}

because of the error:

error[E0277]: the trait bound std::option::Option<std::option::Option<Language>>: IntoActiveValue<_> is not satisfied

where Language is:


#[derive(Hash, Eq, Debug, Clone, PartialEq, EnumIter, DeriveActiveEnum, Serialize, Deserialize)]
#[sea_orm(rs_type = "String", db_type = "Enum", enum_name = "language")]
pub enum Language {
    #[sea_orm(string_value = "en_US")]
    EnUs,
    #[sea_orm(string_value = "fr_FR")]
    FrFr,
    #[sea_orm(string_value = "nl_NL")]
    NlNl,
    ...
}

If really necessary i can still add some tests

wdcocq avatar Jul 02 '22 10:07 wdcocq

Because any downstream implemented IntoActiveValue for Option<V> and Option<Option<V>> will be forbidden.

Is this breaking though, as you already can't do this anyway? Unless I'm missing something :)

Yes, please! Adding more test cases would be appreciated :D

I already added a test that should test whether it compiles with custom types (and some primitives). Not sure what else to add for this PR that's not testing the functionality of IntoActiveValue itself, which this PR isn't really targeting.

wdcocq avatar Aug 27 '22 12:08 wdcocq