nestjs / mongoose

Mongoose module for Nest framework (node.js) 🍸
https://nestjs.com
MIT License
528 stars 118 forks source link

Getting wrong property values of mongoose document using `@nestjs/mongoose` #1190

Closed Cartman720 closed 2 years ago

Cartman720 commented 2 years ago

Is there an existing issue for this?

Current behavior

if we warp property with @Prop() and add mongoose prop options e.g. @Prop({ type: String, unique: true }) in result we can't directly access our field as string, instead we get Document which causes errors. Instead we get Document and have to get property value via Document.get method in order to access value.

For example

Our article.schema.ts contains schema of blog article with 2 string fields slug and title.

title should have options, { trim: true, require: true } slug doesn't have any options, hence we just pass String to @Prop decorator

import { Prop, raw, Schema, SchemaFactory } from '@nestjs/mongoose';
import * as mongoose from 'mongoose';

export type BlogArticleDocument = BlogArticle & mongoose.Document;

@Schema({
  timestamps: true,
  toObject: {
    virtuals: true,
  },
  toJSON: {
    virtuals: true,
  },
})
export class BlogArticle {
  @Prop({
    type: String,
    require: true,
    trim: true,
  })
  title: string;

  @Prop(String)
  slug: string;
}

export const BlogArticleSchema = SchemaFactory.createForClass(BlogArticle);

BlogArticleSchema.loadClass(BlogArticle);

However when we try to access title on document object we can't directly access it as string e.g. title.replace() (throws TypeError: article.title.replace is not a function), thus we need to replace direct access as doc.title prop to doc.get('title') in order to make it work. Which isn't viable for most of the cases in large apps.

If we look carefully, title property contains object instead of desired string.

As opposed to this, slug property works fine if we pass @Prop(String).

It is notable, that if we leave @Prop Decorator without String for slug we will have same erronouse behavior.

Here the recording of the bug

https://jumpshare.com/v/NnaURK76LN0vJBf5x7nt

Minimum reproduction code

https://github.com/Cartman720/nestjs-mongoose-bug

Steps to reproduce

Run npm start to see the wrong behavior

Expected behavior

Get string or whatever type we set in schema for title without explicitly converting it to lean value.

Here the recording desired behavior

https://jumpshare.com/v/cHmKhZD5NELgmPPXt1nz

Run npm run mongoose for desired behavior

Package version

9.0.2

mongoose version

6.1.5

NestJS version

8.0.0

Node.js version

16.13.1

In which operating systems have you tested?

Other

No response

jmcdo29 commented 2 years ago

I haven't found the why yet, but I did find out that if you do @Prop(String), then any other { type: String } gets modified incorrectly to become an Object. Looks like mongoose doesn't really play well with the raw constructors for whatever reason. Removing the String or changing it to { type: String } makes this work fine.

kamilmysliwiec commented 2 years ago

Is there any specific reason why you used @Prop(String) in the first place? @Prop() should be sufficient.

Cartman720 commented 2 years ago

@kamilmysliwiec no specific reason. @Prop is sufficient of course, however it may cause little ambiguity, cause we are able to pass [String] but plain String causes huge problem. Personally I am accustomed to mongoose style type defnition, so logically if you can pass String in mongoose, then you should be able to do same here.

Although, again, not a big deal to omit, since with typescript we already indicate explicitly property type.

but this issue may have a big impact if you don't have an idea about this bug, since it took me and @jmcdo29 while to understand exact problem.

I think it's worth to mention about wrong behavior of passing type constructor to @Prop in docs. Or if you don't mind I will try to create fork to convert type constructors to { type: String }.

kamilmysliwiec commented 2 years ago

@Cartman720 If you can create a PR that automatically wraps primitive types like Number, Boolean, and String into the appropriate { type: ... } syntax that would be great.

Cartman720 commented 2 years ago

Hello everyone,

There is pull request that's going to fix this issue, so closing the issue.