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 regression in util.toDefaultValue not returning the data types. Closes #3324, #3002 and #3410 #3733

Merged
merged 2 commits into from May 26, 2015

Conversation

2 participants
@BridgeAR
Contributor

BridgeAR commented May 16, 2015

I'm uncertain if UUIDV1 and UUIDV4 is actually right for mysql and mssql. I guess it should be just as the general UUID? Fixes #3324 (can't reproduce the error - seems to be fixed already).

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented May 17, 2015

Only postgres has native UUID types, for mysql and other dialects we generate one with node-uuid

@BridgeAR BridgeAR changed the title from Add testcase. Fixes #3324 to Fix regression in util.toDefaultValue not returning the data types. Closes #3324 May 17, 2015

return value().key;
} else {
return value();
}

This comment has been minimized.

@BridgeAR

BridgeAR May 17, 2015

Contributor

Can you come up with something better?

}
return undefined;
};

This comment has been minimized.

@BridgeAR

BridgeAR May 17, 2015

Contributor

I'd actually remove the old __getTimestamp (It's only used twice right now) and only always __getDefaultTimestamp in combination with Utils.now(this.sequelize.options.dialect). What do you think?

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented May 17, 2015

Ready for review

@@ -1792,6 +1794,13 @@ module.exports = (function() {
}
};
Model.prototype.__getDefaultTimestamp = function(attr) {

This comment has been minimized.

@mickhansen

mickhansen May 18, 2015

Contributor

We're chaing to $ prefix for privates.

}
if (updatedAtAttr && !values[updatedAtAttr]) {
values[updatedAtAttr] = this.__getTimestamp(updatedAtAttr);
values[updatedAtAttr] = this.__getDefaultTimestamp(updatedAtAttr) || now;

This comment has been minimized.

@mickhansen

mickhansen May 18, 2015

Contributor

Couldnt defaulTtimestamp just return now?

This comment has been minimized.

@BridgeAR

BridgeAR May 18, 2015

Contributor

That's what getTimestamp does right now. The whole point in my change though is that now will not be in the return value of the function. If you set now twice in a row for updatedAt and createdAt it differs for some parts of a millisecond and produces race condition in our tests.

This comment has been minimized.

@mickhansen

mickhansen May 18, 2015

Contributor

I see.

return value();
if (typeof value === 'function') {
if (value.key) {
return value().key;

This comment has been minimized.

@mickhansen

mickhansen May 18, 2015

Contributor

what is value.key?
If it's to detect a sequelize method use _isSequelizeMethod instead and then the proper handler (usually toString())

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented May 18, 2015

Ready for merge

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented May 19, 2015

I added a test for #3002 and #3410 so they can be closed too. Still waiting for merge ;)

@BridgeAR BridgeAR changed the title from Fix regression in util.toDefaultValue not returning the data types. Closes #3324 to Fix regression in util.toDefaultValue not returning the data types. Closes #3324, #3002 and #3410 May 20, 2015

@@ -1024,7 +1025,7 @@ module.exports = (function() {
delete defaults[this.Model._timestampAttributes.deletedAt];
}
if (Object.keys(defaults).length) {
if (Object.keys(defaults).length !== 0) {

This comment has been minimized.

@mickhansen

mickhansen May 20, 2015

Contributor

Any reason for this?

This comment has been minimized.

@BridgeAR

BridgeAR May 20, 2015

Contributor

I prefer the explicit one check. I'm uncertain if V8 handles it faster but this would not be measurable if. So it's just the style^^

This comment has been minimized.

@mickhansen

mickhansen May 20, 2015

Contributor

Then there's really no reason to change it. It's better to keep style changes out of functional PR's so they can be discucssed seperately. I prefer the other style :)

return value();
if (typeof value === 'function') {
var tmp = value();
if (tmp instanceof DataTypes.ABSTRACT) {

This comment has been minimized.

@mickhansen

mickhansen May 20, 2015

Contributor

You might also want to check instanceof when value is not a function, since default values should be new()'ed when the model is define (although they might not be but would be a good future proof to have.

This comment has been minimized.

@BridgeAR

BridgeAR May 20, 2015

Contributor

Hm, right now there's no call that could reach that point and I doubt that it's possible to know when to call new in toDefaultValue.

This comment has been minimized.

@mickhansen

mickhansen May 22, 2015

Contributor

Defining a model should be calling new on it, so we have to handle both cases.

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented May 20, 2015

It's back to the old style

@BridgeAR

This comment has been minimized.

Contributor

BridgeAR commented May 22, 2015

Rebased

Ruben Bridgewater added some commits May 17, 2015

Ruben Bridgewater
Fix util.toDefaultValue
Fix typo

Fix updatedAt and createdAt not always being 100% identical

Use instanceof Abstract and remove __getTimestamp
@@ -139,7 +138,7 @@ var Utils = module.exports = {
delete attributes[attribute];
}
if (_.isPlainObject(attributes[attribute])) {
if (lodash.isPlainObject(attributes[attribute])) {

This comment has been minimized.

@mickhansen

mickhansen May 26, 2015

Contributor

Why lodash insteadof _?

mickhansen added a commit that referenced this pull request May 26, 2015

Merge pull request #3733 from BridgeAR/testcase
Fix regression in util.toDefaultValue not returning the data types. Closes #3324, #3002 and #3410

@mickhansen mickhansen merged commit e4cc50c into sequelize:master May 26, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

IrfanBaqui pushed a commit to IrfanBaqui/sequelize that referenced this pull request Jun 24, 2015

Merge pull request sequelize#3733 from BridgeAR/testcase
Fix regression in util.toDefaultValue not returning the data types. Closes sequelize#3324, sequelize#3002 and sequelize#3410
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment