Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for Postgresql JSONB #16220

Merged
merged 1 commit into from Jul 25, 2014
Merged

Conversation

pcreux
Copy link
Contributor

@pcreux pcreux commented Jul 18, 2014

This is the updated version of #15195 and #15950 by @pcreux and @chris-teague.

It is tested against postgresql-9.4beta.

Test cases shared between json and jsonb are extracted to a module called PostgresqlJSONSharedTestCases. I made my best to minimise the amount of lines impacted by this extraction.

@chancancode
Copy link
Member

chancancode commented Jul 18, 2014

👍

@@ -1,3 +1,7 @@
* Add support for Postgresql JSONB

Philippe Creux, Chris Teague
Copy link
Member

@arthurnn arthurnn Jul 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

*Philippe Creux*, *Chris Teague*

@chancancode
Copy link
Member

chancancode commented Jul 18, 2014

cc @senny, @matthewd, @sgrif This is the updated version of #15950, final review? 😄

@chancancode chancancode assigned senny and unassigned matthewd Jul 18, 2014
@@ -29,6 +29,22 @@ def accessor
ActiveRecord::Store::StringKeyedHashAccessor
end
end

class Jsonb < Json # :nodoc:
Copy link
Contributor

@sgrif sgrif Jul 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we put this in its own file?

Copy link
Contributor Author

@pcreux pcreux Jul 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure!

@sgrif
Copy link
Contributor

sgrif commented Jul 18, 2014

One minor comment, looks fine otherwise.

@pcreux
Copy link
Contributor Author

pcreux commented Jul 18, 2014

@sgrif I've just added /oid/jsonb.rb. The tests are passing on postgresql-9.4beta.

# the comparison here. Therefore, we need to parse and re-dump the
# raw value here to ensure the insignificant whitespaces are
# consitent with our encoder's output.
raw_old_value = type_cast_for_database(type_cast_from_database(raw_old_value))
Copy link
Contributor

@egilburg egilburg Jul 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this local variable change here recognized in super? does super share lexical scope with the subclass invoking it?

Copy link
Member

@matthewd matthewd Jul 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. Though IMO it's better to go with a more obviously-correct super(raw_old_value, new_value) in situations like this.

Copy link
Contributor Author

@pcreux pcreux Jul 18, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!

@senny
Copy link
Member

senny commented Jul 21, 2014

@pcreux looking good. Added a few comments then we should be good to go.

class PostgresqlJSONTest < ActiveRecord::TestCase
include PostgresqlJSONSharedTestCases

def column_type
Copy link
Contributor Author

@pcreux pcreux Jul 22, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@senny I prefer defining the method column_type over this:

def setup
  @column_type = :json
  super
end

I'm happy to use an instance variable instead if you prefer.

Copy link
Member

@senny senny Jul 23, 2014

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

method is fine.

@pcreux
Copy link
Contributor Author

pcreux commented Jul 24, 2014

@senny Rebased against master. Test are passing. It's ready for prime time!

@chris-teague
Copy link

chris-teague commented Jul 24, 2014

👍

[Philippe Creux, Chris Teague]
arunagw pushed a commit to arunagw/rails that referenced this pull request Jul 25, 2014
@senny senny merged commit 99b82fd into rails:master Jul 25, 2014
@senny senny mentioned this pull request Jul 25, 2014
@senny
Copy link
Member

senny commented Jul 25, 2014

thanks guys 💛

@sdogruyol
Copy link

sdogruyol commented Dec 19, 2014

Thanks for jsonb support 💙

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

Successfully merging this pull request may close these issues.

None yet

9 participants