Can't add or modify a value during $beforeUpdate using upsertGraph

[umutuzgur]

We are trying to push some of calculated logic into the models using instance hooks, there is a case of encryption and decryption as well. This was working correctly with objection 1 but started showing a different behavior with objection 3. The problem is that since objection is doing a diff right before $beforeUpdate is invoked, the values changed in the function don't get picked up as a part of the propsToUpdate and they are not executed in the sql query. I tried to bypass this or somehow tried to change this value. The only thing that work was iterating over $$omitFromDatabaseJson and making sure that the field I want updated is not in this array. It would be great to have an UpsertGraphOptions to skip the diff check. The use case for us is just to update a graph

The code relevant is here

objection.js/lib/queryBuilder/graph/patch/GraphPatchAction.js

Line 60 in 260b284

node.obj.$omitFromDatabaseJson(difference(allProps, propsToUpdate));

let Model

try {
  Model = require('./').Model
} catch (err) {
  Model = require('objection').Model
}

const Knex = require('knex')
const chai = require('chai')

async function main () {
  await createSchema()
  const item = { multiplier: 1 }
  // The random value will get modified in $beforeInsert
  const returned = await Item.query().insertGraphAndFetch(item)

  // The random value won't be persisted into the DB
  const newGeneratedRandom = await Item.query().upsertGraphAndFetch({ ...returned, multiplier: 200 })

  chai.expect(newGeneratedRandom.random).to.not.equal(returned.random)
}

/// ////////////////////////////////////////////////////////////
// Database
/// ////////////////////////////////////////////////////////////
const knex = Knex({
  client: 'sqlite3',
  useNullAsDefault: true,
  debug: false,
  connection: {
    filename: ':memory:',
  },
})

Model.knex(knex)

/// ////////////////////////////////////////////////////////////
// Models
/// ////////////////////////////////////////////////////////////

class Item extends Model {
  static get tableName () {
    return 'item'
  }

  $beforeInsert () {
    this.random = Math.ceil(Math.random() * this.multiplier)
  }

  $beforeUpdate () {
    this.random = Math.ceil(Math.random() * this.multiplier)
  }
}

/// ////////////////////////////////////////////////////////////
// Schema
/// ////////////////////////////////////////////////////////////

async function createSchema () {
  await knex.schema
    .dropTableIfExists('item')

  await knex.schema
    .createTable('item', table => {
      table.increments('id').primary()
      table.integer('multiplier')
      table.integer('random')
    })
}

main()
  .then(() => {
    console.log('success')
    return knex.destroy()
  })
  .catch(err => {
    console.error(err)
    return knex.destroy()
  })

[cheenbabes]

Ran into the same issue upgrading from objection 1.x to 2.x

1. I don't understand why this doesn't work as it seems to be supported by the docs
2. I don't understand why this wasn't documented in the docs or called out as a breaking change

This is going to be extremely common as it follows a common pattern recommended by this repo
https://github.com/Vincit/objection.js/blob/260b284a1cbfb044991894c5a3cf3dedc8ce7267/doc/recipes/timestamps.md
There is a base class with timestamps, and other models extend that class.
This doesn't work with upsertGraphAndFetch! Should be fixed!

[lehni]

This appears to have been fixed. But due to the rounding and low multipliers in your example, the values still often end up being the same. With this adjusted example here, I get success printed on every run. I think this can be closed.

let Model;

try {
  Model = require('./').Model;
} catch (err) {
  Model = require('objection').Model;
}

const Knex = require('knex');
const chai = require('chai');

async function main() {
  await createSchema();
  const item = { multiplier: 100 };
  const returned = await Item.query().insertGraphAndFetch(item);

  returned.multiplier = 200;
  const newGeneratedRandom = await Item.query().upsertGraphAndFetch(returned);

  chai.expect(newGeneratedRandom.random).to.not.equal(returned.random);
}

///////////////////////////////////////////////////////////////
// Database
///////////////////////////////////////////////////////////////

