Skip to content
This repository has been archived by the owner on Sep 21, 2020. It is now read-only.

Default value for hstore column #22

Closed
sxua opened this issue May 21, 2012 · 24 comments
Closed

Default value for hstore column #22

sxua opened this issue May 21, 2012 · 24 comments
Assignees

Comments

@sxua
Copy link

sxua commented May 21, 2012

It would be nice to set default value as an empty array with hstore(array[]::varchar[]). In that case you can smoothly use store_accessor, which is really cool.

@diogob
Copy link
Owner

diogob commented May 22, 2012

It does not seem a good practice database wise. What do you mean with smoothly use store_acessor? To be quite frank, I've never tried to use store_acessor with a hstore column.

@sxua
Copy link
Author

sxua commented May 22, 2012

store_accessor works just fine with hstore column (read-only). And about default, I mean ability to setup column like this: t.hstore :data, default: [] or something else.

@diogob
Copy link
Owner

diogob commented May 22, 2012

Do you mean t.hstore :data, default: {} ? Using an empty array instead of a hash does not seems a consistent behavior.

@sxua
Copy link
Author

sxua commented May 22, 2012

Yeah, hash, not array.

@diogob
Copy link
Owner

diogob commented May 22, 2012

Yeah, it would be nice indeed, I'll take a look into it.

@ghost ghost assigned diogob May 22, 2012
@amacneil
Copy link

Related to this, would it be possible for the default to always return an empty hash, rather than nil?

Currently it's hard to call things like row.data["key"], because if there are no keys it throws undefined method `[]' for nil:NilClass

Let me know if I'm missing something here :)

@sxua
Copy link
Author

sxua commented May 22, 2012

For now you can use default value in database, adding this to your migration:
execute("ALTER TABLE your_table ALTER COLUMN data SET DEFAULT hstore(array[]::varchar[])")

@chrisvariety
Copy link

+1 this would be great to have. I am looking to be able to do

Page.new.data[:test] = 'abc123'

without it blowing up about .data being nil by default.

I tried the ALTER TABLE... but it didn't seem to work.

@alhafoudh
Copy link

+1

1 similar comment
@mohangk
Copy link

mohangk commented Jun 19, 2012

+1

@chancancode
Copy link

@adrianmacneil I agree the ability to set a default column value in the migration would be nice to have, but I would not make that the "default" behaviour of the gem. NULL and {} are two different things.

@amacneil
Copy link

I'm aware NULL and {} are different things, but I don't think NULL is a very useful default (because as mentioned above it throws an exception if you try to access a key, so you would always need to test the nil case before accessing the hash). At least being able to specify the default in a migration would work around this though. But it would be nice if this gem could handle a NULL database value and still allow writing to the hash without testing for nil first.

@chancancode
Copy link

As I mention in #25, I see activerecord-postgres-hstore's role as library that provides lowest-level access (next to writing raw SQL) to HStore in ActiveRecord, so people could build useful stuff on top of it. It should expose all the available features that HStore exposes, and one of these features is the ability to assign NULL as the value for a key. This should take precedence over any connivence/sugar/abstraction that the gem may choose to provide (i.e. any "fix" for this "problem" should not break the ability to assign NULL to a key).

t.hstore :data, default: {} seems to be a good compromise. Or you could also do it at the active record level and initialize the value with an after_initialize callback.

@diogob
Copy link
Owner

diogob commented Jul 20, 2012

@chancancode I was thinking about the explicit default usage inside the migration. But sincerely I do not have the time to work on this now. If someone want to make a pull request with this behaviour though...
I'll leave the issue open.

@chancancode
Copy link

I'm wondering if this already works today. If you assign an empty string to the HStore column in SQL, it becomes an empty HStore. So perhaps...

t.hstore :data, default: ""

already works? I'll check later today.

@axsuul
Copy link

axsuul commented Aug 12, 2012

Just tried this today. Doesn't seem to give us an empty HStore

t.hstore :data, default: ""

@jjb
Copy link

jjb commented Nov 14, 2012

I think there are two things being discussed here:

  1. the basic ability to specify a default value in the migration, sent to the database. this is probably a non-controversial feature that would just be convenient to have in some cases (or irrelevant, depending on 2, below)
  2. how the gem represents a NULL column value (as nil, or as {}). I'll discuss this issue bellow.

