Objection.js: Ambiguous column mapping for RelationJoinBuilder (no table ref)

[fer22f]

Hello! I'm working with a schema that has a lot of columns with identical names, and when doing a leftJoin query I came accross the identity column (which is a composite key) being selected ambiguously with another table (I'm doing weird things like joining tables on filters). I found the culprit as being the function createRelatedJoinFromQuery after debugging a little bit:
https://github.com/Vincit/objection.js/blob/eaab72472586a11747fd66f001dd14f00fb59097/lib/queryBuilder/operations/eager/RelationJoinBuilder.js#L607-L621

As you can see, the columns are being appended without any table reference, they are being selected purely by their names. To fix my issue, I ended up changing findAllForeignKeysForModel to this:

function findAllForeignKeysForModel({ modelClass, allRelations }) {
  const foreignKeys = modelClass.getIdColumnArray().map(col => `${modelClass.getTableName()}.${col}`);

  allRelations.forEach(rel => {
    if (rel.relatedModelClass === modelClass) {
      rel.relatedProp.cols.forEach(col => foreignKeys.push(`${modelClass.getTableName()}.${col}`));
    }

    if (rel.ownerModelClass === modelClass) {
      rel.ownerProp.cols.forEach(col => foreignKeys.push(`${modelClass.getTableName()}.${col}`));
    }
  });

  return uniq(foreignKeys);
}

Which to be honest, isn't a perfect solution, but it does work perfectly with my complex queries.

[koskimas]

Isn't modelClass.getTableName() the same for each of those lines since modelClass is the same? So you still end up adding duplicates, and that cannot even be the problem since one table can contain one column only once.

Also the code that calls findAllForeignKeysForModel expects the returned array to contain only column names, withot table name here:

https://github.com/Vincit/objection.js/blob/eaab72472586a11747fd66f001dd14f00fb59097/lib/queryBuilder/operations/eager/RelationJoinBuilder.js#L595-L604

If that change fixed your problem, the reason is something else than what you described here. Could you create a standalone reproduction of your failing case with? You can use the reproduction-template file in the root of the repo, or any other method, as long as I'll be able to run it easily.

[fer22f]

I think this works as a repro.

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

  console.log(Person.query().leftJoinRelation('movies(hasOnlyOldActors)').toSql())
  await Person.query().leftJoinRelation('movies(hasOnlyOldActors)')
}

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

Model.knex(knex);

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

  static get relationMappings() {
    return {
      movies: {
        relation: Model.ManyToManyRelation,
        modelClass: Movie,
        join: {
          from: 'Person.id',
          through: {
            from: 'Person_Movie.personId',
            to: 'Person_Movie.movieId'
          },
          to: 'Movie.id'
        }
      }
    };
  }
}

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

  static get namedFilters () {
    return {
      hasOnlyOldActors: q => q.select('name').joinRelation('actors').where('actors.age', '>', 40)
    }
  }

  static get relationMappings() {
    return {
      actors: {
        relation: Model.ManyToManyRelation,
        modelClass: Person,
        join: {
          from: 'Movie.id',
          through: {
            from: 'Person_Movie.movieId',
            to: 'Person_Movie.personId'
          },
          to: 'Person.id'
        }
      }
    };
  }
}

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

  await knex.schema
    .createTable('Person', table => {
      table.increments('id').primary();
      table
        .integer('parentId')
        .unsigned()
        .references('id')
        .inTable('Person');
      table.string('firstName');
      table.string('lastName');
      table.integer('age');
      table.json('address');
    })
    .createTable('Movie', table => {
      table.increments('id').primary();
      table.string('name');
    })
    .createTable('Person_Movie', table => {
      table.increments('id').primary();
      table
        .integer('personId')
        .unsigned()
        .references('id')
        .inTable('Person')
        .onDelete('CASCADE');
      table
        .integer('movieId')
        .unsigned()
        .references('id')
        .inTable('Movie')
        .onDelete('CASCADE');
    });
}

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

I now understand why it happens. When you add a select to a filter, it needs to select the ids again, and it doesn't add table refs to those, so when you do a joinRelation inside a filter, it becomes ambiguous.

Someone might be wondering "why would you need such thing?", I use it for simpler anti-joins, a row needs to be present in two other tables, and it needs to not be "cancelled" in those, and I feel like expressing this with filters instead of complicated NOT EXISTS queries is more interesting when using Objection.

[koskimas]

Thanks for the repro!

You can fix this like so:

  static get namedFilters () {
    return {
      hasOnlyOldActors: q => q.select('Movie.name', 'Movie.id').joinRelation('actors').where('actors.age', '>', 40)
    }
  }

but having to add that Movie.id is less than ideal. I'll see if this can be improved. It's normal to have to specify Movie.name instead of name there though.

[fer22f]

I see, so you would have to add all the id columns manually for it work (even though I "technically" don't need them for my query, but Objection probably needs), just felt a bit unintuitive since the primary keys on this old database are all massive composite keys...

I can definitely understand the thought behind Movie.name, however.

[koskimas]

Yeah, I don't yet know why Movie.id needs to be added in this case. I'll see what I can do about it. I'd like to fix that if possible. But as a temporary workaround, you can add the primary keys there manually.

[koskimas]

This seems to be the correct fix:

function createRelatedJoinFromQuery({ filterQuery, relation, allRelations }) {
  const relatedJoinFromQuery = filterQuery.clone();
  const tableRef = filterQuery.tableRefFor(relation.relatedModelClass);

  const allForeignKeys = findAllForeignKeysForModel({
    modelClass: relation.relatedModelClass,
    allRelations
  });

  return relatedJoinFromQuery.select(
    allForeignKeys
      .filter(col => {
        return !relatedJoinFromQuery.hasSelectionAs(col, col);
      })
      .map(col => {
        return `${tableRef}.${col}`;
      })
  );
}

[koskimas]

I'l run and write some tests and this should be released in the next patch release.

[fer22f]

Thanks ;)