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

bug: avoid circular mocks #65

Open
omermorad opened this issue Jul 25, 2021 · 20 comments · May be fixed by #137
Open

bug: avoid circular mocks #65

omermorad opened this issue Jul 25, 2021 · 20 comments · May be fixed by #137
Assignees
Labels
bug 🪲 Something isn't working

Comments

@omermorad
Copy link
Owner

No description provided.

@omermorad omermorad changed the title Feature: Avoid Circular Mocks Fix: Avoid Circular Mocks Jul 25, 2021
@omermorad omermorad self-assigned this Jul 25, 2021
@omermorad omermorad added the bug 🪲 Something isn't working label Jul 25, 2021
@omermorad omermorad changed the title Fix: Avoid Circular Mocks Bug: Circular mocks Aug 5, 2021
@DmitriyNikolenko
Copy link

Is it any solution to resolve the circular dependency issue?

@omermorad
Copy link
Owner Author

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

@DmitriyNikolenko
Copy link

DmitriyNikolenko commented Dec 8, 2021

@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
Copy link
Owner Author

@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
Copy link
Owner Author

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
Copy link

DmitriyNikolenko commented Dec 15, 2021

@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
Copy link
Owner Author

@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
Copy link
Contributor

Ptichi commented Dec 15, 2021

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
Copy link

arvindsedha commented Dec 28, 2021

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
Copy link
Owner Author

omermorad commented Dec 28, 2021

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
Copy link

@omermorad

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

Keep up the great work!

@arvindsedha
Copy link

Hi @omermorad

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

Regards
Arvind

@omermorad
Copy link
Owner Author

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: #65 (comment)

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
Copy link

Hi @omermorad

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

Regards
Arvind

@DmitriyNikolenko
Copy link

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

@omermorad
Copy link
Owner Author

@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
Copy link

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
Copy link
Owner Author

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

@omermorad
Copy link
Owner Author

@idanpt @yNaxon

@arvindsedha
Copy link

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

@omermorad omermorad linked a pull request Jan 22, 2022 that will close this issue
@omermorad omermorad changed the title Bug: Circular mocks bug: avoid circular mocks Jan 22, 2022
@omermorad omermorad linked a pull request Jan 22, 2022 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🪲 Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants