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

Use an enum as a column type #2

Open
ronen opened this issue Mar 13, 2015 · 22 comments
Open

Use an enum as a column type #2

ronen opened this issue Mar 13, 2015 · 22 comments

Comments

@ronen
Copy link
Member

ronen commented Mar 13, 2015

Should be able to use an enum as a column type, via

t.column :colname, :color, ...

or even maybe something like

t.enum.color :colname, ...

See, e.g., discussion at https://coderwall.com/p/azi3ka

[Issue pulled over from SchemaPlus/schema_plus#174 and SchemaPlus/schema_plus#181]

@ronen
Copy link
Member Author

ronen commented Mar 13, 2015

...and make sure it works in validations as per SchemaPlus/schema_validations#10

@ippeiukai
Copy link

If anyone else encountered SchemaPlus/schema_plus#181, I went around the error by setting config.active_record.schema_format = :sql. Makes sense to use db-dependent format since, after all, it's a quite Postgres-specific feature.

@saghaulor
Copy link

@ippeiukai ActiveRecord::Base.schema_format has been deprecated from Rails 4.

http://apidock.com/rails/ActiveRecord/Base/schema_format/class

As such, that will not work for Rails 4+.

@ippeiukai
Copy link

@saghaulor I think the config.active_record.schema_format is still there. Perhaps just the accessor is gone? At least http://edgeguides.rubyonrails.org/configuring.html#configuring-active-record still lists it. I'm using it with Rails 4.2 without a problem.

@saghaulor
Copy link

@ippeiukai It looks like it's still in the source, but it's been moved to lib/activerecord/core.rb from lib/activerecord/base.rb. However, it doesn't seem to be working. I guess I'll continue to dig around until I find a solution.

EDIT:
It's working. I was asleep at the wheel and looking in the wrong directory for the structure.sql dump.

@kellyarwine
Copy link

@ippeiukai I'm attempting to follow your suggestion as I'm having the same problem. Where do you put that line? (Using Rails 4.2)

@saghaulor
Copy link

@ksteensma you add it to config/application.rb

@kellyarwine
Copy link

@saghaulor tried that....I'm still getting an error.

This is my migration:
` def up
add_column :listings, :is_favorite, :boolean, null: false
add_column :listings, :is_error, :boolean, null: false

execute <<-SQL
  CREATE TYPE availability AS ENUM ('active', 'expired');
SQL

add_column :listings, :availability, :availability, null: false

`

Getting this error:
Could not dump table "listings" because of following StandardError Unknown type 'availability' for column 'availability'

Any help would be most appreciated!

@saghaulor
Copy link

@ksteensma you have to do a structure dump, not a schema dump. Rails doesn't support dumping schema for db specific datatypes. Please use rake db:structure:dump and rake db:structure:load

Also, your migration isn't utilizing the built in functions of the gem. Namely, use create_enum instead of executing raw sql.

@kellyarwine
Copy link

@saghaulor Thanks for the help. I didn't know about db:structure stuff. I got it working. Your last note about create_enum though, I did not get working. Is this part of Rails? All of my googling takes me to the PowerEnum gem which I was not using. I am not using any gem for enums.

@saghaulor
Copy link

You're commenting on the schema_plus_enums gem. I assumed that you're using
it.
On Sep 6, 2015 12:45 PM, "Kelly Steensma" [email protected] wrote:

@saghaulor https://github.com/saghaulor Thanks for the help. I didn't
know about db:structure stuff. I got it working. Your last note about
create_enum though, I did not get working. Is this part of Rails? All of
my googling takes me to the PowerEnum gem which I was not using. I am not
using any gem for enums.


Reply to this email directly or view it on GitHub
#2 (comment)
.

@kellyarwine
Copy link

Makes perfect sense. :)

@kevinjalbert
Copy link

I have set the config.active_record.schema_format = :sql so at least the enum type is recorded correctly to the structure.sql file. I really hoped it would be as simple to simply refer to the the defined type in the migration, but that doesn't seem possible.

The work around I found is to simply apply it as a separate add_column in the migration file. add_column :game, :status, :status, index: true appears to do the trick.

