Closed thiagomini closed 1 year ago
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 mapPrimaryKey
should be used for mapping the PK for single entity flush, when the insertId
is returner from the driver, and alternatively the mapReturnedValues
method handles hydrating all the other columns from the returning clause. The reloadVersionValues
method is there to ensure this works with mysql, where we dont have the returning clause support and need to fire a separate query to fetch the actual values. But for that we need the PK (it needs to be mapped via the mapPrimaryKey
).
The weird thing is that it fails only when we run exactly three tests in a row. If you run only one or two, they pass.
This sounds like you are messing up with the context.
I see you are rolling back the changes in your afterEach
hook, maybe its connected to that. I haven't tried this approach myself, but I recall one issue from someone who made it work and provided some example repo...
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 EntitySchema
that often, hard to say.
Thanks for the reply, Martin! I'll check the code within the ORM to see what's going on under the hood here.
but I recall one issue from someone who made it work and provided some example repo...
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.
Could be as well something with resolving the EM from nest DI
I'm almost sure I've tried to get the EM
from the orm
instance with orm.em
, but it didn't work! But I'll try it again and push the change to the repro. Hopefully, I'll get to the cause of this issue by the end of today! Once again, I appreciate the directions.
Hey @B4nan, I found out that the code isn't executing the persistNewEntity
method as we have thought. Actually, on the third run, it's executing the persistManagedEntity. So, for some reason, the ORM is considering this entity to be a managed entity, even though I'm clearing the context orm.em.clear()
after each test. Moreover, here's a stack trace I got from removing the transaction explicit rollback:
FAIL src/mikro-orm/mikro-orm-internal.module.spec.ts > MikroOrmInternalModule > creates a new user
NotNullConstraintViolationException: update "user" set "id" = NULL, "created_at" = '2023-08-15T17:37:14.737Z', "updated_at" = '2023-08-15T17:37:14.738Z' where "id" = '137' - null value in column "id" violates not-null constraint
❯ PostgreSqlExceptionConverter.convertException node_modules/@mikro-orm/postgresql/PostgreSqlExceptionConverter.js:24:24
❯ PostgreSqlDriver.convertException node_modules/@mikro-orm/core/drivers/DatabaseDriver.js:197:54
❯ node_modules/@mikro-orm/core/drivers/DatabaseDriver.js:201:24
❯ PostgreSqlDriver.nativeUpdate node_modules/@mikro-orm/knex/AbstractSqlDriver.js:345:19
❯ ChangeSetPersister.persistManagedEntity node_modules/@mikro-orm/core/unit-of-work/ChangeSetPersister.js:137:21
❯ ChangeSetPersister.executeUpdates node_modules/@mikro-orm/core/unit-of-work/ChangeSetPersister.js:42:13
❯ ChangeSetPersister.runForEachSchema node_modules/@mikro-orm/core/unit-of-work/ChangeSetPersister.js:68:13
❯ UnitOfWork.commitUpdateChangeSets node_modules/@mikro-orm/core/unit-of-work/UnitOfWork.js:789:9
❯ UnitOfWork.persistToDatabase node_modules/@mikro-orm/core/unit-of-work/UnitOfWork.js:707:13
❯ Parser.parseErrorMessage node_modules/pg-protocol/src/parser.ts:369:69
❯ Parser.handlePacket node_modules/pg-protocol/src/parser.ts:188:21
❯ Parser.parse node_modules/pg-protocol/src/parser.ts:103:30
❯ Socket.<anonymous> node_modules/pg-protocol/src/index.ts:7:48
❯ Socket.emit node:events:513:28
⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯⎯
Serialized Error: { length: 241, severity: 'ERROR', code: '23502', detail: 'Failing row contains (null, user@mail.com, John, Doe, 2023-08-15 17:37:14.737, 2023-08-15 17:37:14.738).', hint: undefined, position: undefined, internalPosition: undefined, internalQuery: undefined, where: undefined, schema: 'public', table: 'user', column: 'id', dataType: undefined, constraint: undefined, file: 'execMain.c', line: '1965', routine: 'ExecConstraints' }
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 WrappedEntity.__initialized
property, we could probably unset this when calling em.clear
, but it feels a bit wrong, that specific entity instance is initialized already. Moreover, you could as well merge an existing (and initialized) entity instance, calling em.clear()
would suddenly unset this flag, but that entity was initialized by the time you added it to the context, so why should clearing the context change it anyhow?
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 fork()
.
By the way, I found out that the changeSet
for the third test looks like the following:
{
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 persistNewEntity
:
{
firstName: "John",
lastName: "Doe",
email: "user@mail.com",
createdAt: "2023-08-15T17:47:17.277Z",
updatedAt: "2023-08-15T17:47:17.277Z",
}
In the persistNewEntity
version, we don't have any mention to the id
as being undefined
. I'll try to filter undefined values here to see what happens.
we could probably unset this when calling em.clear, but it feels a bit wrong
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 __originalEntityData
property - that is the state of the entity when it was loaded (or last flushed), the state that reflects what is in the database. If we see that, its considered as update.
Ohh, got it, yea, I believe this is in the right direction. So, for some reason, the wrapped.__originalEntityData
is returning true
for the third test. Do you have any guesses as to why this could be happening? I'll debug and investigate here meanwhile.
One unrelated note about the tests - this will always just give you the user
entity you already have, as it is loaded in the context and you query for it by the primary key. There is no query fired, and you get the exact same object reference as user
.
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 __originalEntityData
is present in the third test from the beginning. When does Mikro-orm populates that value? I noticed that this is true even before calling entityManager.persistAndFlush()
- that is, simply by instantiating a new User
class, it already comes with that value populated.
Hey again, Martin. I just found out something else. There's a way to make these tests pass:
checkDuplicateEntities
to true
jest
to execute tests instead of vitest
.These tests still fail when we have the following combinations:
jest
with checkDuplicateEntities
as false
vitest
with any combination of checkDuplicateEntities
and threads
.So, I believe this has something to do with how vitest works. Considering it uses swc
, could this be the source of the issue? Do you see any possible solution for that?
Ok, this is pretty tricky, but the issue seems to be that you are calling MikroORM.init
in the beforeEach
hook, and discovering the same entity class for each test, over and over. Haven't found the exact cause yet, I'd expect it will be something about rediscovering the same EntitySchema
instance multiple times, as it is you who create that instance, as opposed to decorators where it will be created internally, so every time you call the init
method.
Long time haven't seen anything so weird :]
Ohh, I see, thanks for the context, Martin! Is there a way to enforce this, or am I just using it wrong?
There are several things you can do, the common theme is that you need a new fork. Either first call em.clear()
, or do em.fork().findOne()
, or use disableIdentityMap: true
in the find options (which creates a temporary fork under the hood, so basically the same), or use refresh: true
there, which will ignore the identity map for this request.
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 MikroORM.init()
for each test - but somehow they still pass. In any case, it's indeed odd that this is being called three times. The MikroORMModule
is imported in the @Module
decorator, which supposedly should run only once.
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.
UnitOfWork.registerManaged()
is only called for the first two testsWrappedEntity
constructor is only called for the first two tests. Why is the third test not instantiating a WrappedEntity
in the persist
call?__originalEntityData
is present in the third test, and thus the ChangeSetComputer
considers it to be an update
operation.flush()
method. If I don't call the flush()
method in the second test, the third test also passes.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 __originalEntityData
, but none was run during the tests. As I said, even before calling the entityManager.persistAndFlush(user)
, the user
instance already has a reference to this __originalEntityData
, so I assume Mikro-orm is somehow keeping a state for new entities.
⚠️ Edit: I decided to rename each user data for each test, and I found out that during the third test this is the user.__helper.__originalEntityData
:
{
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 vitest
and jest
to see what was different in those two outputs, and I found out that even though running them with jest
would make them pass, Mikro-ORM behavior was incorrect. Look at this final output after running the three tests (using jest):
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 user.__helper.__originalEntityData
is populated.
Ok, this is pretty tricky, but the issue seems to be that you are calling MikroORM.init in the beforeEach hook, and discovering the same entity class for each test, over and over.
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 MikroORM.init()
will be executed three times as well. I need your further assistance here Martin, as I underestimated this issue's complexity haha
Edit: I've also confirmed that if we change the hooks so we only build MikroORM in the beforeAll
, both the tests and the expected behavior are correct. However, I still believe we should try and fix the underlying behavior - which could be responsible for other unexpected bugs that we don't know yet. 😅
It must be something with how the autoLoadEntities
work, I can see it all starts to work correctly when I use folder-based discovery instead.
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 VITEST
env var set, so we can use that easily.
-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 options
object now, but why did you have to do that?
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 forFeature
calls were adding more entries to the entities
array on each app init. Havent checked it more deeply, but I still think its about the multiple discovery of the entity schema instances.
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