dmfay / massive-js

A data mapper for Node.js and PostgreSQL.
2.49k stars 158 forks source link

Composite Primary Key #533

Closed samjeffress closed 6 years ago

samjeffress commented 6 years ago

Hi,

I was intending to convert my use of db.table.insert to db.table.save because I have events coming in that are okay to just update the table, but I currently use composite primary keys for a number of tables.

Looking through the table record, it seems the primary key is limited to a one column. Would there be any appetite to allow multiple columns?

If not, it might be helpful to update the save section of the Persistance documentation so other users know that only one column is supported.

Thanks

dmfay commented 6 years ago

I don't run into compound keys that often so Massive may not have them covered as thoroughly; however, I have been working at it off and on and I know for a fact that save works:

  it('saves over an existing record if passed both keys', function () {
    return db.compoundpk.save({
      key_one: 123,
      key_two: 456,
      value: 'yet again'
    }).then(res => {
      assert.isOk(res);
      assert.equal(res.key_one, 123);
      assert.equal(res.key_two, 456);
      assert.equal(res.value, 'yet again');
    });
  });

Check out test/table/compound-pk.js for more. If there is a gap in coverage then please do feel free to submit a pull request!

samjeffress commented 6 years ago

Thanks for the quick response @dmfay

My first problem is that I was using v3 :)

The issue I have is that my keys are not autogenerated, they're domain id's that will always be passed in, irrespective of a row existing in the database, so the check to determine if the object is new or not doesn't work for my scenario - https://github.com/dmfay/massive-js/blob/v4.6.4/lib/table.js#L145.

The following test breaks:

  it('saves a new record if passed both keys', function () {
    return db.compoundpk.save({
      key_one: 333,
      key_two: 666,
      value: 'I\'m new here'
    }).then(res => {
      assert.isOk(res);
      assert.equal(res.key_one, 333);
      assert.equal(res.key_two, 666);
      assert.equal(res.value, 'I\'m new here');
    });
  });

I'm not sure what the solution is, besides to do a check to make sure the row is actually in the db - my code currently has table.findOne({id1, id2}).

dmfay commented 6 years ago

save really only works if you autogenerate keys on the database side, since the behavior depends on whether the record object you pass it contains the key(s) or not. You'll need to fall back to calling insert and update yourself, but you could streamline things a little bit: use the onConflictIgnore option with insert. If the new record does conflict, it won't throw, but you won't get a result back from the insert since it didn't insert anything. When that happens, you know the row already exists and needs to be updated instead. So it's one or two roundtrips instead of two guaranteed -- small savings, but they add up.

samjeffress commented 6 years ago

Sweet, thanks for the help! I've created an upsert function using the onconflictignore option you've mentioned.

dmfay commented 6 years ago

I should have mentioned, there's also ON CONFLICT DO UPDATE if you want a true upsert, but you'd have to do that in a script file or database function since Massive doesn't offer that yet.