tannerntannern / ts-mixer

A small TypeScript library that provides tolerable Mixin functionality.
MIT License
379 stars 27 forks source link

Multi mixins when using with @nestjs/mongoose not working #35

Closed mdwekat closed 2 years ago

mdwekat commented 3 years ago

Consider the following case:


import { Prop, Schema, SchemaFactory } from '@nestjs/mongoose';
import { decorate, Mixin } from 'ts-mixer';

export type PostDocument = Post & Document;

@Schema()
class Likeable{
    @decorate(Prop())
    likes:number
}

@Schema()
class Tagable{
    @decorate(Prop())
    tags:string[]
}

@Schema({ timestamps: true })
class Post extends Mixin(Tagable,Likeable){
    @Prop()
    title:string;
    @Prop()
    body:string;
}

export const PostSchema = SchemaFactory.createForClass(Post);

Now using mongoose to update the document will ignore the Mixin props

    // get the post document then do
    post.likes = 10;
    post.save();

the document will not change in the database. but if you used only one mixin the above example will work.

// This will work fine (Note only used one mixin)
@Schema({ timestamps: true })
class Post extends Mixin(Likeable){
    @Prop()
    title:string;
    @Prop()
    body:string;
}
tannerntannern commented 3 years ago

Hi @mdwekat, thanks for finding this. As I've mentioned in other issues, I'm hesitant to dive deep into problems related to 3rd party libraries unless it's very clear that the issue is caused by ts-mixer. I simply don't have time to sort out possible compatibility issues with every library out there. That is not to say that I won't investigate at all, but it's not clear to me that ts-mixer is the culprit here.

Here are a few things I'd try if I were trying to pinpoint the source of this issue:

Please note that I'm not familiar with mongoose or nestjs, so these are just vague intuitions. If neither of these experiments reveal anything, my suggestion would be to fork this repo and create a test file under test/gh-issues that reproduces your issue. This will likely be the quickest way for you to create a minimal reproducible example that I can play with and troubleshoot further. As a bonus, if this is indeed a ts-mixer problem, we'll then have a regression test already built in to the test suite when the issue is resolved. 🙂

TrejGun commented 2 years ago

it does not work with @nestjs/swagger and class-validator too, probably because all of them use dependency injections

actaruss commented 2 years ago

Any news about this issue? Will be great to have the support for nestJs mongoose decorators 😭

tannerntannern commented 2 years ago

@actaruss I've been waiting for someone to provide a minimum reproducible example. And by that I don't just mean a code sample without context -- I need a repository with all dependencies specified, clear description of expected behavior, and code that demonstrates how the violation of expected behavior connects to ts-mixer.

I have very limited time to support this library. I simply can't dig into every "doesn't work with X library" issue without strong reason to believe that ts-mixer is the problem. Hopefully you understand where I'm coming from. I'd be happy to investigate further once I have that minimum reproducible example.

TrejGun commented 2 years ago

@tannerntannern

Hey man! thanks for your efforts. Here is a minimal example you are looking for

https://github.com/GemunIon/nestjs-validation/tree/ts-mixer please note this is not the main branch

how to use:

npm i
npm start

then there are two examples

first one is about class-validator

curl http://localhost:3000/test/ts-mixer

to request URL with bug that prints

{"statusCode":400,"message":[{"target":{},"children":[],"constraints":{"unknownValue":"an unknown value was passed to the validate function"}}],"error":"Bad Request"}

while the real error should be something like this

{"statusCode":400,"message":[{"target":{},"property":"a","children":[],"constraints":{"max":"a must not be greater than 10","min":"a must not be less than 0","isInt":"a must be an integer number"}}],"error":"Bad Request"}

and the second bug is with swagger

navigate to http://localhost:3000/swagger in the browser

the documentation is not populated as shown in the picture

Screen Shot 2022-04-30 at 09 41 13

I believe the bug with mongoose will be solved together with these two because this is one framework

please refer to the last commit for related code and do not hesitate to ask me for more help

tannerntannern commented 2 years ago

@TrejGun Thank you for the example code. I did not spend a whole lot of time on this, but the first thing I changed was adding the @decorate wrapper, as suggested above and described in more detail in the README (please see section on "Mixing with Decorators" if the link doesn't work correctly). For example:

import { ApiProperty } from "@nestjs/swagger";
import { IsInt, Max, Min } from "class-validator";
import { Mixin, decorate } from "ts-mixer";

export class ADto {
  @decorate(ApiProperty())  // instead of `@ApiProperty()`
  @decorate(IsInt())
  @decorate(Min(0))
  @decorate(Max(10))
  public a: number;
}

export class BDto {
  @decorate(ApiProperty())
  @decorate(IsInt())
  @decorate(Min(0))
  @decorate(Max(10))
  public b: number;
}

export class MixedDto extends Mixin(ADto, BDto) {}

After I made those changes, the swagger page seems to have the missing parameters you were looking for: ts-mixer1

I also appear to get the API error response you were expecting when visiting http://localhost:3000/test/ts-mixer:

{"statusCode":400,"message":[{"target":{},"property":"a","children":[],"constraints":{"max":"a must not be greater than 10","min":"a must not be less than 0","isInt":"a must be an integer number"}},{"target":{},"property":"b","children":[],"constraints":{"max":"b must not be greater than 10","min":"b must not be less than 0","isInt":"b must be an integer number"}}],"error":"Bad Request"}

Can you please verify for me that this resolves your issue?

TrejGun commented 2 years ago

thanks this works!

tannerntannern commented 2 years ago

Thanks @TrejGun.

@mdwekat @actaruss Can you also verify that this resolves the problem? I'd like to close this issue.

mdwekat commented 2 years ago

@tannerntannern Thanks for all the support and time. I was testing this today and it worked like a charm 😄