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

Compact Entity format #105

Closed
nappa85 opened this issue Aug 24, 2021 · 65 comments · Fixed by #122
Closed

Compact Entity format #105

nappa85 opened this issue Aug 24, 2021 · 65 comments · Fixed by #122

Comments

@nappa85
Copy link
Contributor

nappa85 commented Aug 24, 2021

I was trying your crate, seems promising, but the way you choose to represent models is too verbose.
For example I've a DB table with 53 fields, using your system I've written like 210 LOC, repeating the same informations like 3 times.

I think everything could be done with a single proc_macro, that takes a standard Rust struct and replaces it with a module with the same name of the struct, with Model struct with more or less the same contents of the original struct, Column enum with structs fields as variants, automatic impl ColumnTrait for Column with a type mapping, where the mapping fails you can use a field notation to override it (comes handy for custom types), table name and primary key can be specified with a struct notation, what's missing?
Relations, well, this have to be specified externally, but you can point the enum with another notation, and if the notation isn't present you generate a void enum.

After that, if I can add something more, I often use Cow on my entities, because this way I'm not forced to clone Strings when updating/creating. With your system I can't, because Model doesn't support a lifetime. It's not a big problem, but it blocks also other custom types with a lifetime

@tqwewe
Copy link
Contributor

tqwewe commented Aug 24, 2021

I haven't thought of this before, but I think I can agree with you somewhat.

For example, Entity, Model, Column, PrimaryKey, PrimaryKeyTrait & ColumnTrait could be combined into a single struct definition with derives or a single SeaORM derive.
From:

#[derive(Copy, Clone, Default, Debug, DeriveEntity)]
pub struct Entity;

impl EntityName for Entity {
    fn schema_name(&self) -> Option<&str> {
        Some("public")
    }

    fn table_name(&self) -> &str {
        "users"
    }
}

#[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel)]
pub struct Model {
    pub id: Uuid,
    pub created_at: DateTimeWithTimeZone,
    pub updated_at: DateTimeWithTimeZone,
    pub username: String,
    pub password: String,
    pub email: Option<String>,
}

#[derive(Copy, Clone, Debug, EnumIter, DeriveColumn)]
pub enum Column {
    Id,
    CreatedAt,
    UpdatedAt,
    Username,
    Password,
    Email,
}


#[derive(Copy, std::clone::Clone, std::fmt::Debug, EnumIter, DerivePrimaryKey)]
pub enum PrimaryKey {
    Id,
}

impl PrimaryKeyTrait for PrimaryKey {
    fn auto_increment() -> bool {
        false
    }
}

#[derive(Copy, Clone, Debug, EnumIter)]
pub enum Relation {}

impl ColumnTrait for Column {
    type EntityName = Entity;
    fn def(&self) -> ColumnDef {
        match self {
            Self::Id => ColumnType::Uuid.def(),
            Self::CreatedAt => ColumnType::Timestamp.def(),
            Self::UpdatedAt => ColumnType::Timestamp.def(),
            Self::Username => ColumnType::String(None).def(),
            Self::Password => ColumnType::String(None).def(),
            Self::Email => ColumnType::String(None).def().null(),
        }
    }
}

Into:

#[derive(Clone, Debug, PartialEq, SeaORM)]
#[schema_name = "public"]
#[table_name = "coupons"]
pub struct Model {
    #[primary_key]
    #[auto_increment = false]
    pub id: Uuid,
    pub created_at: DateTimeWithTimeZone,
    pub updated_at: DateTimeWithTimeZone,
    pub username: String,
    pub password: String,
    pub email: Option<String>,
}

I think this wouldn't be too hard as a lot of information could be inferred from the struct.
For example:

  • If a field is an Option<T>, then .null() would be added in the ColumnTrait
  • The Column enum could be generated from the model's fields

Obviously with this comes a lack of customizability. But this could be solved by allowing manual implementations where needed perhaps?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 24, 2021

Thank you for your interest. I will first talk about the original ideas and choices, and may add more thoughts on later post.

The first thing is, I do not want to invent any DSL or a custom syntax or semantics. It will definitely be much less verbose, but it will be hard to understand and reason about, and people don't want to learn a new micro-language. Sooner or later, someone will have a need to customise the behaviour and hack the thing, and it will be much easier to do so in the plain Rust language.

(Just a small use case, how can I dynamically remap the table name based on environment variables?)

If there are tricks to reduce the verbosity of the Entity file without countering the above, I'll definitely adopt.

A lesser motivation would be code completion. List out the essential symbols and traits will be helpful.

But after all it is about how much information we want to present on plain sight. Too much it is hard to digest, too little it will be hard to see-through.

@nappa85
Copy link
Contributor Author

nappa85 commented Aug 24, 2021

