Objection.js: Issue with upsertGraph() and relations: Sequence of delete, relate, update, unrelate

[lehni]

I've been observing this in my rather complex app, so I've made an isolated test-case that illustrates the issue.

Note that for this to fail on SQLite, I needed to activate its foreign key support. Luckily there is a way:
https://github.com/tgriesser/knex/issues/453#issuecomment-54160324

So basically, the graph holds the pets relation, which is the "owner" of its data (meaning it is allowed to create / delete animals), as well as favoritePet, which doesn't own its data (it's using relate / unrelate to reference one of the animals in pets).

When I delete an entry from pets as well clear favoritePet, the upsert fails due to foreign key constraints, caused by execution sequence:

let Model;

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

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

async function main() {
  await createSchema();

  ///////////////////////////////////////////////////////////////
  // Your reproduction
  ///////////////////////////////////////////////////////////////

  await Person.query().insertGraph({
    name: 'Jennifer',

    pets: [
      {
        name: 'Doggo'
      },
      {
        name: 'Kitty'
      }
    ]
  });

  let jennifer = await Person.query()
    .findOne({ name: 'Jennifer' })
    .eager('pets');

  chai.expect(jennifer.pets[0].name).to.equal('Doggo');
  chai.expect(jennifer.pets[1].name).to.equal('Kitty');

  jennifer.favoritePet = { id: jennifer.pets[1].id };

  const upsertOptions = {
    relate: ['favoritePet'],
    unrelate: ['favoritePet']
  };

  await Person.query().upsertGraph(jennifer, upsertOptions);

  jennifer = await Person.query()
    .findOne({ name: 'Jennifer' })
    .eager('[pets, favoritePet]');

  chai.expect(jennifer.favoritePet.name).to.equal('Kitty');

  // Now remove Kitty, and set favoritePet to an empty array in one operation:
  jennifer.pets.pop();
  jennifer.favoritePet = null;

  await Person.query().upsertGraph(jennifer, upsertOptions);

  jennifer = await Person.query()
    .findOne({ name: 'Jennifer' })
    .eager('[pets, favoritePet]');

  console.log(jennifer);
}

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

const knex = Knex({
  client: 'sqlite3',
  useNullAsDefault: true,
  connection: {
    filename: ':memory:'
  },
  pool: {
    afterCreate(con, cb) {
      con.run('PRAGMA foreign_keys = ON;', cb);
    }
  }
});

setupKnexLogging();

Model.knex(knex);

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

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

  static get jsonSchema() {
    return {
      type: 'object',
      required: ['name'],

      properties: {
        id: { type: 'integer' },
        animalId: { type: ['integer', 'null'] },
        name: { type: 'string', minLength: 1, maxLength: 255 }
      }
    };
  }

  static get relationMappings() {
    return {
      pets: {
        relation: Model.HasManyRelation,
        modelClass: Animal,
        join: {
          from: 'Person.id',
          to: 'Animal.ownerId'
        }
      },

      favoritePet: {
        relation: Model.BelongsToOneRelation,
        modelClass: Animal,
        join: {
          from: 'Person.animalId',
          to: 'Animal.id'
        }
      }
    };
  }
}

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

  static get jsonSchema() {
    return {
      type: 'object',
      required: ['name'],

      properties: {
        id: { type: 'integer' },
        ownerId: { type: ['integer', 'null'] },
        name: { type: 'string', minLength: 1, maxLength: 255 }
      }
    };
  }

  static get relationMappings() {
    return {
      owner: {
        relation: Model.BelongsToOneRelation,
        modelClass: Person,
        join: {
          from: 'Animal.ownerId',
          to: 'Person.id'
        }
      }
    };
  }
}

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

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

  await knex.schema
    .createTable('Person', table => {
      table.increments('id').primary();
      table.string('name');
      table
        .integer('animalId')
        .unsigned()
        .references('id')
        .inTable('Animal');
    })
    .createTable('Animal', table => {
      table.increments('id').primary();
      table
        .integer('ownerId')
        .unsigned()
        .references('id')
        .inTable('Person');
      table.string('name');
    });
}

