SeaQL / sea-orm

🐚 An async & dynamic ORM for Rust
https://www.sea-ql.org/SeaORM/
Apache License 2.0
7.3k stars 513 forks source link

Trait conflict when implementing `IntoActiveModel<A>` outside of entity model definition #547

Closed fairingrey closed 2 years ago

fairingrey commented 2 years ago

Description

Trying to implement IntoActiveModel<A> outside of my entity model definition gives the following error:

  --> src/actions.rs:45:1
   |
45 | impl IntoActiveModel<user_account::ActiveModel> for UpdateUser {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
   = note: conflicting implementation in crate `entity`:
           - impl IntoActiveModel<entity::user_account::ActiveModel> for <entity::prelude::UserAccount as sea_orm::EntityTrait>::Model;

I can't use DeriveIntoActiveModel either since I'm unaware how to specify (via attribute or etc) the specific ActiveModelTrait type that my type converts into since I define it outside of my crate.

Steps to Reproduce

// this is in entity::user_account::Model
#[derive(Clone, Debug, PartialEq, DeriveEntityModel, Serialize, Deserialize)]
#[sea_orm(table_name = "user_account")]
pub struct Model {
    #[sea_orm(primary_key, auto_increment = false)]
    pub id: Uuid,
    pub created_at: DateTimeWithTimeZone,
    pub updated_at: DateTimeWithTimeZone,
    #[sea_orm(column_type = "Text")]
    pub name: String,
    pub email: String,
    #[sea_orm(column_type = "Text")]
    pub password: String,
    pub verified: bool,
}

// this exists in a separate, main crate where I perform other logic
#[derive(Debug)]
pub(crate) struct UpdateUser {
    pub(crate) name: Option<String>,
    pub(crate) email: Option<String>
}

impl IntoActiveModel<user_account::ActiveModel> for UpdateUser {
    fn into_active_model(self) -> user_account::ActiveModel {
        user_account::ActiveModel {
            name: sea_orm::IntoActiveValue::<_>::into_active_value(self.name).into(),
            email: sea_orm::IntoActiveValue::<_>::into_active_value(self.email).into(),
            ..::std::default::Default::default()
        }
    }
}

Expected Behavior

I expect it to be able to convert my type (defined outside of the entity crate) into an ActiveModel that I can use to update rows in my table.

Actual Behavior

Doesn't work as explained above.

Reproduces How Often

Always reproducible.

Versions

❯ cargo tree | grep sea-
│   ├── sea-orm v0.6.0
│   │   ├── sea-orm-macros v0.6.0 (proc-macro)
│   │   ├── sea-query v0.21.0
│   │   │   ├── sea-query-derive v0.2.0 (proc-macro)
│   │   ├── sea-strum v0.23.0
│   │   │   └── sea-strum_macros v0.23.0 (proc-macro)
├── sea-orm v0.6.0 (*)

Database is Postgres 13.5, OS is ArchLinux 5.16.9-zen1-1-zen

Additional Information

Might be related to #322, #323, #340.

koptan commented 2 years ago

facing the same issue :

#[derive(Serialize, Deserialize, Debug)]
pub struct RegistartionResquest {
    pub id: String,
    pub email: String,
    pub password: String,
    pub password_confirmation: String,
}
impl IntoActiveModel<users::ActiveModel> for RegistartionResquest {
    fn into_active_model(self) -> users::ActiveModel {
        let user = users::ActiveModel {
            id: Set(self.id),
            email: Set(self.email),
            password: Set(self.password),
            ..Default::default()
        };
        return user;
    }
}

error message :

conflicting implementations of trait `sea_orm::IntoActiveModel<entity::users::ActiveModel>` for type `modules::user::models::RegistartionResquest`
conflicting implementation in crate `entity`:
- impl IntoActiveModel<entity::users::ActiveModel> for <entity::users::Entity as sea_orm::EntityTrait>::Model;
koptan commented 2 years ago

@fairingrey if you define your implementation of trait in inside entity create the error will gone.

I am still new to rust, but think this is related to orphan rule I guess.

billy1624 commented 2 years ago

Hey @fairingrey, welcome!

The UpdateUser struct should have two non-null fields since both of them are required in the original Model.

So, we have...

#[derive(Debug, DeriveIntoActiveModel)]
pub(crate) struct UpdateUser {
    pub(crate) name: String,
    pub(crate) email: String,
}
billy1624 commented 2 years ago

Hey @koptan, you manage to solve the errors by moving the trait implementation inside entity crate?

fairingrey commented 2 years ago

Hey @fairingrey, welcome!

The UpdateUser struct should have two non-null fields since both of them are required in the original Model.

So, we have...

#[derive(Debug, DeriveIntoActiveModel)]
pub(crate) struct UpdateUser {
    pub(crate) name: String,
    pub(crate) email: String,
}

Thanks for the welcome :slightly_smiling_face:

I know that works, but maybe more related to #322 -- Is it not possible for UpdateUser to act as a sort of changeset? i.e. if name is None then the active value for it should be unchanged. I'm using this for a PUT endpoint, so some values should stay the same.

EDIT: Will also mention that moving UpdateUser did fix the first issue I had, so thank you koptan

fairingrey commented 2 years ago

Just as another question but in https://github.com/SeaQL/sea-orm/blob/master/src/entity/active_model.rs#L573-L599

I've patched it for my use case by including the following:

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

But I'm wondering if or why it wouldn't make sense. Is NotSet intended for updating with NULL values? If so, then why does ActiveValue::Unchanged(V) require a value?

billy1624 commented 2 years ago

But I'm wondering if or why it wouldn't make sense. Is NotSet intended for updating with NULL values? If so, then why does ActiveValue::Unchanged(V) require a value?

NotSet is intended to represent a undefined state. You could say it's a NULL. Unchanged takes sea_query::Value because it represent the state of a value. This info is useful for us to determine which attributes are updated and has to be included in SQL UPDATE statement. Also, to actively set a column to NULL you will need to set Unchanged with sea_query::Value::null(), e.g. NULL integer would be sea_query::Value::Integer(None).

billy1624 commented 2 years ago

I know that works, but maybe more related to #322 -- Is it not possible for UpdateUser to act as a sort of changeset? i.e. if name is None then the active value for it should be unchanged. I'm using this for a PUT endpoint, so some values should stay the same.

With the patch you made, I think it's good enough for your current use case. Btw... #492 might be helpful to you.

fairingrey commented 2 years ago

NotSet is intended to represent a undefined state. You could say it's a NULL. Unchanged takes sea_query::Value because it represent the state of a value. This info is useful for us to determine which attributes are updated and has to be included in SQL UPDATE statement. Also, to actively set a column to NULL you will need to set Unchanged with sea_query::Value::null(), e.g. NULL integer would be sea_query::Value::Integer(None).

With the patch you made, I think it's good enough for your current use case. Btw... #492 might be helpful to you.

I see. Regarding the patch, I'll definitely take a look at that (as I would like not to resort to my own patches if possible). Thanks for the quick responses!

fairingrey commented 2 years ago

I updated this issue to better reflect my original problem. You're free to close it if this is not the intended way of using the library, although it might be worth mentioning in the guides e.g. here

koptan commented 2 years ago

Hey @koptan, you manage to solve the errors by moving the trait implementation inside the entity crate?

Hi @billy1624 , Yes when I moved the trait implementation into entity create I was able to solve this issue.

I think this is happening because we already have an implicit implementation for IntoActiveModel with Marco and when I am trying to define my own implementation it's breaking the orphan rules I guess: https://github.com/Ixrec/rust-orphan-rules

Again, I am still new to rust so maybe my feedback is so wrong.

baoyachi commented 2 years ago

Hey @koptan, you manage to solve the errors by moving the trait implementation inside the entity crate?

Hi @baoyachi, Yes when I moved the trait implementation into entity create I was able to solve this issue.

I think this is happening because we already have an implicit implementation for IntoActiveModel with Marco and when I am trying to define my own implementation it's breaking the orphan rules I guess: https://github.com/Ixrec/rust-orphan-rules

Again, I am still new to rust so maybe my feedback is so wrong.

Oh,Seems to be at the wrong person. @koptan

koptan commented 2 years ago

@baoyachi sorry my bad, I will modify the mention.

onichandame commented 2 years ago

But I'm wondering if or why it wouldn't make sense. Is NotSet intended for updating with NULL values? If so, then why does ActiveValue::Unchanged(V) require a value?

NotSet is intended to represent a undefined state. You could say it's a NULL. Unchanged takes sea_query::Value because it represent the state of a value. This info is useful for us to determine which attributes are updated and has to be included in SQL UPDATE statement. Also, to actively set a column to NULL you will need to set Unchanged with sea_query::Value::null(), e.g. NULL integer would be sea_query::Value::Integer(None).

@billy1624 I am still very confused. Shouldn't Set be used for updating values? I would imagine Set(None) will update the optional field to null instead of Unchanged(<something>) because Unchanged represents an unchanged operation.

billy1624 commented 2 years ago

@billy1624 I am still very confused. Shouldn't Set be used for updating values? I would imagine Set(None) will update the optional field to null instead of Unchanged(<something>) because Unchanged represents an unchanged operation.

Yes, Set should be used for updating values and Unchanged represents an unchanged operation.

Did you spot something odd in the source code of SeaORM? Please point out the lines if there is any :)