Feature request: Document.prototype.update

[simonratner]

It would be nice to have an update() method for concurrent-safe partial updates, not unlike ActiveRecord's update_attributes.

For example:

doc.update({f: 'g'}).then(console.log);

would execute something along the lines of:

r.table('Test').get('1').update({f: 'g'}, {returnChanges: true})

Here's a working sketch, although the devil is obviously in the details (hooks, validation, etc):

Document.prototype.update = function(doc, callback) {
    // If not saved yet, this is equivalent to save().
    if (!this.isSaved()) {
        return this.merge(doc).save(callback);
    }
    var self = this;
    var model = this.getModel();
    var table = model.getTableName();
    var r = model._thinky.r;
    var pk = model._pk;
    var promise = new Promise(function(resolve, reject) {
        r.table(table).get(self[pk]).update(doc, {returnChanges: true}).run(function(err, result) {
            self._merge(result.changes[0].new_val);
            self._generateDefault();
            self.setSaved();
            resolve(self);
        });
    });
    if (typeof callback == 'function') {
        promise.then(function(result) {
            callback(null, result);
        }).error(callback);
    }
    return promise;
};

[neumino]

For:

doc.update({f: 'g'}).then(console.log);

You can do

doc.merge({f: "g'}).save().then(console.log)

It's not exactly r.table('Test').get('1').update({f: 'g'}) but close.
One issue with the current proposal is that there is no way to validate it before saving it, which seems a bit dangerous to me.

We could validate just the provided fields, but even that can raise a few issues.

[simonratner]

The problem with doc.merge().save() is that it isn't safe under concurrent edits, since save() does a replace for existing documents. If one process is updating field 'f' only and another is updating field 'g' only, they could easily clobber each other's edits. The update() version allows you to update just the fields that you need in a single atomic command.

It could be particularly useful for appending things to lists without worrying about races, for example:

doc.update({ tokens: r.row('tokens').append({ id: 1, ts: r.now() }) });

You can of course do all of this directly with:

Model.get(doc.id).update({}, {returnChanges: true}).execute();

But it does not validate, and it does not repopulate the model or regenerate virtuals, so I think it may be convenient to expose this on the document.

The way I imagine doing validation (I started, but haven't finished it yet) is to do doc.merge().validate(), but then only store the fields you changed. On successful write, replace the doc with new_val, on error restore old field values to their state before the merge. I figured validating individual fields isn't good enough since there is also a document-level validator which may have arbitrary data dependencies, so doc.merge().validate() seems like the only sensible way to do it.

Not sure how to handle hooks yet, whether there should be a separate update hook or if it should trigger pre-save hooks as well (for example, I have a pre-save hook that updates the timestmap, so it would be handy to trigger that rather than having to duplicate it in a pre-update hook).

[simonratner]

Of course, doing anything more complicated than simple field sets will make it impossible for the validator to do its job anyway. At least it will validate when reading the results back.

[ThisIsMissEm]

This issue is causing me a great deal of pain right now, as even with the non-atomic merge().validate() method, the validations just don't run, such that I'm getting a load of junk/corrupted data in the database because the end user doesn't know they're putting in invalid data.

I'm very very close to giving up with Thinky, and just using plain javascript constructors with rethinkdbdash commands directly.. Which would be painful, but not as painful as this functionality missing.

[simonratner]

Keep in mind that if you are running doc.validate() yourself, it could be synchronous (and throw exceptions) or asynchronous (and return a promise), depending on the hooks/validators you have defined. I was bitten by that once.

[neumino]

@miksago -- What do you mean by the validations don't run?

[ThisIsMissEm]

As in, I have a function validator for an optional email address field. When doing merge/save, the fields I merge include an invalid email address, but my validation doesn't get triggered.

[ThisIsMissEm]

(aside: I'm ditching thinky later in favour of a different approach to data modeling, most likely around immutable objects, got some research and thoughts here, but yeah. I don't need a lot of what thinky provides, I'm happy to do relations myself)

[neumino]

There was a bug about save not triggering validate in case of a saved object, but it was fixed in v.1.14.2
#112

Can you check the version of thinky? And if you run something more recent than 1.14.2, do you have a snippet to reproduce this?

[ThisIsMissEm]

I'm on 1.15.0

[ThisIsMissEm]

I think it was because of how I was using the promises.. maybe I wasn't catching the error correctly.

[simonratner]

I see that da08ffe adds support for Model.get(doc.id).update({...}), which is awesome. Does that execute as a single query (i.e. no additional roundtrip to db to retrieve the doc again)?

Do you think it still makes sense to also provide a convenience method on Document that just does that for the current doc?

[neumino]

@simonratner -- Model.get(doc.id).update({...}) does one unique query, no round trip. So it's pretty close to what you were looking for.

I haven't implemented yet document.update as I would like it to be able to handle joined documents. Something like

// User
// Account
// User.hasOne(Account, "account", ...)
User.get(user.id).update({name: "Michel", account: {sold: 0}})

Model.update is the first main step. I "just" need to hook up the rest :)

[validkeys]

Just wanted to check in here and see if there had been any further progress related to this issue. Thanks!

[neumino]

Is Model.update not enough?