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

tests & bugfixes for DAO-Factory.update and array of values in where clause #880

Merged
merged 5 commits into from Sep 6, 2013

Conversation

2 participants
@domasx2
Contributor

domasx2 commented Sep 3, 2013

Not funny, guys.
Compare to undefined if you want set default as true, though In this instance default should be false

edit: changed title to reflect PR content

@durango

This comment has been minimized.

Member

durango commented Sep 3, 2013

Validate should be true by default

@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 3, 2013

But if you're bulk updating a single attribute or two, you should either:

  1. validate only those attributes, not the whole model (will still fail for validations that depend on other fields)
  2. retrieve and fill in attributes that are not being updated (poor performance when updating large number of rows)

All use cases I have update a single attribute on a large number of rows. They now fail because other mandatory fields that are not being updated are not filled in for the model instance and thus model fails validation.

@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 3, 2013

And I hope the major bug is clear: it's impossible to set validate option to false for this method, but setting it to 1, "true" or any truthy value other than true will disable validation.

@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 3, 2013

updated PR to set default to true

@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 3, 2013

Added test & fix for postgres when serching for rows where attribute is in a list of values

domasx2 referenced this pull request Sep 3, 2013

Fixes Postgres' ability to search within functions.
This is a pure and simple fix for searching with a Postgres' array column type.
This commit changes hashToWhereConditions by adding a second argument of sending
the factory's details over and checking for column type if the hash/value/first
argument is an array. For the actual search we just utilize a simple overlap &&
boolean search.
@durango

This comment has been minimized.

Member

durango commented Sep 3, 2013

Hmmm no this commit 352f78d only affects column types of ARRAY not the actual array within find itself (otherwise we would've seen a ton of errors in the tests). Did you actually come across this or was it pre-emptive?

@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 3, 2013

I came accross it, all queries that use where: {attribute: [val1, val2..]} are now breaking. The bug is also very obvious, i commented on the precise line: if column is not an ARRAY, there is no else statement to the condition, no SQL is generated.
You don't seem to have any tests for this kind of where statement. Heres the one I wrote, which is failing:

  describe('special where conditions/smartWhere object', function() {
    beforeEach(function(done) {
      var self = this

      this.User.bulkCreate([
        {username: 'boo', intVal: 5, theDate: '2013-01-01 12:00'},
        {username: 'boo2', intVal: 10, theDate: '2013-01-10 12:00'}
      ]).success(function(user2) {
        done()
      })
    })

    it('should be able to find rows where attribute is in a list of values', function () {
      this.User.findAll({
        where: {
          username: ['boo', 'boo2']
        }
      }).success(function(users){
        expect(users).to.have.length(2);
      });
    })

Though even with my fix some queries are still breaking for me. it seems that when filtering for rows primary key is in an array of values, eg

where: {
  id: [x, y,..]
}

col.type is an object but not a string, so it does not have match method. Using col.type.toString().match works, but not sure if this is correct fix. still investigating

@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 3, 2013

added a crude-ish test that reproduces this primary key problem, and a fix. Not the prettiest, but now my app is finally working again and I can go to bed :)

@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 6, 2013

Can we get this merged or otherwise tested and fixed, please?

durango added a commit that referenced this pull request Sep 6, 2013

Merge pull request #880 from domasx2/master
tests & bugfixes for DAO-Factory.update and array of values in where clause

@durango durango merged commit 6aab128 into sequelize:master Sep 6, 2013

1 check passed

default The Travis CI build passed
Details
@domasx2

This comment has been minimized.

Contributor

domasx2 commented Sep 7, 2013

👍

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