Skip to content

Add domain types configuration #4594

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
2 changes: 2 additions & 0 deletions diesel_cli/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -465,6 +465,8 @@ pub struct PrintSchema {
pub custom_type_derives: Option<Vec<String>>,
#[serde(default)]
pub sqlite_integer_primary_key_is_bigint: Option<bool>,
#[serde(default)]
pub domain_types: bool,
}

impl PrintSchema {
Expand Down
44 changes: 26 additions & 18 deletions diesel_cli/src/infer_schema_internals/inference.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,14 +162,17 @@ fn get_column_information(
conn: &mut InferConnection,
table: &TableName,
column_sorting: &ColumnSorting,
domain_types_enabled: bool,
) -> Result<Vec<ColumnInformation>, crate::errors::Error> {
let column_info = match *conn {
#[cfg(feature = "sqlite")]
InferConnection::Sqlite(ref mut c) => {
super::sqlite::get_table_data(c, table, column_sorting)
}
#[cfg(feature = "postgres")]
InferConnection::Pg(ref mut c) => super::pg::get_table_data(c, table, column_sorting),
InferConnection::Pg(ref mut c) => {
super::pg::get_table_data(c, table, column_sorting, domain_types_enabled)
}
#[cfg(feature = "mysql")]
InferConnection::Mysql(ref mut c) => super::mysql::get_table_data(c, table, column_sorting),
};
Expand Down Expand Up @@ -277,26 +280,31 @@ pub fn load_table_data(

let primary_key = get_primary_keys(connection, &name)?;

let column_data = get_column_information(connection, &name, &config.column_sorting)?
.into_iter()
.map(|c| {
let ty = determine_column_type(&c, connection, &name, &primary_key, config)?;
let column_data = get_column_information(
connection,
&name,
&config.column_sorting,
config.domain_types,
)?
.into_iter()
.map(|c| {
let ty = determine_column_type(&c, connection, &name, &primary_key, config)?;

let ColumnInformation {
column_name,
comment,
..
} = c;
let rust_name = rust_name_for_sql_name(&column_name);
let ColumnInformation {
column_name,
comment,
..
} = c;
let rust_name = rust_name_for_sql_name(&column_name);

Ok(ColumnDefinition {
sql_name: column_name,
ty,
rust_name,
comment,
})
Ok(ColumnDefinition {
sql_name: column_name,
ty,
rust_name,
comment,
})
.collect::<Result<_, crate::errors::Error>>()?;
})
.collect::<Result<_, crate::errors::Error>>()?;

let primary_key = primary_key
.iter()
Expand Down
60 changes: 47 additions & 13 deletions diesel_cli/src/infer_schema_internals/pg.rs
Original file line number Diff line number Diff line change
Expand Up @@ -90,25 +90,47 @@ pub fn get_table_data(
conn: &mut PgConnection,
table: &TableName,
column_sorting: &ColumnSorting,
domain_types_enabled: bool,
) -> QueryResult<Vec<ColumnInformation>> {
use self::information_schema::columns::dsl::*;
use diesel::sql_types::{Nullable, SingleValue};

let schema_name = match table.schema {
Some(ref name) => Cow::Borrowed(name),
None => Cow::Owned(Pg::default_schema(conn)?),
};

let query = columns
.select((
column_name,
udt_name,
udt_schema.nullable(),
__is_nullable,
character_maximum_length,
col_description(regclass(table), ordinal_position),
))
.filter(table_name.eq(&table.sql_name))
.filter(table_schema.eq(schema_name));
let query = if domain_types_enabled {
define_sql_function!(#[sql_name = "coalesce"] fn coalesce_maybe<T: SingleValue>(x: Nullable<T>, y: Nullable<T>) -> Nullable<T>);
define_sql_function!(#[sql_name = "coalesce"] fn coalesce_strict<T: SingleValue>(x: Nullable<T>, y: T) -> T);

columns
.select((
column_name,
coalesce_strict(domain_name, udt_name),
coalesce_maybe(domain_schema, udt_schema.nullable()),
__is_nullable,
character_maximum_length,
col_description(regclass(table), ordinal_position),
))
.filter(table_name.eq(&table.sql_name))
.filter(table_schema.eq(schema_name))
.into_boxed()
} else {
columns
.select((
column_name,
udt_name,
udt_schema.nullable(),
__is_nullable,
character_maximum_length,
col_description(regclass(table), ordinal_position),
))
.filter(table_name.eq(&table.sql_name))
.filter(table_schema.eq(schema_name))
.into_boxed()
};

match column_sorting {
ColumnSorting::OrdinalPosition => query.order(ordinal_position).load(conn),
ColumnSorting::Name => query.order(column_name).load(conn),
Expand Down Expand Up @@ -174,6 +196,8 @@ mod information_schema {
ordinal_position -> BigInt,
udt_name -> VarChar,
udt_schema -> VarChar,
domain_name -> Nullable<VarChar>,
domain_schema -> Nullable<VarChar>,
}
}
}
Expand Down Expand Up @@ -290,11 +314,21 @@ mod test {
ColumnInformation::new("array_col", "_varchar", pg_catalog, false, None, None);
assert_eq!(
Ok(vec![id, text_col, not_null]),
get_table_data(&mut connection, &table_1, &ColumnSorting::OrdinalPosition)
get_table_data(
&mut connection,
&table_1,
&ColumnSorting::OrdinalPosition,
false
)
);
assert_eq!(
Ok(vec![array_col]),
get_table_data(&mut connection, &table_2, &ColumnSorting::OrdinalPosition)
get_table_data(
&mut connection,
&table_2,
&ColumnSorting::OrdinalPosition,
false
)
);
}

Expand Down
6 changes: 6 additions & 0 deletions diesel_cli/tests/print_schema.rs
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,12 @@ fn run_infer_schema_django_bool_case() {
);
}

#[test]
#[cfg(feature = "postgres")]
fn run_infer_schema_domain_types_default_case() {
test_print_schema("print_schema_domain_types_default", vec!["--with-docs"]);
}

#[test]
fn run_infer_schema_exclude() {
test_print_schema(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
[print_schema]
file = "src/schema.rs"
with_docs = true
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
source: diesel_cli/tests/print_schema.rs
assertion_line: 509
description: "Test: print_schema_domain_types_default"
---
// @generated automatically by Diesel CLI.

diesel::table! {
/// Representation of the `mytable` table.
///
/// (Automatically generated by Diesel.)
mytable (id) {
/// The `id` column of the `mytable` table.
///
/// Its SQL type is `Int4`.
///
/// (Automatically generated by Diesel.)
id -> Int4,
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- https://www.postgresql.org/docs/current/domains.html
CREATE DOMAIN posint AS integer CHECK (VALUE > 0);

CREATE TABLE mytable (id posint PRIMARY KEY);
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
[print_schema]
file = "src/schema.rs"
with_docs = true
with_domain_types = true
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
---
source: diesel_cli/tests/print_schema.rs
assertion_line: 509
description: "Test: print_schema_domain_types_default"
---
// @generated automatically by Diesel CLI.

/// A module containing custom SQL type definitions
///
/// (Automatically generated by Diesel.)
pub mod sql_types {
/// The `posint` SQL type
///
/// (Automatically generated by Diesel.)
#[derive(diesel::query_builder::QueryId, Clone, diesel::sql_types::SqlType)]
#[diesel(postgres_type(name = "posint"))]
pub struct Posint;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just laying some thoughts here (but I'm not sure we should do what's suggested below): this loses information of what the underlying type is, which makes it the responsibility of the user to make sure that serialization is appropriate.

We might want to add this data to the type here, such as a new Domain derive and underling_type = "..." annotation to the SqlType derive, that would implement a corresponding Domain trait, and allow the user to make use of this type in their implementations of FromSql and ToSql (or even make the relevant implementations conditionally on implementations of some new traits that would define the mapping between input rust types and rust types that have ToSql/FromSql<UnderlyingType>) to fully close the door for mistakes here.

If we were to add that later by default, that would be a breaking change for users who already manually define their FromSql/ToSql implementations for these types.

But I'm not sure if that's ever worth the extra maintenance cost within Diesel itself...
Probably that wouldn't be very consistent with Diesel not supporting enums out-of-the-box, where ideally the CLI would also infer enum variants...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I fully agree that some information is being lost here and this should be avoided. I'd be willing to try something along the lines you suggested with some help (since I'm a newbie in this project).

And about this breaking compatibility, is there a way in diesel to add features in an "unstable" manner so it can be worked over multiple releases before being stabilized? If so, maybe we should think of this feature in layers, but I'm not sure about it either.

Copy link
Author

@lsunsi lsunsi May 10, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Ten0 I'm re-reading your comment and I'm positive this is the right path, but I'm not sure how to begin to propose a solution. Is there any pointer you could give me to similar code I could read up on in order to get up to speed on this implementation?

Edit:

Or even, how would the Domain trait look like? From what I can gather, you are suggesting: (1) the cli should output postgres_type = "posint" but also underlying_type = "bigint" for example, and (2) the derive should understand the "underlying type" property and generate a Domain implementation (3) could be used on the actual user-level From/ToSql implementations without messing up the type. Does this tracks or I'm way off mark?

In this situation, the Domain trait would be responsible for carrying the information about the "bigint" which was lost when "postgres_type" was set for the domain name.

(I'm just phrasing it so I can serialize my current understanding haha)

Edit2:

or even make the relevant implementations conditionally on implementations of some new traits that would define the mapping between input rust types and rust types that have ToSql/FromSql

I didn't fully grasp this one but it seems interesting too if you're willing to elaborate

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I'm not sure how to begin to propose a solution

I understand that this comment is blocked on me giving this issue some more thought. I'll come back to you when I have time to do that 😊🙏

For me the main question is whether we even want to do this here:

But I'm not sure if that's ever worth the extra maintenance cost within Diesel itself...
Probably that wouldn't be very consistent with Diesel not supporting enums out-of-the-box, where ideally the CLI would also infer enum variants...

Maybe other reviewers will have an opinion.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, I wasn't trying to pressure you or anything @Ten0 , sorry if it came off this way.

Is there any way we could release something without having stability guarantees on it? Maybe a feature a feature or something, maybe this could relieve some pressure and give us some time to think on this.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do currently not have a way to say: This feature is unstable, that by itself would need to be implemented first. That written: I'm not opposed to have something like that as unstable feature that can be explicitly enabled via an unstable.domain_type = true flag in the diesel.toml configuration.

Finally for the actual discussion: Given that domain types often introduce custom requirements on the underlying types it's hard to see how providing the built-in FromSql/ToSql impls for the underlying types would be helpful. I would guess that users ~always want to express these requirements in their rust code as well, so having these impls by default might be not helpful for them.

As for the discussion whether this should be a boolean flag or a list of type names: If somewhat possible I would prefer a list of type names or even a list of type name regexes there so that users can have finer control around where thy want domain types and where not.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @weiznich , thanks for the feedback!

I like the idea of prefixing with unstable, seems like a simpler way to avoid this possible improvement. That said, I'll leave this discussion for when we're comfortable with the PR so we know exactly what implementation we are talking about!

About the list of type names, I updated the implementation on 14cbf02 trying to implement a version of this idea. I think it looks way better than the boolean, do you mind checking it out and giving feedback?

I updated the tests too to reflect usage.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I do anything special for the diesel.toml file to be read on the test? I swear the commit passed the test once but I changed something and can't seem to do it again. haha.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it out 74f763d, added the CLI option

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I'm not sure if that's ever worth the extra maintenance cost within Diesel itself...

Let's decide that for now it isn't worth and not add an additional trait. (Discussed with weiznich today)

Regex seems OK. pg_domains_as_custom_types setting name seems OK.

}

diesel::table! {
use diesel::sql_types::*;
use super::sql_types::Posint;

/// Representation of the `mytable` table.
///
/// (Automatically generated by Diesel.)
mytable (id) {
/// The `id` column of the `mytable` table.
///
/// Its SQL type is `Posint`.
///
/// (Automatically generated by Diesel.)
id -> Posint,
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- https://www.postgresql.org/docs/current/domains.html
CREATE DOMAIN posint AS integer CHECK (VALUE > 0);

CREATE TABLE mytable (id posint PRIMARY KEY);
Loading