Wanted breaking changes in sea_orm

[Expurple]

Breaking changes that I want to see in SeaORM 2.0.

Reuse ExprTrait where it makes sense

Background and motivation

• Unify `Expr` and `SimpleExpr`, add `ExprTrait`? sea-query#771
• Another motivating example: Add support for ESCAPE clause in LIKE operator #2546

Make ActiveEnum: Into<SimpleExpr>

• See Import missing ExprTrait #2606 (comment)

Delete "operator" methods from ColumnTrait, make ColumnTrait: Into<SimpleExpr>

• TODO

We won't be able to write a blanket

impl<T> ExprTrait for T
where
    T: ColumnTrait

Because there's already a very meaningful blanket

impl<T> ExprTrait for T
where
    T: Into<SimpleExpr>

So, we'll have to use supertraits to achieve a similar effect. I think, we should have ColumnTrait: Into<SimpleExpr>. This makes sense on its own, and also makes ColumnTrait automatically implement ExprTrait (via the existing blanket impl above).

Method signatures and compatibility

The current "operator" methods on ColumnTrait accept (&Self, Into<Value>), but ExprTrait methods accept (Self, Into<SimpleExpr>). I think, the latter is correct (see #771 as to why). But these differences may cause implementation trouble for us and migration trouble for the users. I need to experiment and see how it goes.

Automatically qualify columns

We should try to implement these methods such that foo::Column::Bar.equals(foo::Column::Baz) automatically includes the table name in the generated SQL: foo.bar = foo.baz. See SeaQL/sea-query#681.

Remove Into* traits in favor of From/Into

• Explanation of existance of `IntoSimpleExpr` instead of `Into<SimpleExpr>` #2327
• Also, there's TryIntoModel that should be replaced with TryFrom/TryInto.

To keep some backwards-compatibility, we can make IntoFoo: Into<Foo> and blanket impl IntoFoo for T where T: Into<Foo>, instead of removing these traits outright. However, I'd still replace all IntoFoo uses in our APIs with Into<Foo>. And we need to document on every IntoFoo that it's a legacy trait kept for compatibility. We can deprecate these sometime later in 2.x., to make the 2.0 upgrade less annoying.

on_empty_do_nothing() by default

• on_empty_do_nothing() should be enabled by default #2528

  DONE: Insert many #2628

Handle bulk inserts above the SQLx parameter limit

• See Easier batching of bulk inserts #710 (reply in thread). We could silently add a workaround and generate a different query (it's debatable whether this is a breaking change), or we could do a full-on breaking change and add a mandatory enum parameter to control the behavior.

Move one and all into a trait

• See Exists #730 (reply in thread)

Mark some enums an #[non_exhaustive]

The motivation is described in SeaQL/sea-query#795.

Which enums:

• In sea_orm: DatabaseBackend, DatabaseConnection, errors. Adding new variants of these shouldn't be a breaking change.
• All enums in sea_orm_cli. Adding new CLI options shouldn't be a breaking change.
• All enums in sea_orm_codegen. Adding new options or errors shouldn't be a breaking change.
• TODO: also check structs will all-public fields. See Add is_partial for pg index sea-schema#150 (comment)

Experiment with new async features

• Migrate to async closures. #2576 (also independently reported on Discord)
• Maybe we can also re-evaluate the use of async_trait ???

Upgrade the public dependencies

• sqlx 0.9.0 is not released yet, but see Upcoming SQLx Breakage: TransactionManager trait reexport is being deleted in 0.9.0 #2600 and branch 0.9.0-dev in sqlx.
• We should also check other public dependencies like ipnetwork. Something like crates VSCode extension can be used to quickly see new major versions of dependencies.

---

Related plans in sea_query

• Wanted breaking changes in `sea_query` sea-query#795

[tyt2y3]

Thank you. I've decided - let's do it! We can start promoting sea-query to 1.0 rc and then merge in all the breaking changes,
and promote sea-orm to 2.0 rc as well.
I'd make one more sea-orm 1.2 release before that though.

[Expurple]

I've decided - let's do it!

Great!

We can start promoting sea-query to 1.0 rc and then merge in all the breaking changes

Are you starting the 1.0-rc process right now? Should I get started with the breaking pull requests? Or are you going to start it sometime later and ping me, and I wait until then?

sea_query 1.0 (instead of 0.33) makes a lot of sense, since it basically has to be as stable as sea_orm. Sure, you could release it independently and break it more often. But in practice, it's very commonly used through sea_orm. And sea_orm would be stuck on a non-latestsea_query branch, either stagnant or supported/backported with extra effort. I wouldn't want that.

I'd make one more sea-orm 1.2 release

What's the distinction from 1.1.x? Honestly, sea_orm version numbers are unpredictable and don't make sense to me, since it doesn't follow semver. 1.1.0 (a minor release) included breaking changes (sqlx bump), and 1.1.x patch releases regularly include new features. If deviations from semver are intentional, you should document this somewhere and be at least self-consistent.

[Expurple]

Are you starting the 1.0-rc process right now? Should I get started with the breaking pull requests?

Got a response here: SeaQL/sea-query#795 (comment)

Everyone can start working on the breaking changes now!

[Huliiiiii]

https://docs.rs/sea-orm/latest/sea_orm/query/trait.QuerySelect.html#method.join_as

We should also make such methods as generic as possible.

[Expurple]

Do you have any specific problems / ideas / suggestions?

I guess, the separate join_*_rev methods aren't really needed, because we have already have RelationDef::rev for that. But I wouldn't delete these methods just yet, as having these around isn't a global architectural flaw to be fixed ASAP. To smooth things out for the users, we can deprecate these first, document the recommended migration towards RelationDef::rev, and finally delete sometime in the future.

In theory, the separate join_as method could be absoleted as well, if we added something like RelationDef::to_alias (similar to RelationDef::from_alias). But to_alias can be less obvious / readable / discoverable than join_as, which reads directly like the corresponsing JOIN ... AS ... in the generated SQL. I'm more in favor of just adding to_alias without removing join_as. And then we can document on join_as that .join_as(join_type, relation_def, alias) is the same as .join(type, relation_def.to_alias(alias))

[Huliiiiii]

I think it might be better if these methods accepted Into<RelationDef> or impl RelationTrait. There are a lot of repeated def calls in the actual code.

Similarly, there are many repeated clone calls on aliases, and IntoIden will create a new SeaRc. If we make Alias ​​itself an SeaRc, it may reduce some overhead.

I am also confused as to why we need to use DynIden. I checked the code and most of the time it's just match enum to &'static str. I think it's possible to create a Cow<'static, str> wrapper instead.

[Huliiiiii]

As mentioned in #2615, I think it would be better if these methods returned a Result instead of panic.

[tyt2y3]

Addressed in #2627 and #2630

[Huliiiiii]

As mentioned in #2614, I suggest removing the delete method on ModelTrait and instead adding the following implementation:

impl<T> ModelTrait for &T
where
  T: ModelTrait

This helped to reduce a lot of redundant code and simplified the features I planned to add next. For example, instead of having to implement:

impl LoaderTrait for &[Option<ModelTrait>];
impl LoaderTrait for Vec<Option<ModelTrait>>;
impl LoaderTrait for Vec<Option<&ModelTrait>>;
impl LoaderTrait for [&Option<ModelTrait>];

We can just write:

impl LoaderTrait for Vec<Option<ModelTrait>>;
impl LoaderTrait for &[Option<ModelTrait>];

[Expurple]

Where else should the delete method go? It looks ok to me. Maybe we can just make it accept &self by reference? Deleting a record from the database doesn't have to imply that we also immediately delete it from memory*.

*Unless there's some implied invariant that a Model is always backed by a row in a database. But that's nonsense. It's Clone, it's publicly-constructible, and I do construct "synthetic" Models in my codebase.

By the way, please consider documenting LoaderTrait and converting (some of) its tests into doctests! I didn't have a fun time trying to understand LoaderTrait for the first time right now.

[Huliiiiii]

delete is actually a shorthand for into_active_model().delete()

[tyt2y3]

@Expurple shall we use anyhow::Error in place of String in DbErr? good or bad idea?

[Expurple]

What's your motivation for this change?

String seems OK to me, because it's inside of an enum, and not a "standalone" error type that we return. If we did that, then sure, it would be inconvenient to propagate (doesn't implement Error, doesn't auto-convert into anyhow::Error, etc).

Box<dyn Error + Send + Sync + 'static> would be OK too. If someone wanted to match and propagate that specific error variant, it's more convenient than a string. But I'm not aware of such cases, so it probably doesn't matter.

I wouldn't use anyhow::Error specifically. It's an unnecessary dependency that doesn't bring anything useful in this case. It's useful as a function's return type that everything auto-converts into. I'm not aware of any benefits when using it in an enum field. Maybe backtraces?

[tyt2y3]

I mean using it in the error variants: DbErr::Type(String) -> DbErr::Type(anyhow::Error)
I think the biggest motivation is size: 24 vs 8 bytes.

[Expurple]

The size difference only matters if:

• there's a real-world performance problem
• and DbErr doesn't have any other large variants that can cause it to be 24 bytes or larger.

But DbErr is currently 56 bytes. Sure, you could box every other variant too. But are you sure that it's justified, that the size will keep being tested and controlled, and that we won't end up with SeaQL/sea-query#863 again?

I think, it's probably not worth it at all. My models in Result<Model, DbErr> are hundreds of bytes anyway (larger than the error). I actually want to reduce the mentioned boxing of Value variants, just to make the API easier to use. It's probably more important

[zrll12]

I think the current Model, ActionModal and Entity are a little bit confusing.

• Creating a new row needs to use ActionModal
• Using select with needs to use Column

I seggest

• Add a builder function in Entity
• Add a column function in Entity
• Or make these structure together

[tyt2y3]

ActiveModel builder is now added:

let user = user::ActiveModel::builder()
    .set_name("Bob")
    .set_email("[email protected]")
    .set_profile(profile::ActiveModel::builder().set_picture("image.jpg"))
    .add_post(
        post::ActiveModel::builder()
            .set_title("Nice weather")
            .add_tag(tag::ActiveModel::builder().set_tag("sunny")),
    )
    .save(db)
    .await?;

COLUMN constant is also added:

fruit::Entity::find()
    .filter(fruit::Entity::COLUMN.name.contains("Orange"))
    .all(db)
    .await?;

[sfried]

One breaking change that I would like to see is to use a separate newtype(i32) (created by the DeriveEntityModel) for each table's PK. In this way, an id for a table can only be used as the PK for that table (or appropriate FK in another table), to further reduce the chance for bugs.

[Expurple]

Yeah, I thought about this too. A configurable option for this would be nice. It doesn't even have to be a breaking change if we keep the old behavior the default. Can you create a separate feature request for this?

[sfried]

Created here: #2861

[Huliiiiii]

Any thoughts on removing with-prelude from the CLI?

I found it unintuitive and inconsistent from the moment I first started using it, and it made the learning curve steeper: In other languages, entity classes typically include the functionality of our Entity, Model, and ActiveModel, and I’ve seen several users get confused when the Entity couldn’t do what they expected.

Also, regardless of how things are generated, you still have to use table::{Model, ActiveModel} when working with other structs.

[Expurple]

I agree that the prelude isn't very useful. I never use it.

As for removing it, I'm not so sure that the clarity is worth the breakage in this case

[Huliiiiii]

Atleast, we can deprecate it and add relevant documentation.