change_column for postgres table with {array: true} option does not create array column in 4.0.0.rc2 #11062

Closed
Christian-G opened this Issue Jun 23, 2013 · 20 comments

Comments

Projects
None yet
8 participants

Although Rails 4.0.0.rc2 fixed the issue for add_column, the problem still exists with change_column:

class ChangeStoresInEvents < ActiveRecord::Migration
  def change
    change_column :events, :regmax, :integer, array: true
  end
end

The migration runs fine:

==  ChangeStoresInEvents: migrating ==============================
-- change_column(:events, :regmax, :integer, {:array=>true})
   -> 0.0008s
==  ChangeStoresInEvents: migrated (0.0009s) =====================

But the type is unchanged:

                                           Table "public.events"
        Column         |            Type             |                      Modifiers                      
-----------------------+-----------------------------+----------------------------------------------
 id                    | integer                     | not null default nextval('events_id_seq'::regclass)
 name                  | character varying(255)      | 
 created_at            | timestamp without time zone | 
 updated_at            | timestamp without time zone | 
 regmax                | integer                     | 

Member

senny commented Jun 23, 2013

this sounds familiar, I'll take a look and report back.

@ghost ghost assigned senny Jun 23, 2013

Member

senny commented Jun 23, 2013

currently change_table uses a different code path than add_table and ignores the option completely. It should be fairly simple to add support for array: true. However the question is, can you even change a column to an array type? What SQL would you expect? When changing an integer to and integer[] column I correctly get:

ActiveRecord::StatementInvalid: PG::Error: ERROR:  column "my_column" cannot be cast to type integer[]
: ALTER TABLE "pg_arrays" ALTER COLUMN "my_column" TYPE integer[]

Yeah, I'm afraid that simply changing the type to be array is not possible, you'd have to create a new column, migrate the data, drop the old column and rename the new one, in separate steps. But I may be wrong, so worth looking further :)

I far as I could find out, there is no implicit conversion from any data type to arrays, however that's what the USING clause is for in ALTER TABLE.

As Rails doesn't support the USING clause, there is no need to support {array: true} on change_column. However I think that failing silently for something that may be a common mistake is bad.

What about raising an exception when trying to change type to array?

Member

senny commented Jul 3, 2013

I'll take a look at this one tomorrow.

I am seeing this in master with string array. Any progress with this bug?

Contributor

take commented Aug 4, 2013

@filipegiusti

As Rails doesn't support the USING clause

why doesn't rails suppurt USING clause ? :O

Member

senny commented Aug 4, 2013

because you can fall back to raw SQL for special cases like this.

Sent from Mailbox for iPhone

On Sun, Aug 4, 2013 at 4:55 PM, Takehiro Adachi notifications@github.com
wrote:

@filipegiusti

As Rails doesn't support the USING clause

why doesn't rails suppurt USING clause ? :O

Reply to this email directly or view it on GitHub:
#11062 (comment)

Contributor

take commented Aug 5, 2013

I c, @senny ur not gonna push that commit to rails/rails? 😃 raising an ArugumentError sounds good to me :)

Member

senny commented Aug 5, 2013

I'd like to get other opinions before I push the commit. We can't protect the user from every possible mistake but this might be a reasonable case to catch. This issue is on my todo list.

Contributor

take commented Aug 5, 2013

I'd like to get other opinions before I push the commit. We can't protect the user from every possible mistake but this might be a reasonable case to catch.

I c! 😃 sounds right :)

Btw I thought that raising an error might be good since add_index started to raise error when invalid key is given.

And I went to see how add_index is raising error, and I found out that it's using {}.assert_valid_keys

Using that might be better since it'll raise an error when any invalid keys are given 😃

This issue is on my todo list.

ok! awesome!

Contributor

take commented Aug 5, 2013

Hmmm seems like all the db adapter's add_column and change_column doesn't check if the keys are valid or not, but add_index does... I'll look into it and see if I can send a PR

Member

senny commented Aug 5, 2013

@TAKEHIRO please wait until we discussed how to move further with this issue.

Contributor

take commented Aug 5, 2013

@senny sure thing 😄

Contributor

take commented Aug 5, 2013

Btw the add_index fails like this when an invalid key is given.

-- add_index(:tags, :name, {:limit=>30, :null=>false, :unique=>true})
rake aborted!
An error has occurred, all later migrations canceled:

Unknown key: limit/PATH/TO/APP//db/migrate/20XXXXXXXXXX_migration_file_name.rb:4:in `up'

add_index_error

I think a new line after "~key: limit" is needed :P

@senny senny closed this in 179deea Aug 6, 2013

Member

senny commented Aug 6, 2013

Recently I committed this patch 754a373 . It is necessary to make change_column work when changing the type of an array column. This also fixes this issue as it will raise:

ActiveRecord::StatementInvalid: PG::Error: ERROR:  column "my_column" cannot be cast to type integer[]
: ALTER TABLE "pg_arrays" ALTER COLUMN "my_column" TYPE integer[]

If you are going to try to change a non-array column to an array column.

ehtb added a commit to ehtb/rails that referenced this issue Aug 7, 2013

Contributor

take commented Aug 8, 2013

@senny aha i c awesome ! 😃

Shelvak commented Oct 3, 2013

In rails 4.0.0 don't work also.... I'm trying this... So... Well ^^
Thanks for prevent the spend time for the future attempts ^^

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