djangonauts / django-hstore

PostgreSQL HStore support for Django.
http://django-hstore.readthedocs.io/
Other
517 stars 142 forks source link

HSTORE adapter registration in psycopg2 conflicts with SQLAlchemy #45

Closed gthb closed 10 years ago

gthb commented 10 years ago

If both django_hstore and SQLAlchemy are used in the same Python process with psycopg2, then django_hstore's adapter registration will cause psycopg2 to return HStoreDict instances to SQLAlchemy, which expects str. This causes errors like AttributeError: 'HStoreDict' object has no attribute 'decode' in sqlalchemy/dialects/postgresql/hstore.py:

    def process(value):
        if value is not None:
            return _parse_hstore(value.decode(encoding))

Some solution for the two to coexist would be good, for people who are using both Django and SQLAlchemy together.

One way might be to register the adapter only per connection/cursor, rather than globally (probably per cursor would be required for people using https://github.com/kennethreitz/django-postgrespool since the connections get shared).

niwinz commented 10 years ago

Hi!

Registering in each connection seems to be much overhead for normal use case, in my opinion, and per connection registering works well with connection pools because it register hstore on each connection retrieval from pool.

(With https://github.com/djangonauts/djorm-ext-pool works well and it not requires special backend...)

gthb commented 10 years ago

“Registering in each connection seems to be much overhead” — did you mean “in each cursor”?

I agree (if that's what you meant) that per-connection makes more sense. The main thing is to get rid of globally=True, to allow django_hstore to coexist with SQLAlchemy.

nemesifier commented 10 years ago

Another settings like: https://github.com/djangonauts/django-hstore/commit/02d398ce1e108748e8c657938600b1f6eaf0a7a4 ?

niwinz commented 10 years ago

Yes @gthb! I was wrong!

Additional config with good entry on documentation for notify about it for any sqlalchemy users seems a good solution...

I'm currently very busy and I can't find time for it en almost three weeks :(

nemesifier commented 10 years ago

I'm terribly busy too. @gthb pheraphs you could send a pull request with a patch similar to the one added by @niwibe: https://github.com/djangonauts/django-hstore/commit/02d398ce1e108748e8c657938600b1f6eaf0a7a4

DataMarket commented 10 years ago

[Edit: This comment was actually from @gthb, mistakenly logged in as DataMarket.]

Me too : ) ... so I'll probably only do this if it becomes necessary to us to use HSTORE fields via Django. We don't need that (yet) so I just reported this and then backed out of using django-hstore for now.

nemesifier commented 10 years ago

I was wondering... the settings instroduced in 02d398c is called HSTORE_GLOBAL_REGISTER, how would this setting be called?

nemesifier commented 10 years ago

also, shouldn't this line https://github.com/djangonauts/django-hstore/blob/master/django_hstore/apps.py#L59 pass unique=HSTORE_GLOBAL_REGISTER too as in line https://github.com/djangonauts/django-hstore/blob/master/django_hstore/apps.py#L75?

nemesifier commented 10 years ago

discussion should continue on #50