Skip to content
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

Support enumerated types #343

Closed
mkroman opened this issue May 24, 2016 · 22 comments

Comments

@mkroman
Copy link

commented May 24, 2016

Diesel does not currently support enumerated types created with PostgreSQL.

Documentation on enumerated types can be found here.

More documentation on CREATE TYPE can be found here.

Example

CREATE TYPE track_type AS ENUM ('download', 'stream');

CREATE TABLE track (
  id SERIAL,
  type_ track_type
);

When compiling, the generated code expects a type named like the custom data type.

error: type name `Track_type` is undefined or not in scope [--explain E0412]
 --> src/lib.rs:1:1
1 |> #![feature(custom_derive, custom_attribute, plugin)]
  |> ^ undefined or not in scope
<diesel macros>:5:1: 5:71: note: in this expansion of table_body! (defined in <diesel macros>)
src/schema.rs:4:1: 4:37: note: in this expansion of table! (defined in <diesel macros>)
src/schema.rs:4:1: 4:37: note: in this expansion of infer_schema! (defined in src/lib.rs)
help: no candidates by the name of `Track_type` found in your project; maybe you misspelled the name or forgot to import an external crate?
@killercup

This comment has been minimized.

Copy link
Member

commented Aug 30, 2016

FYI in SQLite you could try to do something similar with a text column and CHECK:

CREATE TABLE track (
  id INTEGER PRIMARY KEY,
  track TEXT CHECK(track IN ('download', 'stream')) NOT NULL
);
@carsonmyers

This comment has been minimized.

Copy link

commented Dec 19, 2016

Is there currently a good way around this? Like a custom de/serializer?

@dstu

This comment has been minimized.

Copy link
Contributor

commented Dec 21, 2016

As an outsider who looked through the code to figure out if there is a way to do this, it appears that supporting a user-defined Postgres type requires knowing the object identifier that is in use for that type. In the case of enums, this requires extracting information from pg_type and pg_enum tables. All told, you're looking at:

  • Getting your type information out of relevant postgres system catalogs. You'll need to refer to pg_type and pg_enum, along the lines of:
select pt.typname, pt.typtype, pt.typcategory, pe.enumlabel, pt.oid
  from pg_type pt
  inner join pg_enum pe on pe.enumtypid = pt.oid
  where pt.typcategory = 'E';
  • Creating peer/placeholder types, analogous to diesel::types::Date, for each type so discovered.
  • Registering these types with diesel. Refer to diesel/src/types/impls/ for examples.

Since runtime database information is needed, some sort of codegen macro along the lines of infer_schema! seems necessary. I'll probably take a crack at this over the next few days, but I can't promise much.

@dstu

This comment has been minimized.

Copy link
Contributor

commented Dec 23, 2016

I've put together a rough, partial implementation of Postgres enum support, which creates a macro called infer_enums that acts much like infer_schema, except it creates Rust types for each enum type it discovers in the database. Please be aware that these changes are owned by Google and not actually mine to redistribute until I get a copyright release, so I've (temporarily) deleted my public fork. I hope that I'll get an okay to make my changes public after the Christmas holiday.

Unfortunately, there are currently a few assumptions about type definitions that make it difficult to use custom enum types in tables:

  1. It is necessary to create a fresh impl of HasSqlType for each type that infer_enums discovers, but you cannot do that outside of the diesel crate. I think this can be worked around, but at present I don't see how to do this without a significant refactor.
  2. infer_schema assumes that column types correspond to types declared in the namespace diesel::types, which is no longer true if arbitrary new enum types can be created in other crates.

I haven't actually done the legwork necessary to know how to deal with encoding or decoding enum values to/from raw bytes for handoff with Postgres. I assume that this will essentially entail handling type-tagged integers.

Finally, handling schemas correctly is on the back burner until everything else is taken care of. It'll require an additional join.

@dstu

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2016

I've been able to address (1) and (2), but getting everything working in the integration tests has run head-long into #348. If anyone has successfully figured out the minimal footprint of traits needed to make a simple, atomic SQL type, some documentation or example code would be welcome.

@killercup

This comment has been minimized.

Copy link
Member

commented Dec 24, 2016

@dstu

This comment has been minimized.

Copy link
Contributor

commented Dec 24, 2016

My pleasure. I suspect that it will not be too difficult to extend this work to create Rust struct types automatically from user-created composite types in Postgres, and I'm also considering that.

What you described sound like for an inferred enum, SQL and Rust type are the same. I haven't thought much about this, maybe it makes sense.

