Update ActiveModelBehavior API

[billy1624]

Resolve #161

[MuhannadAlrusayni]

Great job @billy1624, there is a few things that would be cool to have in this PR.

1st) is to rollback db changes on failure.
2nd) I think we should change the error type (DbErr) to user defined error associated with ActiveModelBehavior::Error type or Box<dyn Error + Send + Sync + 'static>` which would result to make the user code cleaner, I would prefer to use user defined error type for more flexibility in the API. Something like this maybe?:

pub trait ActiveModelBehavior: ActiveModelTrait {
    Type Error: std::error::Error;

    fn after_save(self, insert: bool, db: &DatabaseConnection) -> Result<Self, Self::Error> {
        Ok(self)
    }

    // .. omitted
}

[billy1624]

1st) is to rollback db changes on failure.

This requires Transaction support and it's not merged into master yet.

2nd) I think we should change the error type (DbErr) to user defined error associated with ActiveModelBehavior::Error type or Box<dyn Error + Send + Sync + 'static>` which would result to make the user code cleaner, I would prefer to use user defined error type for more flexibility in the API.

Perhaps we can introduce DbErr::Custom then user can convert their custom errors with .map_err(|e| DbErr::Custom( ... )). And we don't have to force user define extra type def ActiveModelBehavior::Error.

#[derive(Debug)]
pub enum DbErr {
    Conn(String),
    Exec(String),
    Query(String),
    Custom(Box<dyn Error>),
}

[MuhannadAlrusayni]

Using DbError::Custom is fine pick, but I would still like the signature of ActiveModelBehavior::* to return Result<Self, Box<dyn Error>> so user code don't need to do the mapping every time they override these methods, we can do these mapping in SeaORM right ?

[MuhannadAlrusayni]

we can also have an alias for such a result type like this:

// we can also name this BehaviorResult<T> or other good name, so we can add it in the prelude 
type Result<T> = Result<T, Box<dyn Error>>;

pub trait ActiveModelBehavior: ActiveModelTrait {
    fn before_save(self, insert: bool, db: &DatabaseConnection) -> Result<Self> { .. } 

    // .. omited
}

[MuhannadAlrusayni]

Or another option is to impl<T: Error> From<T> for DbErr if possible so user can use ? inside these functions.

[billy1624]

Or another option is to impl<T: Error> From<T> for DbErr if possible so user can use ? inside these functions.

But this conversion will have a long lasting impact. Converting all errors defined in third-party crate into DbErr::Custom. Is this a good idea? @tyt2y3

I think it's okay as long as we don't change this conversion afterward. Otherwise, this will be a headache for downstream users.

[tyt2y3]

I think it is better to not provide a blanket implementation for now.

It looks good otherwise.

[billy1624]

Hey @MuhannadAlrusayni, I will not include db: &DatabaseConnection at this stage. Because...

• Now the connection is not a simple enum but an async trait. So it's complicated to use it on top of another async trait ActiveModelBehavior.
  

  sea-orm/src/database/connection.rs

  Lines 7 to 10 in 75e625f

  	#[async_trait::async_trait]
  	pub trait ConnectionTrait<'a>: Sync {
  	type Stream: Stream<Item = Result<QueryResult, DbErr>>;

• Also, doing CRUD inside ActiveModelBehavior::before_*() & ActiveModelBehavior::after_*() might cause unexpected cyclic actions.

db: &DatabaseConnection was removed at 75e625f for...

• ActiveModelBehavior::before_*()
• ActiveModelBehavior::after_*()

[billy1624]

Hey @tyt2y3, this is ready to merge. Will add unit tests for this PR together with transaction once it's merged