main()
  .then(() => console.log('success'))
  .catch(console.error);

function setupKnexLogging() {
  const startTimes = {};

  function trim(str, length = 1024) {
    return str.length > length ? `${str.substring(0, length - 3)}...` : str;
  }

  function end(query, { response, error }) {
    const id = query.__knexQueryUid;
    const diff = process.hrtime(startTimes[id]);
    const duration = diff[0] * 1e3 + diff[1] / 1e6;
    delete startTimes[id];
    const bindings = query.bindings.join(', ');
    console.log(
      chalk.yellow.bold('knex:sql'),
      chalk.cyan(trim(query.sql)),
      chalk.magenta(duration + 'ms'),
      chalk.gray(`[${trim(bindings)}]`),
      response
        ? chalk.green(trim(JSON.stringify(response)))
        : error ? chalk.red(trim(JSON.stringify(error))) : ''
    );
  }

  knex
    .on('query', query => {
      startTimes[query.__knexQueryUid] = process.hrtime();
    })
    .on('query-response', (response, query) => {
      end(query, { response });
    })
    .on('query-error', (error, query) => {
      end(query, { error });
    });
}

[lehni]

I've tinkered with addressing this myself, and I think I'm on to something here:

https://github.com/lehni/objection.js/commit/086e5e3c7964d2e54e33e92e2d4a1c6c145d0c8b

This has only one failing test (should delete and insert belongsToOneRelation), which is related to belongsToOneRelation that is handled in InsertGraph, but I can't figure out how.

PS: That setupKnexLogging() is potentially useful. Maybe worth including it in the template?

PPS: I noticed that activating PRAGMA foreign_keys = ON also activate support for ON DELETE on SQLite, potentially interesting and useful!

[lehni]

Note that if favoritePet was a ManyToManyRelation named favoritePets instead of a BelongsToOneRelation (as I originally used in the example above, and later changed), this particular example could actually be solved with using ON DELETE, but if the relation is a BelongsToOneRelation, then that doesn't work.

[koskimas]

@lehni Sorry for the radio silence. I've moved these upsertGraph issues to the bottom of my TODO list, since there seems to be as many of them as there are people using upsertGraph 😄. Objection seems to be pretty stable at the moment, so I'll soon have time to dig into these.

[lehni]

@koskimas no worries! I'd be happy to help out with this one. I believe i almost managed to solve it in the commit referenced above, but it clashes with the InsertGraph fallback... If I had more insight there, then I could maybe be of more use :)

[koskimas]

Yup, that probably works, but we need to consider what use cases it breaks. The tests for upsertGraph are by no means extensive enough to cover all current use cases. I'd be really careful changing the order of the operations like that.

[lehni]

I understand... What potential problems do you se if the relates and unrelates happen before the deletes?

[koskimas]

None from the top of my head, otherwise there would be test for it. We need to spend some time figuring out if there could be a problem.

[lehni]

PS: Great news about Objection.js being so stable! That must be a great feeling after all the hard labor. I'm also not seeing any new issues right now, but I guess I'm not moving much beyond my current patterns of use now.

[koskimas]

@lehni This can be fixed simply by adding

      table
        .integer('animalId')
        .unsigned()
        .references('id')
        .inTable('Animal')
        .onDelete('SET NULL')

Would this be an option? When objection deletes a BelongsToOneRelation it doesn't set the owner's foreign key to null and that causes upsertGraph to update the owner with that value, which doesn't exist anymore.

In the currently released version, even SET NULL fails, but I've fixed that in master.

[lehni]

Oh nice! I'll check!

[koskimas]

Fixed by onDelete('SET NULL') (heard from lehni through another channel)

[lehni]

Yes I can officially confirm it, this works splendidly. Thank you @koskimas!