Skip to content
Closed
53 changes: 52 additions & 1 deletion diesel/src/macros/macros_from_codegen.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,22 @@
/// This macro can only be used in combination with the `diesel_codegen` or
/// `diesel_codegen_syntex` crates. It will not work on its own.
macro_rules! infer_schema {
(extra_types_module = $extra_types_module: expr, $database_url: expr) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I really don't like the solution of the user manually specifying extra_types_module.

From a usability standpoint, it would be nicer to be able to infer_everything! and just get tables, types, and (eventually) views from the database.

From a design standpoint, this is just kicking the problem down the field a short distance. As-is, all peers to SQL types live in diesel::types, and library internals freely assume this. Looking up types that are not on a whitelist on a single user-specified alternate path works for now, but a few problems are readily evident:

  • All DB-derived types are assumed to live in one namespace. In Postgres, you have the potential for multiple namespaces (which naturally arise from each distinct database connection string, or even from schemas nested inside of a database). This could lead to name clashes if, for example, identically named types are declared in different databases or schemas. (It seems like a good solution is to recommend strongly that different invocations of infer_schema! use different extra_types_module settings, but I'd prefer a cleaner design where remembering to do that sort of thing isn't necessary.)
  • The binding between a DB type and a Rust type is implicit (based on type name alone). This loose coupling could lead to bugs whenever there are name clashes.
  • It is quite conceivable that a user will want to use types that come from disparate Rust namespaces, at which point they will have to do a lot of tedious and potentially confusing re-exporting with pub use (so that everything is in one namespace), or we will have to extend this mechanism to support having multiple extra type paths.

This could be solved by creating an intermediate representation of types in a database schema and mapping DB types to Rust types through that. This would require threading some sort of global table state through most of the code generation process, so the effects of adopting this design would be felt throughout a lot of Diesel. (Perhaps simply having a global shared structure would be adequate, since it will only be used at compilation time.)

Having a central, explicit set of data structures that describe types, tables, views, procedures, etc., could be quite useful. I suspect that some sort of compile-time metadata model would be very helpful for validating what is read from the database ("table has a primary key structure we don't support"), generating good error messages ("typename foobaz is not a valid Rust identifier"), debugging/testing (write tests against the intermediate representation code), streamlining code generation (all necessary state is in one obvious place), and error reporting (have richer local state from which to produce meaningful error messages).

This approach could go very wrong by being too top-heavy and prematurely putting straightjacketing Diesel. The project has gotten really far without building a top-down intermediate model of database components, and maybe it's best to keep it that way for now. It would be unpleasant to have to refactor a big metadata layer every time you want to support a new DB feature.

If it seems a good idea, I suggest working towards merging just the enum declaration part of this PR and opening a separate issue to discuss whether a metadata model is necessary or a good idea. That discussion may yield solutions to the problems I've described here which can be used to add DB type support to infer_schema! without having to commit to a great deal more overhead.

mod __diesel_infer_schema {
#[derive(InferSchema)]
#[infer_schema_options(database_url=$database_url, extra_types_module=$extra_types_module)]
struct _Dummy;
}
pub use self::__diesel_infer_schema::*;
};
(extra_types_module = $extra_types_module: expr, $database_url: expr, $schema_name: expr) => {
mod __diesel_infer_schema {
#[derive(InferSchema)]
#[infer_schema_options(database_url=$database_url, schema_name=$schema_name, extra_types_module=$extra_types_module)]
struct _Dummy;
}
pub use self::__diesel_infer_schema::*;
};
($database_url: expr) => {
mod __diesel_infer_schema {
#[derive(InferSchema)]
Expand Down Expand Up @@ -51,11 +67,16 @@ macro_rules! infer_schema {
/// This macro can only be used in combination with the `diesel_codegen` or
/// `diesel_codegen_syntex` crates. It will not work on its own.
macro_rules! infer_table_from_schema {
(extra_types_module = $extra_types_module: expr, $database_url: expr, $table_name: expr) => {
#[derive(InferTableFromSchema)]
#[infer_table_from_schema_options(database_url=$database_url, table_name=$table_name, extra_types_module=$extra_types_module)]
struct __DieselInferTableFromSchema;
};
($database_url: expr, $table_name: expr) => {
#[derive(InferTableFromSchema)]
#[infer_table_from_schema_options(database_url=$database_url, table_name=$table_name)]
struct __DieselInferTableFromSchema;
}
};
}

#[macro_export]
Expand Down Expand Up @@ -88,3 +109,33 @@ macro_rules! embed_migrations {
}
}
}

#[macro_export]
/// Queries the database for the names of all enum types and creates a Rust enum
/// type for each one in the current namespace. The enum type name and variants
/// of the database enum type will be translated from snake case to camel case
/// (so enum_type::variant becomes EnumType::Variant). For types in a schema
/// other than the default, the type will be created in a submodule with the
/// schema name.
///
/// This macro can only be used in combination with the `diesel_codegen` or
/// `diesel_codegen_syntex` crates. It will not work on its own.
Copy link
Member

Choose a reason for hiding this comment

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

Just a heads up: diesel_codegen_syntex will be removed in 0.10

macro_rules! infer_enums {
($database_url: expr) => {
mod __diesel_infer_enums {
#[derive(InferEnums)]
#[infer_enum_options(database_url=$database_url)]
struct _Dummy;
}
pub use self::__diesel_infer_enums::*;
};

($database_url: expr, $schema_name: expr) => {
mod __diesel_infer_enums {
#[derive(InferEnums)]
#[infer_enum_options(databsae_url=$database_url, schema_name=$schema_name)]
struct _Dummy;
}
pub use self::__diesel_infer_enums::*;
};
}
6 changes: 3 additions & 3 deletions diesel/src/types/impls/option.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,12 +7,12 @@ use expression::*;
use expression::bound::Bound;
use query_builder::QueryId;
use query_source::Queryable;
use types::{HasSqlType, FromSql, FromSqlRow, Nullable, ToSql, IsNull, NotNull};
use types::{ProvidesSqlTypeFor, HasSqlType, FromSql, FromSqlRow, Nullable, ToSql, IsNull, NotNull};

impl<T, DB> HasSqlType<Nullable<T>> for DB where
impl<T, DB> ProvidesSqlTypeFor<DB> for Nullable<T> where
DB: Backend + HasSqlType<T>, T: NotNull,
{
fn metadata() -> DB::TypeMetadata{
fn self_metadata() -> DB::TypeMetadata{
<DB as HasSqlType<T>>::metadata()
}
}
Expand Down
6 changes: 3 additions & 3 deletions diesel/src/types/impls/tuples.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use query_source::{QuerySource, Queryable, Table, Column};
use result::QueryResult;
use row::Row;
use std::error::Error;
use types::{HasSqlType, FromSqlRow, Nullable, IntoNullable, NotNull};
use types::{ProvidesSqlTypeFor, HasSqlType, FromSqlRow, Nullable, IntoNullable, NotNull};

macro_rules! tuple_impls {
($(
Expand All @@ -16,11 +16,11 @@ macro_rules! tuple_impls {
}
)+) => {
$(
impl<$($T),+, DB> HasSqlType<($($T,)+)> for DB where
impl<$($T),+, DB> ProvidesSqlTypeFor<DB> for ($($T,)+) where
$(DB: HasSqlType<$T>),+,
DB: Backend,
{
fn metadata() -> DB::TypeMetadata {
fn self_metadata() -> DB::TypeMetadata {
unreachable!("Tuples should never implement `ToSql` directly");
}
}
Expand Down
10 changes: 10 additions & 0 deletions diesel/src/types/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -249,10 +249,20 @@ pub type VarChar = Text;
#[cfg(feature = "postgres")]
pub use pg::types::sql_types::*;

pub trait ProvidesSqlTypeFor<DB> where DB: TypeMetadata {
Copy link
Member

Choose a reason for hiding this comment

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

I would love to see a doc comment here telling me what this type is all about. I assume it abstracts over structs and enums providing SQL types? Maybe also mention HasSqlType is implemented for everything that implements this trait.

fn self_metadata() -> DB::TypeMetadata;
}

pub trait HasSqlType<ST>: TypeMetadata {
fn metadata() -> Self::TypeMetadata;
}

impl<ST, DB> HasSqlType<ST> for DB where DB: TypeMetadata, ST: ProvidesSqlTypeFor<DB> {
fn metadata() -> Self::TypeMetadata {
<ST as ProvidesSqlTypeFor<DB>>::self_metadata()
}
}

pub trait NotNull {
}

Expand Down
6 changes: 5 additions & 1 deletion diesel_codegen/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,15 @@ repository = "https://github.com/diesel-rs/diesel/tree/master/diesel_codegen"
keywords = ["orm", "database", "postgres", "sql", "codegen"]

[dependencies]
syn = "0.10.3"
syntex_syntax = "0.52.0"
Copy link
Member

Choose a reason for hiding this comment

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

I hope we can not introduce this dependency. Syntex takes ages to compile.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed. I don't think this will be needed.

quote = "0.3.10"
diesel = { version = "0.9.0", default-features = false }
diesel_codegen_shared = { version = "0.9.0", default-features = false }

[dependencies.syn]
version = "0.10.3"
features = ["full"]
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the same inline table syntax as above?

syn = { version = "0.10.3", features = ["full"] }


[lib]
proc-macro = true

Expand Down
10 changes: 10 additions & 0 deletions diesel_codegen/src/lib.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
#![deny(warnings)]
#![recursion_limit = "512"]
Copy link
Member

Choose a reason for hiding this comment

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

What is this needed for? (It's fine, just maybe add a comment what requires this, so we can remove it when it's no longer needed.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The quote! macro makes use of (potentially deep) recursion, and my large invocations of it are running into the default limit. This number should be tuned to fit whatever this PR's eventual needs are; it may be that we wind up needing something smaller at first.


macro_rules! t {
($expr:expr) => {
Expand All @@ -13,6 +14,7 @@ extern crate diesel_codegen_shared;
extern crate diesel;
#[macro_use]
extern crate quote;
extern crate syntex_syntax;
extern crate proc_macro;
extern crate syn;

Expand All @@ -25,6 +27,8 @@ mod identifiable;
mod insertable;
mod model;
mod queryable;
#[cfg(feature = "postgres")]
mod type_inference;
#[cfg(any(feature = "postgres", feature = "sqlite"))]
mod schema_inference;
mod util;
Expand Down Expand Up @@ -74,6 +78,12 @@ pub fn derive_embed_migrations(input: TokenStream) -> TokenStream {
expand_derive(input, embed_migrations::derive_embed_migrations)
}

#[proc_macro_derive(InferEnums, attributes(infer_enum_options))]
#[cfg(feature = "postgres")]
pub fn derive_infer_enums(input: TokenStream) -> TokenStream {
expand_derive(input, type_inference::derive_infer_enums)
}

fn expand_derive(input: TokenStream, f: fn(syn::MacroInput) -> quote::Tokens) -> TokenStream {
let item = parse_macro_input(&input.to_string()).unwrap();
f(item).to_string().parse().unwrap()
Expand Down
39 changes: 25 additions & 14 deletions diesel_codegen/src/schema_inference.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
use syn;
// use syntex_syntax::symbol::keywords;
Copy link
Member

Choose a reason for hiding this comment

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

This is the only place where you use syntex right? Please please please remove this dependency if it's not going to be used, it takes ages to compile. (And if we need something that syn doesn't have in this regard, I'd gladly make a PR to syn!)

Copy link
Contributor Author

@dstu dstu Jan 18, 2017

Choose a reason for hiding this comment

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

Yes, I was trying to use syntex to work around some issues but don't need it right now. I'll kill the dependency if it's not needed (which it probably won't be) before any merge.

use quote;

use diesel_codegen_shared::*;
Expand All @@ -15,8 +16,9 @@ pub fn derive_infer_schema(input: syn::MacroInput) -> quote::Tokens {
.unwrap_or_else(|| bug());
let database_url = get_option(&options, "database_url", bug);
let schema_name = get_optional_option(&options, "schema_name");
let extra_types_module = get_optional_option(&options, "extra_types_module");

infer_schema_for_schema_name(&database_url, schema_name.as_ref().map(|s| &**s))
infer_schema_for_schema_name(&database_url, schema_name.as_ref().map(|s| &**s), extra_types_module)
}

pub fn derive_infer_table_from_schema(input: syn::MacroInput) -> quote::Tokens {
Expand All @@ -27,6 +29,7 @@ pub fn derive_infer_table_from_schema(input: syn::MacroInput) -> quote::Tokens {

let options = get_options_from_input("infer_table_from_schema_options", &input.attrs, bug)
.unwrap_or_else(|| bug());
let extra_types_module = get_optional_option(&options, "extra_types_module");
let database_url = get_option(&options, "database_url", bug);
let table_name = get_option(&options, "table_name", bug);

Expand All @@ -36,7 +39,7 @@ pub fn derive_infer_table_from_schema(input: syn::MacroInput) -> quote::Tokens {
.into_iter().map(syn::Ident::new);
let table_name = syn::Ident::new(table_name);

let tokens = data.iter().map(|a| column_def_tokens(a, &connection));
let tokens = data.iter().map(|a| column_def_tokens(extra_types_module, a, &connection));

quote!(table! {
#table_name (#(#primary_keys),*) {
Expand All @@ -45,19 +48,28 @@ pub fn derive_infer_table_from_schema(input: syn::MacroInput) -> quote::Tokens {
})
}

fn infer_schema_for_schema_name(database_url: &str, schema_name: Option<&str>) -> quote::Tokens {
fn infer_schema_for_schema_name(database_url: &str, schema_name: Option<&str>,
extra_types_module: Option<&str>) -> quote::Tokens {
let table_names = load_table_names(&database_url, schema_name).unwrap();
let schema_inferences = table_names.into_iter().map(|table_name| {
let mod_ident = syn::Ident::new(format!("infer_{}", table_name));
let table_name = match schema_name {
Some(name) => format!("{}.{}", name, table_name),
None => table_name,
};
quote! {
mod #mod_ident {
infer_table_from_schema!(#database_url, #table_name);
}
pub use self::#mod_ident::*;
match extra_types_module {
Some(m) => quote! {
mod #mod_ident {
infer_table_from_schema!(extra_types_module=#m, #database_url, #table_name);
}
pub use self::#mod_ident::*;
},
None => quote! {
mod #mod_ident {
infer_table_from_schema!(#database_url, #table_name);
}
pub use self::#mod_ident::*;
},
Copy link
Member

Choose a reason for hiding this comment

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

Can you refactor this to let macro_call = if let Some(type_module) = extra_types_module { quote! { infer_table_from_schema!(extra_types_module=#ype_module, #database_url, #table_name); } } else { quote! { infer_table_from_schema!(#database_url, #table_name); } } so the mod and pub use doesn't need to be repeated?

}
});

Expand All @@ -71,16 +83,15 @@ fn infer_schema_for_schema_name(database_url: &str, schema_name: Option<&str>) -
}

fn column_def_tokens(
extra_types_module: Option<&str>,
column: &ColumnInformation,
connection: &InferConnection,
) -> quote::Tokens {
let column_name = syn::Ident::new(&*column.column_name);
let column_type = determine_column_type(column, connection).unwrap();
let path_segments = column_type.path
.into_iter()
.map(syn::PathSegment::from)
.collect();
let tpe = syn::Path { global: true, segments: path_segments };
let column_type = determine_column_type(extra_types_module, column, connection).unwrap();
let path_segments =
column_type.path.into_iter().map(syn::PathSegment::from).collect();
let tpe = syn::Path { global: column_type.is_builtin, segments: path_segments };
let mut tpe = quote!(#tpe);

if column_type.is_array {
Expand Down
Loading