If it was possible to do this while retaining the :ruby schema.rb file that would be the best of both worlds. I have a feeling thats not really possible as the enum we're using is a SQL feature.

@ronen
Copy link
Member Author

ronen commented Sep 8, 2015

Thanks for the discussion, all. Unfortunately I don't have time to work on this, but I'd be happy to accept PRs.

It might not be super-hard to add another middleware module to the Dumper middleware, to check for enum-typed columns, and replace the inline t.column method with an add_column method (or even raw SQL) after the table definition block.

@pik
Copy link
Contributor

pik commented Nov 11, 2015

So if I understand correctly, currently the only way to schema:load while using schema_plus_enums is to use structure:dump / structure:load instead?

@ronen I would be willing to work on this if it means being able to generate something that's schema:load compatible.

@ronen
Copy link
Member Author

ronen commented Nov 11, 2015

@pik that's roughly it, but here's the detailed version

If you use an enum column in a table (we're talking postgresql here of couse), rails can't dump it as ruby since rails doesn't recognize the enum column type. So if you're using enums in a table you need to dump as sql. That's all independent of schema_plus_enums.

This gem is currently only minimally useful: it defines create_enum etc. methods that can be used in migrations, and dutifully dumps the appropriate create_enum statements to schema.rb. But it doesn't add any support for using enums. I.e. it:

  • Does not help define enum columns in a migration; it'd be natural to want to do something like

    t.enum :enumtype, :colname, ...  # maybe this syntax?
    t.enum[:enumtype] :colname, ...  # or this might be even better

    But currently you need to use add_column or raw SQL

  • Critically as per this thread it does not support dumping if a table has enum columns. At minimum it should prevent rails from raising an exception, and dump out an add_column statement after the table definition. But much better would of course be to dump out tables using a new t.enum syntax as per above. (A potential problem with just dumping out add_column statements is that it might complicate dumping out indexes--would need to make sure that any index referencing one of these columns wouldn't be defined in the dump until after its add_column statement.)

If you're willing to work on this (minimally or more) that'd be great! I'm happy to answer any questions you may have, but don't have time for serious work on it myself.

(And if you do get this gem more up to speed I'd be happy to give you ownership of it if you want!)

@ippeiukai
Copy link

re: defining enum columns in a migration,

t.enum :enumtype, :colname, ... is a bit inconsistent with t.column methods' arg order, and
t.enum[:enumtype] :colname, ... unfortunately is not achievable as correct Ruby.

t.enum :colname, enum_type: :enumtype perhaps?
I think we would end up with something similar to t.column :colname, :type anyway, which I believe already works well with create_enum:

  def change

    reversible do |direction|
      direction.up do
        create_enum :grade_letter, *%w[A+ A A- B+ B B- C+ C C- D+ D D- F]
      end
      direction.down do
        drop_enum :grade_letter
      end
    end

    create_table :course_grades do |t|
      t.references   :student,       null: false
      t.references   :course,        null: false
      t.column       :grade,         :grade_letter, null: false
    end

  end

@ronen
Copy link
Member Author

ronen commented Nov 11, 2015

@ippeiukai

t.enum[:enumtype] :colname, ... unfortunately is not achievable as correct Ruby.

Oops yeah have been writing too much coffeescript lately : ) But this could work:

t.enum.grade_letter :grade, ...

where t.enum returns a proxy object with a method_missing that looks up & verifies the enum type, then issues the appropriate underlying t.column call.

Though if t.column :colname, :type does work then the first pass/PR could be just to fix the schema dump to generate that instead of throwing an exception, and adding t.enum in the migration and dump could come later.

@pik
Copy link
Contributor

pik commented Dec 28, 2015

Hello, so I've started looking into this - I'm still familiarizing myself with schema_plus_core and the migration components of AR, so please correct me where I may be off.

The specification for adding an enum - with something like:

add_column(<table_name>, <column_name>, 'enum', { enumtype: <enum_name> })

Seems like it can be achieved quite readily with the current schema_plus_core/schema_monkey callback structure e.g. (in a before(env) block):

if type.to_sym == :enum && options[:enumtype]
  type = options.delete(:enumtype)
end

On the other-hand correct schema-dumping does not seem very nice or easy implementable from my understanding. The schema-dumper calls the super method which does all of the lifting https://github.com/SchemaPlus/schema_plus_core/blob/master/lib%2Fschema_plus%2Fcore%2Factive_record%2Fschema_dumper.rb#L50 and the super method does not return a partial dump of a table, but raises for the entire table (otherwise we could simply specify the rest of the columns additionally I think if an error existed along side valid parsed table structure).

Looking at AR the entire table dump block is one single begin/rescue: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/schema_dumper.rb#L111 which will trigger here: https://github.com/rails/rails/blob/master/activerecord/lib/active_record/schema_dumper.rb#L148

Valid types are determined here:
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L371
https://github.com/rails/rails/blob/master/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb#L73

@ronen would be great to have your advice on how you think we might do this without actively monkey-patching AR or the AR postgresql adapter and making such patching fit nicely into into the current schema_monkey/schema_core exposed API (it does not look like currently there is anything that can accomplish this - but I may be missing something).

@ronen
Copy link
Member Author

ronen commented Jan 18, 2016

@pik thanks for looking into this! (And sorry for the long delay. I haven't had a moment to spare in real life for the past month or so, just now having one or two moments and trying to catch up on github issues.)

You tracked down (thanks!) that the error is triggered at https://github.com/rails/rails/blob/master/activerecord/lib/active_record/schema_dumper.rb#L148, when

@connection.valid_type?(column.type)

returns false. So it seems like what's necessary is to arrange that @connection.valid_type?(column.type) will return true when column.type is a valid enum.

One way to do this would be as you point out, to patch the AR postgresql adapter. This gem already has a module that gets mixed in to the adapter; so all that would be needed would be to add to lib/schema_plus/enums/active_record.rb something along the lines of:

def valid_enum?(type)
   enums.include? type
end

def valid_type?(type)
   super || valid_enum?(type)
end

I'd probably try that first, and then do whatever else might be needed after that to get dumping working. (I don't know whether that would be it or whether more things would then fail farther down the line, which would need fixing.)

But as you sense, it's against the spirit of schema_plus to patch existing methods of AR... except in schema_plus_core. So once dumping is working using the above, I'd go add a new middleware stack to schema_plus_core that would wrap around valid_type? so that in this gem instead of overriding valid_type? I'd be able to get the same result in a middleware callback. I.e. in lib/schema_plus/enums/middleware.rb something along the lines of:

module SchemaPlus::Enums
  module Middleware
    module Postgresql
      module Schema
        module ValidTypeP
            def after(env)
                env.result ||= env.connection.valid_enum? env.type
            end
        end
    end
  end
end

Defining the middleware stack in schema_plus_core will be reasonably straightforward, just pattern matching on the existing stack definitions. (Trickiest thing I think will be to just be sure to define that stack so that it'll work for all adapters, even though in this case we'll only use it for postgresql.)

Does all that seem reasonable?

@quantumpotato
Copy link

Hey, any update? I tried what @kellyarwine did, found it on https://naturaily.com/blog/ruby-on-rails-enum
Still get this error. Rails 5.2.3

@hansololai
Copy link

hansololai commented Aug 21, 2019

The solution to change the schema dump to :sql for some reason is not acceptable for us, so I had to work out some magic to dump the schema.rb file with the extra stuff. But this will make your schema not runable by other adapters.

Basically I found the place where the SchemaDumper dump all tables, it is here, follow this function, you should see how every table is dumped.

If you make a file in initiallizers folder,

# postgres_enum_dumper.rb
module ActiveRecord
  module ConnectionAdapters
    class PostgreSQLAdapter
      NATIVE_DATABASE_TYPES.merge!(
        enum:  { name: 'character varying' },
      )
    end
   class SchemaDumper
      def dump(stream)
         header(stream)
         # Dump created enums
         extensions(stream)
         tables(stream)
         trailer(stream)
         stream
      end
   end
  end

This may be temporary as you are actually replacing the private functions and they are subject to change. But it is the best solution for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants