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

Better way to upsert (if querying isn't done on id's)? #101

Closed
haywirez opened this issue Mar 23, 2016 · 10 comments
Closed

Better way to upsert (if querying isn't done on id's)? #101

haywirez opened this issue Mar 23, 2016 · 10 comments
Labels

Comments

@haywirez
Copy link

Wondering if there's a better way of doing upserts, perhaps via a custom base class function? The example listed in the documentation doesn't really cover my use case, as the update isn't done on an id-based query, but on other column values. This is the workaround I find using, but it isn't great as I would also like to return the updated object and not just if it was inserted:

Model.query()
      .where({...})
      .update({...})
      .then((nrUpdated) => {
        if (nrUpdated === 0) { return Model.query().insertAndFetch({...}) }
      })
@koskimas
Copy link
Collaborator

UPDATE: I just realized that this won't work. It will also copy the runBefore method to the whereQuery. This will cause an infinite loop 😞 I'll update this as soon as I come up with a working solution.

UPDATE 2: Now it may work. It's pretty hacky and ugly though. It may be more clear to just do all this where you need this functionality, and not try to write a generic method for it. This is why objection doesn't have a generic upsert method 😄

It is possible with some restrictions. The following code is an example that I haven't tested, so it may not work, but it should lead you to the right track. The restrictions are mentioned in the comments of the example:

class MyQueryBuilder extends QueryBuilder {
  upsert(obj) {
    let modelClass = this.modelClass();
    let whereQuery;

    return this
      .query()
      .runBefore((result, builder) => {
        if (!builder.context().isWhereQuery) {
          // At this point the builder should only contain a bunch of `where*`
          // operations. Store the `where` query for later use in the `runAfter`
          // method. Also mark the query with `isWhereQuery: true` so that
          // we can skip all this when this function is called for the `whereQuery`.
          whereQuery = builder.clone().context({isWhereQuery: true});

          // Call the `update` method on the original query turning it into an
          // update operation.
          builder.update(obj);
        }

        return result;
      })
      .runAfter(result => {
        if (!builder.context().isWhereQuery) {
          let numUpdated = result;

          if (numUpdated === 0) {
            return modelClass.query().insertAndFetch(obj);
          } else {
            // Now we can use the `where` query we saved in the `runBefore`
            // method to fetch the inserted results. It is noteworthy that this
            // query will return the wrong results if the update changed any
            // of the columns the where operates with. This also returns all
            // updated models. Chain `.first()` if you only want the first one.
            return whereQuery;
          }
        }

        return result;
      });
  }
}

Now you should be able to use this like this:

Person
  .query()
  .upsert({firstName: 'Jack'})
  .where('foo', 'bar')
  .orWhere('a', '<', 'b')
  .then(updatedOrInsertedResult => {

  });

runAfter is just like then but it doesn't execute the query and convert it into a promise.

As I mentioned, I'm not 100% sure this works. Let me know how it goes 😄

@koskimas
Copy link
Collaborator

Ugh, I changed the example one more time. I added the parts with modelClass variable.

@haywirez
Copy link
Author

Thanks so much! Haven't had the chance to work with the upper concept yet, but meanwhile I found these:

knex upsert monkey patch gist
another one

Maybe they're relevant. The first approach doesn't cover cases when there wouldn't be a conflict due to no unique constraints on the columns. There's a knex issue that would make this much easier if implemented.

@koskimas
Copy link
Collaborator

koskimas commented Oct 9, 2016

We'll just have to wait for knex on this one.

@koskimas koskimas closed this as completed Oct 9, 2016
@lehni
Copy link
Collaborator

lehni commented Oct 25, 2017

@koskimas this is great, thanks for posting the code! Your code almost works, just two tweaks were needed:

  • this.query() doesn't make sense as we're already in a query builder.
  • The runAfter() method is missing the 2nd builder argument.
class MyQueryBuilder extends QueryBuilder {
  upsert(obj) {
    let modelClass = this.modelClass();
    let whereQuery;
    return this
      .runBefore((result, builder) => {
        if (!builder.context().isWhereQuery) {
          // At this point the builder should only contain a bunch of `where*`
          // operations. Store the `where` query for later use in the `runAfter`
          // method. Also mark the query with `isWhereQuery: true` so we can
          // skip all this when this function is called for the `whereQuery`.
          whereQuery = builder.clone().context({ isWhereQuery: true });
          // Call the `update` method on the original query turning it into an
          // update operation.
          builder.update(obj);
        }
        return result;
      })
      .runAfter((result, builder) => {
        if (!builder.context().isWhereQuery) {
          if (result === 0) {
            return modelClass.query().insertAndFetch(obj);
          } else {
            // Now we can use the `where` query we saved in the `runBefore`
            // method to fetch the inserted results. It is noteworthy that this
            // query will return the wrong results if the update changed any
            // of the columns the where operates with. This also returns all
            // updated models. Chain `.first()` if you only want the first one.
            return whereQuery;
          }
        }
        return result;
      });
  }
}

@lehni
Copy link
Collaborator

lehni commented Nov 2, 2017

I've just found a problem with this code, see https://gitter.im/Vincit/objection.js?at=59fb57aff7299e8f53643386

The solution is simple: Instead of taking a reference to the modelClass, I use the whereQuery as a version of the original query, and use that to run insertAndFetch() if update() wasn't successful. This works and correctly sets foreign keys when inserting into a relation:

class MyQueryBuilder extends QueryBuilder {
  upsert(data) {
    let mainQuery
    return this
      .runBefore((result, builder) => {
        if (!builder.context().isMainQuery) {
          // At this point the builder should only contain a bunch of `where*`
          // operations. Store the `where` query for later use in the `runAfter`
          // method. Also mark the query with `isMainQuery: true` so we can
          // skip all this when this function is called for the `mainQuery`.
          mainQuery = builder.clone().context({ isMainQuery: true })
          // Call the `update` method on the original query turning it into an
          // update operation.
          builder.update(data)
        }
        return result
      })
      .runAfter((result, builder) => {
        if (!builder.context().isMainQuery) {
          if (result === 0) {
            return mainQuery.insertAndFetch(data)
          } else {
            // Now we can use the `where` query we saved in the `runBefore`
            // method to fetch the inserted results. It is noteworthy that this
            // query will return the wrong results if the update changed any
            // of the columns the where operates with. This also returns all
            // updated models.
            return mainQuery.first()
          }
        }
        return result
      })
  }
}

@lehni
Copy link
Collaborator

lehni commented Nov 3, 2017

@koskimas I've tried to use mergeContext({...}) instead of context({...}) on this line here, but it doesn't work correctly then:

mainQuery = builder.clone().context({ isMainQuery: true })

Do you understand why? As far as I can tell, context({}) replaces the internal context object, while mergeContext({}) merges it. But at the time of the call console.log(builder.context()) displays an empty object, so why does it matter? Does it have something to do with the forking of queries and the sharing of contexts?

@koskimas
Copy link
Collaborator

koskimas commented Nov 3, 2017

@lehni You found a small bug/inconsistency there. I'll fix that for 0.9.1 if it doesn't turn out to be a breaking change.

@koskimas
Copy link
Collaborator

koskimas commented Nov 3, 2017

@lehni the mergeEager thingy is now fixed in master

@lehni
Copy link
Collaborator

lehni commented Nov 7, 2017

@koskimas great, thanks! It all seems to work fine for me now.

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

3 participants