That is exactly what the current implementation does. I have wondered a bit as to whether this is really the right thing to do, but I still believe it is. It might be a good idea store some metadata with the generated type (probably in a static impl of some DieselEnum trait), but I see no reason to stick it on a separate "backend" type. Future extensions might be backend-specific, such as when creating proper Rust enums for MySQL enum columns or providing an impl of Cmp that matches the ordering that Postgres imposes on enum variants, but I don't see a particular need for a different type.

There is probably a long-range design issue lurking behind the scenes here. I've already run into a few instances of code assuming that SQL types live in diesel::types, and the proposed change for #429 also does that. I'm working around this for now with a hard-coded list of built-in SQL types, but that seems a poor solution. If types are going to be generated during compilation like this, a compile-time type registry might be a very good idea. Otherwise, we are going to be left with relying on heuristics to determine how to resolve a given type name. For now, the heuristic is: if the type is builtin, resolve it absolutely in diesel::types, or else resolve it locally because a macro just generated it in the current namespace. This is brittle.

@jethrogb was [on Gitter][1] a few weeks ago and wanted to implement a custom type. Maybe you can get some additional information from the log there.

I think I have the necessary traits implemented, but I'm still running into problems. A second pair of eyes will help once I have permission to share a patch.

@kybishop

This comment has been minimized.

Copy link

commented Jan 5, 2017

Hey @sgrif, does your comment and gist here still apply as a valid workaround until this issue is sorted?

@dstu

This comment has been minimized.

Copy link
Contributor

commented Jan 9, 2017

I just got the OK to contribute and copyright release from my friendly neighborhood corporate overlords, so I'll reinstate my fork soon.

@dstu

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2017

Feature bumps and deprecations from the 1.15 release and deprecation of custom_derive (rust-lang/rust#29644) seem to have complicated things a little bit. I note that sh bin/test integration no longer runs successfully on the master branch (under rustc 1.16.0-nightly (47c8d9fdc 2017-01-08)). I believe that I was previously able to run that command to success on top of my changes in late December, on some older rust nightly. Please let me know what I should be doing to run tests.

I've pushed what was (more or less) working back in December to the custom_types_in_database_columns branch of my fork. I'd open a PR, but I don't think things are quite working yet. What's the deal with the test script?

@sgrif

This comment has been minimized.

Copy link
Member

commented Jan 10, 2017

Try rebasing on master and running on beta

@dstu

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2017

The most significant test failures seem to be #571, which is not directly related to adding support for enums. With a fix patched in locally, the problem I am currently running into is that types which are declared in a dummy namespace by infer_enums! don't seem to be visible when they are used in table declarations generated by infer_schema!. For example, diesel_tests/schema.rs currently reads:

infer_enums!("dotenv:DATABASE_URL");
pub use self::__diesel_infer_enums::UserType;  // Inserted by hand; shows that the type exists.
infer_schema!("dotenv:DATABASE_URL");  // Errors here because UserType isn't known.

I have tried changing the table declarations generated by InferSchema so that they use a relative type name. This is done in diesel_codegen_shared/src/schema_inference/pg.rs, where the path to non-builtin types is constructed. Using a type like super::UserType or super::__diesel_infer_enums::UserType doesn't work. In fact, if a type name like super::UserType is provided, the compiler helpfully says:

error[E0412]: unresolved type `super::UserType`
 --> tests/schema.rs:7:1
  |
7 | infer_schema!("dotenv:DATABASE_URL");                                                                                                                                                        
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ no resolution found
  |
  = help: possible candidates are found in other modules, you can import them into scope:
  = help:   `use schema::__diesel_infer_enums::UserType;`
  = help:   `use type_inference::postgres::__diesel_infer_enums::UserType;`
  = note: this error originates in a macro outside of the current crate

I suspect that converting a literal "self" or "super" with syn::PathSegment::from is not quite the right thing to do, since those are keywords that may require special treatment. But that does not explain why UserType wouldn't be visible when it is being exported explicitly with pub use.

A couple of different hacks (running infer_enums! in its own module and importing from that, trying to do enum declaration and table schema declaration in the same procedural macro) haven't panned out so far.

I'll have to try to boil this down to a simple example to verify it, but this is acting an awful lot like the enum types whose declarations I'm generating are not visible in the code that generates the table schema.

@dstu

This comment has been minimized.

Copy link
Contributor

commented Jan 14, 2017

You can work around the question of how to look up dynamically generated types by requiring that the user specify the module where any non-builtin types can be found, so that's how things now work. This isn't great (and raises questions about how to deal with types derived from schemas, which one might reasonably want to live in sub-modules), but it looks like it works for now.

I must have been recollecting wishfully when I said that I thought most tests were passing earlier. I'm now running into some deficiencies in implementing the interfaces needed to make a custom type queryable/insertable/updateable/etc. This is all in diesel_codegen/src/type_inference.rs. An experienced eye there would be very helpful in noticing any obvious omissions or deficiencies.

@dstu dstu referenced this issue Jan 15, 2017
0 of 6 tasks complete
@dstu

This comment has been minimized.

Copy link
Contributor

commented Jan 15, 2017

I will continue further discussion of the implementation I'm working on in #580.

@dstu

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2017

#580 has been closed as discussed there. (It needed to be factored apart and fell too far behind diesel main.)

I have been swamped with events at work and in my personal life, and this has prevented me from pulling a set of new PRs together. I'd be happy to discuss this further with anyone who is interested in getting a PR together sooner, and I will share whatever I can put together, when I can.

@coder543

This comment has been minimized.

Copy link

commented Jun 2, 2017

I just encountered this bug. Not having this feature means I'm (at least for now) switching from strongly-typed enums in my database to varchars... which is not great. I would love to have this feature.

@weiznich weiznich referenced this issue Jul 5, 2017
1 of 4 tasks complete
@dsvensson

This comment has been minimized.

Copy link

commented Aug 13, 2017

@dstu it would be nice if inferring schema is not required, other than perhaps creating a first example of a Rust enum (which might then be renamed, while keeping some annotation or so for the name used in Postgres... mapping)... and then connecting the dots upon actual connect, rather than having any kind of ids hardcoded. Or did I misunderstand you there when you mentioned "Since runtime database information is needed, some sort of codegen macro along the lines of infer_schema! seems necessary" ?

@turboladen

This comment has been minimized.

Copy link

commented Oct 26, 2017

FWIW, if working with pg enums in diesel meant having to define a rust annotated struct (or... enum?) to represent the pg enum, that'd seem totally reasonable to me.

@sgrif

This comment has been minimized.

Copy link
Member

commented Dec 16, 2017

So I'm going to close this as a duplicate of #162. At this point, everything needed for absolute minimum "support" for PG enums is there. We even have a test case for this in Diesel itself.

I'm aware that this requires more code than people would like right now (this is true of implementing new types in general, not just enums). #162 is the tracking issue for that, and is going to be the main focus of 1.1.

Ideally you'll just need to provide an impl of ToSql, FromSql, and HasSqlType (I originally wanted to have the rest of the traits come from blanket impls, which requires changes in the language, and is why there hasn't been much movement on this. At this point I'm thinking we'll just provide some derives for the other traits).

At this point, I do not want to provide any special support for PG enums (e.g. infer_enums!(), diesel print-enums, or #[derive(PgEnum)]). Such a feature would have to have some very strong opinions on the mapping, that I don't want to have in Diesel. That said, there's no reason that one of those features couldn't be supplied by another crate. I'm happy to help someone with the initial implementation of such a crate, but I wouldn't want it under the diesel org.

TL;DR: Custom types in general are "supported". It's going to get more ergonomic in 1.1. We aren't going to provide special handling for PG enums.

@sgrif sgrif closed this Dec 16, 2017

@coder543

This comment has been minimized.

Copy link

commented Dec 16, 2017

I don't necessarily agree with the decision, but I appreciate the way you're approaching the situation. I personally think enums are important enough to be a special case, but as long as it is possible to support them in Diesel, that's good enough for me. As you said, an outside crate could be developed to provide an opinionated solution to this issue that could act as a way to reduce boilerplate for those who are interested.

Looking at the example code you linked to, it seems pretty reasonable to me. My only real question at this point: is using infer_schema! is possible with a database that has custom types in some/all of the tables? I've been busy with other projects for awhile now, but from what I recall, infer_schema! might have actually generated code which could not compile in such cases, and I don't remember finding a way to inject the custom types into the namespace that infer_schema! created.

@sgrif

This comment has been minimized.

Copy link
Member

commented Dec 17, 2017

@adwhit

This comment has been minimized.

Copy link

commented Jan 4, 2018

Just thought I'd mention to anyone who comes across this issue that I've made a crate to derive the necessary trait impls, it's up on crates.io. Feedback welcome.

@Boscop Boscop referenced this issue Jan 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.