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]: Loaded type does not catch missing population types when Collection is in populate array #5580

Closed
5 tasks done
kpervin opened this issue May 15, 2024 · 7 comments
Closed
5 tasks done

Comments

@kpervin
Copy link
Contributor

kpervin commented May 15, 2024

Describe the bug

When adding a function that has the loaded type as an argument, it should only accept an entity that has those populated relations. However, if you introduce a Collection relation into the populate array when fetching the entity before passing it to the function, Loaded seems to not throw an error on the function argument.

Reproduction

https://github.com/kpervin/mikro-orm-reproduction/tree/loaded-population-issue

What driver are you using?

@mikro-orm/mysql

MikroORM version

next

Node.js version

20.9.0

Operating system

5.15.146.1-microsoft-standard-WSL2

Validations

@B4nan
Copy link
Member

B4nan commented May 15, 2024

You need to use the Ref wrapper on the M:1 properties, otherwise this cannot work.

@B4nan B4nan closed this as completed May 15, 2024
@kpervin
Copy link
Contributor Author

kpervin commented May 15, 2024

Alright, glad to know that's the issue. I figured that was likely the case, as it was resolved as soon as I added the Ref attribute. That being said, the types still don't match so therefore it should throw an error, no? Because it errors as expected if you only add the non-ref M:1 relations into the populate array, but as soon as you add the posts collection in there, it seems to resolve fine.

@B4nan
Copy link
Member

B4nan commented May 15, 2024

Nope, without using the Ref wrapper, there is no difference between a populated and not populated relation property (on type level), without that the assignability cannot work properly - its not about comparing the populate hint, its about assignability of what the Loaded type resolves.

@kpervin
Copy link
Contributor Author

kpervin commented May 15, 2024

Well I have a lot of refactoring to do it seems given we did not use the Ref property in the beginning...

@B4nan
Copy link
Member

B4nan commented May 15, 2024

It would be technically possible to make this work without the Ref wrapper, but it would be quite a breaking change. It's not just about the assignability of the Loaded type itself in explicit hints, it would affect the assignability of relations in general.

Maybe something to think about when I start thinking about v7 (next year+).

@kpervin
Copy link
Contributor Author

kpervin commented May 15, 2024

Not sure if it's worth it, tbh. It's kind of in the name: Type-Safe relations :P

@kpervin
Copy link
Contributor Author

kpervin commented May 16, 2024

On second thought, it would be nice to have relations loaded via populate and it affects the returned value, where any omitted values from the populate array is simply omitted from the returned entity type not unlike Prisma. That would possibly eliminate the need for the Ref type in its entirety. It would be a fundamental change however, but what are your thoughts on that? Possibly a means to do so would require that every entity extends BaseEntity at least to some degree to pick up that it is an entity type. The Ref type could be then be relegated to only situations of lazy-loading (possibly renamed to Lazy or something).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants