Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

handle freaky (heroku) bytea configurations #7383

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
10 participants
Contributor

ahoward commented Aug 17, 2012

see

http://stackoverflow.com/questions/8539207/activerecord-loads-binary-field-incorrectly-on-heroku-fine-on-osx

https://gist.github.com/3382094

https://devcenter.heroku.com/articles/heroku-postgresql#troubleshooting

to try to understand the issue this works around.

essentially, under certain conditions, PGconn.unescape_bytea is returning bunk/different data. this feature detects the situation and corrects it on the fly.

Member

steveklabnik commented Sep 21, 2012

This no longer merges cleanly, and will need to be rebased.

Contributor

frodsan commented Oct 26, 2012

@frodsan frodsan commented on the diff Oct 26, 2012

...verecord/test/cases/adapters/postgresql/bytea_test.rb
+require 'active_record/base'
+require 'active_record/connection_adapters/postgresql_adapter'
+
+class PostgresqlByteaTest < ActiveRecord::TestCase
+ class Bytea < ActiveRecord::Base
+ self.table_name = 'byteas'
+ end
+
+ Binary = Array.new(255){|i| i.chr}.join
+
+ def setup
+ @connection = ActiveRecord::Base.connection
+ begin
+ @connection.transaction do
+ @connection.create_table(Bytea.table_name) do |t|
+ t.binary 'data', :default => nil
@frodsan

frodsan Oct 26, 2012

Contributor

Please, use 1.9 hash syntax here. Thanks.

@tenderlove

tenderlove Oct 26, 2012

Owner

Let's not be so gung-ho with the 1.9 hash syntax. If we commit everything with 1.9 hash syntax, it means that backporting becomes a PITA.

@frodsan

frodsan Oct 26, 2012

Contributor

Sounds good, maybe after 4.0 release. But I think there is no problem to encourage this with new features because they can't be backported.

@tenderlove

tenderlove Oct 26, 2012

Owner

Encouraging it in new features is fine, but this is definitely a bug fix. :-)

@frodsan

frodsan Oct 26, 2012

Contributor

👍 I'll be more careful henceforth. Thanks.

@tenderlove

tenderlove Oct 26, 2012

Owner

No worries! :-D

@frodsan frodsan commented on the diff Oct 26, 2012

...verecord/test/cases/adapters/postgresql/bytea_test.rb
+ end
+
+ def test_unhex_bytea
+ assert_equal 'foobar', unhex_bytea('\\\\x666f6f626172')
+ end
+
+# c.query("select ''::bytea").values[0][0]
+ private
+ def assert_cycle data
+ # test creation
+ x = Bytea.create!(:data => data)
+ x.reload
+ assert_equal(data, x.data)
+
+ # test updating
+ x = Bytea.create!(:data => nil)
@frodsan

frodsan Oct 26, 2012

Contributor

ditto.

@frodsan frodsan commented on the diff Oct 26, 2012

...verecord/test/cases/adapters/postgresql/bytea_test.rb
+ assert_equal :binary, @column.type
+ end
+
+ def test_cycle
+ assert_cycle Binary
+ end
+
+ def test_unhex_bytea
+ assert_equal 'foobar', unhex_bytea('\\\\x666f6f626172')
+ end
+
+# c.query("select ''::bytea").values[0][0]
+ private
+ def assert_cycle data
+ # test creation
+ x = Bytea.create!(:data => data)
@frodsan

frodsan Oct 26, 2012

Contributor

ditto.

Member

lukaszx0 commented Mar 15, 2013

@tenderlove @jonleighton what's decided on that? Can we merge it?

Owner

rafaelfranca commented Mar 15, 2013

This needs a CHANGELOG entry and a rebase

Member

lukaszx0 commented Mar 15, 2013

@ahoward could you please rebase your branch and add CHANGELOG entry describing your changes? Thanks.

@schneems schneems commented on the diff Mar 23, 2013

...tive_record/connection_adapters/postgresql_adapter.rb
+ PGconn.unescape_bytea(value)
+ else
+ unhex_bytea(value)
+ end
+ end
+ end
+
+ # under some postgresql configurations, for instance on heroku, data may
+ # not round trip through bytea fields. this method detects that and
+ # caches the result
+ #
+ # ref: http://www.postgresql.org/docs/9.1/static/datatype-binary.html
+ # ref: http://www.postgresql.org/docs/9.1/static/runtime-config-client.html#GUC-BYTEA-OUTPUT
+ # ref: https://gist.github.com/3382094
+ # ref: http://stackoverflow.com/questions/8539207/activerecord-loads-binary-field-incorrectly-on-heroku-fine-on-osx
+ def symmetric_bytea
@schneems

schneems Mar 23, 2013

Member

It would be helpful for this method to have a question mark at the end as it returns a boolean value.

symetric_bytea?

@schneems schneems commented on the diff Mar 23, 2013

...tive_record/connection_adapters/postgresql_adapter.rb
@@ -545,7 +545,39 @@ def escape_bytea(value)
# NOTE: This is NOT an inverse of escape_bytea! This is only to be used
# on escaped binary output from database drive.
def unescape_bytea(value)
- PGconn.unescape_bytea(value) if value
@schneems

schneems Mar 23, 2013

Member

We can get rid of the nested conditional with an early return

return unless value
if symmetric_bytea?
  PGconn.unescape_bytea(value)
else
  unhex_bytea(value)
end
Member

schneems commented Mar 23, 2013

@ahoward are you still looking at this code, I made some style comments. We also need a rebase and a merge.

Member

arunagw commented May 18, 2013

Ping. Rebase required :-)

Member

arthurnn commented Apr 18, 2014

@schneems is this still a thing? what else is left so we can merge this?

Owner

matthewd commented Apr 18, 2014

The originally linked gist shows this as not being a thing on 4.0. (Which is a slight misdirect, what matters is actually the version of libpq... but you'd have to try pretty hard to find a pre-9.0 libpq these days.)

If we want to handle ancient libpq versions connecting to comparatively-recent servers, I think the correct fix would be in configure_connection:

# Per pg gem's specs, `escape_literal` was added to libpq in 9.0.
# `library_version`, which is what we really want, was only added in 9.1.
if postgresql_version >= 90000 && !PG::Connection.instance_methods.include?(:escape_literal)
  # The server supports hex encoding, but our libpq does not.
  execute("SET bytea_encoding TO 'escape'", 'SCHEMA')
end

@matthewd matthewd added the PostgreSQL label Apr 18, 2014

Member

arthurnn commented Apr 18, 2014

I will close this one, as seems like it doesnt apply anymore on latest rails 4.x versions.
Thanks for the PR, please let us know if this is still a problem, and how we could reproduce it.
thanks

@arthurnn arthurnn closed this Apr 18, 2014

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