Yes, I understand your point of view, and it was exactly the reply I thought for the question "why didn't they already made this?".
But, as @acidic9 pointed, there can be default impl via derive macro and custom impl in plain Rust without any problems.
As far as I can see, code completion works with macro-produced code without problems.
Macros aren't made to replace code, but to easy the life of who writes it.

For example, serde has a derive macro for Serialize and Deserialize, but anyone can manually derive the traits for non-standard situations, and there are lots of docs about it.

I would prefer a serde-like approach, where the default is easily reached with a macro, and non-default-fitting situations can be covered in plain Rust.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 24, 2021

I also think that having to use both CamelCase and snake_case when referring to attributes is slightly awkward.

Is it better or worse if we break the Rust convention and use snake_case enums?

@tqwewe
Copy link
Contributor

tqwewe commented Aug 24, 2021

I don't think so honestly, I think keeping the enum as CamelCase... and just renaming it through the derive macro. And the derive macro would just generate an implementation of std::string::ToString

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 24, 2021

Serde is a brilliant library and there is definitely a lot to learn and borrow from.

Yes I would prefer to have a more compact form, as long as it is compatible with the longer form.

And that the derive macro should not do too much magic other than providing syntax sugars.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021

Also, it'd be better if we settle on a 'base format' first, and versions afterwards to be backward compatible.
That way, we can incrementally release enhancements towards the 'short format', while the codegen can maintain the capability to generate both formats.

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021

@acidic9 the format you propose already looks good. Perhaps we just need to think about the relations.

@nappa85
Copy link
Contributor Author

nappa85 commented Aug 25, 2021

I've put together a minimal impl, have a look https://github.com/nappa85/sea-orm-macro/
Inside tests there is a working example.
Sure there is space for improvement, but it can be a starting point

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021

I've put together a minimal impl, have a look https://github.com/nappa85/sea-orm-macro/
Inside tests there is a working example.
Sure there is space for improvement, but it can be a starting point

Wow thank you for the quick update. Looks great so far. I was just thinking how to name it (lol) perhaps, AutoEntity, EntityDef, or simply Entity?

@nappa85
Copy link
Contributor Author

nappa85 commented Aug 25, 2021

Wow thank you for the quick update. Looks great so far. I was just thinking how to name it (lol) perhaps, AutoEntity, EntityDef, or simply Entity?

I've named it AutoColumn because it's main purpose is to generate Column from Model, but it generates optionally also Entity and PrimaryKey, so maybe a more generic AutoDef?

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021

Yeah AutoDef sounds good too.

@tqwewe
Copy link
Contributor

tqwewe commented Aug 25, 2021

I feel like AutoDef is a little ambiguous. For example, serde just has Serialize and Deserialize. I like the sound of Schema or Entity or Table

@tyt2y3
Copy link
Member

tyt2y3 commented Aug 25, 2021

Well, Entity is the most almighty I guess.

@tyt2y3 tyt2y3 changed the title There must be a less verbose way to do the same thing More compact Entity format Aug 26, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Sep 2, 2021

@nappa85 we are releasing 0.2.0 soon.
Since you were working on this, do you think you will be able to contribute a bit in the coming month?
Such that we may incorporate and release in 0.3.0

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 2, 2021

Yes, if you let me know what you want me to edit, I'll do it ASAP

Here we discussed only the derive macro name

@tqwewe
Copy link
Contributor

tqwewe commented Sep 2, 2021

@tyt2y3 I spent most of my day yesterday and today creating a PR for this too. It's at a working stage, just need the columntrait to be generated too.

I'll create the PR in a few hours!

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 2, 2021

@tyt2y3 I spent most of my day yesterday and today creating a PR for this too. It's at a working stage, just need the columntrait to be generated too.

I'll create the PR in a few hours!

Thank you. After 0.2.0 we will make our focus on this matter, with 0.3.0 on the horizon of early Oct.

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 2, 2021

Yes, if you let me know what you want me to edit, I'll do it ASAP

Here we discussed only the derive macro name

Great, good to know. Thank you for the suggestion and everything up to now.

@tqwewe
Copy link
Contributor

tqwewe commented Sep 2, 2021

I've created a PR for this #122

If @nappa85 's version is preferred thats fine with me, just thought I'd share the work as a PR in case it can be used or to share any thoughts.

Let me know what you guys think

@tyt2y3 tyt2y3 added this to the 0.3.0 milestone Sep 2, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Sep 2, 2021

I've created a PR for this #122

If @nappa85 's version is preferred thats fine with me, just thought I'd share the work as a PR in case it can be used or to share any thoughts.

Let me know what you guys think

Thank you so much for the big PR. I think yours are very promising!

@nappa85 What's your view on this?

@acidic9 Can you show some more examples on the proposed format and annotation syntax (even if they are not implemented yet)?

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 2, 2021

I'm not telling my version in better than @acidic9 one, but I'm already using mine in my projects, it's almost complete, at least for my use cases, and I made almost no changes since I created it 8 days ago

@tqwewe
Copy link
Contributor

tqwewe commented Sep 2, 2021

@nappa85 I didn't get a chance to get an example working with your code, I'm not sure how exactly to use it. Do you think you could briefly explain what the API looks like for your version?

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 2, 2021

@nappa85 I didn't get a chance to get an example working with your code, I'm not sure how exactly to use it. Do you think you could briefly explain what the API looks like for your version?

you can find a working example in the tests folder of my repo https://github.com/nappa85/sea-orm-macro/blob/master/tests/it_works.rs

I can write some docs once we decided what has to be changed

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 3, 2021

Thank you for everyone's input here. We will think a bit and perhaps draft a design document.

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 3, 2021

I've wrote a little readme to better explain the proc macro: https://github.com/nappa85/sea-orm-macro/blob/master/README.md

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 6, 2021

Yes, I think we should use the nullable method for consistency.

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 6, 2021

Yes, I think we should use the nullable method for consistency.

Perfect, so the macro code is already aligned

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 7, 2021

Thank you everyone for everything!
I am now working on it. Hopefully will have something out soon.

@tyt2y3 tyt2y3 mentioned this issue Sep 7, 2021
@tyt2y3
Copy link
Member

tyt2y3 commented Sep 7, 2021

I am done for today. Feel free to peek at the current status.

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 7, 2021

I am thinking, whether DeriveEntityModel should automatically derive DeriveModel and DeriveActiveModel, to make it less verbose.

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 7, 2021

I am thinking, whether DeriveEntityModel should automatically derive DeriveModel and DeriveActiveModel, to make it less verbose.

Could be a good idea

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 8, 2021

I am thinking, whether DeriveEntityModel should automatically derive DeriveModel and DeriveActiveModel, to make it less verbose.

@billy1624 can you help a bit on this?

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 8, 2021

I am thinking, whether DeriveEntityModel should automatically derive DeriveModel and DeriveActiveModel, to make it less verbose.

I've made some tests about it, maybe I'm wrong, but it seems that a derive macro can't edit the original struct, to do so you need a proc-macro.

I've tried with this code, inside DeriveEntityModel macro:

    attrs.push(Attribute {
        pound_token: Pound(Span::call_site()),
        style: AttrStyle::Outer,
        bracket_token: Bracket(Span::call_site()),
        path: Path {
            leading_colon: None,
            segments: {
                let mut temp = Punctuated::new();
                temp.push(PathSegment {
                    ident: Ident::new("derive", Span::call_site()),
                    arguments: PathArguments::None,
                });
                temp
            },
        },
        tokens: quote! { DeriveModel, DeriveActiveModel },
    });

@tqwewe
Copy link
Contributor

tqwewe commented Sep 8, 2021

Rather than modifying the derive attribute of the struct itself, it's probably better to just reuse the code of DeriveModel and DeriveActiveModel.

For example, if they both use the struct approach, then you could just call expand them directly.

struct DeriveEntityModel {
    derive_model: DeriveModel,
    derive_active_model: DeriveActiveModel,
    ...
}

impl DeriveEntityModel {
    fn new(input: syn::DeriveInput) -> syn::Result<Self, Error> {
        ...
    
        Ok(DeriveEntityModel {
            derive_model: DeriveModel::new(input)?,
            derive_active_model: DeriveActiveModel::new(input)?,
            ...
        })
     }
     
     fn expand(&self) -> syn::Result<TokenStream> {
         let expanded_derive_model = self.derive_model.expand()?;
         let expanded_derive_active_model = self.derive_active_model.expand()?;
         ...
         
         Ok(TokenStream::from_iter([
            expanded_derive_model,
            expanded_derive_active_model,
            ...
        ]))
     }
}

I hope that makes sense.

@billy1624
Copy link
Member

Oh I just committed 54bb358 for this, feel free to comments :)

@tqwewe
Copy link
Contributor

tqwewe commented Sep 8, 2021

Ah yea you did the same as I explained haha, just call the function directly.

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 8, 2021

Oh I just committed 54bb358 for this, feel free to comments :)

Are we ok with the macro always deriving the other macros?
All other features, like deriving PrimaryKey, were controlled by an attribute.

@billy1624
Copy link
Member

I am thinking, whether DeriveEntityModel should automatically derive DeriveModel and DeriveActiveModel, to make it less verbose.

I've made some tests about it, maybe I'm wrong, but it seems that a derive macro can't edit the original struct, to do so you need a proc-macro.

I've tried with this code, inside DeriveEntityModel macro:

    attrs.push(Attribute {
        pound_token: Pound(Span::call_site()),
        style: AttrStyle::Outer,
        bracket_token: Bracket(Span::call_site()),
        path: Path {
            leading_colon: None,
            segments: {
                let mut temp = Punctuated::new();
                temp.push(PathSegment {
                    ident: Ident::new("derive", Span::call_site()),
                    arguments: PathArguments::None,
                });
                temp
            },
        },
        tokens: quote! { DeriveModel, DeriveActiveModel },
    });

This is crazy loll

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 8, 2021

This is crazy loll

Yeah, it was a really raw approach

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 9, 2021

pub struct Model {
#[sea_orm(primary_key)]
pub id: i32,
pub name: String,
#[sea_orm(column_type = "Text", nullable)]
pub notes: Option<String>,
}

@nappa85 I discovered a strange behavior. when column_type is specified, nullable can no longer be inferred from the Option<_> type

So the following is correct:

#[sea_orm(column_type = "Text", nullable)]
pub notes: Option<String>,

But the following is wrong:

#[sea_orm(column_type = "Text")]
pub notes: Option<String>,

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 9, 2021

#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
pub enum Relation {
#[sea_orm(
belongs_to = "super::bakery::Entity",
from = "Column::BakeryId",
to = "super::bakery::Column::Id",
on_update = "Cascade",
on_delete = "Cascade"
)]
Bakery,
}

Our current DeriveRelation is quite 'copy & paste' now.

I am thinking if instead of

#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
pub enum Relation {
    #[sea_orm(
        belongs_to = "super::bakery::Entity",
        from = "Column::BakeryId",
        to = "super::bakery::Column::Id",
        on_update = "Cascade",
        on_delete = "Cascade"
    )]
    Bakery,
}

we do

#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
pub enum Relation {
    #[sea_orm(
        belongs_to = "bakery",
        from = "bakery_id",
        to = "bakery.id",
        on_update = "cascade",
        on_delete = "cascade"
    )]
    Bakery,
}

But is this 'too much'?

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 9, 2021

@nappa85 I discovered a strange behavior. when column_type is specified, nullable can no longer be inferred from the Option<_> type

Yes, it's kind-of wanted, IMHO if you're specifying the type, you've to specify it entirely

@nappa85
Copy link
Contributor Author

nappa85 commented Sep 9, 2021

Our current DeriveRelation is quite 'copy & paste' now.

I am thinking if instead of

#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
pub enum Relation {
    #[sea_orm(
        belongs_to = "super::bakery::Entity",
        from = "Column::BakeryId",
        to = "super::bakery::Column::Id",
        on_update = "Cascade",
        on_delete = "Cascade"
    )]
    Bakery,
}

we do

#[derive(Copy, Clone, Debug, EnumIter, DeriveRelation)]
pub enum Relation {
    #[sea_orm(
        belongs_to = "bakery",
        from = "bakery_id",
        to = "bakery.id",
        on_update = "cascade",
        on_delete = "cascade"
    )]
    Bakery,
}

But is this 'too much'?

I think it's becoming too much magic, what if the other entity isn't on the the parent module but it's on another crate?
The first version is better, more explicit

@tqwewe
Copy link
Contributor

tqwewe commented Sep 9, 2021

I agree that it's becoming a little bit too much magic. Perhaps the entire DeriveRelation can just be scrapped as it just implements RelationTrait..? I think it just makes sense at this point to let the user write their own implementation of it.

@tyt2y3
Copy link
Member

tyt2y3 commented Sep 9, 2021

Yes, it's kind-of wanted, IMHO if you're specifying the type, you've to specify it entirely

Understood

I agree that it's becoming a little bit too much magic. Perhaps the entire DeriveRelation can just be scrapped as it just implements RelationTrait..? I think it just makes sense at this point to let the user write their own implementation of it.

um... it does save a few lines and another impl block, innit?

I think it's becoming too much magic, what if the other entity isn't on the the parent module but it's on another crate?
The first version is better, more explicit

Great, then seemingly everything is set now

@tyt2y3 tyt2y3 closed this as completed Sep 10, 2021
@tyt2y3 tyt2y3 changed the title More compact Entity format Compact Entity format Sep 10, 2021
@tyt2y3 tyt2y3 modified the milestones: 0.3, 0.2 Sep 15, 2021
billy1624 added a commit to SeaQL/seaql.github.io that referenced this issue Sep 17, 2021
@github-actions
Copy link

github-actions bot commented Aug 2, 2023

🎉 Released In 0.12.1 🎉

Thank you everyone for the contribution!
This feature is now available in the latest release. Now is a good time to upgrade!
Your participation is what makes us unique; your adoption is what drives us forward.
You can support SeaQL 🌊 by starring our repos, sharing our libraries and becoming a sponsor ⭐.

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