Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JoinEagerAlgorithm is not working #684

Closed
seeden opened this issue Dec 31, 2017 · 15 comments
Closed

JoinEagerAlgorithm is not working #684

seeden opened this issue Dec 31, 2017 · 15 comments

Comments

@seeden
Copy link

seeden commented Dec 31, 2017

When I use next code

const items = await QuizCoverPhoto
      .query()
      .whereIn('quizCoverPhoto.id', quizIds)
      .eagerAlgorithm(QuizCoverPhoto.JoinEagerAlgorithm)
      .eager('photo');

It will produce next SQL query (it is correct)

select "photo"."id" as "photo:id", "photo"."created_at" as "photo:created_at", "photo"."updated_at" as "photo:updated_at", "photo"."name" as "photo:name", "photo"."position" as "photo:position", "photo"."size" as "photo:size", "photo"."type" as "photo:type", "photo"."ext" as "photo:ext", "photo"."description" as "photo:description", "photo"."storage" as "photo:storage", "photo"."metadata" as "photo:metadata", "photo"."reactions" as "photo:reactions", "photo"."banned" as "photo:banned", "photo"."album_id" as "photo:album_id", "photo"."user_id" as "photo:user_id" from "quiz_cover_photo" left join "photos" as "photo" on "photo"."id" = "quiz_cover_photo"."photo_id" where "quiz_cover_photo"."id" in ($1, $2, $3)

But after query I can see next error

TypeError: Cannot read property 'photo' of undefined
    at OneToOnePathInfo.getModelFromBranch (/Users/seeden/Documents/git/node_modules/objection/lib/queryBuilder/operations/eager/RelationJoinBuilder.js:712:18)
    at RelationJoinBuilder.rowsToTree (/Users/seeden/Documents/git/node_modules/objection/lib/queryBuilder/operations/eager/RelationJoinBuilder.js:142:23)
    at JoinEagerOperation.onRawResult (/Users/seeden/Documents/git/node_modules/objection/lib/queryBuilder/operations/eager/JoinEagerOperation.js:42:29)
    at promise.then.result (/Users/seeden/Documents/git/node_modules/objection/lib/queryBuilder/QueryBuilder.js:1099:45)
    at tryCatcher (/Users/seeden/Documents/git/node_modules/bluebird/js/release/util.js:16:23)
    at Promise._settlePromiseFromHandler (/Users/seeden/Documents/git/node_modules/bluebird/js/release/promise.js:512:31)
    at Promise._settlePromise (/Users/seeden/Documents/git/node_modules/bluebird/js/release/promise.js:569:18)
    at Promise._settlePromise0 (/Users/seeden/Documents/git/node_modules/bluebird/js/release/promise.js:614:10)
    at Promise._settlePromises (/Users/seeden/Documents/git/node_modules/bluebird/js/release/promise.js:693:18)
    at Async._drainQueue (/Users/seeden/Documents/git/node_modules/bluebird/js/release/async.js:133:16)
    at Async._drainQueues (/Users/seeden/Documents/git/node_modules/bluebird/js/release/async.js:143:10)
    at Immediate.Async.drainQueues [as _onImmediate] (/Users/seeden/Documents/git/node_modules/bluebird/js/release/async.js:17:14)
    at runCallback (timers.js:773:18)
    at tryOnImmediate (timers.js:734:5)
    at processImmediate [as _immediateCallback] (timers.js:711:5)
@seeden
Copy link
Author

seeden commented Dec 31, 2017

After little bit of debugging I can see that encParentPath is empty. Did I miss something?

OneToOnePathInfo {
  path: 'photo',
  encPath: 'photo',
  encParentPath: '',
  modelClass: { [Function: Model] dataLoader: null },
  relation:
   BelongsToOneRelation {
     name: 'photo',
     ownerModelClass: { [Function: Model] dataLoader: null },
     relatedModelClass: { [Function: Model] dataLoader: null },
     ownerProp:
      RelationProperty {
        _refs: [Array],
        _modelClass: [Function],
        _props: [Array],
        _cols: [Array],
        _propGetters: [Array],
        _propSetters: [Array] },
     relatedProp:
      RelationProperty {
        _refs: [Array],
        _modelClass: [Function],
        _props: [Array],
        _cols: [Array],
        _propGetters: [Array],
        _propSetters: [Array] },
     joinTableModelClass: null,
     joinTableOwnerProp: null,
     joinTableRelatedProp: null,
     joinTableBeforeInsert: null,
     joinTableExtras: [],
     modify: [Function: modify],
     beforeInsert: [Function: beforeInsert] },
  omitCols: {},
  children: {},
  idGetter: [Function],
  relationAlias: 'photo',
  parentPath: '' }

@koskimas
Copy link
Collaborator

It does work. There are hundreds of tests that make sure of it. Could you provide a reproduction? You can use the reproduction-template.js in the repo root.

@seeden
Copy link
Author

seeden commented Dec 31, 2017

First of all I will try to fix it by myself. Maybe I missed something. Thanks

@koskimas
Copy link
Collaborator

koskimas commented Dec 31, 2017

That's one of the reasons I always ask for a reproduction: trying to reproduce the problem from the scratch is a great way to find the problem. If the problem ends up being an objection bug, the reproduction makes my life a lot easier.

@koskimas
Copy link
Collaborator

