Bug: loadClass and virtual getters or setters (no args possible, getters and setters can not be overwritten at inheritance)

[CDaxi]

Do you want to request a feature or report a bug?

BUG

What is the current behavior?

loadClass will add virtuals in following order:

• Extended class
• Class

applyGetters and applySetters will execute the functions in reverse order.

• Later added
• Earlier added

  let v = value;
  for (let l = this.getters.length - 1; l >= 0; l--) {
    v = this.getters[l].call(doc, v, this, doc);
  }
  return v;

1. TechnicalUser.displayAs -> name
2. User.displayAs -> null

Because of the order mismatch the wrong value will be used.
I am not sure which order is the intended. I have also not testet what happens if a virtual with the same name is registered twice without loadClass (but if I have a look at the code line, the same problem should be reproducible).

If the current behavior is a bug, please provide the steps to reproduce.
Example Schemas (written in NestJS):

Base User Model:

@Schema({ discriminatorKey: 'type' })
@ObjectType()
export class User {
  @Prop()
  @Field((returns) => ObjectIdScalar, { name: 'id' })
  _id: ObjectId;

  @Field((type) => UserTypeEnum)
  type: string;

  @Field((type) => String, { nullable: true, name: 'displayAs' })
  get displayAs() {
    return null;
  }
}

export const UserSchema = SchemaFactory.createForClass(User);
UserSchema.loadClass(User);

Extending TechnicalUser (shortened)

@Schema()
@ObjectType()
export class TechnicalUser extends User {
  @Prop()
  @Field()
  name: string;

  @Prop()
  @Field()
  identifier: string;

  @Field((type) => String, { nullable: true })
  get displayAs() {
    return this.name;
  }
}
export const TechnicalUserSchema = SchemaFactory.createForClass(TechnicalUser);
TechnicalUserSchema.loadClass(TechnicalUser);

The loadClass method will detect the displayAs virtual getters in both models.
But if I use extends in TechnicalUser, the execution order is the following:

• Detect prototype class (User)
  • Add displayAs virtual getter from User (which returns null)
• Add displayAs virtual getter from TechnicalUser (which returns the name Prop)

Codelines:

• Recursive call for extended classes first: https://github.com/Automattic/mongoose/blob/master/lib/schema.js#L1903
• Add new virtual function: https://github.com/Automattic/mongoose/blob/master/lib/schema.js#L1930

The getters in VirtualType has these order:

1. User.dispayAs
2. TechnicalUser.displayAs

After fetching the user records by find without any condition, the getters will be applied in reversed order:

1. TechnicalUser.displayAs -> name
2. User.displayAs -> null
   all functions will be executed and overwrites the previous return value.
   https://github.com/Automattic/mongoose/blob/master/lib/virtualtype.js#L141-L153

What is the expected behavior?

In my opinion, there are two options:

1. Early returns
2. Reverse the execution order at getters

For early returns:
Replace https://github.com/Automattic/mongoose/blob/master/lib/virtualtype.js#L148-L152

  let v = value;
  for (let l = this.getters.length - 1; l >= 0; l--) {
    v = this.getters[l].call(doc, v, this, doc);
  }
  return v;

with

  let v = value;
  for (let l = this.getters.length - 1; l >= 0; l--) {
    return this.getters[l].call(doc, v, this, doc);
  }
  return v;

Or reverse the order (causes unnecessary calls):

  let v = value;
  for (let l = 0; l < this.getters.length; l++) {
    v = this.getters[l].call(doc, v, this, doc);
  }
  return v;

For setters the reversed order does also makes no sense because the more specific return will also be overwritten.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.

