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

UpdateQuery generic type makes no sense on a schema with virtual getter and setter #10689

Closed
DaDoBaag opened this issue Sep 7, 2021 · 6 comments
Labels
typescript Types or Types-test related issue / Pull Request
Milestone

Comments

@DaDoBaag
Copy link

DaDoBaag commented Sep 7, 2021

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

What is the current behavior?
UpdateQuery requires a Document interface generic but this doesn't work when your document has a virtual with getter and setter. If a virtual has both getter and setter I want to only allow the user to update either the get properties or the set property but the required document interface includes both.

If the current behavior is a bug, please provide the steps to reproduce.

// sample.model.ts

import mongoose from "mongoose"

export interface SampleInput {
  name: {
    firstName: string,
    lastName: string
  } | {
    fullName: string
  }
}

export interface SampleDoc extends SampleInput, mongoose.Document {
  name: { firstName: string, lastName: string, fullName: string }
}

const SampleSchema = new mongoose.Schema<SampleDoc>({
  name: {
    firstName: { type: String, required: true },
    lastName: { type: String, required: true }
  }
}

SampleSchema.virtual("name.fullName")
  .get(function (this: SampleDoc) { return this.name.firstName + " " + this.name.lastName })
  .set(function (this: SampleInput, fullName: string) {
    this.name = {
      firstName: fullName.split(" ")[0],
      lastName: fullName.split(" ")[1]
    }
  })

export const Sample = mongoose.model<SampleDoc>("Sample", SampleSchema)
// sample.service.ts

import { SampleInput, Sample } from "sample.model"

export const update = async (id: string, doc: SampleInput /* or UpdateQuery<SampleInput> */) => {
  // findByIdAndUpdate requires doc to be of type SampleDoc (or UpdateQuery<SampleDoc>) but that makes no sense
  // because SampleDoc requires all 3 name properties even though that would interfere with the virtual setter
  // hence why I want it to be SampleInput instead
  return await Sample.findByIdAndUpdate(id, doc).exec()
}

/* Error:
await Sample.findByIdAndUpdate(id, doc).exec()
                                   ^^^
No overload matches this call.
  The last overload gave the following error.
    Argument of type 'SampleInput' is not assignable to parameter of type 'UpdateWithAggregationPipeline | UpdateQuery<SampleDoc> | undefined'.
      Type 'SampleInput' is not assignable to type 'UpdateQuery<SampleDoc>'.
        Type 'SampleInput' is not assignable to type 'ReadonlyPartial<_AllowStringsForIds<LeanDocument<SampleDoc>>>'.
          Types of property 'name' are incompatible.
            Type '{firstName: string, lastName: string} | {fullName: string}' is not assignable to type '({firstName: string, lastName: string, fullName: string}) | undefined'.
              Type '{firstName: string, lastName: string}' is not assignable to type '{firstName: string, lastName: string, fullName: string}'.
                Property 'fullName' is missing in type '{firstName: string, lastName: string}' but required in type '{firstName: string, lastName: string, fullName: string}'. [ts(2769)]
*/
// tsconfig.json

{
  "compilerOptions": {
    "target": "ES2018",
    "module": "ESNext",
    "moduleResolution": "Node",
    "lib": ["ESNext", "ESNext.AsyncIterable", "DOM"],
    "esModuleInterop": true,
    "allowJs": true,
    "sourceMap": true,
    "strict": true,
    "noEmit": true,
    "experimentalDecorators": true,
    "baseUrl": ".",
    "types": ["@types/node"]
  },
  "exclude": ["node_modules", "dist"]
}

What is the expected behavior?
UpdateQuery integrates getters and setters in its generic type so it doesn't allow for both to be set at the same time.

What are the versions of Node.js, Mongoose and MongoDB you are using? Note that "latest" is not a version.
node v14.16.1
mongoose 5.13.3
mongodb 5.0.1

@IslandRhythms IslandRhythms added the typescript Types or Types-test related issue / Pull Request label Sep 7, 2021
@vkarpov15 vkarpov15 added this to the 6.0.7 milestone Sep 8, 2021
@vkarpov15
Copy link
Collaborator

vkarpov15 commented Sep 20, 2021

Our TypeScript docs don't recommend passing in an object that extends Document to Schema and Model. The spirit of DocType is that it should represent what your document looks like in the database. No virtuals, methods, etc.

We unfortunately don't have an ideal solution for virtuals right now, but the workaround is to pass a separate interface containing virtuals as the TInstanceMethods generic as shown below.

import { Schema, Model, model } from 'mongoose';

interface SampleInput {
  name: {
    firstName: string;
    lastName: string;
  }
}

interface SampleInputVirtuals {
  name: {
    fullName: string;
  }
}

type SampleModel = Model<SampleInput, {}, SampleInputVirtuals>; // <-- add virtuals here...

const schema = new Schema<SampleInput, SampleModel, SampleInputVirtuals>({ // <-- and here
  name: {
    firstName: String,
    lastName: String
  }
});

schema.virtual('name.fullName').get(function() {
  return `${this.name.firstName} ${this.name.lastName}`;
});

const Sample = model<SampleInput, SampleModel>('Sample', schema);

const doc = new Sample({ name: { firstName: 'Jean-Luc', lastName: 'Picard' } });
doc.name.firstName.toUpperCase();
doc.name.lastName.toUpperCase();
doc.name.fullName.toUpperCase();

In v6.0.7 we're adding a TVirtuals to the Model interface so you'll be able to do:

type SampleModel = Model<SampleInput, {}, {}, SampleInputVirtuals>; // <-- add virtuals as a separate generic so they don't conflict with methods

@DaDoBaag
Copy link
Author

Thank you for your suggestion @vkarpov15 however I think there's a mistake in your code and in the docs too actually:

type SampleModel = Model<SampleInput, {}, SampleInputVirtuals>; // <-- add virtuals here...

const schema = new Schema<SampleInput, SampleModel, SampleInputVirtuals>({ // <-- and here
  name: {
    firstName: String,
    lastName: String
  }
});

When I look at the type definition for Schema I find this:

class Schema<
  DocType = Document,
  M extends Model<DocType, any, any> = Model<any, any, any>,
  SchemaDefinitionType = undefined,
  TInstanceMethods = ExtractMethods<M>
> //...

It has 4 generic parameters, with the 3rd being SchemaDefinitionType (I don't know what that does, it's set to undefined by default) and the 4th being TInstanceMethods. In your example you suggest adding the SampleInputVirtuals interface to the TInstanceMethods generic but you put it in the 3rd slot instead the 4th. The same mistake happens in the docs:

The Mongoose Schema class in TypeScript has 3 generic parameters:

  • DocType - An interface descibing how the data is saved in MongoDB
  • M- The Mongoose model type. Can be omitted if there are no query helpers or instance methods to be defined.
    default: Model<DocType, any, any>
  • TInstanceMethods - An interface containing the methods for the schema.
    default: {}

This is wrong. The 3rd generic isn't TInstanceMethods. It's SchemaDefinitionType.

The reason why your example works (erroneously) is because there's no need to add SampleInputVirtuals to TInstanceMethods. That is because its default value is TInstanceMethods = ExtractMethods<M>, so if you already added the interface to TMethods of M it automatically extracts these methods.

@DaDoBaag
Copy link
Author

Should I open a new issue for this?

@vkarpov15
Copy link
Collaborator

@DaDoBaag that looks like the syntax from Mongoose 5.x, my example assumes Mongoose 6.x.

@DaDoBaag
Copy link
Author

DaDoBaag commented Oct 1, 2021

Oh I didn't realize that some typings changed with version 6, is there an overview of what changed? I don't find it in the changelog.

@vkarpov15
Copy link
Collaborator

@DaDoBaag https://mongoosejs.com/docs/migrating_to_6.html#typescript-changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
typescript Types or Types-test related issue / Pull Request
Projects
None yet
Development

No branches or pull requests

3 participants