Replace "DynIden" with "IdenImpl"

[Huliiiiii]

Background: https://github.com/SeaQL/sea-query/discussions/795#discussioncomment-13311660

PR Info

• Closes

• Dependencies:

• Dependents:

New Features

Bug Fixes

Breaking Changes

• [x] Replaced DynIden with concrete struct IdenImpl.

  • How to migrate:
    • If you use the derive macro Iden, you don't need to do anything.
    • If you implemented trait Iden manually, you need to replace it with impl From<YourType> for IdenImpl.

• [x] Unlike DynIden, IdemImpl is compared purely by string. Same strings rendered from different Rust identifier types will now compare as equal.

• [x] Remove SeaRc.

  • How to migrate:
    • In SeaORM and SeaQuery, SeaRc was only used for DynIden. So, after you fix the errors around DynIden, you shouldn't have any errors left.
    • But if you used SeaRc for your own purposes outside of DynIden, there can be more errors. There, you can try replacing SeaRc with RcOrArc (which was the underlying implementation of SeaRc anyway).

Changes

[Expurple]

If you don't mind, I'll review when the CI passes. But feel free to ask if you need my review earlier

[Huliiiiii]

Because it is too troublesome to fix the tests and examples, I also implemented the corresponding behavior for the derive macro Iden in this PR.

[Huliiiiii]

It seems that all errors are caused by deprecation.

[tyt2y3]

sorry, it passed CI already. then there's a conflict

[tyt2y3]

sorry for the late reply. I think this is in the right direction. however, I am afraid there's still too much breaking change.

having said that, I think most of the changes can be polyfilled:

1. Iden::to_string we can have a struct Iden and a generic function to_string?
2. SeaRc::new we can again have a helper struct SeaRc and a method new?
3. .into_iden we can add this method to Alias?

[Huliiiiii]

1. First, I don't think it's a good practice to name this method to_string as it conflicts with the Display trait method, which violates Rust conventions. I suggest renaming it to to_iden_string or to_sql_string instead. Second, the current issue is that most existing code calls the trait method Iden::to_string on enums or zero size types. There's no clean migration path for the existing code with this approach.
2. SeaRc can be kept, but it will be unused.
3. Yes, we can add into_iden for Alias, but I think a better approach would be to remove Alias (Or keep it for compatibility), since the current changes implement Into<IdenImpl> for both &'static str and String.

[Expurple]

I don't think it's a good practice to name this method to_string as it conflicts with the Display trait method, which violates Rust conventions. I suggest renaming it

I agree with you in principle, but we're faced with a peculiar task of balancing between APIs that make more sense and APIs that are more compatible. I think, the right way is keeping to_string in 2.0.0 and perhaps deprecating it sometime later in 2.x.x, giving plenty of time to migrate before 3.0.0. 2.0.0 has plenty of other, absolutely necessary breaking changes that have to be fixed in one go.

There's no clean migration path for the existing code with this approach.

There is one that I proposed here: blanket impl<T> OldIdenTrait for T where T: Into<NewIdenStruct> (OldIdenTrait and NewIdenStruct is for clarity, it's not the actual proposed naming). It will keep all Iden method calls working.

This will still break manual impl Iden (if the type also implements Into<NewIdenStruct>) and the code calling methods that accept NewIdenStruct (if the type does not implement Into<NewIdenStruct>). But that breakage is almost inherent to your design.

The only really-non-breaking alternative is reversing the blanket impl as impl<T> From<T> for NewIdenStruct where T: OldIdenTrait. As you've noted, this implies that the Cow will be Owned more often than we'd like.

SeaRc::new we can again have a helper struct SeaRc and a method new?

Yeah, we can keep if for compatibility and later deprecate in 2.x.x.

.into_iden we can add this method to Alias?

This PR already keeps impl<T> IntoIden for T where T: Into<NewIdenStruct> as an extension trait for compatibility. The method calls will keep working.

This still breaks manual impl IntoIden when the type also implements Into<NewIdenStruct>, but that's pretty much by design.

If the type manually implements IntoIden without implementing Into<NewIdenStruct>, then it can't be passed into methods that accept Into<NewIdenStruct>. But we can solve this by making trait IntoIden: Into<NewIdenStruct>. I think, this makes a lot of sense. That's what I plan doing with all Into* traits.

I think a better approach would be to remove Alias (Or keep it for compatibility), since the current changes implement Into<IdenImpl> for both &'static str and String.

I'm also strongly in favor of adding an impl for String and keeping Alias only for compatibility: https://github.com/SeaQL/sea-query/pull/882. And two more users share their frustrations and hacks in that thread.

[tyt2y3]

I would probably even not put in the deprecated notice to avoid causing tons of compiler warnings in the first release.

I'm also strongly in favor of adding an impl for String and keeping Alias only for compatibility: https://github.com/SeaQL/sea-query/pull/882. And two more users share their frustrations and hacks in that thread.

well, I guess my argument is that String is data and I don't want it to be confused as identifier. example:

let a = String::new("a");
let b = String::new("b");

`a.eq(b)` would result in `"a" = 'b'`
`b.eq(a)` would result in `"b" = 'a'`

one is quoted as identifier, one is quoted as literal. am I right that it will cause this confusion ultimately?

[Expurple]

I would probably even not put in the deprecated notice to avoid causing tons of compiler warnings in the first release.

Yeah, agree. Definitely not in 1.0.0.

I guess my argument is that String is data and I don't want it to be confused as identifier

That's an interesting argument and we should explore it. But it needs a convincing example. Your pseudocode example is incorrect. The non-pseudocode

// [dependencies]
// sea-query = { version = "0.32.3", features = ["tests-cfg"] }

use sea_query::{ExprTrait, PostgresQueryBuilder, QueryBuilder};

fn main() {
    let a = String::from("a");
    let b = String::from("b");

    let mut sql = String::new();
    PostgresQueryBuilder.prepare_simple_expr(&a.clone().eq(&b), &mut sql);
    println!("{}", sql);

    let mut sql = String::new();
    PostgresQueryBuilder.prepare_simple_expr(&b.eq(&a), &mut sql);
    println!("{}", sql);
}

prints

'a' = 'b'
'b' = 'a'

And to me, that's obvious and expected. Identifiers don't even implement ExprTrait. It's impossible to call eq directly on an identifier string. "a" = 'b' can be produced only from something like Expr::col(Alias::new(a)).eq(b). Or Expr::col(a).eq(b), after we impl Iden for String. The latter looks great to me.

[Expurple]

@Huliiiiii, we've chatted with Chris and decided to split the PR in order to proceed.

Initially, we want to merge these two small changes as separate PRs:

• impl Iden for String
• Alter the PartialEq impl to be safe, ignore the types and compare DynIdens as strings

That's a blocker for now.

[Huliiiiii]

@Huliiiiii, we've chatted with Chris and decided to split the PR in order to proceed.

Initially, we want to merge these two small changes as separate PRs:

• impl Iden for String
• Alter the PartialEq impl to be safe, ignore the types and compare DynIdens as strings

That's a blocker for now.

I haven’t had time to work on this lately. I’m totally fine if you wants to take over or use this as a base for further changes.

[tyt2y3]

I’m totally fine if you wants to take over or use this as a base for further changes.

I will try some experiments

[tyt2y3]

thank you so much for the initiative!