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 uuid type with primary key coercion #32

Merged
merged 7 commits into from
May 18, 2016
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions lib/waterline/query/deferred.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,9 @@ Deferred.prototype.where = function(criteria) {

if(!criteria) return this;

// Normalize any primary keys in criteria
criteria = normalize.expandPK(this._context, criteria);

// Normalize criteria
criteria = normalize.criteria(criteria);

Expand Down
20 changes: 20 additions & 0 deletions lib/waterline/utils/normalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ var switchback = require('node-switchback');
var errorify = require('../error');
var WLUsageError = require('../error/WLUsageError');

var uuidPattern = /[a-f0-9]{8}-[a-f0-9]{4}-4[a-f0-9]{3}-[89aAbB][a-f0-9]{3}-[a-f0-9]{12}/i

var normalize = module.exports = {

// Expand Primary Key criteria into objects
Expand Down Expand Up @@ -50,6 +52,24 @@ var normalize = module.exports = {
else if (context.attributes[pk].type == 'string') {
coercePK = function(pk) {return String(pk).toString();};
}

// If the the data type is uuid (both according to the model definition
// as well as the database schema) attempt to coerce the value to a
// properly formatted uuid, stripping any extraneous characters from it.
else if (context.attributes[pk].type == 'uuid' &&

Choose a reason for hiding this comment

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

Should we use triple equals here and below?

Copy link
Author

Choose a reason for hiding this comment

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

That’d be my normal preference, though I was deferring to the existing style. I could go either way, but since the RHS is a string literal, I’m not super worried about it.

context.schema &&
context.schema[pk] &&
context.schema[pk].type == 'uuid') {
coercePK = function(pk) {
var matches = uuidPattern.exec(pk);
var uuid = matches && matches[0];
if (uuid) {
return uuid;
} else {
throw new TypeError("value '" + pk + "' cannot be coerced to UUID");

Choose a reason for hiding this comment

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

String addition always works right? Like its impossible for this to throw a second error because pk has some bad type.

In Python for example if you try to do "a" + 3 it will raise an exception.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah, it'll always work. If pk is an object we might get a slightly weird error message like value [object Object] cannot be coerced to UUID, but it won't throw.

}
};
}
// If the data type is unspecified, return the key as-is
else {
coercePK = function(pk) {return pk;};
Expand Down
10 changes: 10 additions & 0 deletions test/unit/query/query.find.js
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,16 @@ describe('Collection Query', function() {
});
});

it('should normalize primary keys when using deferreds', function(done) {
query.find()
.where({ id: '123' })
.exec(function(err, results) {
assert(!err);
assert(results[0].where.id === 123);
done();
});
});

describe('.paginate()', function() {
it('should skip to 0 and limit to 10 by default', function(done) {
query.find()
Expand Down
126 changes: 125 additions & 1 deletion test/unit/utils/utils.normalize.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,128 @@ describe("Normalize utility", function() {

});

});
describe(".expandPK()", function() {
it("casts integers", function() {
var context = {
attributes: {
id: {
type: 'integer',
primaryKey: true
}
}
};

var options = {
id: '123'
}

var result = normalize.expandPK(context, options);

assert(result.id === 123);
});

it("casts uuids", function() {

Choose a reason for hiding this comment

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

can we add a test here if you try to pass like 'foo' or 7 or null for the value?

Copy link
Author

Choose a reason for hiding this comment

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

Yep, and I'll check that an error gets thrown.

var context = {
attributes: {
id: {
type: 'uuid',
primaryKey: true
}
},
schema: {
id: {
type: 'uuid'
}
}
};

var options = {
id: 'prefix_0b6c28e0-a117-4a9e-9a0d-60f0992edbee'
}

var result = normalize.expandPK(context, options);

assert(result.id === '0b6c28e0-a117-4a9e-9a0d-60f0992edbee');
});

it("casts uuids with capitals", function() {
var context = {
attributes: {
id: {
type: 'uuid',
primaryKey: true
}
},
schema: {
id: {
type: 'uuid'
}
}
};

var options = {
id: '0B6C28E0-A117-4A9E-9A0D-60F0992EDBEE'
}

var result = normalize.expandPK(context, options);

assert(result.id === '0B6C28E0-A117-4A9E-9A0D-60F0992EDBEE');
});

it("does not cast uuids with an underlying non-uuid type", function() {

Choose a reason for hiding this comment

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

Oh this is neat

var context = {
attributes: {
id: {
type: 'uuid',
primaryKey: true
}
},
schema: {
id: {
type: 'text'
}
}
};

var options = {
id: 'prefix_0b6c28e0-a117-4a9e-9a0d-60f0992edbee'
}

var result = normalize.expandPK(context, options);

assert(result.id === 'prefix_0b6c28e0-a117-4a9e-9a0d-60f0992edbee');

});

it("throws a TypeError when attempting to cast a non-uuid", function() {
var context = {
attributes: {
id: {
type: 'uuid',
primaryKey: true
}
},
schema: {
id: {
type: 'uuid'
}
}
};

var options = {
id: 'hello'
}

try {
normalize.expandPK(context, options);
throw new Error('wrong error');
} catch (e) {
assert(e);
assert(e instanceof TypeError);
assert(e.message === "value 'hello' cannot be coerced to UUID");
}

});
});

});