Generate SQL type definitions for unknown types

[weiznich]

This commit adds support for generating SQL type definitions for unknown
types to diesel print-schema. The basic idea is to generate the
corresponding marker type to always end up with an existing schema.
Especially this does not generate any code that is required for
serializing/deserializing rust values.

Additionally this commit tweaks the table! macro to import types from
the parent scope. This allows us to just reference those newly generated
types easily + simplifies the handling of other custom type imports in
my opinion. The old behaviour of having as part of the table!
definition remains supported.

I open this unfinished version of this PR to gather some feedback on the general idea. Before merging this PR we should figure out how to handle this for the mysql backend.

[pksunkara]

I am actually unsure about this change. If my postgres db has an enum status and diesel generates a type for it in schema.rs file, I would need to edit the schema file to make that generated Status struct an actual enum with all possible values. But users are not supposed to edit the schema.rs file.

[Ten0]

If my postgres db has an enum status and diesel generates a type for it in schema.rs file, I would need to edit the schema file to make that generated Status struct an actual enum with all possible values.

That's what I thought too at first glance but then I realized what this generates is the SqlType unit struct and not what would be the enum anyway. (The equivalent of what diesel-derive-enum generates as XxxxxMapping:
https://github.com/adwhit/diesel-derive-enum/blob/e4286e346fb7063577454cf2aee008f057d20f82/src/lib.rs#L32-L33
https://github.com/adwhit/diesel-derive-enum/blob/e4286e346fb7063577454cf2aee008f057d20f82/src/lib.rs#L206-L210
). You'd still have to declare your enum struct in a separate file, but would only have to implement FromSql/ToSql & co for it. (Which diesel-derive-enum would have to be slightly updated for.)

Overall it looks like this would remove the large majority of the amount of patch we have to do on our schema, so I think in general this is a very good idea. (By the way now that I look into this I realize this would have been a much nicer pattern. 😅) I'll take some time later to look at this more in depth and form an opinion about the specific implementation and in particular the naming of the newly generated structs. On that topic, do you have some reference on why it was chosen to not have support for generating rust enums as part of schema inference, apart from #343 (comment)?

[weiznich]

By the way now that I look into this I realize this would have been a much nicer pattern. 😅

It should be quite easy to change the generated schema in such a way that it look like this:

// this will only be generated if we generate any new sql type
pub mod sql_types {
    pub use diesel::sql_types::*;

    // generated sql types here
}

table! {
    use super::sql_types::*;
    
    foo {
        id -> Integer,
    }
}

particular the naming of the newly generated structs

The naming currently just use the same pattern as previously generated by diesel cli.

On that topic, do you have some reference on why it was chosen to not have support for generating rust enums as part of schema inference, apart from #343 (comment)?

Basically this + my comment from gitter pasted in the thread below. If the corresponding SQL types are generated third party crates can easily provide an opinionated derive for those use cases.

[Ten0]

Looks good :)

About the naming/scoping of the structs

With the current proposition, SqlType structs are named at top level with the sql type name.
This may conflict/not be super intuitive/ergonomic should we later add auto-generation for the rust enums too via the CLI (even if it were to use diesel-derive-enum with derives not directly implemented in Diesel).

Why it may be important

Basically this + my comment from gitter pasted in the thread below. If the corresponding SQL types are generated third party crates can easily provide an opinionated derive for those use cases.

The advantage of having this as part of Diesel would be that in order to generate the rust enums by querying the database schema (to prevent mismatches between enum declarations in Rust and in SQL and reduce boilerplate, same reason why the Diesel CLI exists...), if this is an external crate, we would need an additional CLI , while this would otherwise easily be integrated as part of the existing Diesel CLI.

Considering the existing crate that serves this purpose: https://github.com/adwhit/diesel-derive-enum/blob/e4286e346fb7063577454cf2aee008f057d20f82/src/lib.rs#L81-L103, it looks like:

1. Such a feature would actually not have to be harshly opinionated: it could be a setting of the CLI.
2. Creating and maintaining an additional CLI seems to not have been worth the hassle enough that someone wanted to do it.

So anyway, not saying we should do it right now, but although this may not have been something that was appropriate to support four years ago, it might eventually become something we want to do and I think it's best if we leave the possibility for the structs namings to still be sound should we later decide to add support for it.

Solving

The design described at #2787 (comment) solves this issue (along with another one).

[Ten0]

Suggested change

	writeln!(out, "/// The `{}` SQL type", t.rust_name)?;
	writeln!(out, "/// The `{}` SQL type", t.sql_name)?;

Otherwise the name does not provide any new information (and may even be confusing).

[Ten0]

So if I understand correctly this one was not automatically added when there was an --import-types parameter, so people had to always specify it in --import-types themselves. Is that correct?

[weiznich]

Yes that's correct

[Ten0]

Additionally this commit tweaks the table! macro to import types from
the parent scope. This allows us to just reference those newly generated
types easily + simplifies the handling of other custom type imports in
my opinion. The old behaviour of having as part of the table!
definition remains supported.

The way I initially understood it, the table! macro itself was modified to always import items from the parent scope.
Instead, it seems we're adding the uses to the generated schema, which although is indeed practical to use still seems to keep the macro call a bit boilerplate-ish.

I'd favor always importing everything from the parent scope + ::diesel::sql_types::* from within the macro, making the file smaller.

If we don't want to pollute the table's namespace with all the other tables, we could consider generating custom types in a schema::sql_types module and make a table! call with no uses use super::sql_types::* as well as use diesel::sql_types::* (alternately instead-of and re-exported in schema::sql_types):

diesel::table! {
    custom_types (id) {
        id -> Int4,
        custom_enum -> MyType,
    }
}

pub mod sql_types {
    #[derive(diesel::SqlType)]
    #[postgres(type_name = "my_type")]
    pub struct MyType;
}

This would have the additional advantage that the SqlType Rust types would be scoped to the sql_types modules, not conflicting with their rust counterparts should we one day want to generate them as part of Diesel.

[weiznich]

Putting the types into a separate module is a great idea. I will change that as soon as I find some time to work on this again. That written I fear we cannot just automatically import a parent sql_types module here behind the scenes as this module may not exists for hand written table! macro calls and I would prefer not to require that module in that cases.

[weiznich]

Why it may be important

Basically this + my comment from gitter pasted in the thread below. If the corresponding SQL types are generated third party crates can easily provide an opinionated derive for those use cases.

The advantage of having this as part of Diesel would be that in order to generate the rust enums by querying the database schema (to prevent mismatches between enum declarations in Rust and in SQL and reduce boilerplate, same reason why the Diesel CLI exists...), if this is an external crate, we would need an additional CLI , while this would otherwise easily be integrated as part of the existing Diesel CLI.

It may be reasonable to add this to diesel itself in some future version, but I would prefer to the already low amount of time I can spend on working on diesel. So this is some feature I would put onto the "Implement this someday if there are more maintainers" list. That written, we should definitively design this feature in such a way that it enables this future extension.