AR supporting new JSON data type on PostgreSQL >= 9.2 #7527

Merged
merged 1 commit into from Sep 6, 2012

Conversation

Projects
None yet
5 participants
@guedes
Contributor

guedes commented Sep 5, 2012

Hello all!

The next PostgreSQL version (9.2) will supports a native JSON type. Once the 9.2 version will be release soon I think that would be nice if AR supports it too.

Before started this, I searched for someone that could be working on this already, and tweeted @tenderlove asking if he remembers about someone doing this job, since seems that nobody is working on this I'm sending this pull request and I'd like to know your opinions about this feature and about my implementation. I marked two "FIXMEs" that could be a DRY candidate, IMO. Maybe should AR:Store be changed too?

Thanks.

+ end
+
+ def teardown
+ @connection.execute 'drop table if exists json_data_type'

This comment has been minimized.

@robin850

robin850 Sep 5, 2012

Member

Do you mean this instead ?

@connection.execute 'drop table' if exists json_data_type
@robin850

robin850 Sep 5, 2012

Member

Do you mean this instead ?

@connection.execute 'drop table' if exists json_data_type

This comment has been minimized.

@guedes

guedes Sep 5, 2012

Contributor

Hi!

No, I'm not. json_data_type is the name I'm using for the test table. See line 16 above.

Thanks for your review.

@guedes

guedes Sep 5, 2012

Contributor

Hi!

No, I'm not. json_data_type is the name I'm using for the test table. See line 16 above.

Thanks for your review.

This comment has been minimized.

@robin850

robin850 Sep 5, 2012

Member

Sorry, I thought. ^^ BTW, thanks for this pull request, awesome.

@robin850

robin850 Sep 5, 2012

Member

Sorry, I thought. ^^ BTW, thanks for this pull request, awesome.

@@ -82,6 +82,15 @@
_SQL
end
+ if 't' == select_value("select 'json'=ANY(select typname from pg_type)")

This comment has been minimized.

@robin850

robin850 Sep 5, 2012

Member

Do you mean typename ? ^^

@robin850

robin850 Sep 5, 2012

Member

Do you mean typename ? ^^

This comment has been minimized.

@guedes

guedes Sep 5, 2012

Contributor

No, that statement is correct it must be typname because this is the column name from pg_type that gives me the name of type.

Thanks for your review!

@guedes

guedes Sep 5, 2012

Contributor

No, that statement is correct it must be typname because this is the column name from pg_type that gives me the name of type.

Thanks for your review!

This comment has been minimized.

@robin850

robin850 Sep 5, 2012

Member

Okay, sorry once again. ;)

@robin850

robin850 Sep 5, 2012

Member

Okay, sorry once again. ;)

@robin850

This comment has been minimized.

Show comment
Hide comment
@robin850

robin850 Sep 5, 2012

Member

I'm 👍 for this feature ! :-)

Member

robin850 commented Sep 5, 2012

I'm 👍 for this feature ! :-)

@carlosantoniodasilva

View changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+ else
+ string
+ end
+ end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

Shouldn't it use ActiveSupport::JSON?

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

Shouldn't it use ActiveSupport::JSON?

This comment has been minimized.

@guedes

guedes Sep 5, 2012

Contributor

Yes, I'm changing.

@guedes

guedes Sep 5, 2012

Contributor

Yes, I'm changing.

@carlosantoniodasilva

View changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+
+ def string_to_json(string)
+ if string.nil?
+ nil

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

I don't think you need to check for nil? here, since it's not a String, it'll return whatever string is, even if it's nil.

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

I don't think you need to check for nil? here, since it's not a String, it'll return whatever string is, even if it's nil.

This comment has been minimized.

@guedes

guedes Sep 5, 2012

Contributor

Agreed.

@guedes

guedes Sep 5, 2012

Contributor

Agreed.

@carlosantoniodasilva

View changes

activerecord/lib/active_record/connection_adapters/postgresql_adapter.rb
+ PostgreSQLColumn.json_to_string(value)
+ else
+ return super
+ end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

I think a case statement, pretty much as the other in line 586 that you changed, would come in handy here.

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

I think a case statement, pretty much as the other in line 586 that you changed, would come in handy here.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

Looks good, I've made a few comments. Thanks!

/cc @rafaelfranca @tenderlove

Looks good, I've made a few comments. Thanks!

/cc @rafaelfranca @tenderlove

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

And apparently after 9e0a14f you'll have to rebase and move some code around :)

And apparently after 9e0a14f you'll have to rebase and move some code around :)

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 5, 2012

Member

Oopss! I didn't see this pull request. Sorry

Member

rafaelfranca commented Sep 5, 2012

Oopss! I didn't see this pull request. Sorry

@guedes

This comment has been minimized.

Show comment
Hide comment
@guedes

guedes Sep 5, 2012

Contributor

@carlosantoniodasilva Thank you for your suggestions, I rebased from master and did the changes squashing my commits. I ran the tests against postgres 9.1 and 9.2RC1 and everything passed.

