objection.js: Can't add or modify a value during $beforeUpdate using upsertGraph

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 https://github.com/Vincit/objection.js/blob/260b284a1cbfb044991894c5a3cf3dedc8ce7267/lib/queryBuilder/graph/patch/GraphPatchAction.js#L60

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()
  })

About this issue

  • Original URL
  • State: closed
  • Created 2 years ago
  • Reactions: 2
  • Comments: 26 (1 by maintainers)

Commits related to this issue

Most upvoted comments

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 @umutuzgur I’ve just run our test suites with the changes from https://github.com/Vincit/objection.js/pull/2452 applied and nothing broke.

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

Thank you for this fix !!!

Released in v3.1.0

Tested it, everything works as expect 👏

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 e946c13a85058145cdcb361bd5244574bdc7b177 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?