Additional hstore tests, supporting null values, better compliance with postgres docs

2 files changed, 76 insertions, 28 deletions

diff --git a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
index ed82c4d6f6..52693e19d9 100644
--- a/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+++ b/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
@@ -56,36 +56,32 @@ module ActiveRecord
           if Hash === object
             object.map { |k,v|
               "#{escape_hstore(k)}=>#{escape_hstore(v)}"
-            }.join ', '
+            }.join ','
           else
-            kvs = object.scan(/(?<!\\)".*?(?<!\\)"/).map { |o|
-              unescape_hstore(o[1...-1])
-            }
-            Hash[kvs.each_slice(2).to_a]
+            hash_from_hstore_string(object)
           end
         end
 
         private
-        HSTORE_ESCAPE = {
-            ' '  => '\\ ',
-            '\\' => '\\\\',
-            '"'  => '\\"',
-            '='  => '\\=',
-        }
-        HSTORE_ESCAPE_RE   = Regexp.union(HSTORE_ESCAPE.keys)
-        HSTORE_UNESCAPE    = HSTORE_ESCAPE.invert
-        HSTORE_UNESCAPE_RE = Regexp.union(HSTORE_UNESCAPE.keys)
+        def hash_from_hstore_string(string)
+          Hash[string.scan(HstorePair).map { |k,v|
+            v = v.upcase == 'NULL' ? nil : v.gsub(/^"(.*)"$/,'\1').gsub(/\\(.)/, '\1')
+            k = k.gsub(/^"(.*)"$/,'\1').gsub(/\\(.)/, '\1')
+            [k,v]
+          }]
+        end
 
-        def unescape_hstore(value)
-          value.gsub(HSTORE_UNESCAPE_RE) do |match|
-            HSTORE_UNESCAPE[match]
-          end
+        HstorePair = begin
+          quoted_string = /"[^"\\]*(?:\\.[^"\\]*)*"/
+          unquoted_string = /(?:\\.|[^\s,])[^\s=,\\]*(?:\\.[^\s=,\\]*|=[^,>])*/
+          /(#{quoted_string}|#{unquoted_string})\s*=>\s*(#{quoted_string}|#{unquoted_string})/
         end
 
         def escape_hstore(value)
-          value.gsub(HSTORE_ESCAPE_RE) do |match|
-            HSTORE_ESCAPE[match]
-          end
+            value.nil?         ? 'NULL'
+          : value =~ /[=\s,>]/ ? '"%s"' % value.gsub(/(["\\])/, '\\\\\1')
+          : value == ""        ? '""'
+          :                      value.gsub(/(["\\])/, '\\\\\1')
         end
       end
       # :startdoc:
diff --git a/activerecord/test/cases/adapters/postgresql/hstore_test.rb b/activerecord/test/cases/adapters/postgresql/hstore_test.rb
index 33bf4478cc..28b5a50126 100644
--- a/activerecord/test/cases/adapters/postgresql/hstore_test.rb
+++ b/activerecord/test/cases/adapters/postgresql/hstore_test.rb
@@ -1,4 +1,6 @@
 require "cases/helper"
+require 'active_record/base'
+require 'active_record/connection_adapters/postgresql_adapter'
 
 class PostgresqlHstoreTest < ActiveRecord::TestCase
   class Hstore < ActiveRecord::Base
@@ -16,6 +18,7 @@ class PostgresqlHstoreTest < ActiveRecord::TestCase
     rescue ActiveRecord::StatementInvalid
       return skip "do not test on PG without hstore"
     end
+    @column = Hstore.columns.find { |c| c.name == 'tags' }
   end
 
   def teardown
@@ -23,19 +26,64 @@ class PostgresqlHstoreTest < ActiveRecord::TestCase
   end
 
   def test_column
-    column = Hstore.columns.find { |c| c.name == 'tags' }
-    assert column
-    assert_equal :hstore, column.type
+    assert_equal :hstore, @column.type
   end
 
   def test_type_cast_hstore
-    column = Hstore.columns.find { |c| c.name == 'tags' }
-    assert column
+    assert @column
 
     data = "\"1\"=>\"2\""
-    hash = column.class.cast_hstore data
+    hash = @column.class.cast_hstore data
     assert_equal({'1' => '2'}, hash)
-    assert_equal({'1' => '2'}, column.type_cast(data))
+    assert_equal({'1' => '2'}, @column.type_cast(data))
+
+    assert_equal({}, @column.type_cast(""))
+    assert_equal({'key'=>nil}, @column.type_cast('key => NULL'))
+    assert_equal({'c'=>'}','"a"'=>'b "a b'}, @column.type_cast(%q(c=>"}", "\"a\""=>"b \"a b")))
+  end
+
+  def test_gen1
+    assert_equal(%q(" "=>""), @column.type_cast({' '=>''}))
+  end
+
+  def test_gen2
+    assert_equal(%q(","=>""), @column.type_cast({','=>''}))
+  end
+
+  def test_gen3
+    assert_equal(%q("="=>""), @column.type_cast({'='=>''}))
+  end
+
+  def test_gen4
+    assert_equal(%q(">"=>""), @column.type_cast({'>'=>''}))
+  end
+
+  def test_parse1
+    assert_equal({'a'=>nil,'b'=>nil,'c'=>'NuLl','null'=>'c'}, @column.type_cast('a=>null,b=>NuLl,c=>"NuLl",null=>c'))
+  end
+
+  def test_parse2
+    assert_equal({" " => " "},  @column.type_cast("\\ =>\\ "))
+  end
+
+  def test_parse3
+    assert_equal({"=" => ">"},  @column.type_cast("==>>"))
+  end
+
+  def test_parse4
+    assert_equal({"=a"=>"q=w"},   @column.type_cast('\=a=>q=w'))
+  end
+
+  def test_parse5
+    assert_equal({"=a"=>"q=w"},   @column.type_cast('"=a"=>q\=w'))
+  end
+
+  def test_parse6
+    assert_equal({"\"a"=>"q>w"},  @column.type_cast('"\"a"=>q>w'))
+  end
+
+  def test_parse7
+    assert_equal({"\"a"=>"q\"w"}, @column.type_cast('\"a=>q"w'))
   end
 
   def test_select
@@ -54,6 +102,10 @@ class PostgresqlHstoreTest < ActiveRecord::TestCase
     assert_cycle('a' => 'b', '1' => '2')
   end
 
+  def test_nil
+    assert_cycle('a' => nil)
+  end
+
   def test_quotes
     assert_cycle('a' => 'b"ar', '1"foo' => '2')
   end