Vincit / objection.js

An SQL-friendly ORM for Node.js
https://vincit.github.io/objection.js
MIT License
7.24k stars 635 forks source link

Overwritten fromDatabaseJson does not fetch relations #2251

Closed yannbriancon closed 1 year ago

yannbriancon commented 2 years ago

Objectionjs version: 3.0.1

I am referencing the same table in several models and followed the resolution of the issue #1674.

This comment from @koskimas looked exactly like what I needed to have a generic model and fetch specific models from it.

You should be able to override the static query method like this

class Message extends Model {
  static get tableName() {
    return 'posts'
  }

  static query(...args) {
    return super.query(...args).where('post_type', 'message')
  }
}

Model instances are always created using either fromJson or fromDatabaseJson methods. You should be able to override those. Something like this

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

  static fromDatabaseJson(json) {
    if (json.post_type === 'message') {
      return Model.fromDatabaseJson.call(Message, json)
    } else if (json.post_type === 'blog') {
      return Model.fromDatabaseJson.call(Blog, json)
    } else {
      return Model.fromDatabaseJson.call(Post, json)
    }
  }
}

The Model.fromDatabaseJson.call trick is needed to not invoke the inherited fromDatabaseJson method again causing an infinite recursion. You could also pass a second argument indicating the case or whatever you can think of.

Let me know if this works.

It works perfectly except when I add a relation to the Message model.

Then the query Post.query().select().withGraphFetched('user') returns a Message with the field user: undefined. Though, the query Message.query().select().withGraphFetched('user') returns the Message with the field user filled.

I manage to track down the issue to the method onAfter2 in WhereInEagerOperation.js.

The check !(models[0] instanceof modelClass) below return false because Message is not an instance of Post. So the relations are not fetched.

async onAfter2(builder, result) {
    const modelClass = builder.resultModelClass();

    if (!result) {
      return result;
    }

    const models = asArray(result);

    if (!models.length || !(models[0] instanceof modelClass)) {
      return result;
    }

    await promiseUtils.map(
      this.relationsToFetch,
      (it) => this.fetchRelation(builder, models, it.relation, it.childExpression),
      { concurrency: modelClass.getConcurrency(builder.unsafeKnex()) }
    );

    const intOpt = builder.internalOptions();

    if (!this.omitProps.length || intOpt.keepImplicitJoinProps) {
      return result;
    }

    // Now that relations have been fetched for `models` we can omit the
    // columns that were implicitly selected by this class.
    for (let i = 0, l = result.length; i < l; ++i) {
      const model = result[i];

      for (let c = 0, lc = this.omitProps.length; c < lc; ++c) {
        modelClass.omitImpl(model, this.omitProps[c]);
      }
    }

    return result;
  }

I would like to have your opinion on this issue to see if we should consider it as a bug or propose a workaround.

Thank you in advance for your help ;)

yannbriancon commented 2 years ago

@koskimas I traced the condition !(models[0] instanceof modelClass) back to the initial commit of the library in the file MoronQueryBuilder.js.

function eagerFetch(builder, models) {
  if (models instanceof builder._modelClass || (_.isArray(models) && models[0] instanceof builder._modelClass)) {
    return builder._modelClass.loadRelated(models, builder._eagerExpression, builder._allowedExpression);
  } else {
    return models;
  }
}

Do you remember why it is needed?

yannbriancon commented 1 year ago

Anyone has any lead on this? @koskimas