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

Fixes #1358 #1364

Merged
merged 9 commits into from Feb 12, 2014

Conversation

3 participants
@SohumB
Contributor

SohumB commented Feb 8, 2014

Buffer essentially needs handling like a base type, as far as I can tell. That, and isHash should really encapsulate the logic of finding actual hashes rather than JS objects.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 8, 2014

Could you provide a test aswell? So we don't have regression bugs later :)

@SohumB

This comment has been minimized.

Contributor

SohumB commented Feb 8, 2014

Thanks for the push to do so - and I found another bug while writing the test :p

I'll add a test for the associations pathway I found the bug by in a sec.

where: {
binary: [ this.buf, this.buf ]
}
}).success(function(users){

This comment has been minimized.

@janmeier

janmeier Feb 8, 2014

Member

Could you please check that the user matching the buffer is actually returned, and that the data is a buffer?

users[0].binary instanceof Buffer

This comment has been minimized.

@SohumB

SohumB Feb 8, 2014

Contributor

AFAIK, the database itself returns the data as a string here. Sequelize would have to check input to binary fields (in .build?) and specifically convert them into a buffer, I think.

This comment has been minimized.

@janmeier

janmeier Feb 8, 2014

Member

We return data in BLOBs as buffers (https://github.com/sequelize/sequelize/blob/master/test/dao-factory.test.js#L1826), so I just though it might make sense to do the same for BINARY fields. I actually though it was already impemented though :)

The conversion happen in query.js as i recall it

This comment has been minimized.

@SohumB

SohumB Feb 8, 2014

Contributor

Judging from a grep of the source code, Sequelize only does a conversion into Buffer for mariadb and sqlite.

This comment has been minimized.

@janmeier

janmeier Feb 8, 2014

Member

Jup, mysql and postgres handles the conversion automagically :)

it('should not break when using smart syntax on binary fields', function (done) {
this.User.findAll({
where: {
binary: [ this.buf, this.buf ]

This comment has been minimized.

@janmeier

janmeier Feb 8, 2014

Member

Why provide the same buffer twice? :)

This comment has been minimized.

@SohumB

SohumB Feb 8, 2014

Contributor

To trigger the smartWhere pathway, as I understood it. It may not be necessary.

@SohumB

This comment has been minimized.

Contributor

SohumB commented Feb 8, 2014

Why is Travis is failing on postgres here? All that it's getting to is declaring the field as a STRING(16, true)...

if (elem instanceof DAOFactory || elem instanceof Utils.col || elem instanceof Utils.literal || elem instanceof Utils.cast || elem instanceof Utils.fn || elem instanceof Utils.and || elem instanceof Utils.or) {
return elem
}
// Unfortunately, lodash.cloneDeep doesn't preserve Buffer.isBuffer, which we have to rely on for binary data
if (Buffer.isBuffer(elem)) { return new Buffer(elem); }

This comment has been minimized.

@mickhansen

mickhansen Feb 8, 2014

Contributor

return elem should be enough

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 8, 2014

Postgres must not support STRING BINARY, any experience with this @janmeier ?

@janmeier

This comment has been minimized.

Member

janmeier commented Feb 8, 2014

Nope, seems to only support bytea (basically BLOB) http://www.postgresql.org/docs/7.2/static/datatype-binary.html

Must have been missing tests for that :D

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 8, 2014

Guess so. I guess we'll have to wrap the tests for this in != postgres.

@janmeier

This comment has been minimized.

Member

janmeier commented Feb 8, 2014

If we do so we should also throw an error, if the user tries to create a binary string column under postgres. Or perhaps just use a bytea column?

In most respects, you can regard a BLOB column as a VARBINARY column that can be as large as you like.

http://stackoverflow.com/questions/8476968/varbinary-vs-blob-in-mysql

The distinction seem to be neglible

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 8, 2014

Hmm good question. Overall we need to decide if we want to make compatability code for stuff like this or just let the user pick the right types and throw errors if not supported. Ideally an ORM should work regardless of the database behind (ofcourse thats utopian but a nice idea)

@SohumB

This comment has been minimized.

Contributor

SohumB commented Feb 9, 2014

A bytea column seems to me to be the best option. The docs do say it's postgres' version of binary strings.

(For my usecase, I'd just use the UUID type :P)

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 10, 2014

@SohumB Want to take a crack at bytea?
If not, please change it so tests are only run for the supported dialects and an error is thrown if attempting to use STRING BINARY with Postgres.

@SohumB

This comment has been minimized.

Contributor

SohumB commented Feb 12, 2014

Done! I think. Found another bug, too - you're depending on this in the specialised QueryGenerators, which sometimes you pass this.something to without binding this.

But yea, done! Probably!

@SohumB

This comment has been minimized.

Contributor

SohumB commented Feb 12, 2014

Wait, hang on. Why are the expect(binaryRetrieved.id).to.be.an.instanceof.string tests passing on postgres, then?

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 12, 2014

@SohumB it is? Some test was failing originally atleast.

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 12, 2014

Looks like solid work. @janmeier mind taking a look too?

@SohumB

This comment has been minimized.

Contributor

SohumB commented Feb 12, 2014

It is now, I mean. The original error was that postgres didn't understand BINARY, then that this wasn't getting bound and so SqlString.escape would use X'' syntax rather than E'\x' syntax. What I'm confused about is that the test appears to return a Buffer object, but for some reason the test that checks that it's an instance of string is passing.

@janmeier

This comment has been minimized.

Member

janmeier commented Feb 12, 2014

Jup, looks solid to me as well

janmeier added a commit that referenced this pull request Feb 12, 2014

@janmeier janmeier merged commit 18910ff into sequelize:master Feb 12, 2014

1 check passed

default The Travis CI build passed
Details
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 12, 2014

Tests are green! Let's assume it works! :D

@SohumB SohumB deleted the SohumB:bugfix/use-ishash-to-avoid-buffers branch Feb 12, 2014

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