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

FindAll on models without primaryKey failing #4607

Closed
cuckoopt opened this Issue Oct 5, 2015 · 14 comments

Comments

4 participants
@cuckoopt

cuckoopt commented Oct 5, 2015

Hi there,

I noticed that with the latest release, at least the FindAll is not working for models without a primaryKey,
since here lib/model.js#L1349 you are concatenating the attributes with this.primaryKeyAttribute and when there isn't a primaryKey the result is [undefined, attr1, attr2, ..., attrn] and when the Find gets here lib/dialects/abstract/query-generator.js#L999 it throws the error TypeError: Cannot read property '_isSequelizeMethod' of undefined

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 5, 2015

What release did it work on previously? (that code is pretty old AFAIR)

@cuckoopt

This comment has been minimized.

cuckoopt commented Oct 5, 2015

it did work on 3.10.0

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 5, 2015

@janmeier could it be related to the attributes.include/exclude feature?

@janmeier

This comment has been minimized.

Member

janmeier commented Oct 5, 2015

Yeah, seems I remove an if options.attribtes check here https://github.com/sequelize/sequelize/blob/v3.11.0/lib/model.js#L1347

@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 5, 2015

Yep i just checked the commit and you did indeed :)

@janmeier

This comment has been minimized.

Member

janmeier commented Oct 6, 2015

Yeah, with my change options.attributes will always be defined at that point.