const knex = Knex({
  client: 'sqlite3',
  useNullAsDefault: true,
  debug: false,
  connection: {
    filename: ':memory:',
  },
});

Model.knex(knex);

///////////////////////////////////////////////////////////////
// Models
///////////////////////////////////////////////////////////////

class Item extends Model {
  static get tableName() {
    return 'item';
  }

  $beforeInsert() {
    this.random = Math.ceil(Math.random() * this.multiplier);
  }
  $beforeUpdate() {
    this.random = Math.ceil(Math.random() * this.multiplier);
  }
}

///////////////////////////////////////////////////////////////
// Schema
///////////////////////////////////////////////////////////////

async function createSchema() {
  await knex.schema.dropTableIfExists('item');

  await knex.schema.createTable('item', (table) => {
    table.increments('id').primary();
    table.integer('multiplier');
    table.integer('random');
  });
}

main()
  .then(() => {
    console.log('success');
    return knex.destroy();
  })
  .catch((err) => {
    console.error(err);
    return knex.destroy();
  });

[s123121]

This bug didn't seem to be fixed in 3.0.2. Newly created row worked fined though.
My workaround is to add updated whenever I have to used upsertGraphAndFetch

[umutuzgur]

Hey @lehni, I tested the latest release and I can verify that this is still not fixed. I think we should reopen the issue

[umutuzgur]

I think this is the piece of code that is causing the issue as it is treating update and patch the same

objection.js/lib/queryBuilder/graph/patch/GraphPatchAction.js

Line 36 in 260b284

const propsToUpdate = difference(

    const allProps = union(changedProps, unchangedProps);
    const propsToUpdate = difference(
      shouldPatch || shouldUpdate
        ? changedProps
        : [...changedPropsBecauseOfBelongsToOneInsert, ...changePropsBecauseOfBelongsToOneDelete],

      // Remove id properties from the props to update. With upsertGraph
      // it never makes sense to change the id.
      node.modelClass.getIdPropertyArray()
    );

[umutuzgur]

This should be something like this IMO

    const allProps = union(changedProps, unchangedProps);
    let compareSet
    if (shouldPatch) {
      compareSet = changedProps
    } else if (shouldUpdate) {
      compareSet = allProps
    } else {
      compareSet = [...changedPropsBecauseOfBelongsToOneInsert, ...changePropsBecauseOfBelongsToOneDelete]
    }
    const propsToUpdate = difference(
      compareSet,
      // Remove id properties from the props to update. With upsertGraph
      // it never makes sense to change the id.
      node.modelClass.getIdPropertyArray()
    );

[umutuzgur]

I found a way by hacking the $set method and manipulating the $$omitFromDatabaseJson field. It is basically the change detection code in GraphPatchAction baked into the $set function

  $set (obj) {
    return this.setFast(this, obj)
  }

  setFast (model, obj) {
    if (obj) {
      // Don't try to set read-only properties. They can easily get here
      // through `fromJson` when parsing an object that was previously
      // serialized from a model instance.
      const readOnlyAttr = model.constructor.getReadOnlyAttributes()
      const keys = Object.keys(obj)
      const changed = []

      for (let i = 0, l = keys.length; i < l; ++i) {
        const key = keys[i]
        const value = obj[key]

        if (!isInternalProp(key) && !isFunction(value) && !readOnlyAttr.includes(key)) {
          if (this.nonStrictEquals(model[key], value)) {
            changed.push(key)
          }
          model[key] = value
        }
      }
      if (this.$$omitFromDatabaseJson) {
        this.$$omitFromDatabaseJson = this.$$omitFromDatabaseJson?.filter(column => !changed.includes(column))
      }
    }

    return model
  }

[umutuzgur]

@lehni my example was buggy. I see why you couldn't repro it. I updated the example in the issue description. I can also create a new issue if you want

[lehni]

Ok I understand it now. I am looking into a fix.

[lehni]

This doesn't seem to fix the reproduction though. Did it work for you @umutuzgur?

This should be something like this IMO

    const allProps = union(changedProps, unchangedProps);
    let compareSet
    if (shouldPatch) {
      compareSet = changedProps
    } else if (shouldUpdate) {
      compareSet = allProps
    } else {
      compareSet = [...changedPropsBecauseOfBelongsToOneInsert, ...changePropsBecauseOfBelongsToOneDelete]
    }
    const propsToUpdate = difference(
      compareSet,
      // Remove id properties from the props to update. With upsertGraph
      // it never makes sense to change the id.
      node.modelClass.getIdPropertyArray()
    );

[lehni]

The reason is that the hooks get triggered by this piece of code here:

objection.js/lib/queryBuilder/graph/patch/GraphPatchAction.js

Lines 67 to 71 in 2dac69e

	if (shouldPatch) {
	updateBuilder.patch(node.obj);
	} else {
	updateBuilder.update(node.obj);
	}

But this gets called after propsToUpdate were determined already. We'd have to separate the calling of the hooks from the calling of the actual patch() / update() methods, but doing so is currently a bit above my know-how of objection's internals.

@koskimas if you have a minute, could you let me know your opinion on this?

[umutuzgur]

This doesn't seem to fix the reproduction though. Did it work for you @umutuzgur?

This should be something like this IMO

    const allProps = union(changedProps, unchangedProps);
    let compareSet
    if (shouldPatch) {
      compareSet = changedProps
    } else if (shouldUpdate) {
      compareSet = allProps
    } else {
      compareSet = [...changedPropsBecauseOfBelongsToOneInsert, ...changePropsBecauseOfBelongsToOneDelete]
    }
    const propsToUpdate = difference(
      compareSet,
      // Remove id properties from the props to update. With upsertGraph
      // it never makes sense to change the id.
      node.modelClass.getIdPropertyArray()
    );

@lehni So this would work if we pass update: true as a upsert options. It would then look like this
const newGeneratedRandom = await Item.query().upsertGraphAndFetch({ ...returned, multiplier: 200 }, { update: true })

[lehni]

Yes but I am not confident enough about graph upserts to change it in this way, only for it to be a partial fix. If I understand it right update: true is more about validation than the underlying database commands, and the current code filters out things that don't change for reasons of efficiency, which I think is good.

The reason why changes in the before hook aren't taken into consideration is that the hook is fired after the check of the properties that are going to change, so this should be addressed. I am not sure how though.

[umutuzgur]

We can also follow the $set fix if we want to. We will probably follow that path internally

[lehni]

That's a hack at best, not a fix :) I wouldn't merge that into core.

[umutuzgur]

Gotcha. To be honest, this has been broken since the upgrade to version 2, so a while, and probably some people already got used to this new behaviour. I don't think this will be easily fixed without an extra option, like the update:true, or a new major version to change the behaviour one more time

[lehni]

So I was curious about how to solve this, and learn more bout Objection's internals along the way. I found a way to do so in e946c13 using runBefore(), but it's a bit of a hack, and also a breaking change, as you can see in the changed tests that now get beforeUpdateCalled increased before determining that there's nothing to udpdate.

I am not sure if this should be merged or not, but if we would do so, it would be v3.1.0.

@falkenhawk do you have an opinion on this?

[lehni]

@umutuzgur your reproduction works with this change.

[umutuzgur]

@lehni awesome, this great news! I can give it a go in our app as well this week

[umutuzgur]

Tested it, everything works as expect 👏

[lehni]

I will test it against our rather complex code-base as well, to see if there are edge-cases. I'm still not entirely confident with merging this. I also still need to write tests for the situation that it solves.

[umutuzgur]

Gotcha. It makes to be careful about it 👍 I will also deploy our whole system with this commit id on Friday to our dev env and see if there is are any regression issues

[lehni]

Good news: Not a single fail across all our tests, and we use graph upserts quite extensively

[falkenhawk]

@lehni @umutuzgur I've just run our test suites with the changes from #2452 applied and nothing broke.

[lehni]

Released in v3.1.0

[francois06]

Thank you for this fix !!!