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

Add JsonBinary attribute #1346

Merged
merged 4 commits into from
Jan 18, 2023
Merged

Conversation

AngelOnFira
Copy link
Contributor

@AngelOnFira AngelOnFira commented Dec 30, 2022

PR Info

Notes

Currently, this only seems to work on compact entity structures. As of writing this, I'm unsure on how the attribute should be added in the expanded form, so I'll need some insights on that.

TODO

- Get this to work on expanded entity structure

  • Write tests

@AngelOnFira
Copy link
Contributor Author

So @billy1624 this seems to work correctly now that

Before (compact form)

pub struct Model {
    #[sea_orm(primary_key)]
    pub id: i32,
    pub status: Json,
    pub payload: Json,
}

After (compact form):

pub struct Model {
    #[sea_orm(primary_key)]
    pub id: i32,
    #[sea_orm(column_type = "JsonBinary")]
    pub status: Json,
    #[sea_orm(column_type = "JsonBinary")]
    pub payload: Json,
}

After (expanded form):

#[derive(Clone, Debug, PartialEq, DeriveModel, DeriveActiveModel, Eq, Serialize, Deserialize)]
pub struct Model {
    pub id: i32,
    pub status: Json,
    pub payload: Json,
}

...

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

impl PrimaryKeyTrait for PrimaryKey {
    type ValueType = i32;
    fn auto_increment() -> bool {
        true
    }
}

So as we can see, in the expanded form the attributes aren't added. I don't exactly know how they expand, but in the case above I've included the PrimaryKey as it appears to be the expansion of #[sea_orm(primary_key)]

What would the next steps be to properly add this attribute to the expanded structure?

Also, re: tests, I'm not sure where a new one would fit best, and I think that only Postgres has Binary Json, so would the path sea-orm-codegen/tests/postgres/binary_json.rs work?

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AngelOnFira, thanks for contributing as always!

For the expanded entity, I think it already handled the JsonBinary column type.

ColumnType::Json => quote! { ColumnType::Json },
ColumnType::JsonBinary => quote! { ColumnType::JsonBinary },

Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, re: tests, I'm not sure where a new one would fit best, and I think that only Postgres has Binary Json, so would the path sea-orm-codegen/tests/postgres/binary_json.rs work?

Good call! Yes, please :)

@AngelOnFira
Copy link
Contributor Author

For the expanded entity, I think it already handled the JsonBinary column type.

Oh, is this the part of the extended format that covers that?

impl ColumnTrait for Column {
    type EntityName = Entity;
    fn def(&self) -> ColumnDef {
        match self {
            Self::Id => ColumnType::Integer.def(),
            Self::Status => ColumnType::JsonBinary.def(),
            Self::Payload => ColumnType::JsonBinary.def(),
        }
    }
}

It does look like it's working now, so I must have had something out of sync when I first thought it was an issue 😅 either way, glad to get the tests wrapped up on this PR 👍

@AngelOnFira AngelOnFira marked this pull request as ready for review December 30, 2022 16:12
@AngelOnFira AngelOnFira force-pushed the add-json-binary-attribute branch from b7c3631 to 8aadc9b Compare December 30, 2022 16:17
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @AngelOnFira, thanks for adding test case for the compact entity format. Would it be possible to include a test case for the expanded format as well?

@AngelOnFira
Copy link
Contributor Author

@billy1624 that should be everything :) let me know if any test could be improved!

@AngelOnFira AngelOnFira requested a review from billy1624 January 16, 2023 03:26
@AngelOnFira AngelOnFira force-pushed the add-json-binary-attribute branch from df79cbf to 44d6575 Compare January 16, 2023 03:49
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!! @AngelOnFira Appreciated!

@billy1624 billy1624 merged commit bb39684 into SeaQL:master Jan 18, 2023
@AngelOnFira AngelOnFira deleted the add-json-binary-attribute branch January 18, 2023 13:57
denwong47 pushed a commit to denwong47/sea-orm that referenced this pull request Jan 20, 2023
* Add JsonBinary attribute to column

* Add Postgres test section, test binary json

* Added expanded entity format test

* Fixed unit test
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

Successfully merging this pull request may close these issues.

Generating entity from Postgres with JsonBinary field doesn't add attribute to model
3 participants