Wanted breaking changes in sea_query

[Expurple]

As discussed here, I won't be able to release everything I want in SeaORM 1.0. So, I need to track my wishlist somewhere. I also need a URL to include in the TODOs in the code. This is the place.

Cleanups around ExprTrait

Background

• Unify `Expr` and `SimpleExpr`, add `ExprTrait`? #771
• Add ExprTrait (#771) #791

My plans

• Delete the inherent methods of Expr and SimpleExpr which have been copied to ExprTrait and now just call the implementation from the trait.
  • DONE: Remove inherent SimpleExpr methods that duplicate ExprTrait #890
  • This is a relatively small breaking change, because all signatures are exactly the same and the compiler will simply suggest to add a use sea_query::ExprTrait.
• Replace all impl SqliteExpr blocks with a single wider impl<T> SqliteExpr for T where T: ExprTrait {}.
  • DONE: Blanket-implement PgExpr and SqliteExpr #914
• Replace all impl PgExpr blocks with a single wider impl<T> PgExpr for T where T: ExprTrait {}.
  • DONE: Blanket-implement PgExpr and SqliteExpr #914
• Experiment with unifying Expr and SimpleExpr into one type.

  • So far it seems to me that Expr shouldn't be a type. It's mostly just a namespace for static SimpleExpr consructors. The type only confuses everyone. It's not meant for passing around, but I still have such instances in my codebase, with confusing conversions between SimpleExpr and Expr, etc.

    I imagine moving these constructors into SimpleExpr, and then we can pub type Expr = SimpleExpr for compatibility. But it's still a breaking change, because implementing the same trait for Expr and SimpleExpr becomes illegal (two implementations for one type).

    DONE: Unify Expr and SimpleExpr as one type #889

  • Perhaps, the same reasoning also applies to Func, PgFunc, Type and Table, and we should remove these types too. Although, in practice, these haven't been a problem in my codebase. And unlike Expr, these are unit types with obviously no data attached. That's probably why I don't pass these around and there's no confusion.

Allow schema-qualified enums in cast_as_quoted

• Incorrect Quoting of Schema-Qualified Enums in cast_as_quoted #827

DONE: #922

Into* traits

• Remove Into* traits in favor of From/Into.
  • See Explanation of existance of `IntoSimpleExpr` instead of `Into<SimpleExpr>` sea-orm#2327.
  • To keep some backwards-compatibility, we can blanket impl IntoFoo for T where T: Into<Foo> instead of removing these traits outright.

DONE:

• Make into_condition a shorthand for Into::<Condition>::into #939
• Refactor IntoTableRef into a sub trait of Into<TableRef> #958
• Refactor IntoColumnRef into a sub trait of Into<ColumnRef> #959
• Refactor IntoValueTuple into a sub trait of Into<ValueTuple> #960
• Unify TypeRef and TypeName, update IntoTypeRef #969
• Refactor IntoIden into a sub trait of Into<Iden> #973
• Refactor IntoLikeExpr into a sub trait of Into<LikeExpr> #974
• Refactor IntoColumnDef into a sub trait of Into<ColumnDef> #975
• Refactor IntoIndexColumn into a sub trait of Into<IndexColumn> #976

Related: #905

Mark more enums as #[non_exhaustive]

• Mark most public enums as #[non_exhaustive]. Specifically, enums that correspond to SQL features / types / functions.

It's impossible to exhaustively list these, because databases always keep gaining new features. Adding a new variant shouldn't be a breaking change. See #828 (comment) for an example of an SQL feature that can't be merged without a major release.

DONE: #891

Stop using deprecated SERIAL column type

• Stop using deprecated serial column type #688
  • DONE: Replace serial with GENERATED BY DEFAULT AS IDENTITY #918

Reconsider boxing the variants of Value

• Reason for boxing inner values of `Value` #863
  • DONE: Unbox values #925, Value: box BigDecimal and Array #931

Experiment with deleting ConditionHolder(Contents)

See this thread: #795 (reply in thread)

• Unify ConditionHolder and ConditionHolderContents into one type (ConditionHolder).
• Try to delete the remaining type and replace it with Condition.

Experiment with deleting / hiding ConditionExpression

• It seems like some weird implementation detail for conversions, that shouldn't be public. Maybe it was created as a workaround when Condition wasn't Into<SimpleExpr>. But now it is: Conversions from Condition and ConditionExpression into SimpleExpr #886. So, now we can try to delete ConditionExpression, or make it private, or at least make it #[doc(hidden)] (if we can make only macros use it). If we won't be able to, then at least we'll learn and document why this type is needed.

  DONE: Make ConditionExpression private #915

Experiment with turning Iden trait into a struct

• See this thread: Wanted breaking changes in `sea_query` #795 (comment)
  • DONE: Replace "DynIden" with "IdenImpl" #892, New iden paradigm #909

Upgrade the public dependencies

• We should check our public dependencies like sqlx and ipnetwork. Something like crates VSCode extension can be used to quickly see new major versions of dependencies.

---

Related plans in sea_orm

• Wanted breaking changes in `sea_orm` sea-orm#2548

[tyt2y3]

Put this here as well #688 : use identity column by default for postgres

[Expurple]

Done. Although I probably won't implement a PR for this.

Also, I'm unlikely to work on on_empty_do_nothing() in sea_orm. But you already have a PR for that.

I'll try to implement the other items and let you know if I have any problems or need help

[tyt2y3]

@Expurple feel free to start opening PRs for the breaking changes wanted now.
I will branch off 0.32 if needed.

[Huliiiiii]

List of Into*

• IntoSimpleExpr
• IntoColumnRef
• IntoCondition
• IntoIndexColumn
• IntoTypeDef
• IntoIden

[Expurple]

The original discussion (SeaQL/sea-orm#2327) has links to search all trait Into* in sea_query and all trait Into* in sea_orm on docs.rs

[Huliiiiii]

Implemented unification of Expr and SimpleExpr in #888

[tyt2y3]

#889 is merged

[Huliiiiii]

I noticed that Conditon has an abbreviation alias Cond.
Also, the names of all Condition* are inconsistent with other abbreviations in the library (Expr, Iden). Should we unify the naming and export the old names as aliases?

[Expurple]

I think, we should go even deeper than naming. I have a few personal TODO notes for digging around Condition*:

• Added a section about ConditionExpression to the main post above. Check it out! You can try making a PR if you're interested.
• I have no idea, what the hell are ConditionHolder and ConditionHolderContents, why they're public, and why they exist. There's not a single line of documentation. Someone should dig around, document these and change if necessary.

@tyt2y3, do you have any knowledge that you could quickly document about these types?

[tyt2y3]

the reason they are pub I think is because of this:

pub enum JoinOn {
    Condition(Box<ConditionHolder>),
    Columns(Vec<SimpleExpr>),
}

the types used in the AST are mostly pub, but they can change across minor releases. may be we need to label them "semver-exempt" or expose accessor methods instead.

[Expurple]

Condition(Box<ConditionHolder>),

This doesn't look convincing to me. struct Condition is perfectly capable of representing logical chains (which is what ConditionHolderContents inside of ConditionHolder seems to be doing here). So far this variant looks like it could be just Condition(Box<Condition>),.

But even if ConditionHolderContents somehow needs to exist and not be a Condition, I still don't see why ConditionHolder needs to exist. It's just a newtype around ConditionHolderContents that doesn't add anything. It's exhaustive. The field is publicly accessible. It implements the same traits. It looks that we should at least delete the current ConditionHolder newtype and rename ConditionHolderContents to ConditionHolder.

the types used in the AST are mostly pub, but they can change across minor releases. may be we need to label them "semver-exempt" or expose accessor methods instead.

This doesn't make sense to me. sea_query is a query builder. Of course the AST types are its public API!

I think, we should keep them versioned, but strive for extensibility at the same time. See #891

[tyt2y3]

#891 is a good idea.

It looks that we should at least delete the current ConditionHolder newtype and rename ConditionHolderContents to ConditionHolder.

yeah we can try that

[Huliiiiii]

I don't understand why Iden is a trait and not a struct. I tried rewriting Iden as a wrapper of Cow<'static, str> and all the tests passed except these:

assert_ne!(
    ColumnRef::Column(Character::Id.into_iden()),
    ColumnRef::Column("id".into_iden())
);
assert_ne!(
    ColumnRef::Column(Character::Id.into_iden()),
    ColumnRef::Column(Font::Id.into_iden())
);

&str and String always thread safe and we don't need to modify them across threads.
iden is 'static most of the time, which also reduces unnecessary clones (on Rc or Arc).
Unless the original design goal is to keep the eq behavior based on vtable and value.
Or is there some other point I've overlooked?

[Huliiiiii]

Anyway, we need more benches to compare the performance of these two approaches.

[Expurple]

This bench doesn't look trustworthy at all. I wouldn't base any conclusions on it. It's constant-foldable. It's microscopic. On my laptop, the reported time per iteration is in nanoseconds.

Diesel has benchmarks for various Rust ORMs, including SeaORM: blog post, repo. These are much more high-level and realistic. I doubt that we'll even notice a difference on such real-world benchmarks against a database.

And even in that case, I want to merge your change because it's good from the API standpoint (removes a layer of indirection, compares Idens as strings) and from the implementation standpoint (removes unsafe).

[tyt2y3]

The original problem that DynIden is trying to solve is "enumerable columns", in which, there is a wide table with col_1 to col_99. (I know this is bad schema, but I couldn't control that). and so there is a type struct Col(i32) that impl Iden dynamically. This is the only use case I've had, and may be a procedural macro could be a better solution, because the number of columns is still bounded.

So I agree we could probably use Cow internally. However preserving the type was also the original intention. Character::Id != Font::Id, even though they both serializes to "id". This matters in tests where the ASTs are compared.

I don't think performance is much of a concern here, and I expect Cow to be fine. The microbenchmark does serve a purpose, and I think that it reflects that extra branching is worse than indirection, in the same sense that short-string-optimisation doesn't work well in rust.

[Expurple]

However preserving the type was also the original intention. Character::Id != Font::Id, even though they both serializes to "id".

I get where you're coming from, regarding the user intentions when building a query. But the PartialEq behavior doesn't really protect against bugs like that. I never compare expressions, and most people probably don't do that either. Comparing expressions is very unreliable anyway, because where are many equivalent ways of expressing the same thing. Where is PartialEq actually used outside of tests?

The thing that would actually protect me from such bugs is: more methods automatically generating not just the column name, but also the table name in the resulting SQL. I've already written about this somewhere a year ago, but can't find the thread now. That would actually solve the root issue.

I'd say, in their current form, Character::Id and Font::Id are the same, because they generate the same SQL and result in the same behavior. I think, PartialEq should also be same and compare by string. You haven't yet provided a real-world use case where it needs to compare by type

[tyt2y3]

I don't disagree with you, and can merge the change provided that the breakage is small enough

[Huliiiiii]

• Exposition of Postgres functions to_tsquery, to_tsvector, phraseto_tsquery, plainto_tsquery, websearch_to_tsquery are unergonomic because they only accept the regconfig as an u32 #784

  My thoughts:

  If we need to maintain compatibility, we can replace the parameter with a trait and implement the trait for Option. Then, we provide a newtype wrapping &str that also implements that trait. If compatibility is not required, we can simply change the parameter to &str.

• Custom Functions keyword arguments #870

  Keyword argument support

  My thoughts:

  Change FunctionCall from

  #[derive(Debug, Clone, PartialEq)]
  pub struct FunctionCall {
      pub(crate) func: Function,
      pub(crate) args: Vec<Expr>,
      pub(crate) mods: Vec<FuncArgMod>,
  }

  to

  #[derive(Debug, Clone, PartialEq)]
  pub struct FunctionCall {
      pub(crate) func: Function,
      pub(crate) args: Vec<FuncArg>,
  }
  
  #[derive(Debug, Clone, PartialEq)]
  struct FuncArg {
      pub(crate) kind: FuncArgKind,
      pub(crate) r#mod: FuncArgMod,
  }
  
  #[derive(Debug, Clone, PartialEq)]
  enum FuncArgKind {
      Expr(Expr),
      Kw {
          name: Cow<'static, str>,
          value: Expr,
      },
  }

• All index hints appear after the last from statement #760

  Maybe we need to bind index_hints to columns

[Huliiiiii]

Merge Func/Function and PgFunc/PgFunction.

The former is just a static method namespace for function construction

[Huliiiiii]

Hmm, why do we need the Function enum? Its only purpose seems to be providing a name, and most of the time we construct FunctionCalls using methods on the namespace. The enum is mostly used internally. Why not turn them into free functions instead? That would better separate concerns and improve modularity.

[Expurple]

Merging these types would be nice. I already mention this possibility in the original post. It's very similar to Expr/SimpleExpr (except that Expr wasn't a unit struct for some reason). This "separate types as namespaces for helper methods" idea is a little weird.

Another possible direction is turning Func-like types into namespaces (modules) before deprecating these PascalCased names later. We can do that if we don't want to "pollute" Function-like enums with static constructors. But I think, static constructors are fine.

Hmm, why do we need the Function enum? Its only purpose seems to be providing a name, and most of the time we construct FunctionCalls using methods on the namespace.

To be honest, now that Iden is more ergonomic and allows using string literals directly, I don't want a Function enum at all. I just want FunctionCall.func to be something similar to ColumnRef or TableRef*, so that I can just write FunctionCall::new(("my_schema", "my_function")). We can still have static constructors that shape the function signatures for you, but the Function enum itself is pointless. It creates a false impression that the other variants are not supported. I often have to tell people that they can use Function::Custom.

---

* As I think about it, we need a separate type (MaybeQualified?) for a "potentially-qualified name". ColumnRef and TableRef variants are very repetitive. We can do better:

#[non_exhaustive]
pub enum MaybeQualified {
    Unqualified(DynIden),
    QualifiedOnce(DynIden, DynIden),
    QualifiedTwice(DynIden, DynIden, DynIden),
}

#[non_exhaustive]
pub enum TableRef {
    /// `Option<DynIden>` is an optional alias.
    TableName(MaybeQualified, Option<DynIden>),
    SubQuery(Box<SelectStatement>, DynIden),
    ValuesList(Vec<ValueTuple>, DynIden),
    FunctionCall(FunctionCall, DynIden),
}

#[non_exhaustive]
pub enum ColumnRef {
    Column(MaybeQualified),
    Asterisk,
    TableAsterisk(DynIden),
}

@tyt2y3

[tyt2y3]

FunctionCall::new(("my_schema", "my_function")). We can still have static constructors that shape the function signatures for you, but the Function enum itself is pointless. It creates a false impression that the other variants are not supported. I often have to tell people that they can use Function::Custom.

I agree. The enums only serves to provide an Iden impl in the first place. the real value is to have a constructor that constraint the arguments that the SQL function accepts.

[Expurple]

Opened #927 in preparation

[Huliiiiii]

Should we wrap enums (like Value) in opaque NewTypes? That way, implementation details would be fully encapsulated, and we wouldn't break compatibility when making changes.

I assume no one is actually using the internal variants of Value, right? Most people just pass them as parameters to SQL statements.

[Expurple]

Hard to tell. Maybe someone actually matches over Value to implement something. It should be rare, but I'm not sure if I want to move forward with this

[tyt2y3]

agree with expurple. it'd probably make things more complicated

[Huliiiiii]

How about make ValueType a sub trait of TryFrom<Value>？

pub trait ValueType: Sized {
    fn try_from(v: Value) -> Result<Self, ValueTypeErr>;
}

[Expurple]

Makes sense. pub trait ValueType: Sized + TryFrom<Value, Error = ValueTypeErr>

[Huliiiiii]

Maybe it's too late to discuss this, but I’ve noticed the issue of ColumnSpec.

It’s an enum, but its variants aren’t exclusive—they can coexist in the same Vec. This makes it easy to generate invalid SQL during serialization, which can lead to the following issues:

SeaQL/sea-orm#2652
SeaQL/sea-orm#2396
SeaQL/sea-orm#2337

pub enum ColumnSpec {
    Null,
    NotNull,
    Default(Expr),
    AutoIncrement,
    UniqueKey,
    PrimaryKey,
    Check(Check),
    Generated { expr: Expr, stored: bool },
    Extra(String),
    Comment(String),
    Using(Expr),
}

pub struct ColumnDef {
    pub(crate) table: Option<TableRef>,
    pub(crate) name: DynIden,
    pub(crate) types: Option<ColumnType>,
    pub(crate) spec: Vec<ColumnSpec>,
}

fn prepare_column_spec(&self, column_spec: &ColumnSpec, sql: &mut dyn SqlWriter) {
    match column_spec {
        ColumnSpec::Null => write!(sql, "NULL").unwrap(),
        ColumnSpec::NotNull => write!(sql, "NOT NULL").unwrap(),
        ColumnSpec::Default(value) => {
            write!(sql, "DEFAULT ").unwrap();
            QueryBuilder::prepare_simple_expr(self, value, sql);
        }
        ColumnSpec::AutoIncrement => {
            write!(sql, "{}", self.column_spec_auto_increment_keyword()).unwrap()
        }
        ColumnSpec::UniqueKey => write!(sql, "UNIQUE").unwrap(),
        ColumnSpec::PrimaryKey => write!(sql, "PRIMARY KEY").unwrap(),
        ColumnSpec::Check(check) => self.prepare_check_constraint(check, sql),
        ColumnSpec::Generated { expr, stored } => {
            self.prepare_generated_column(expr, *stored, sql)
        }
        ColumnSpec::Extra(string) => write!(sql, "{string}").unwrap(),
        ColumnSpec::Comment(comment) => self.column_comment(comment, sql),
        ColumnSpec::Using(_) => {}
    }
}

[tyt2y3]

let's say before pushing something new to spec, we look for an item with same variant first, then replace it if found?
would that solve the problem?

[Huliiiiii]

Only mysql support comment in create table statement.
What do you think is the best way to fix this issue or improve the developer experience?

SeaQL/sea-orm#2404

[tyt2y3]

great point! I think it can go the way of create_index_from_entity, basically a separate method and generate statements to be executed separately.