Incorrect Quoting of Schema-Qualified Enums in `cast_as_quoted`

[mpyw]

Description

When using the cast_as_quoted function to cast schema-qualified enums (e.g., schema_name.enum_name), the generated SQL incorrectly quotes the entire identifier as "schema_name.enum_name". Instead, each part of the schema and enum should be quoted separately, like "schema_name"."enum_name", to ensure correctness.

Steps to Reproduce

1. Define a schema-qualified enum:

   CREATE TYPE schema_name.enum_name AS ENUM ('value1', 'value2');

2. Use SeaQuery to cast a value using cast_as_quoted:

   use sea_query::{tests_cfg::*, *};
   
   let query = Query::select()
       .expr(Func::cast_as_quoted(
           "some_value",
           Alias::new("schema_name.enum_name"),
           '"'.into(),
       ))
       .to_owned();

3. Generate the SQL for PostgreSQL:

   assert_eq!(
       query.to_string(PostgresQueryBuilder),
       r#"SELECT CAST('some_value' AS "schema_name.enum_name")"#
   );

Expected Behavior

The SQL should be generated as:

SELECT CAST('some_value' AS "schema_name"."enum_name")

Actual Behavior

The SQL is incorrectly generated as:

SELECT CAST('some_value' AS "schema_name.enum_name")

Reproduces How Often

Always.

Versions

• SeaQuery: 0.31.1
  • Due to Case-sensitive enum type casts #789
• PostgreSQL: Irrelevant
• OS: Irrelevant

Additional Information

[mpyw]

I find it counterintuitive that calling with Alias::new("schema_name.enum_name") results in "schema_name"."enum_name". The identifier should be fully determined at the alias definition stage and should not be modified.

To address this, we could modify cast_as_quoted to also accept Expr, allowing for cases like:

Expr::col((Alias::new("schema_name"), Alias::new("enum_name")))

Additionally, DeriveActiveEnum (sea-orm-macros) definition:

#[sea_orm(
    rs_type = "String",
    db_type = "Enum",
    enum_name = "schema_name.enum_name"
)]

it could be updated to handle dot-chain parsing. Alternatively:

• Introduce enum_name_raw field could allow for manual quoting:

#[sea_orm(
    rs_type = "String",
    db_type = "Enum",
    enum_name_raw = "Expr::col((Alias::new(\"schema_name\"), Alias::new(\"enum_name\")))"
)]

• Introduce schema_name:

#[sea_orm(
    rs_type = "String",
    db_type = "Enum",
    schema_name = "schema_name"
    enum_name = "enum_name"
)]

What do you think?

[mpyw]

Looks to be reverted in 0.32.0-rc.2 6265311#diff-47f0d53e0e29c3ecf9dd0171849266fa1329f552eb74a2819292650d7177d095R47

[Huliiiiii]

This don't relate to that commit. It relates to how Iden escapes quotes.

@Expurple
What are your thoughts on this? Keeping the Iden trait seems to allow for type-specific behavior. Alternatively, we can change the function parameters, or perhaps make IdenImpl into an enum with Raw and Escape variants.

[Expurple]

@mpyw,

I find it counterintuitive that calling with Alias::new("schema_name.enum_name") results in "schema_name"."enum_name". The identifier should be fully determined at the alias definition stage and should not be modified.

I don't find this counter-intuitive. I think, implicit quoting is a good default to prevent unexpected runtime errors when a certain keyword identifier finally hits the database. All methods consistently accept a non-quoted representation as input and add quotes under the hood. I don't think that the users should bother with writing out ugly stuff like Alias::new("\"order\"") instead of Alias::new("order").

To address this, we could modify cast_as_quoted to also accept Expr

Fixing this method signature is the right way forward. The current API is too limited and forces you to use it incorrectly. schema_name.enum_name is not an identifier/alias! It's a compound ColumnRef. This method should accept IntoColumnRef instead of IntoIden. That's the proper solution. Added this to #795. I think, Expr would be too flexible and wouldn't provide any additional benefits.

Although, this misuse of Alias will still be possible after the change. I wonder if we should also update our provided quoting methods to handle this "compound" case automatically and generate "schema_name"."enum_name" as you expected. Why not. This doesn't change the behavior for actual single identifiers with no dots inside.

Another possible approach to prevent misuse is validating and returning an error if an Iden::unquoted is not actually a single identifier with no quotes, dots, and so on. But I'm afraid of accidentally being too restrictive with this. And my previous solution seems more simple, backwards-compatible, and it already closes the original real-world issue.

[Expurple]

@Huliiiiii,

What are your thoughts on this?

See above.

Keeping the Iden trait seems to allow for type-specific behavior. Alternatively, we can change the function parameters, or perhaps make IdenImpl into an enum with Raw and Escape variants.

I'm not sure what you mean here. The choice between using quoted and unquoted representation isn't made in Iden, it's made in the SQL generation backend that calls one or the other method on DynIdens. You can keep IdenImpl an unquoted struct, and the backend will decide whether it needs to post-process it into a quoted version, just as it does with the trait today.

Sure, you could make IdenImpl an enum to give users an ability to say: "This identifier doesn't need to be post-processed and quoted". But I don't think that it's necessary. I like the implicit quoting. And we can always make the provided quoting smarter, so that it can detect when the identifier is already quoted.

[Huliiiiii]

A non-breaking approach is to extend the parameters of this method to accept both IntoIden and IntoColumnRef, which would cover most such use cases.

[Expurple]

@Huliiiiii,

If you mean literally accepting something like T: IntoIdenOrColumnRef, that wouldn't work, I think. We can't implement that new trait both for T: IntoIden and T: IntoColumnRef. There can only be one blanket impl of any given trait.

But anyway, IntoColumnRef is already implemented for T: IntoIden + 'static. In the current master, IntoIden types are converted into a string early instead of being stored as a trait object, so we can just remove that 'static bound, I think. Then, we can make cast_as_quoted accept T: IntoColumnRef, and that would be a fully backwards-compatible change*. I'll open a PR.

* within master (SeaQuery 1.0), which still has other breaking changes compared to 0.32

[Expurple]

Turns out, ColumnRef doesn't make sense either. We need to add a separate type for potentially-qualified names (type names, function names...). See #922 (comment). I'll do that

[Expurple]

Turns out, type names are their own thing, kinda like column refs. Both are more complicated than "just" schema-qualified names (like tables and functions): #922 (comment) 🫠