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

Issue 2581 -- support for postgresql json type #50

Merged
merged 6 commits into from Dec 17, 2013

Conversation

Projects
None yet
2 participants
@nathan-rice
Contributor

nathan-rice commented Dec 9, 2013

Very simple, based on the HSTORE data type.

I added support on the type itself for custom serializer/deserializer, and a different json module can be monkey patched in and will work properly (assuming it is api-compatible with python's json module, which most of them are).

nathan-rice added some commits Dec 9, 2013

sqlalchemy/dialects/postgresql/__init__.py:
- Added import references to JSON class

 sqlalchemy/dialects/postgresql/base.py:
 - Added visitor method for JSON class

 sqlalchemy/dialects/postgresql/pgjson (new):
 - JSON class, supports automatic serialization and deserialization of json data, as well as basic json operators.
@zzzeek

View changes

Show outdated Hide outdated lib/sqlalchemy/dialects/postgresql/pgjson.py Outdated
@zzzeek

View changes

Show outdated Hide outdated lib/sqlalchemy/dialects/postgresql/pgjson.py Outdated
else:
def process(value):
return self.json_deserializer(value)
return process

This comment has been minimized.

@zzzeek
@zzzeek

zzzeek Dec 9, 2013

Owner

This comment has been minimized.

@nathan-rice

nathan-rice Dec 9, 2013

Contributor

Thanks for this note. It turns out that psycopg2's json support is pretty recent (up to date ubuntu packages don't have it), and it is on by default, so this code will actually error tests if psycopg2 is up to date. I'll see what I can do about this.

@nathan-rice

nathan-rice Dec 9, 2013

Contributor

Thanks for this note. It turns out that psycopg2's json support is pretty recent (up to date ubuntu packages don't have it), and it is on by default, so this code will actually error tests if psycopg2 is up to date. I'll see what I can do about this.

This comment has been minimized.

@nathan-rice

nathan-rice Dec 9, 2013

Contributor

There is a problem having a use_native_json kwarg. Specifically, in order to not use native json with psycopg2 >= 2.5, you have to monkey-patch the module. I could check on dialect init to see if the module has been monkey-patched and revert if necessary, but I think it would be better to just pass on letting the user turn off json processing.

@nathan-rice

nathan-rice Dec 9, 2013

Contributor

There is a problem having a use_native_json kwarg. Specifically, in order to not use native json with psycopg2 >= 2.5, you have to monkey-patch the module. I could check on dialect init to see if the module has been monkey-patched and revert if necessary, but I think it would be better to just pass on letting the user turn off json processing.

This comment has been minimized.

@zzzeek

zzzeek Dec 10, 2013

Owner

that's not a problem, we don't need a "use_native_json" kwarg then - just use psycopg2's facility when it is detected (though we have to figure out a decent way to test the non-psycopg2 version considering that the tests usually run just on psycopg2...)

@zzzeek

zzzeek Dec 10, 2013

Owner

that's not a problem, we don't need a "use_native_json" kwarg then - just use psycopg2's facility when it is detected (though we have to figure out a decent way to test the non-psycopg2 version considering that the tests usually run just on psycopg2...)

This comment has been minimized.

@zzzeek

zzzeek Dec 10, 2013

Owner

ok we maybe can test it if we detect psycopg2 using their Json object with a null-dumper, but don't worry about that part, that's kind of intricate and I'll figure out how that might work. if we can just get tests to pass for now that's enough for the PR.

@zzzeek

zzzeek Dec 10, 2013

Owner

ok we maybe can test it if we detect psycopg2 using their Json object with a null-dumper, but don't worry about that part, that's kind of intricate and I'll figure out how that might work. if we can just get tests to pass for now that's enough for the PR.

@zzzeek

View changes

Show outdated Hide outdated test/dialect/postgresql/test_types.py Outdated
@zzzeek

This comment has been minimized.

Show comment
Hide comment
@zzzeek

zzzeek Dec 9, 2013

Owner

looks very nice, if we can add the psycopg2 support and fix a few typos we'll be in good shape :)

Owner

zzzeek commented Dec 9, 2013

looks very nice, if we can add the psycopg2 support and fix a few typos we'll be in good shape :)

