omermorad / mockingbird

🐦 Decorator Powered TypeScript Library for Creating Mocks
MIT License
87 stars 5 forks source link

bug: avoid circular mocks #65

Open omermorad opened 3 years ago

DmitriyNikolenko commented 2 years ago

Is it any solution to resolve the circular dependency issue?

omermorad commented 2 years ago

Can you please give an example here? I'm working on a fix

DmitriyNikolenko commented 2 years ago

@omermorad Here is the simplest example of the issue made within NestJS application. https://codesandbox.io/s/nestjs-mockingbird-ts-issue-00uqo

The case when the circular dependency issue is absent: https://ibb.co/xXNF2CK

The case when the circular dependency issue is present: https://ibb.co/sR7dVZK

omermorad commented 2 years ago

@DmitriyNikolenko, this requires a quite large change - moving into version 3.0.0 which will be released soon(!), in order to prevent this kind of scenarios. It will use the same mechanism as TypeORM and ClassValidator/ClassTransformer:

export class User {
  @Mock()
  name: string;

  // Using a callback here instead of putting the class explicitly 
  @Mock(() => Product)
  product: Product;
}

I will let you know as soon as we finish 🙏🏻

omermorad commented 2 years ago

Is it any solution to resolve the circular dependency issue?

@DmitriyNikolenko, now that I'm diving into your example, it looks like you are creating an infinite loop:

export class Product {
  @Mock()
  title: string;

  @Mock(User)
  user: User;
}
export class User {
  @Mock()
  name: string;

  @Mock(Product)
  product: Product;
}

Think about it, what result would you expect from this kind of fixture?

DmitriyNikolenko commented 2 years ago

@omermorad I expect to avoid war with this error in the project :) (https://ibb.co/sR7dVZK)

Because TypeORM decorators line OneToMany, class-transformer decorators like Type and nestjs/swagger decorators like ApiProperty allows me to annotate cross types, but when I add mockingbird the app can crash on the build stage. I begin to struggle with errors and have to find out a combination of imports that will not crush the app.

It makes me sad because I am really excited about your library and I was going to keep on using it.

omermorad commented 2 years ago

@DmitriyNikolenko I understand the nature of cross-types, but I think it's different in Mockingbird; What I meant to ask is for an example of the end-result of the fixture.

Let's take User for example:

{
  name: 'AwesomeUser',
  product: {
     title: 'Some Product'
     user: // Whats gonna be here?
  }
}

The only solution I can think of is to indicate of many "iterations" you want until you stop filling those fixtures

@idanpt @potoos @yNaxon what do you think?

Ptichi commented 2 years ago

Maybe it's worth setting up a configuration of max-nesting-level and support circular references (maybe with an exit condition) up to the configured nesting limit

arvindsedha commented 2 years ago

Hi @omermorad

We are using your library and currently having lots of issues due to nesting problems. I have seen in the conversation that you are planning to release the fix in version 3.0.

Is there any timeline you have in mind for the version release with this fix?

Thanks for your effort and great work :)

omermorad commented 2 years ago

Hi @arvindsedha!

I'm sorry to hear! We don't want any problems with the library of course, and we are working on a fix ASAP, it's going to be released in the next two weeks for sure!

Can you share the use cases you have problems with? And how would you expect it to work from your side?

I guess you mean:

class Bird {
  @Mock('Absolute Name')
  name: string;

  @Mock(() => Bird)
  bird: Bird;
}

?

arvindsedha commented 2 years ago

@omermorad

Yes I mean the problem @DmitriyNikolenko mentioned. Great to hear that fix is soon on the way :)

Keep up the great work!

arvindsedha commented 2 years ago

Hi @omermorad

What's the situation of the release with this bug fix? When it is expected to be shipped?

Regards Arvind

omermorad commented 2 years ago

Yo @arvindsedha,

I tried to get an idea from you guys about how would you expect this kind of situation to behave; We both understand that generated mocks cannot be infinite, look at my previous replay: https://github.com/omermorad/mockingbird/issues/65#issuecomment-994465614

If you can tell me more about the solution you are expecting to get, it will be very easy for me to manage a discussion about it and to understand what's the best solution in terms of refactoring and immediate fix.

arvindsedha commented 2 years ago

Hi @omermorad

Thanks. @DmitriyNikolenko would you please answer or explain how it should work?

Regards Arvind

DmitriyNikolenko commented 2 years ago

I expect behaviour like class-transformer does https://github.com/typestack/class-transformer#how-does-it-handle-circular-references

omermorad commented 2 years ago

@DmitriyNikolenko It says:

Circular references are ignored. For example, if you are transforming class User that contains property photos with type of Photo, and Photo contains link user to its parent User, then user will be ignored during transformation. Circular references are not ignored only during classToClass operation.

So you expect it to be ignored?

DmitriyNikolenko commented 2 years ago

Expected behavior stems from real-world use cases. I use your splendid library for generating fixtures in order to insert to database. Imagine we have the User and UserPermissons entities. When I create User mock I expect to get permissions created a s well, but it's unnecessary to create User mock in each permission because they are already have a user as a parent. Like { Id: 1, name: " Dmitriy", permissions: [{ title: "admin"}, { title: "member"}] }

Otherwise, when I'm going to create an array of permissions they should have user created for each permission but without references back. Like: [{ Title : " admin", user: { id: 2, name: " Bob" } },{ title: "member", user: { id: 5, name: "John" } }]

It's my opinion.

omermorad commented 2 years ago

@DmitriyNikolenko makes sense. @arvindsedha this kind of idea gives you a solution as well?

omermorad commented 2 years ago

@idanpt @yNaxon

arvindsedha commented 2 years ago

@omermorad yes. @DmitriyNikolenko knows this area well! Thanks @DmitriyNikolenko