Skip to content
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

updates using UNIQUE (but not PRIMARY) key #51

Closed
dmalan opened this issue Aug 21, 2013 · 4 comments
Closed

updates using UNIQUE (but not PRIMARY) key #51

dmalan opened this issue Aug 21, 2013 · 4 comments
Labels

Comments

@dmalan
Copy link

dmalan commented Aug 21, 2013

Hi all,

We currently have a MySQL table whose rows include, e.g.:

  • id
  • uuid
  • name

The uuid is a UUIDv4 that exists so that we have a unique, essentially unguessable identifier for an entity that can be used in URLs, without exposing the underlying primary key (id). We haven't been sure, though, of the best way to fetch and update models using that unique uuid, though. Altering idAttribute hasn't felt ideal, because then uuid becomes our primary key, which isn't what we want for joins. We're currently using

models.Entity
.forge()
.query()
.where({uuid: req.params.uuid})
.update(attributes)
.then(function() {
    // success
}, function(err) {
    // error
});

which works. But we ran into a side effect whereby, because we're dropping down to the query layer with the above, a hook we've defined in our model's initialize method isn't invoked:

this.on('updating', function(model, attributes, options) {
    model.set('updated_at', dateFormat(new Date(), 'yyyy-mm-dd HH:MM:ss'));
});

Are we better off implementing one or both of these in some other way perhaps? We did notice that the documentation for Bookshelf.Sync.update alludes to "a custom query", but we weren't sure if that might help us with both goals?

Many thanks!

@tgriesser
Copy link
Member

I may need to improve the documentation here, but when dropping into the query layer with the intention of continuing to use the current model (or collection) as the context for querying, you'll want to pass either a function or conditions to the query method... so what you'll want is either:

var entity = models.Entity.forge(); // for brevity

entity.query('where', 'uuid', '=', req.params.uuid).save(attributes).then(function(model) {...

// or
entity.query({where: ['uuid', '=', req.params.uuid]}).save(attributes).then(function(model) {...

// or (as of 0.2.5)
entity.query(function(qb) {
  qb.where('uuid', '=', req.params.uuid);
}).save(attributes).then(...

This way when you call save, you're still acting with the context of the model and not the Knex builder chain directly, so the model (or collection) is passed as the value for the promise handler, and the events will be triggered, etc.

This can also be useful for debugging, as adding:

entity.query('debug').save(...

Would effectively be the same as calling .debug() on the Knex query chain to debug an individual query.

The documentation section for Bookshelf.Sync might also be a little misleading, because while it is documented, it's mainly meant to be used internally... and overriding it would really only be useful if you wanted to modify every insert, update, select, etc. used throughout the library.

Also, just incase you'd find this useful, if you set the hasTimestamps property on the model to true, it will do something along the lines of what you look to be after there with the updating event (assuming there's a created_at datetime or timestamp as well).

Let me know if that helps with the questions you have!

@dmalan
Copy link
Author

dmalan commented Aug 22, 2013

Hi Tim,

Thanks so much for the follow-up. I just tried out .query, as in the below, though I did have to add {method: 'update'} to force an UPDATE instead of an INSERT (presumably because the entity's idAttribute (id) is undefined in this context, and so .save otherwise defaulted to INSERT):

models.Entity
.forge()
.query({where: ['uuid', '=', req.params.uuid]})
.save(attributes, {method: 'update'})
.then(function(model) {
    // success
}, function(err) {
    // error
});

But by forcing the UPDATE query, I fear I forced a SQL query that isn't quite right. In particular, the automatically generated query included id = NULL as well as created_at = '2013-08-21 22:17:51':

--> ComQueryPacket
{ command: 3,
  sql: 'update `collection` set `created_at` = \'2013-08-21 22:17:51\', `name` = \'foo\', `updated_at` = \'2013-08-21 22:17:51\' where `uuid` = \'110ec58a-a0f2-4ac4-8393-c866d813b8d1\' and `id` = NULL' }

As a workaround, we could first .fetch the id of the entity via its uuid, but we'd definitely prefer to avoid the additional SELECT if there's a cleaner way perhaps?

If related, I also tried passing .query a function (after upgrading to 0.2.5), but the call to qb.where actually seemed to hang on me, whereby after never prints:

models.Entity
.forge()
.query(function(qb) {
    console.log('before');
    qb.where('uuid', '=', '110ec58a-a0f2-4ac4-8393-c866d813b8d1');
    console.log('after');
})
.save(attributes, {method: 'update'})
.then(function(model) {
    // success
}, function(err) {
    // error
});

Much obliged!

tgriesser added a commit that referenced this issue Aug 22, 2013
Should use call rather than apply, test included
@tgriesser
Copy link
Member

I agree it's ideal to avoid the extra select there - in the upcoming 0.3.0 version there will be an option of to passing {partial: true} as a flag to only save the data passed in the save call.

In the meantime, I changed a bit of the logic dealing with the timestamp to check the method passed, and also provided a check to not set an extra where clause if the idAttribute doesn't exist (which should only happen in the case of a forced update with {method: 'update'} as you are) - these should be available on the latest with some tests (0.2.7). Let me know if you think of anything else that might seem more intuitive for cases like these.

Also, thanks a lot for pointing out the bug in the query function, I forgot to add a test - should've changed it from .apply to .call there after changing the functionality from the original PR. Should also be fixed.

@dmalan
Copy link
Author

dmalan commented Aug 23, 2013

Thanks very much, both changes seem to be working nicely now!

@dmalan dmalan closed this as completed Aug 23, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants