-
-
Notifications
You must be signed in to change notification settings - Fork 29
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
Entity id is not populated #134
Comments
Hey Martin, if you can confirm that this is related to the Schema definition, and point me in some direction in the Mikro-orm code that I can investigate, I'd be happy to try to fix that. |
This is indeed pretty weird. We have tests for the EntitySchema definition, but looking at the entities now, we dont have any for the bigints. Maybe there will be some in the tests/issues, havent check those... The code that handles mapping the PK to entity is here: Its an autoincrement column, so with postgres it should be part of the returning clause, that is the first thing to ensure. Then the
This sounds like you are messing up with the context. |
I see you are rolling back the changes in your Could be as well something with resolving the EM from nest DI - so first thing I would do is to verify whether its an issue with the DI or with the ORM itself. I would bet on the nest DI, as I'd expect to see such issue already if it would be about the ORM. But maybe people don't use the |
Thanks for the reply, Martin! I'll check the code within the ORM to see what's going on under the hood here.
I believe you are talking about this Repo - which is from Tolga, a colleague that works at Trilon with me. I actually also asked him help with that, but we didn't find the root cause yet.
I'm almost sure I've tried to get the |
Hey @B4nan, I found out that the code isn't executing the
Does it shed some light? |
Clearing the context means you remove the link from the EM to the entities, but they still hold the state object (WrappedEntity instance), that is what decides whether the entity is considered managed or not. You should never reuse the same entity instance across multiple EM forks, this is a side effect of doing so. It's basically about the Just thinking, I am not entirely sure about this, and I am open to discussion. But doing such changes would be almost certainly breaking and would have to go to v6 if something. |
I believe I'm not using the same entity instance across multiple EM forks between each test 🤔. Is there a way to force the reset of that state object? - but that would be a patch. I'm still trying to understand why only the third test considers the entity as managed - I'm not explicitly using any By the way, I found out that the {
id: undefined,
firstName: "John3",
lastName: "Doe3",
email: "user3@mail.com",
createdAt: "2023-08-15T17:42:39.367Z",
updatedAt: "2023-08-15T17:42:39.368Z",
} I noticed a clear difference from the payload we got from the {
firstName: "John",
lastName: "Doe",
email: "user@mail.com",
createdAt: "2023-08-15T17:47:17.277Z",
updatedAt: "2023-08-15T17:47:17.277Z",
} In the
I also think it feels a bit wrong, and I believe if we find out why that specific test is considering that entity to be managed, we can avoid that. For some reason, the first two tests correctly consider the new user entity as a new entity. |
Sorry for the confusion, it's actually about something else, its about having the |
Ohh, got it, yea, I believe this is in the right direction. So, for some reason, the |
One unrelated note about the tests - this will always just give you the const newUserInDb = await repository.findOne(user.id); |
Ohh, I see, thanks for the context, Martin! Is there a way to enforce this, or am I just using it wrong? By the way, I found out that the |
Hey again, Martin. I just found out something else. There's a way to make these tests pass:
These tests still fail when we have the following combinations:
So, I believe this has something to do with how vitest works. Considering it uses |
Ok, this is pretty tricky, but the issue seems to be that you are calling Long time haven't seen anything so weird :]
There are several things you can do, the common theme is that you need a new fork. Either first call |
Oh, that's great, Martin, thank you for that clarification! Is there anywhere you can point me to in the code so I can investigate this further? I'd really like to understand and fix that so we can finally start using Mikro ORM. |
Hey @B4nan, I'd like to add that when using Jest, my tests are still calling the |
Hello again, Martin, I'd like to summarize my findings and ask you a question that I believe is the key to solving this issue.
My question is: When do we populate the __originalEntityData? I've tried to debug the whole application but couldn't track it down. I put a debug breakpoint in every place that was supposed to change the
{
firstName: "John2",
lastName: "Doe2",
email: "user2@mail.com",
createdAt: "2023-08-16T13:50:38.557Z",
updatedAt: "2023-08-16T13:50:38.557Z",
id: "358",
} That means Mikro-orm thinks the third user here is actually an update on the second user. In fact, any additional user I instantiate during this third test is considered an update on the second user. How does Mikro-orm track that info? 🤔 I'm not sure yet, but I felt that additional information could be useful for us. |
⛔ More context to this bug. I'm not sure why, but this has deeper problems than I'd have imagined at first. I've been trying to execute the tests using both We should have 3 entities in the database, but Mikro-orm is always updating the second entity data in the third test. So, once again this has to do with how the
About that Martin, I've checked with Kamil, and creating the MikroORM instance three times is the expected behavior. This is the line that creates a new provider for MikroORM: https://github.com/mikro-orm/nestjs/blob/41ba27b34be069795337a3c2bc73fc9fa45c8b3f/src/mikro-orm.providers.ts#L26C26-L26C26 This is indeed executed once for each test, since each test will build the module (because of the beforeEach hook), and then Edit: I've also confirmed that if we change the hooks so we only build MikroORM in the |
It must be something with how the |
For vitest to work I had to also adjust the import provider, as that is how it brings the TS support, and also enable it explicitly, as the auto-detection won't work with vitest. But I just checked and there seems to be -autoLoadEntities: true,
+entities: ['dist/**/*.schema.js'],
+entitiesTs: ['src/**/*.schema.ts'],
+dynamicImportProvider: (id) => import(id),
+tsNode: process.env.VITEST === 'true', |
Ok, I think I found it. |
Try v5.2.1 |
It worked, @B4nan! Thank you very much for all the support! :) Do you care explaining what was the root cause of that? I see that you are making a shallow copy of the I also believe it would be helpful to have a comment in the code so we don't forget why that change was added |
Because the |
Describe the bug
User
entity class with its schemaautoLoadEntitites: true
option.Error: You cannot call 'EntityManager.findOne()' with empty 'where' parameter
This is how each test looks like:
Stack trace
To Reproduce
Steps to reproduce the behavior:
yarn
docker compose up -d
yarn test
Expected behavior
All tests should have passed
Additional context
jest
by executingyarn run jest
EntityManager.unitOfWork.identityMap
is empty - even when calling thepersistAndFlush()
, it looks like the ORM isn't able to recognize itVersions
The text was updated successfully, but these errors were encountered: