Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[SeaORM] migration guide for SeaQuery users #72

Open
billy1624 opened this issue Nov 15, 2022 · 8 comments
Open

[SeaORM] migration guide for SeaQuery users #72

billy1624 opened this issue Nov 15, 2022 · 8 comments

Comments

@billy1624
Copy link
Member

Proposed by SeaQL/sea-orm#1227 (comment)

Content

I think there are two types of use cases.

Mapping query result into struct

This serve as a way to execute any SeaQuery statement. And the query result of select statement can be mapped into a struct in Rust.

Basically, as mentioned in this cookbook article: https://www.sea-ql.org/sea-orm-cookbook/003-run-sea-query-statement-on-sea-orm.html

Full SeaORM with access to alter the SeaQuery statement

Setup SeaORM entity with sea-orm-cli or defining it manually.

Then, You can get a mutable reference of the SeaQuery select statement from sea_orm::Select by QuerySelect::query method. With that you can construct complex query.

@billy1624
Copy link
Member Author

Ideas? @tyt2y3

@tyt2y3
Copy link
Member

tyt2y3 commented Nov 17, 2022

I think the change in type system from SeaQuery to SeaORM is worth mentioning.

In SeaQuery, the type system is lower-level:

#[derive(Iden)]
enum Cake {
    Table,
    Id,
    Name,
}

Query::select().column(Cake::Name).from(Cake::Table).and_where(Expr::col(Cake::Id).eq(1))

In SeaORM, Entity (hence table name) is a separate type from columns:

#[derive(DeriveEntity, ..)]
#[sea_orm(table_name = "cake")]
pub struct Entity;

#[derive(DeriveActiveModel, ..)]
pub struct Model {
    pub id: i32,
    pub name: String,
}

Cake::find().filter(cake::Column::Id.eq(1))

Resulting in more concise code and a nicer experience.

@billy1624
Copy link
Member Author

Related discussion on Discord https://discord.com/channels/873880840487206962/900758302294704188/1045647323075710998

(Just a note to self)

@nitnelave
Copy link

Quoting from discord for posterity and people without an account:

if you're interested in seeing a sea-query -> sea-orm migration in the wild, here's the migration for LLDAP: lldap/lldap#382
It contains a bit more than just the migration (while I was elbow deep in the code it was hard to avoid fixing up little things here and there, and now it's annoying to separate again), but it covers a good chunk of the API: Relations, Links, queries at the level of sea-orm, sea-query, raw SQL, implementing traits for better types in the DB (TryGetable, PrimaryKey, deriving FromQueryResult, ...), find_with_related, find_also_linked, in_subquery and so on.
I've hit a couple of pain points here and there (the need for aliases when using find_also_linked, the lack of find_with_linked, the implicit order_by introduced by find_with_related, the fact that you can't derive FromQueryResult for structs with other structs as fields, and so on), but overall the migration went well. I am very glad that I had a pretty good test suite though, otherwise it would have been a disaster.

Some notes:

  • If you're using custom types (or type wrappers) in the DB, the entity generation will not know about them (it works from the DB), so the generated models have the "wrong" types (the raw representation rather than the semantic Rust type).
  • Custom types require a quite verbose setup: https://github.com/nitnelave/lldap/pull/382/files#diff-27e26a3b85b610666d7dd0a3fa3e848582ab2d3999f7b1bdfed61052f82af741R96-R168 Could we derive ValueType for instance? TryGetable when there's a TryFrom or From implementation? Not require TryFromU64 for primary keys?
  • Sometimes you also need to implement Nullable : https://github.com/nitnelave/lldap/pull/382/files#diff-27e26a3b85b610666d7dd0a3fa3e848582ab2d3999f7b1bdfed61052f82af741R291 or IntoActiveValue https://github.com/nitnelave/lldap/pull/382/files#diff-27e26a3b85b610666d7dd0a3fa3e848582ab2d3999f7b1bdfed61052f82af741R297
  • Deriving something extra on the column enum (e.g. Debug/PartialEq/Eq for storage and comparison) requires expanding the entity macro into its components (see server/src/domain/model/users.rs for instance).
  • Maybe we need some extra documentations about what an inverse relation is and when to use it.
  • I don't know if that's possible, but it would be nice to have the Column, ActiveModel and so on as inner types to the entity (at least as aliases), so we can have a single alias pub use model::users::Entity as User; and then use User::find() and User::Column::UserId. Or that can be an extra encompassing type that just forwards the find() function and related.
  • Maybe automatically derive the required columns from the model, at least when it's simple enough? I don't know to what extend that's possible. Or at least have a shortcut .select_only_columns(&[UserColumn::UserId, UserColumn::DisplayName]) for simple queries to make writing them easier.
  • find_with_related will introduce a order_by on the primary key. If you want to order by something else in the base table, you have to add the order_by before. Sorting by something on the second table will presumably break the aggregation.
  • It would be really nice to have find_with_related work with a model looking like:
struct UsersAndGroups {
  user: User,
  groups: Vec<Group>,
}

Finally, it would be nice if the entity generation would warn about column types that are not compatible with other DBs (I'm thinking of e.g. u8 with postgres).

@tyt2y3
Copy link
Member

tyt2y3 commented Jan 2, 2023

Custom types require a quite verbose setup

SeaQL/sea-orm#1349 tracking issue created

Deriving something extra on the column enum

I think we should simply add PartialEq, Eq derives to Column

I don't know if that's possible, but it would be nice to have the Column, ActiveModel and so on as inner types to the entity (at least as aliases)

I also wanted it (and tried many things) back then, and finally I believed that it cannot be done. rust-lang/rust#8995 might be relevant

find_with_related will introduce a order_by on the primary key

This is due to how the aggregator currently works. It works by iterating through the rows without using a hashmap to resolve the association. If we change the implementation and uses a hashmap, we can drop this constraint.

However it is currently blocked by SeaQL/sea-orm#1254

It would be really nice to have find_with_related ...

This PR already made it possible SeaQL/sea-orm#1238 . At least it is very easy to write a wrapper function to construct the UsersAndGroups from Vec<User>, Vec<Vec<Group>>

Maybe automatically derive the required columns from the model

SeaQL/sea-orm#1311 this might instead do what you want?

Maybe we need some extra documentations about what an inverse relation is and when to use it.

Well received

tyt2y3 added a commit that referenced this issue Jan 2, 2023
@tyt2y3
Copy link
Member

tyt2y3 commented Jan 3, 2023

I think we should simply add PartialEq, Eq derives to Column

The actual reason we didn't add these is that it conflicts with ColumnTrait::eq which unfortunately will be a pain if we rename it to something else at this point

@nitnelave
Copy link

Thanks for the reply!

find_with_related will introduce a order_by on the primary key

This is due to how the aggregator currently works. It works by iterating through the rows without using a hashmap to resolve the association. If we change the implementation and uses a hashmap, we can drop this constraint.

I'm not necessarily in favor of switching to a hashmap (it sounds like it would probably be slower?) but it should at least be documented that it introduces an implicit order_by. Although it's true that a hashmap would make things easier on the API. Maybe expose both ways? e.g. add a find_with_related_ordered method or something like that, and replace the find_with_related with a hashmap.

SeaQL/sea-orm#1311 this might instead do what you want?

This looks great :)

@nitnelave
Copy link

One small note on SeaQL/sea-orm#1238: It's nice that it's possible, but it requires another round-trip to the DB, where the query could be done all at once.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants