Add support for root arrays in JSON in SeaORM

[a14e]

Description

Greetings! I've been trying to use jsonb with a root array object in SeaORM, but it appears that the current implementation doesn't support it.

Steps to Reproduce

#[derive(Clone, Debug, PartialEq, DeriveEntityModel)]
#[sea_orm(table_name = "my_model")]
pub struct Model {
    #[sea_orm(primary_key)]
    pub id: String,
    #[sea_orm(column_type = Json)]
    pub issue_tags: Vec<IssueTag>
}

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

impl ActiveModelBehavior for ActiveModel {}

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, FromJsonQueryResult)]
pub struct IssueTag {
    name: String,
    time: chrono::DateTime<Utc>,
}

Expected Behavior

I expect the code to compile without errors.

Actual Behavior

Instead, I encounter the following error:

#[derive(Clone, Debug, PartialEq, DeriveEntityModel)]
   |                                   ^^^^^^^^^^^^^^^^^ the trait `From<Vec<IssueTag>>` is not implemented for `sea_orm::Value`

It would be great if SeaORM could support root array objects in JSON. Thank you for your attention to this matter!

[t348575]

I also want this, in the mean time this simple workaround works (I use postgres) @a14e :
Get stored in the db as if its a Vec<IssueTagData>

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, FromJsonQueryResult)]
pub struct IssueTag(pub Vec<IssueTagData>);

#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize, FromJsonQueryResult)]
pub struct IssueTagData {
    pub name: String,
    pub time: chrono::DateTime<Utc>
}

[anshap1719]

@a14e @t348575

I was facing similar issue and created a PR today but I didn't realize this issue already exists. Updated my PR to reference this issue and also added support for root array objects. Can you also confirm that the PR works for your case by testing locally?

After more careful inspection, it doesn't look to be the same thing. Sorry for the false alarm 😅

[anshap1719]

@billy1624 @tyt2y3 If this is something you guys don't mind supporting, I'd be happy to create a PR.

[IgnisDa]

@tyt2y3 Are there any plans to implement this?

In case more information is required, I normally create these columns with the Json data type in the database.

            manager
                .alter_table(
                    Table::alter()
                        .table(Exercise::Table)
                        .add_column(ColumnDef::new(Exercise::Muscles).json().not_null())
                        .to_owned(),
                )
                .await?;

[tyt2y3]

Yes. A PR would be welcomed. Or we'll see if @darkmmon have the capacity to investigate.

[IgnisDa]

Could you guide me on what to do? Maybe I can make a PR.

[anshap1719]

@IgnisDa I don't want to take away this issue from you, but are you able to proceed? This issue has raised in priority for me so I'd like to create a PR for it if you're unable to.

[IgnisDa]

@anshap1719 I don't plan working on this soon. I would be glad if you take it up! I can provide feedback on the PR you make.

[anshap1719]

@IgnisDa Thanks for confirming. I'll take this up then.

[anshap1719]

Update: Although this looked to be a simple change (and it is), there's a conflicting trait implementation that's impossible to get around without changing too much of the code. I'm gonna think more about this, and if I can't see a simple solution without too much refactor, will delegate the decision to one of the maintainers.

[tyt2y3]

@anshap1719 Thank you for the effort! Can you briefly describe the blockers you are facing? What are the errors?

[anshap1719]

@tyt2y3 Basically, we need this more or less:

#[cfg(feature = "with-json")]
impl<T> TryGetableFromJson for Vec<T> where T: TryGetableFromJson {}

Even the blanket implementation is enough.

But this causes a conflicting trait implementation for TryGetable due to the following code:

impl<T> TryGetable for Vec<T>
where
    T: ActiveEnum,
    T::ValueVec: TryGetable,
{
    fn try_get_by<I: crate::ColIdx>(res: &QueryResult, index: I) -> Result<Self, TryGetError> {
        <T::ValueVec as TryGetable>::try_get_by(res, index)?
            .into_iter()
            .map(|value| T::try_from_value(&value).map_err(Into::into))
            .collect()
    }
}

This happens because we also have

#[cfg(feature = "with-json")]
impl<T> TryGetable for T
where
    T: TryGetableFromJson,
{
    fn try_get_by<I: ColIdx>(res: &QueryResult, index: I) -> Result<Self, TryGetError> {
        T::try_get_from_json(res, index)
    }
}}

[tyt2y3]

Thank you for pointing this out. I would say it's unfortunate, because impl<T> TryGetable for Vec<T> where T: ActiveEnum is only for Postgres Array and we could have feature-guarded this implementation.

However without negative trait bounds, it means that both features cannot be enabled at the same time.

What we wanted is a non-existient trait bound that can allow us to exclude ActiveEnum, something like where T: !ActiveEnum.

I imagine we have to rework the Vec<T: ActiveEnum> impl and have a 'union trait' that represents 'ActiveEnum OR TryGetableFromJson' where it will do runtime dispatch.

In the end there will only be one impl<T> TryGetable for Vec<T> where T: ActiveEnumOrJson {}.

This should be possible, because in the end, the number of sqlx types we want to support are bounded: we only support string (we always cast as text) or integers for enums, and Json for Json.

It still require the user to implement ActiveEnumOrJson on T in some (may be can help with a macro) way, but at least it's possible under the orphan rule.

[anshap1719]

@tyt2y3 Thanks for that direction. I think I can try that out.

We need a FromJsonArrayQueryResult macro anyway which I added in branch. Because the internal type T also needs to implement NotU8.

[tyt2y3]

Thanks, you see, NotU8 is also our crude attempt in having a negative trait bound. It will be quite an exercise!

[anshap1719]

@tyt2y3 Yup, I saw the doc comment on why it was needed. Makes sense. Thanks :)

[tyt2y3]

sea-orm/src/executor/query.rs

Lines 153 to 178 in af52e86

	pub trait ColIdx: std::fmt::Debug + Copy {
	#[cfg(feature = "sqlx-mysql")]
	/// Type surrogate
	type SqlxMySqlIndex: sqlx::ColumnIndex<sqlx::mysql::MySqlRow>;
	#[cfg(feature = "sqlx-postgres")]
	/// Type surrogate
	type SqlxPostgresIndex: sqlx::ColumnIndex<sqlx::postgres::PgRow>;
	#[cfg(feature = "sqlx-sqlite")]
	/// Type surrogate
	type SqlxSqliteIndex: sqlx::ColumnIndex<sqlx::sqlite::SqliteRow>;
	#[cfg(feature = "sqlx-mysql")]
	/// Basically a no-op; only to satisfy trait bounds
	fn as_sqlx_mysql_index(&self) -> Self::SqlxMySqlIndex;
	#[cfg(feature = "sqlx-postgres")]
	/// Basically a no-op; only to satisfy trait bounds
	fn as_sqlx_postgres_index(&self) -> Self::SqlxPostgresIndex;
	#[cfg(feature = "sqlx-sqlite")]
	/// Basically a no-op; only to satisfy trait bounds
	fn as_sqlx_sqlite_index(&self) -> Self::SqlxSqliteIndex;
	/// Self must be `&str`, return `None` otherwise
	fn as_str(&self) -> Option<&str>;
	/// Self must be `usize`, return `None` otherwise
	fn as_usize(&self) -> Option<&usize>;
	}

Another type shenanigan I just remembered. This basically 'unioned' str and usize and do runtime selection.

I think T::ValueVec: TryGetable can also be implemented in a similar way. Because we already know we only needed i64[] and text[]

[anshap1719]

@tyt2y3 An initial implementation is available here now: master...anshap1719:sea-orm:master

I've tested this with my project and everything seems to work as expected. Checked against json (and it's array counterpart) as well as enum (and it's array counterpart).

Still quite a lot to be done, such as unit tests, documentation and some cleanup (specially around feature configs), but would be great if I could receive an initial feedback on whether or not this is the right direction.

Another type shenanigan I just remembered. This basically 'unioned' str and usize and do runtime selection.

Also, I didn't go into this as it wasn't needed. But let me know if you'd prefer this to be done as well in this PR.

Thanks for all your help.

[IgnisDa]

@anshap1719 I tried your commit. It does not work for me. Is the data type I have selected right? Here is the commit (in apps/backend/src/entities/exercise.rs L-37).

[anshap1719]

@IgnisDa I think I'll have to run your project locally to see why. Would that be fine? Do you have a sample db dump for me to use?

[IgnisDa]

Sure give me a sec.

[anshap1719]

@IgnisDa Yeah, sorry about that. I did add tests 2 weeks ago but they were not ready, and also broken. I finally got some time today for this so I've made the PR ready for review and will ensure that tests are passing.

[IgnisDa]

@anshap1719 sorry for the nudge. When will you be able to get the PR ready?

[tyt2y3]

I think there is a PR already (or did I rmb wrongly)? But there are some intricate details I need to figure out.

[anshap1719]

@tyt2y3 @IgnisDa There is indeed a PR already :)

[IgnisDa]

cool thanks!

[tyt2y3]

cool thanks!

@IgnisDa I think we are pretty close, if you are interested you can check out https://github.com/SeaQL/sea-orm/pull/1898/files and try?

[IgnisDa]

@tyt2y3 I am not able to figure out how to put this git branch as a dependency. I have:

sea-orm = { git = "https://github.com/SeaQL/sea-orm.git", branch = "rework-active-enum", features = [
    "debug-print",
    "macros",
    "runtime-tokio-rustls",
    "sqlx-mysql",
    "sqlx-postgres",
    "sqlx-sqlite",
    "with-chrono",
    "with-uuid",
    "with-rust_decimal",
] }

but it complains that sea_orm is ambiguous.

Probably because it is a workspace?

[tyt2y3]

Yes. Try the patch syntax in Cargo.toml

[patch.crates-io]
sea_orm = { git = 'https://github.com/SeaQL/sea-orm.git' }

https://doc.rust-lang.org/cargo/reference/overriding-dependencies.html#the-patch-section

[IgnisDa]

@tyt2y3 The patching part is fine, but I am still getting this error:

[anshap1719]

@tyt2y3 The patching part is fine, but I am still getting this error:

@IgnisDa

The error looks self-explanatory to me (you need to import the trait?). Am I missing something?

[anshap1719]

@IgnisDa Ah, sorry. Looks like it's the macro that needs to be updated in this case, and not a change that user of the crate should be doing...

Please disregard my previous comment.

[IgnisDa]

Yep i have already made the imports but it doesn't work.

[IgnisDa]

@tyt2y3 Any updates on what I can do to test the changes you made?

[tyt2y3]

@IgnisDa

I guess you also need to patch sea-orm-macros since the macros are also changed.

[patch.crates-io]
sea-orm = { git = 'https://github.com/SeaQL/sea-orm.git' }
sea-orm-macros = { git = 'https://github.com/SeaQL/sea-orm.git' } # perhaps?

[anshap1719]

@IgnisDa @tyt2y3 There's a small change needed in the macro code.

@tyt2y3 I hope you don't mind me pushing to your branch directly 😅

[anshap1719]

@tyt2y3 Ah, I can't push to it (permission denied). Are you able to grant me push access, or should I create a PR to merge to your branch?

[tyt2y3]

Yes, you can create a PR targeting this branch.

…

________________________________ From: Anshul Sanghi ***@***.***> Sent: Sunday, October 8, 2023 11:48:03 AM To: SeaQL/sea-orm ***@***.***> Cc: Chris Tsang ***@***.***>; Mention ***@***.***> Subject: Re: [SeaQL/sea-orm] Add support for root arrays in JSON in SeaORM (Issue #1517) @tyt2y3<https://github.com/tyt2y3> Ah, I can't push to it (permission denied). Are you able to grant me push access, or should I create a PR to merge to your branch? — Reply to this email directly, view it on GitHub<#1517 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AANTHCFHTZK2YGPFUHKY7LTX6KAGHAVCNFSM6AAAAAAVMMJN4CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTONJRHE4TKNJYGU>. You are receiving this because you were mentioned.Message ID: ***@***.***>

[IgnisDa]

@tyt2y3 @anshap1719 I am trying to get this working with Vec<MyEnum>. Its trying to save the field as jsonb[], but it should be saved as json. How can I do it?

[negezor]

@IgnisDa Add an attribute for your model field

#[sea_orm(column_type = "JsonBinary")]
pub muscles: Vec<MyEnum>,

[IgnisDa]

@negezor The type in my database is JSON, not JSON Binary.

[negezor]

@IgnisDa Then it could just be like that, although I haven’t tested it yet.

#[sea_orm(column_type = "Json")]
pub muscles: Vec<MyEnum>,

[IgnisDa]

@negezor Awesome that works!