Hm, gonna get some duplication then:

 }).then(function() {
    if (options.include) {
      options.hasJoin = true;

      validateIncludedElements.call(this, options, tableNames);

      // If we're not raw, we have to make sure we include the primary key for deduplication
      if (options.attributes && !options.raw) {
        this.$handleAttributes(options);

        if (options.attributes.indexOf(this.primaryKeyAttribute) === -1) {
          options.originalAttributes = options.attributes;
          options.attributes = [this.primaryKeyAttribute].concat(options.attributes);
        }
      }
    } else {
      this.$handleAttributes(options);
    }

    // whereCollection is used for non-primary key updates
    this.options.whereCollection = options.where || null;

    Utils.mapFinderOptions(options, this);

    options = paranoidClause(this, options);

    if (options.hooks) {
      return this.runHooks('beforeFindAfterOptions', options);
    }
  }).then(function() {

Otherwise we should remove the handling of defaulting to all attributes if empty from $handleAttributes - I just felt i fitted nicely in there :D. Either that or we could perhaps set originalAttributes in $handleAttributes?

@cuckoopt If you can provide a small unit test along the lines of these https://github.com/sequelize/sequelize/blob/v3.11.0/test/unit/model/include.test.js#L70 that would speed things along

@cuckoopt

This comment has been minimized.

cuckoopt commented Oct 6, 2015

@janmeier I think something like this should work, no?

    beforeEach(function () {
      this.User = this.sequelize.define('User');
      this.Task = this.sequelize.define('Task', {
        title: Sequelize.STRING
      });
      this.Income = this.sequelize.define('Income', {
        amount  :  {
          type      : DataTypes.Decimal,
          allowNull : false,
        },
        applyAt : {
          type      : DataTypes.DATE,
          field     : "apply_at",
          allowNull : false,
        }
      });

      ...

      this.Income.User = this.Income.belongsTo(this.User, {foreignKey : "userId", allowNull : false});
      this.Income.removeAttribute("id");

      ...
    });

    describe('include / exclude', function () {
        it('allows me to include additional attributes', function () {
          var options = Sequelize.Model.$validateIncludedElements({
            model: this.User,
            include: [
              {
                model: this.Income,
                attributes: {
                  include: ['foobar']
                }
              }
            ]
          });

          expect(options.include[0].attributes).to.deep.equal([
            ['apply_at', 'applyAt'],
            'createdAt',
            'updatedAt',
            'amount',
            'userId',
            'foobar',
          ]);
        });

        it('allows me to exclude attributes', function () {
          var options = Sequelize.Model.$validateIncludedElements({
            model: this.User,
            include: [
              {
                model: this.Income,
                attributes: {
                  exclude: ['amount']
                }
              }
            ]
          });

          expect(options.include[0].attributes).to.deep.equal([
            ['apply_at', 'applyAt'],
            'createdAt',
            'updatedAt',
            'userId',
          ]);
        });

        it('include takes precendence over exclude', function () {
          var options = Sequelize.Model.$validateIncludedElements({
            model: this.User,
            include: [
              {
                model: this.Income,
                attributes: {
                  exclude: ['name'],
                  include: ['name']
                }
              }
            ]
          });

          expect(options.include[0].attributes).to.deep.equal([
            ['apply_at', 'applyAt'],
            'createdAt',
            'updatedAt',
            'amount',
            'userId',
          ]);
        });
    });
@mickhansen

This comment has been minimized.

Contributor

mickhansen commented Oct 6, 2015

originalAttributes is only needed in the case of includes, but yeah we could do that.

@janmeier

This comment has been minimized.

Member

janmeier commented Oct 6, 2015

@cuckoopt Those tests seem to work fine for includes :) - regular attributes and include.attributes are handled by two different pieces of code.

My bad though, should have linked to this instead https://github.com/sequelize/sequelize/blob/v3.11.0/test/unit/model/findall.test.js

@janmeier

This comment has been minimized.

Member

janmeier commented Oct 6, 2015

Easiest fix is probably to just check for this.primaryKeyAttribute before adding it.

Bonus question though - How is includes working for you without primary key for de-duplication? Don't you have any joins returning duplicated rows?

@janmeier janmeier closed this in f6b7855 Oct 11, 2015

janmeier added a commit that referenced this issue Oct 11, 2015

@zhuoahe

This comment has been minimized.

zhuoahe commented Nov 17, 2015

node\node_modules\sequelize\lib\model.js:245
self.$expandAttributes(options);
^

TypeError: self.$expandAttributes is not a function...

The original code is available in the 3.11.0 version, but the 3.12.0 throws this exception
way???

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 17, 2015

@zhuoahe There is no way for us to debug that without seeing any code....

@zhuoahe

This comment has been minimized.

zhuoahe commented Nov 17, 2015

var sequelize   = require('./sequelizeBean');
var Sequelize   = require('sequelize');
var tItem       = require('./tItem');
var tSku = sequelize.define('tSku',{
    uniqueId: {
        type : Sequelize.STRING(32),
        //autoIncrement : true,
        primaryKey : true, unique : true,
        field: 'UNIQUE_ID'
    },
    name: {type : Sequelize.STRING,field: 'NAME'},
    version: {type : Sequelize.INTEGER,field: 'VERSION'},
    price: {type: Sequelize.INTEGER,field: 'PRICE'},
    sold: {type: Sequelize.INTEGER, field: 'SOLD'},
    stock: {type: Sequelize.INTEGER, field: 'STOCK'},
    itemId: {
        type:Sequelize.STRING(32),
        field:'ITEM_ID',
        references: {
            // This is a reference to another model
            model: tItem,
            // This is the column name of the referenced model
            key: 'uniqueId'
            // This declares when to check the foreign key constraint. PostgreSQL only.
            // deferrable: Sequelize.Deferrable.INITIALLY_IMMEDIATE
        }
    }
},{
    freezeTableName: true,
    tableName: 'T_SKU',
    timestamps: false,
    scopes: {
        item:{
            include:[{
                model: tItem,
                as: 'item'
            }]
        },
        page:function(offset, limit){
            return {
                offset: offset,
                limit: limit
            };
        },
        scopeValue: function(value){
            return value;
        }
    }
});
module.exports = tSku;

Error occurred in 4 lines

@zhuoahe

This comment has been minimized.

zhuoahe commented Nov 18, 2015

After I tested it, when I commented out the

include:[{
model: tItem,
as: 'item'
}]

, it would be able to work properly.

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