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

Fix HSTORE encoding by using pg-hstore library. #695

Merged
merged 3 commits into from Jun 14, 2013

Conversation

4 participants
@tadman
Contributor

tadman commented Jun 12, 2013

The work involved in properly encoding an HSTORE attribute is surprisingly complicated, but the pg-hstore library does a good job of implementing it correctly.

That project is MIT licensed, so it should be possible to internalize the relevant code inside Sequelize and give credit to the original author.

@tadman

This comment has been minimized.

Contributor

tadman commented Jun 12, 2013

This is in relation to Issue#692.

@durango

This comment has been minimized.

Member

durango commented Jun 12, 2013

Could you fix up the specs/tests? And I created the hstore functionality for sequelizejs.

Taking a quick glance at: https://github.com/scarney81/pg-hstore/blob/master/lib/index.js

the only advantage that I see is the "advanced" types:

https://github.com/scarney81/pg-hstore/blob/master/test/parse.js#L45

I wish they provided tests by spacing out their characters. If you could throw more tests in (and make sure spacing is OK) I'll merge this and eventually remove my .toHstore function. Good find :)

@tadman

This comment has been minimized.

Contributor

tadman commented Jun 13, 2013

The toHstore method is very confusing since it actually does the opposite if I read it correctly.

pg-hstore provides a consistent interface for parsing and stringifying HSTORE values. What's in Sequelize now is pretty ad-hoc and strips rather than encodes.

A simple test is: Can you store and retrieve a JSON stringified object that's intentionally abusive like {key: "It's all good!", value: "{1,2,3}=>\"test\""}

@durango

This comment has been minimized.

Member

durango commented Jun 13, 2013

A simple test is: Can you store and retrieve a JSON stringified object that's intentionally abusive like

No, but at the time I also didn't know of any decent libraries (even after a Google search) and I simply did not have the time to write something else. As I said, I'm far more than welcoming this patch :) Would like to see some tests though since it will do two things:

  1. It'll make Sequelize "more complete"
  2. It'll help us recognize any problems within the library that you're using (if any) and then we can help improve pg-hstore (if applicable).

Makes sense?

@tadman

This comment has been minimized.

Contributor

tadman commented Jun 13, 2013

I'm having a look a the tests for this feature and will try and adapt them to expose this problem, and also apply the pg-hstore solution.

@durango

This comment has been minimized.

Member

durango commented Jun 13, 2013

@tadman awesome and thanks! :) Will keep a close eye on this PR. If you use IRC at all, join us at #sequelizejs on freenode. We need more postgres people hah.

@sdepold

This comment has been minimized.

Member

sdepold commented Jun 14, 2013

;) @durango is this good to merge?

durango added a commit that referenced this pull request Jun 14, 2013

Merge pull request #695 from twg/master
Fix HSTORE encoding by using pg-hstore library.

@durango durango merged commit cd8cc16 into sequelize:master Jun 14, 2013

1 check passed

default The Travis CI build passed
Details
@durango

This comment has been minimized.

Member

durango commented Jun 14, 2013

@sdepold yessir, @tadman awesome PR man, thanks :)

@kevinmartin

This comment has been minimized.

Contributor

kevinmartin commented on lib/dao.js in 4a4d27b Jun 17, 2013

Why is HSTORE being required for all dialects and not just when Postrgres is being used?

This comment has been minimized.

Contributor

tadman replied Jun 17, 2013

DAO isn't modular like that, but it probably should be. It's not clear if there's a way to conditionally require it.

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