From package-lock.json:

    "node_modules/@nestjs/mongoose": {
      "version": "7.2.4",
      "resolved": "https://registry.npmjs.org/@nestjs/mongoose/-/mongoose-7.2.4.tgz",
      "integrity": "sha512-NTE/IwijFUEJytPHLoHRYe0X5p16W1Awf8tm6I3mWsIUuBnSDfMyN0fy30uVaM/exYvCkMwEsAVpeSeKVPMHxg==",
      "peerDependencies": {
        "@nestjs/common": "^6.0.0 || ^7.0.0",
        "@nestjs/core": "^6.0.0 || ^7.0.0",
        "mongoose": "^5.11.15",
        "reflect-metadata": "^0.1.12",
        "rxjs": "^6.0.0"
      }
    },
    "node_modules/mongoose": {
      "version": "5.11.18",
      "resolved": "https://registry.npmjs.org/mongoose/-/mongoose-5.11.18.tgz",
      "integrity": "sha512-RsrPR9nhkXZbO3ml0DcmdbfeMvFNhgFrP81S6o1P+lFnDTNEKYnGNRCIL+ojD69wj7H5jJaAdZ0SJ5IlKxCHqw==",
      "dependencies": {
        "@types/mongodb": "^3.5.27",
        "bson": "^1.1.4",
        "kareem": "2.3.2",
        "mongodb": "3.6.4",
        "mongoose-legacy-pluralize": "1.0.2",
        "mpath": "0.8.3",
        "mquery": "3.2.4",
        "ms": "2.1.2",
        "regexp-clone": "1.0.0",
        "safe-buffer": "5.2.1",
        "sift": "7.0.1",
        "sliced": "1.0.1"
      },
      "engines": {
        "node": ">=4.0.0"
      },
      "funding": {
        "type": "opencollective",
        "url": "https://opencollective.com/mongoose"
      }
    },

[CDaxi]

GIT Patch:

Index: lib/virtualtype.js
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
diff --git a/lib/virtualtype.js b/lib/virtualtype.js
--- a/lib/virtualtype.js	(revision abf7b025c3dd17195d3f9df456e68a6a0ce956b0)
+++ b/lib/virtualtype.js	(revision 6c8968e94ee4dd4e139732f52e78903e343536a2)
@@ -147,9 +147,8 @@
 
   let v = value;
   for (let l = this.getters.length - 1; l >= 0; l--) {
-    v = this.getters[l].call(doc, v, this, doc);
+    return this.getters[l].call(doc, v, this, doc);
   }
-  return v;
 };
 
 /**
@@ -164,9 +163,8 @@
 VirtualType.prototype.applySetters = function(value, doc) {
   let v = value;
   for (let l = this.setters.length - 1; l >= 0; l--) {
-    v = this.setters[l].call(doc, v, this, doc);
+    return this.setters[l].call(doc, v, this, doc);
   }
-  return v;
 };
 
 /*!

[CDaxi]

Ok, I know the intention behind multiple getters and setters now to have inheritance on value level too.
But with typescript a getter function can not receive the arguments v, this and doc which will be handed over at this.getters[l].call or this.setters[l].call.

This means something like this does not work in typescript because parameters are not allowed in getters:

get displayAs (prevValue, virtualType, doc) {
  return prevValue || null;
}

[CDaxi]

There are two additional ways to cover the getters problem in typescript:

Option one: add a new clearGetters / clearSetters function

virtualtype.js

VirtualType.prototype.clearGetters = function() {
  this.getters = [];
  return this;
};

VirtualType.prototype.get = function(fn) {
  this.getters.push(fn);
  return this;
};

schema.js

    if (typeof method.get === 'function') {
      this.virtual(name).clearGetters().get(method.get);
    }
    if (typeof method.set === 'function') {
      this.virtual(name).clearSetters().set(method.set);
    }

Pro: Keeps the logic as it is, daisy chaining is possible for short and clear logic.
Con: Additional complexity caused by new function (new edge cases possible)

Option two: add a parameter to .get / .set to define overwriting
virtualtype.js

VirtualType.prototype.get = function(fn, overwrite = false) {
  if (overwrite === true) {
    this.getters = [fn];
  } else {
    this.getters.push(fn);
  }
  return this;
};

schema.js

    if (typeof method.get === 'function') {
      this.virtual(name).get(method.get, true);
    }
    if (typeof method.set === 'function') {
      this.virtual(name).set(method.set, true);
    }

Pro: Number of functions are the same, behavior is the same as before only with additional switch param
Con: Less clear to understand what happens if no code completion is available.

Summary

• In both solutions, the original behavior is still possible.
• The new behavior is only active for ES6 classes
• ES6 classes can use super.displayAs or whatever to get parent getters value.

Example for parent access:

  get displayAs() {
    return `${this.firstName} ${this.lastName} (${super.displayAs})`;
  }

[vkarpov15]

We'll be applying virtual getters in forward order rather than reverse order in 6.0, see #8897.

The issue here is that loadClass() is adding virtual getters from all the way up the prototype chain, and it shouldn't. Fix will be in v5.11.19 👍