koskimas commented Dec 31, 2017

@seeden What are you using for snake_case --> camelCase conversion?

@seeden
Copy link
Author

seeden commented Dec 31, 2017

I have used your knexSnakeCaseMappers for knex.

@seeden
Copy link
Author

seeden commented Jan 1, 2018

I am trying to create minimal example right now.
I found that you are trying to get information about schema but with bad camelCase

  knex:query select * from information_schema.columns where table_name = $1 and table_catalog = $2 and table_schema = current_schema +14ms
  knex:bindings [ 'quizCoverPhotoTest', 'quizana' ]

as you can see table name is quizCoverPhotoTest but in the table is quizCoverPhotoTest

@seeden
Copy link
Author

seeden commented Jan 1, 2018

Here is my reproduction

import Knex from 'knex';
import { Model, knexSnakeCaseMappers } from 'objection';

class Photo extends Model {
  static modelName = 'PhotoTest';
  static tableName = 'photosTest';

  static relationMappings() {
    return {
      quizCoverPhoto: {
        relation: this.HasOneRelation,
        modelClass: QuizCoverPhoto,
        join: {
          from: `${this.tableName}.id`,
          to: `${QuizCoverPhoto.tableName}.photoId`,
        },
      },
    };
  }
}

class QuizCoverPhoto extends Model {
  static modelName = 'QuizCoverPhotoTest';
  static tableName = 'quizCoverPhotoTest';

  static relationMappings() {
    return {
      photo: {
        relation: this.BelongsToOneRelation,
        modelClass: Photo,
        join: {
          from: `${this.tableName}.photoId`,
          to: `${Photo.tableName}.id`,
        },
      },
    };
  }

  static async getPhotos(ids) {
    const items = await this
      .query()
      .whereIn(`${QuizCoverPhoto.tableName}.id`, ids)
      .eagerAlgorithm(this.JoinEagerAlgorithm)
      .eager('photo');

    const byIds = {};
    items.forEach(item => byIds[item.id] = item);

    return ids.map(id => byIds[id] && byIds[id].photo);
  }
}

const knex = Knex({
  ...knexSnakeCaseMappers(),
  client: 'postgresql',
  connection: process.env.POSTGRESQL_URL,
  // debug: true,
  pool: {
    min: 5,
    max: 15,
    idleTimeoutMillis: 30000,
  },
  migrations: {
    tableName: 'knex_migrations',
  },
});

const models = {
  Photo,
  QuizCoverPhoto,
};

// we need to register all models
Object.keys(models).forEach((m) => {
  // extend current model
  const model = models[m];
  model.knex(knex);
});

async function test() {
  await knex.schema.dropTableIfExists(QuizCoverPhoto.tableName);
  await knex.schema.dropTableIfExists(Photo.tableName);

  // init db
  await knex.schema.createTableIfNotExists(Photo.tableName, (table) => {
    table.increments('id').primary().unsigned();
    table.timestamps(true, true);
    table.string('name');
  });

  await knex.schema.createTableIfNotExists(QuizCoverPhoto.tableName, (table) => {
    table.increments('id').primary().unsigned();
    table.timestamps(true, true);

    table
      .integer('photoId')
      .unsigned()
      .notNullable()
      .index()
      .references('id')
      .inTable(Photo.tableName)
      .onDelete('CASCADE');
  });

  // add db items
  const photo = await models.Photo
    .query()
    .insert({
      name: 'test',
    })
    .returning('*');

  const quizCoverPhoto = await models.QuizCoverPhoto
    .query()
    .insert({
      photoId: photo.id,
    })
    .returning('*');

  // run code
  const [firstPhoto] = await models.QuizCoverPhoto.getPhotos([quizCoverPhoto.id]);
  if (firstPhoto) {
    console.log('TEST OK');
  } else {
    console.log('TEST FAILED');
  }

  console.log('first photo is: ', firstPhoto);
}

test();

I think that it is related to previous comment. Objection are trying to download all columns via information_schema.columns but it is using camelCase name for tables.

@seeden
Copy link
Author

seeden commented Jan 1, 2018

It looks like knex.columnInfo is not using wrapIdentifier or postProcessResponse. The problem is in the https://github.com/tgriesser/knex/blob/7de07c966a6ab63087b31cbdd10216d3ddbf8230/src/dialects/postgres/query/compiler.js#L79

I am not sure if they are going to fix it because they are very slow :(

@seeden
Copy link
Author

seeden commented Jan 1, 2018

I have tried to add snakeCase into the knex and it is working fine now. But it is just local solution

@koskimas
Copy link
Collaborator

koskimas commented Jan 2, 2018

@seeden Did you open an issue in knex github? This should be fixed there.

@koskimas
Copy link
Collaborator

koskimas commented Jan 2, 2018

Actually this is pretty much impossible to fix in objection without copying the knex code to objection for all dialects. There is a possible fix, but it will break if they ever do fix this in knex.

@koskimas
Copy link
Collaborator

koskimas commented Jan 2, 2018

@seeden knex/knex#2405

@seeden
Copy link
Author

seeden commented Jan 2, 2018

thanks @koskimas you rock 👍

@seeden
Copy link
Author

seeden commented Jan 2, 2018

I am closing this and waiting for merge in knex

@seeden seeden closed this as completed Jan 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants