balderdashy / sails

Realtime MVC Framework for Node.js
https://sailsjs.com
MIT License
22.84k stars 1.95k forks source link

Populate gives wrong results when no primary key is given. #6150

Closed mattilu closed 5 years ago

mattilu commented 10 years ago

(Copying from https://github.com/balderdashy/sails/issues/1565)

Hi,

I am experiencing a problem with sails associations, let me expain with an example:

I have a model, call it Master:

// api/models/Master.js
module.exports = {
  autoPK: false,
  autoCreatedAt: false,
  autoUpdatedAt: false,

  schema: true,

  attributes: {
    ID: {
      type: 'integer',
      primaryKey: true,
      required: true
    },

    Name: {
      type: 'string',
      required: true
    }
  }
};

And another model, call it Slave, which is in relation with Master:

// api/models/Slave.js
module.exports = {
  autoPK: false,
  autoCreatedAt: false,
  autoUpdatedAt: false,

  schema: true,

  attributes: {
    Master: {
      model: 'Master',
      via: 'ID',
      type: 'integer',
      foreignKey: true,
      references: 'Master',
      on: 'ID',
      required: true
    },

    Attr1: {
      type: 'integer',
      required: true
    },

    Attr2: {
      type: 'integer',
      required: true
    }
  }
};

Slave doesn't have a primary key, as in my application the primary key is the row itself, and I didn't find a way to specify multiple-column primary keys.

The problem is, when I access /slave/find, the Master field always gets populated with data from the first Master.

For example, if my database contains:

  {
    "master": [ {
        "ID": 1,
        "Name": "first"
      }, {
        "ID": 2,
        "Name": "second"
      }, {
        "ID": 3,
        "Name": "third"
      }
    ],
    "slave": [ {
        "Master": 1,
        "Attr1": 1,
        "Attr2": 1
      }, {
        "Master": 2,
        "Attr1": 1,
        "Attr2": 2
      }
    ]
  }

Requesting /slave/find produces the following output:

[
  {
    "Master": {
      "ID": 1,
      "Name": "first"
    },
    "Attr1": 1,
    "Attr2": 1
  },
  {
    "Master": {
      "ID": 1,
      "Name": "first"
    },
    "Attr1": 1,
    "Attr2": 2
  }
]

While expected output should be:

[
  {
    "Master": {
      "ID": 1,
      "Name": "first"
    },
    "Attr1": 1,
    "Attr2": 1
  },
  {
    "Master": {
      "ID": 2,
      "Name": "second"
    },
    "Attr1": 1,
    "Attr2": 2
  }
]

If I remove the model property from the scheme I can see that the IDs are fetched correctly, so I think the problem should be somewhere in .populate(); I have also seen that adding primaryKey: true to Slave.attributes.Master somehow manages to give the correct results, but it seems more of a workaround than a solution, as my table doesn't actually have Master as the primary key.

Am I missing something?

podger commented 10 years ago

I've this issue as well !

particlebanana commented 10 years ago

Hey guys the primaryKey flag is used in the Integrator when doing in-memory joins. Once the SQL adapters are updated it should work with actual SQL Joins I believe. In the mean time we are doing in-memory joins and need this flag to correctly group records together. Waterline assumes all models have a unique key (primaryKey). If you don't want it to represent an actual primary key in your database and be indexed the following should work:

Master: {
    primaryKey: true,
    index: false
}

Also I'm not sure where you guys are seeing the foreignKey, references and on keys but those are built up automatically inside of Waterline-Schema and you won't need to ever define them yourself. The only attributes that need via keys are collection attributes where you will point to the attribute on a child that contains the parent's primary key.

irlnathan of SailsCasts is working on an associations screencast but in the meantime you can see some of the docs he has at https://github.com/irlnathan/s-assoc-docs/blob/master/associations2.md

mattilu commented 10 years ago

I saw the foreignKey, references and on keys somewhere in the documentation, but it might have been something pre-associactions.

Anyways, the problem is not related, and in fact it is also adapter-independent, as I can see this happening both with the MySql adapter and the localDiskDb adapter.

The problem is not the primaryKey on Master, but the lack of primaryKey on Slave. Anyways, you say I can do

Slave: {
    primaryKey: true,
    index: false
}

to introduce an index which is not a primaryKey, which works, but seems at least counter-intuitive to me; shouldn't it be

Slave: {
    primaryKey: false,
    index: true
}

(which also works, but as far as I could tell from the code it's just because the actual value of primaryKey gets ignored, as the only check is hasOwnProperty('primaryKey'))

Anyways, if you want to try this yourself, these are the steps to reproduce the problem:

$ sails -v
0.10.0-rc4
$ sails new foobar
info: Created a new Sails app `foobar`!
$ cd foobar
$ npm install
[snip]
$ [ $(sails -v) == "0.10.0-rc4" ] || npm install sails@0.10.0-rc4
[snip]
$ sails generate api Master
$ sails generate api Slave

Then edit api/models/Master.js and api/models/Slave.js with the following content:

// Master.js
module.exports = {
  autoPK: false,
  autoCreatedAt: false,
  autoUpdatedAt: false,

  schema: true,

  attributes: {
    ID: {
      type: 'integer',
      primaryKey: true,
      required: true
    },

    Name: {
      type: 'string',
      required: true
    }
  }
};
// Slave.js
module.exports = {
  autoPK: false,
  autoCreatedAt: false,
  autoUpdatedAt: false,

  schema: true,

  attributes: {
    Master: {
      model: 'Master',
      via: 'ID',
      type: 'integer',
      required: true
    },

    Attr: {
      type: 'integer',
      required: true
    }
  }
};

And create example records:

$ sails lift
[snip]
$ wget -O/dev/null -o/dev/null 'http://localhost:1337/master/create?ID=1&Name=first'
$ wget -O/dev/null -o/dev/null 'http://localhost:1337/master/create?ID=2&Name=second'
$ wget -O/dev/null -o/dev/null 'http://localhost:1337/slave/create?Master=1&Attr=1'
$ wget -O/dev/null -o/dev/null 'http://localhost:1337/slave/create?Master=2&Attr=2'

Then, visiting http://localhost:1337/slave, the result is:

[
  {
    "Master": {
      "ID": 1,
      "Name": "first"
    },
    "Attr": 1
  },
  {
    "Master": {
      "ID": 1,
      "Name": "first"
    },
    "Attr": 2
  }
]

While expected behaviour would be:

[
  {
    "Master": {
      "ID": 1,
      "Name": "first"
    },
    "Attr": 1
  },
  {
    "Master": {
      "ID": 2,
      "Name": "second"
    },
    "Attr": 2
  }
]

Adding primaryKey: false (or true, or any other value) on Slave's model does the trick, but

1) Shouldn't this be documented somewhere? 2) Is this really intended behaviour?

I think it would be better to have this fixed to work "out of the box", since the default behaviour makes no sense to me