Hstore arrays fix (follow up for #11444) #13512

gsamokovarov · 2013-12-27T23:21:15Z

I took #11444, rebased it to the current master and addressed @senny comments.

I'm getting the following error locally, and I'm not currently sure if its happening because of the changes:

  1) Error:
TimestampTest#test_bc_timestamp:
ActiveRecord::StatementInvalid: PG::DatetimeFieldOverflow: ERROR:  date/time field value out of range: "0000-01-01 07:52:57.000000 BC"
: INSERT INTO "developers" ("created_at", "created_on", "name", "updated_at", "updated_on") VALUES ($1, $2, $3, $4, $5) RETURNING "id"
    /vagrant/rails/activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb:818:in `get_last_result'
   ...

Using this as a test bed to see if it also fails on Travis :)

egilburg · 2013-12-28T10:25:23Z

activerecord/lib/active_record/connection_adapters/postgresql/cast.rb

          if Hash === object
            object.map { |k,v|
              "#{escape_hstore(k)}=>#{escape_hstore(v)}"
-            }.join ','
+            }.join(',').tap { |s| s.replace escape_hstore(s) if array_member }


IMO .tap makes things confusing here. Perhaps assign to a variable and operate on it

Sounds good. Dropped the tap and force pushed.

guilleiguaran · 2013-12-29T06:25:04Z

Looks like pg tests are green on Travis!!!

/cc @senny

gsamokovarov · 2013-12-29T16:33:17Z

activerecord/CHANGELOG.md

@@ -1,3 +1,10 @@
+*   Perform necessary deeper encoding when hstore is inside an array, and return the
+    hstores as HashWithIndifferentAccess to be consistent with other forms.


I'm gonna change this since we dropped the indifferent access.

guilleiguaran · 2014-01-08T04:14:49Z

/cc @rafaelfranca

inopinatus · 2014-01-20T08:07:08Z

+1 endorsement by original author, with thanks for picking this up

hilem · 2014-01-23T18:26:44Z

+1

inopinatus · 2014-02-02T02:00:36Z

@senny please consider for merge, this one's been going over six months now but current PR appears to address all your remarks from #11444

carlosantoniodasilva · 2014-02-03T11:19:21Z

PR looks good, but it might be better if we extract all the tests to a separate file like array_hstore_test or something like that.

gsamokovarov · 2014-02-03T14:41:16Z

@carlosantoniodasilva They used to be in a separate file in #11444, but because they had some code duplication, @senny suggested to move them into one file.

senny · 2014-02-03T15:03:46Z

@gsamokovarov my comment also mentioned that most tests are duplication of existing functionality, judging from the patch size, is it necessary to keep all the tests? Isn't most functionality delegated and verified elsewhere already?

gsamokovarov · 2014-02-03T22:14:47Z

Oh, @senny, I got you now. Sorry for my misunderstanding.

Took a look at array_test.rb and removed the similar tests. Do you think we can actually move the new tests we have to array_test.rb as we're duplicating assert_array_cycle?

I did the changes in a separate commit (instead of amending), so you can see exactly what I did. @inopinatus do you think I cut something, that might have been important?

senny · 2014-02-13T09:19:00Z

activerecord/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Perform necessary deeper encoding when hstore is inside an array.
+
+    Fixes: #11135


nitpick, can you change the format to: Fixes #11135.

Sure. Found another Fixes: #12345 reference a couple of lines below, so put that into the desired format as well.

senny · 2014-02-13T09:21:09Z

@gsamokovarov can you squash your commits together and push a rebased version?

senny · 2014-02-13T09:26:13Z

activerecord/lib/active_record/connection_adapters/postgresql/cast.rb

              "#{escape_hstore(k)}=>#{escape_hstore(v)}"
-            }.join ','
+            }.join(',')
+            string.replace(escape_hstore(string)) if array_member


is there a reason to use string.replace over string = ?

Good catch.

gsamokovarov · 2014-02-13T14:39:39Z

Hey, I squashed this to 2 commits, so the original author still keeps his commit in the history.

senny · 2014-02-13T14:46:00Z

@gsamokovarov please squash the other one as well. You can set him as author and yourself as committer to give credits to both. Having a single commit makes the history more clear and possible reverts and backports more straight forward.

gsamokovarov · 2014-02-13T15:58:24Z

@senny done.

senny · 2014-02-13T16:03:36Z

@rafaelfranca any chance getting this in before 4.0.3 release?

hilem · 2014-02-14T01:42:21Z

+1, I've been using a patched version for awhile now waiting for this to get in.

rafaelfranca · 2014-02-14T01:45:10Z

@senny seems good to me.

inopinatus · 2014-02-16T22:02:26Z

@gsamokovarov I was happy with your update, you didn't cut anything I thought important. And I am not precious about attribution :-). Thanks for running with this, I am distracted by other things right now. Hoping this makes it into 4.0.3.

guilleiguaran · 2014-02-16T22:12:48Z

@gsamokovarov can you please rebase this and ping us immediately (via CF or IM) to merge it before of getting merge conflicts again? 😄

We didn't have enough encoding for the wire protocol to store an array of hstore types. So, further encode any hstore that is an array member. Whilst we're here, ensure it's an HashWithIndifferentAccess being returned, to be consistent with other serialized forms, and add testing for arrays of hstore. So now the following migration: enable_extension "hstore" create_table :servers do |t| t.string :name t.hstore :interfaces, array: true end produces a model that can used like this, to store an array of hashes: server = Server.create(name: "server01", interfaces: [ { name: "bge0", ipv4: "192.0.2.2", state: "up" }, { name: "de0", state: "disabled", by: "misha" }, { name: "fe0", state: "up" }, ]) More at http://inopinatus.org/2013/07/12/using-arrays-of-hstore-with-rails-4/

gsamokovarov · 2014-02-16T22:41:46Z

@guilleiguaran should merge cleanly now :)

Hstore arrays fix (follow up for #11444)

Backport #13512 to 4.0 stable

egilburg reviewed Dec 28, 2013
View reviewed changes

gsamokovarov reviewed Dec 29, 2013
View reviewed changes

rafaelfranca mentioned this pull request Jan 17, 2014

encoding issues with postgresql array-of-hstore columns #11135

Closed

ghost assigned rafaelfranca Jan 20, 2014

senny reviewed Feb 13, 2014
View reviewed changes

guilleiguaran added a commit that referenced this pull request Feb 16, 2014

Merge pull request #13512 from gsamokovarov/hstore_arrays_fix

7c32db1

Hstore arrays fix (follow up for #11444)

guilleiguaran merged commit 7c32db1 into rails:master Feb 16, 2014

gsamokovarov mentioned this pull request Feb 16, 2014

Backport #13512 to 4.0 stable #14078

Merged

guilleiguaran added a commit that referenced this pull request Feb 16, 2014

Merge pull request #14078 from gsamokovarov/hstore_arrays_fix_4_0

ad84512

Backport #13512 to 4.0 stable

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hstore arrays fix (follow up for #11444) #13512

Hstore arrays fix (follow up for #11444) #13512

gsamokovarov commented Dec 27, 2013

egilburg Dec 28, 2013

gsamokovarov Dec 28, 2013

guilleiguaran commented Dec 29, 2013

gsamokovarov Dec 29, 2013

guilleiguaran commented Jan 8, 2014

inopinatus commented Jan 20, 2014

hilem commented Jan 23, 2014

inopinatus commented Feb 2, 2014

carlosantoniodasilva commented Feb 3, 2014

gsamokovarov commented Feb 3, 2014

senny commented Feb 3, 2014

gsamokovarov commented Feb 3, 2014

senny Feb 13, 2014

gsamokovarov Feb 13, 2014

senny commented Feb 13, 2014

senny Feb 13, 2014

gsamokovarov Feb 13, 2014

gsamokovarov commented Feb 13, 2014

senny commented Feb 13, 2014

gsamokovarov commented Feb 13, 2014

senny commented Feb 13, 2014

hilem commented Feb 14, 2014

rafaelfranca commented Feb 14, 2014

inopinatus commented Feb 16, 2014

guilleiguaran commented Feb 16, 2014

gsamokovarov commented Feb 16, 2014

		@@ -1,3 +1,10 @@
		* Perform necessary deeper encoding when hstore is inside an array, and return the
		hstores as HashWithIndifferentAccess to be consistent with other forms.

		@@ -1,3 +1,9 @@
		* Perform necessary deeper encoding when hstore is inside an array.

		Fixes: #11135

Hstore arrays fix (follow up for #11444) #13512

Hstore arrays fix (follow up for #11444) #13512

Conversation

gsamokovarov commented Dec 27, 2013

egilburg Dec 28, 2013

Choose a reason for hiding this comment

gsamokovarov Dec 28, 2013

Choose a reason for hiding this comment

guilleiguaran commented Dec 29, 2013

gsamokovarov Dec 29, 2013

Choose a reason for hiding this comment

guilleiguaran commented Jan 8, 2014

inopinatus commented Jan 20, 2014

hilem commented Jan 23, 2014

inopinatus commented Feb 2, 2014

carlosantoniodasilva commented Feb 3, 2014

gsamokovarov commented Feb 3, 2014

senny commented Feb 3, 2014

gsamokovarov commented Feb 3, 2014

senny Feb 13, 2014

Choose a reason for hiding this comment

gsamokovarov Feb 13, 2014

Choose a reason for hiding this comment

senny commented Feb 13, 2014

senny Feb 13, 2014

Choose a reason for hiding this comment

gsamokovarov Feb 13, 2014

Choose a reason for hiding this comment

gsamokovarov commented Feb 13, 2014

senny commented Feb 13, 2014

gsamokovarov commented Feb 13, 2014

senny commented Feb 13, 2014

hilem commented Feb 14, 2014

rafaelfranca commented Feb 14, 2014

inopinatus commented Feb 16, 2014

guilleiguaran commented Feb 16, 2014

gsamokovarov commented Feb 16, 2014