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

Set updatedAt and createdAt values before validation #5367

Merged
merged 1 commit into from Feb 11, 2016

Conversation

3 participants
@rafis
Contributor

rafis commented Feb 4, 2016

If model has updatedAt or createdAt fields without allowing nulls:

        createdAt: {
            type: DataTypes.DATE,
            allowNull: false,
            field: 'created_at'
        },
        updatedAt: {
            type: DataTypes.DATE,
            allowNull: false,
            field: 'update_at'
        },

then I'm receiving this error:

{ [SequelizeValidationError: notNull Violation: createdAt cannot be null,
notNull Violation: updatedAt cannot be null]
name: 'SequelizeValidationError',
message: 'notNull Violation: createdAt cannot be null,\nnotNull Violation: updatedAt cannot be null',
errors:
[ { message: 'createdAt cannot be null',
type: 'notNull Violation',
path: 'createdAt',
value: null },
{ message: 'updatedAt cannot be null',
type: 'notNull Violation',
path: 'updatedAt',
value: null } ] }

My proposal is to set updatedAt and createdAt values before doing validation.

if (updatedAtAttr && options.fields.indexOf(updatedAtAttr) === -1) {
if (updatedAtAttr && options.fields.length && options.fields.indexOf(updatedAtAttr) === -1) {

This comment has been minimized.

@mickhansen

mickhansen Feb 4, 2016

Contributor

Not necessary since options.fields will always be defined by then

This comment has been minimized.

@rafis

rafis Feb 6, 2016

Contributor

I've added this check not to check if options.fields is defined, but to check if we update at least one field, because if we update 0 fields then updatedAt should not be changed.

@rafis rafis force-pushed the rafis:master branch from 3543ce6 to aac8eab Feb 6, 2016

@rafis rafis force-pushed the rafis:master branch from aac8eab to 743b997 Feb 6, 2016

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Feb 7, 2016

LGTM i think, would like @janmeier's input.

@janmeier

This comment has been minimized.

Member

janmeier commented Feb 11, 2016

Yep, looks good to me too

janmeier added a commit that referenced this pull request Feb 11, 2016

Merge pull request #5367 from rafis/master
Set updatedAt and createdAt values before validation

@janmeier janmeier merged commit 751fe37 into sequelize:master Feb 11, 2016

1 check passed

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

This comment has been minimized.

Member

janmeier commented Feb 11, 2016

Thanks :)

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