/cc @rafaelfranca

Contributor

guedes commented Sep 5, 2012

@carlosantoniodasilva Thank you for your suggestions, I rebased from master and did the changes squashing my commits. I ran the tests against postgres 9.1 and 9.2RC1 and everything passed.

/cc @rafaelfranca

@carlosantoniodasilva

View changes

activerecord/test/cases/adapters/postgresql/json_test.rb
+ @connection.create_table('json_data_type') do |t|
+ t.json 'payload', :default => {}
+ end
+ end

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

Do you need the transaction block to create only one table?

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

Do you need the transaction block to create only one table?

This comment has been minimized.

@guedes

guedes Sep 5, 2012

Contributor

No, I don't. I'll remove it.

@guedes

guedes Sep 5, 2012

Contributor

No, I don't. I'll remove it.

@carlosantoniodasilva

View changes

activerecord/lib/active_record/connection_adapters/postgresql/quoting.rb
+ when 'hstore' then PostgreSQLColumn.hstore_to_string(value)
+ when 'json' then PostgreSQLColumn.json_to_string(value)
+ else
+ super

This comment has been minimized.

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

I know this is nitpicking, but would you mind using else super like the other one above? :)

@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

I know this is nitpicking, but would you mind using else super like the other one above? :)

This comment has been minimized.

@guedes

guedes Sep 5, 2012

Contributor

Sure.

@guedes

guedes Sep 5, 2012

Contributor

Sure.

@carlosantoniodasilva

This comment has been minimized.

Show comment
Hide comment
@carlosantoniodasilva

carlosantoniodasilva Sep 5, 2012

Member

@guedes everything looks great! I just added another question, and I have to ask you to add a changelog entry to Active Record with the new json type for PostgreSQL.

Just ping us after that, and we'll merge. Thanks!

@guedes everything looks great! I just added another question, and I have to ask you to add a changelog entry to Active Record with the new json type for PostgreSQL.

Just ping us after that, and we'll merge. Thanks!

@guedes

This comment has been minimized.

Show comment
Hide comment
@guedes

guedes Sep 5, 2012

Contributor

Thanks for suggestions!

Contributor

guedes commented Sep 5, 2012

Thanks for suggestions!

@guedes

This comment has been minimized.

Show comment
Hide comment
@guedes

guedes Sep 5, 2012

Contributor

@carlosantoniodasilva and @rafaelfranca : I fixed the code following your suggestions. Thank you for you time on this!

Contributor

guedes commented Sep 5, 2012

@carlosantoniodasilva and @rafaelfranca : I fixed the code following your suggestions. Thank you for you time on this!

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 5, 2012

Member

Great! Should you squash the commits?

Member

rafaelfranca commented Sep 5, 2012

Great! Should you squash the commits?

@guedes

This comment has been minimized.

Show comment
Hide comment
@guedes

guedes Sep 5, 2012

Contributor

@rafaelfranca I squashed the commits and changed the commit message too. Tests still passing.

Thanks!

Contributor

guedes commented Sep 5, 2012

@rafaelfranca I squashed the commits and changed the commit message too. Tests still passing.

Thanks!

@steveklabnik

This comment has been minimized.

Show comment
Hide comment
@steveklabnik

steveklabnik Sep 6, 2012

Member

This is somehow out of date. CHANGELOGs! /me shakes his fist.

Member

steveklabnik commented Sep 6, 2012

This is somehow out of date. CHANGELOGs! /me shakes his fist.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 6, 2012

Member

Yes. Please rebase it.

Member

rafaelfranca commented Sep 6, 2012

Yes. Please rebase it.

ActiveRecord support to PostgreSQL 9.2 JSON type
This implements the support to encode/decode JSON
data to/from database and creating columns of type
JSON using a native type [1] supported by PostgreSQL
from version 9.2.

[1] http://www.postgresql.org/docs/9.2/static/datatype-json.html
@guedes

This comment has been minimized.

Show comment
Hide comment
@guedes

guedes Sep 6, 2012

Contributor

@rafaelfranca Sorry, I hope this is OK now. Thanks @steveklabnik to point me that.

Contributor

guedes commented Sep 6, 2012

@rafaelfranca Sorry, I hope this is OK now. Thanks @steveklabnik to point me that.

rafaelfranca added a commit that referenced this pull request Sep 6, 2012

Merge pull request #7527 from guedes/pg9.2_json_support
AR supporting new JSON data type on PostgreSQL >= 9.2

@rafaelfranca rafaelfranca merged commit a690935 into rails:master Sep 6, 2012

@guedes

This comment has been minimized.

Show comment
Hide comment
@guedes

guedes Sep 6, 2012

Contributor

Thanks for accepting this!

Contributor

guedes commented Sep 6, 2012

Thanks for accepting this!

@vjpr vjpr referenced this pull request in tgriesser/knex Jun 17, 2013

Merged

JSON datatype support for Postgres #20

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