Here's a problem I have, which might be (very?) common for users of this gem: I have a model Foo with an hstore column metadata. It's possible that it has no metadata at all, or many key/values. Wherever I access the metadata, I have to do ye olde @foo.metadata && @foo.metadata['my_key'] because @foo.metadata might be nil.

I would rather that in the case of a NULL metadata column, @foo.metadata returned {}. In my use case, metadata being nil has no semantic purpose.

In another use case, it might. However, even if I did have such a use case, I imagine that checking @foo.metadata.empty? would be semantically sufficient, and possibly preferable.

So maybe a good question to explore to move this forward: can anyone think of use cases where there is a semantic difference between {} and nil, so that support for both is desired?

@zacksiri
Copy link

zacksiri commented Dec 7, 2012

yes there is, what if i am iterating through a hash like this

my_stuff.data.each do |k, v| 
  puts "#{k}, #{v}"
end

.each fails because when initializing the hstore the data field = nil and each doesn't work with nil

for now i am doing after_initialize as a work around

@jjb
Copy link

jjb commented Dec 7, 2012

@zacksiri so if I understand, you would like data to be {} in that case, right?

I was asking if anyone had a case for supporting both {} and nil.

@zacksiri
Copy link

zacksiri commented Dec 7, 2012

@jjb yeah it makes sense to allow us to define a default option for the hstore field to an empty hash or nil to who ever wishes it to be nil

t.hstore :data, default: {}

@adamrobbie
Copy link

+1

@stevo
Copy link

stevo commented Dec 18, 2012

AR's extract_value_from_default method seems to ignore hstore-type defaults. This short patch deals with the issue

module ActiveRecord
  module ConnectionAdapters
    class PostgreSQLColumn < Column
      private
      class << self
        def extract_value_from_default_with_hstore(default)
          case default
            when NilClass
              nil
            when /(^+hstore|hstore$+)/
              {}
            else
              extract_value_from_default_without_hstore(default)
          end
        end

        alias_method_chain :extract_value_from_default, :hstore
      end
    end
  end
end

obviously we could extract default key/value pairs etc... but this is the simplest solution to make SET DEFAULT hstore(array[]::varchar[]) or SET DEFAULT '' actually work with ActiveRecord...

@alhafoudh
Copy link

@stevo That fix does something nasty and is not correct. The initial hstore value leaks from somewhere randomly.

I got:

...
t.hstore :options, null: false, default: {}
...

and your code snippet in config/initializers/hstore_fix.rb

Sometimes I got hstore initial value from other test case that I used on blank test case.

[8] pry(#<RSpec::Core::ExampleGroup::Nested_4::Nested_1>)> Action.new
=> #<Action id: nil, trigger_id: nil, type: nil, options: {"foo"=>"bar"}, position: 1, created_at: nil, updated_at: nil>
[9] pry(#<RSpec::Core::ExampleGroup::Nested_4::Nested_1>)> Action.new
=> #<Action id: nil, trigger_id: nil, type: nil, options: {"foo"=>"bar"}, position: 1, created_at: nil, updated_at: nil>
[10] pry(#<RSpec::Core::ExampleGroup::Nested_4::Nested_1>)> Action.new
=> #<Action id: nil, trigger_id: nil, type: nil, options: {"foo"=>"bar"}, position: 1, created_at: nil, updated_at: nil>
[11] pry(#<RSpec::Core::ExampleGroup::Nested_4::Nested_1>)> Action.new
=> #<Action id: nil, trigger_id: nil, type: nil, options: {"foo"=>"bar"}, position: 1, created_at: nil, updated_at: nil>
[12] pry(#<RSpec::Core::ExampleGroup::Nested_4::Nested_1>)> Action.new
=> #<Action id: nil, trigger_id: nil, type: nil, options: {"foo"=>"bar"}, position: 1, created_at: nil, updated_at: nil>
[13] pry(#<RSpec::Core::ExampleGroup::Nested_4::Nested_1>)> Action.new

@diogob diogob closed this as completed Feb 4, 2013
@diogob diogob closed this as completed Feb 4, 2013
@jjb
Copy link

jjb commented Feb 7, 2013

Looks like @chrisrhoden and @diogob actually added support for this a few days ago, whoo hoo!

5c2efd4

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

No branches or pull requests