nathan-rice added some commits Dec 10, 2013

sqlalchemy/dialects/postgresql/pgjson:
 - Fixed reference to HSTORE
 - Corrected spelling of SQLAlchemy

 sqlalchemy/dialects/postgresql/psycopg2:
 - Added psycopg2 specific wrapper type for JSON which uses inherent json deserialization facilities
 - Added code to detect and utilize the JSON wrapper if psycopg2 >= 2.5

test/dialect/postgresql/test_types:
- removed reference to use_native_hstore
sqlalchemy/dialects/postgresql/psycopg2:
 - Removed unneeded import of psycopg2.extensions
sqlalchemy/dialects/postgresql/pgjson:
 - Added support for additional operators
 - Made return as json default (rather than text)
sqlalchemy/dialects/postgresql/pgjson:
 - Updated documentation for JSON class
@nathan-rice

This comment has been minimized.

Show comment
Hide comment
@nathan-rice

nathan-rice Dec 11, 2013

Contributor

This should be good to go now.

Contributor

nathan-rice commented Dec 11, 2013

This should be good to go now.

@@ -363,6 +377,7 @@ def initialize(self, connection):
self._has_native_hstore = self.use_native_hstore and \
self._hstore_oids(connection.connection) \
is not None
self._has_native_json = self.psycopg2_version >= (2, 5)

This comment has been minimized.

@zzzeek

zzzeek Dec 17, 2013

Owner

nailed it! nice.

@zzzeek

zzzeek Dec 17, 2013

Owner

nailed it! nice.

)
class JSONRoundTripTest(fixtures.TablesTest):

This comment has been minimized.

@zzzeek

zzzeek Dec 17, 2013

Owner

great job on this

@zzzeek

zzzeek Dec 17, 2013

Owner

great job on this

this when you need to cast the type of the returned value."""
return self.expr.op('->>', precedence=5)(other)
def get_path(self, other):

This comment has been minimized.

@zzzeek

zzzeek Dec 17, 2013

Owner

im thinking of streamlining the operators here. Looking at http://www.postgresql.org/docs/9.3/static/functions-json.html it seems like our operators are: getitem, getarray, and then both have the option to be "text".

so how about this:

json_col['some_element']    # get the element
json_col.astext['some_element']    # get the element as text
json_col[('a', 'b', 2)]  # get by path
json_col.astext[('a', 'b', 2)] # get by path as text

seems like also these ops are only in pg 9.3....ill add some qualifiers to the tests and I also want to add some round trips for these. going to install 9.3 now...

@zzzeek

zzzeek Dec 17, 2013

Owner

im thinking of streamlining the operators here. Looking at http://www.postgresql.org/docs/9.3/static/functions-json.html it seems like our operators are: getitem, getarray, and then both have the option to be "text".

so how about this:

json_col['some_element']    # get the element
json_col.astext['some_element']    # get the element as text
json_col[('a', 'b', 2)]  # get by path
json_col.astext[('a', 'b', 2)] # get by path as text

seems like also these ops are only in pg 9.3....ill add some qualifiers to the tests and I also want to add some round trips for these. going to install 9.3 now...

This comment has been minimized.

@nathan-rice

nathan-rice Dec 17, 2013

Contributor

That is a much nicer interface, for sure.

What all are you planning on coding? I'm happy to code up all the stuff you mentioned, I just don't want to duplicate work.

@nathan-rice

nathan-rice Dec 17, 2013

Contributor

That is a much nicer interface, for sure.

What all are you planning on coding? I'm happy to code up all the stuff you mentioned, I just don't want to duplicate work.

This comment has been minimized.

@zzzeek

zzzeek Dec 17, 2013

Owner

i think you're done! im just going to try to test this stuff out and familiarize, make that change, add a few more tests. you've laid out all the groundwork here so thanks!

@zzzeek

zzzeek Dec 17, 2013

Owner

i think you're done! im just going to try to test this stuff out and familiarize, make that change, add a few more tests. you've laid out all the groundwork here so thanks!

@zzzeek zzzeek merged commit c64b7aa into zzzeek:master Dec 17, 2013

zzzeek pushed a commit that referenced this pull request Jan 7, 2016

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