Objection.js: asFindQuery() returns wrong builder for patchAndFetchById() operations

[lehni]

I am using asFindQuery() to get the affected items in a static beforeUpdate() model hook that gets called when patchAndFetchById() is executed.

In this situation, asFindQuery() does not return a query that only selects the affected model items, it returns a query without a where clause, returning all model items of the given model class.

According to the docs, it should only return the model item with the given id?

[lehni]

Looking at the involved UpdateAndFetchOperation, I understand now why this is happening. It only runs findById() inside the onBuild() step, which only gets called upon execution, not before:

  onBuild(builder) {
    if (!this.skipIdWhere) {
      builder.findById(this.id);
    }

    super.onBuild(builder);
  }

toFindQuery() is defined like this:

  toFindQuery() {
    return this.clone().clear(
      op => op.is(InsertOperation) || op.is(UpdateOperation) || op.is(DeleteOperation)
    );
  }

Should onBuild() of the removed operations be called on the builder? I can't tell if that's a correct way to fix this issue.

[lehni]

Redefining toFindQuery() as follows works for me indeed:

  toFindQuery() {
    const builder = this.clone()
    const selector = op => op.is(InsertOperation) || op.is(UpdateOperation) || op.is(DeleteOperation)
    builder.forEachOperation(selector, op => op.onBuild(builder));
    return builder.clear(selector);
  }

I quickly checked all possible InsertOperation, UpdateOperation, and DeleteOperation classes, and I think this is fine to do.

Another class where the same problem will occur otherwise is InstanceUpdateOperation, where onBuild() is defined as follows:

  onBuild(builder) {
    super.onBuild(builder);

    assertHasId(this.instance);
    builder.findById(this.instance.$id());
  }

[lehni]

Quick fix using monkey-patching in user space:

function getOperationClass(modify) {
  const query = Model.query()
  let constructor = null
  // Locally override QueryBuilder#addOperation() in order to extract the
  // private operation constructor / class:
  query.addOperation = operation => {
    constructor = operation.constructor
  }
  modify(query)
  return constructor
}

const InsertOperation = getOperationClass(query => query.insert())
const UpdateOperation = getOperationClass(query => query.update())
const DeleteOperation = getOperationClass(query => query.delete())

QueryBuilder.prototype.toFindQuery = function() {
  const builder = this.clone()
  const selector = op => (
    op.is(InsertOperation) ||
    op.is(UpdateOperation) ||
    op.is(DeleteOperation)
  )
  builder.forEachOperation(selector, op => op.onBuild(builder))
  return builder.clear(selector)
}

[fvinogradov]

I'm facing the same issue with updateAndFetchById. In code below asFindQuery executing returns all rows from DB:

class CampaignModel extends Model {
  static async beforeUpdate(ops) {
    await super.beforeUpdate(ops);
    const { asFindQuery, inputItems } = ops;
    const foundCampaigns = await asFindQuery().select('id', 'status');
  }
}

await CampaignModel
  .query()
  .updateAndFetchById(
    id,
    {
      status: 'PROCESSING',
    },
  );

As a temporal solution, for this model with beforeUpdate hook I replaced updateAndFetchById with

await CampaignModel
  .query()
  .update(
    {
      status: 'PROCESSING',
    },
  )
  .where({ id });

[JaneJeon]

Hi, sorry to bother you again @koskimas but this is sort of a blocker for an objection plugin I’m writing as it relies _heavily_ on asFindQuery (unless I use the utterly hacky, ‘should-not-be-done-in-prod’ style of monkeypatching asFindQuery listed above, which, to be fair, _works_).

It would be greatly appreciated if this fix was implemented directly in Objection itself. Thanks!

[koskimas]

I'll start working on this now.

[lehni]

Thanks @koskimas for taking care of this!