Persisting collection after wrap(...).assign fails

[Dschoordsch]

Describe the bug
Ids assigned to a collection via wrap(...).assign are not persisted.

Stack trace

...

To Reproduce
Steps to reproduce the behavior:

@Entity()
export class Book2 {
  @ManyToMany({ entity: () => BookTag2, cascade: [], fixedOrderColumn: 'order' })
  tags = new Collection<BookTag2>(this);
}
...
const id1 = 1; //id of existing tag
const book = new Book2();
wrap(book).assign({
  tags: [id1]
})
orm.em.persistAndFlus(book);

Expected behavior
I expect that assigning ids to collections works just like assigning an id to a reference with wrap(...).assign and would be persisted normally.

Additional context
Problem also exists when assigning entities, see PR.

Versions

Dependency	Version
node	10.18
typescript	?
mikro-orm	3.6.4
your-driver	test is mysql but I observed it with postgres

[B4nan]

You disabled cascading, therefore the persist is not propagated to the collection. Working as expected.

edit: I see it was not you, but me in the tests :] but the point stays, the property has disabled cascading, and you need Cascade.PERSIST there for this to work.
edit2: this has nothing to do with assign helper, it would behave the same if you work with the collection directly (via Collection.add() etc).
edit3: this assumption was probably wrong, cascading should not be needed in this case.

[Dschoordsch]

Sorry, I missed that one, I just tried to reproduce the failure I've seen where I did not set cascade explicitly (which defaults to 'persist' according to docs). But I assign ids directly, not entities. Is this a supported use case?

Edit: What I have is:

@Entity()
export class Exhibit {

  @OneToMany(
    () => Location,
    (location) => location.exhibit
  )
  public locations = new Collection<Location>(this);

...
wrap(exhibit).assign({
  locations: exhibitUpdate.locationIds,
});

[B4nan]

The test is also assigning ids directly. Also the PR does not reproduce anything, the changes before your assert are not flushed, therefore the state stays (there are all 3 tags).

[B4nan]

Try to create reproduction in a separate file, like here: https://github.com/mikro-orm/mikro-orm/blob/master/tests/issues/GH463.test.ts

Define the entities there, do not reuse the existing ones so I can clearly see what you are doing and it won't be affected with how the existing entities are defined.

[Dschoordsch]

Thanks, will do that.

[B4nan]

Hmm actually it does not work even with the added em.flush(), I think I know where the problem is...

[B4nan]

Ok got it, it is indeed a problem with assign() and the new snapshots diffing of collection. Will send fix in a minute and release it later this evening.

[Dschoordsch]

You're too fast, I just finished fixing the test. Thanks.

[Dschoordsch]

I still have this issue, see #469

[B4nan]

Your test would pass if it was not having that typo. You load b2 but assert on b :]

[B4nan]

Hmm ok looks like you actually found a bit different problem.

Your test was missing em.clear(), without that, you are not really testing what you think - you have the context filled, both entities are there when you do the last findOneOrFail call. So it was giving me false positive feedback when I fixed your typo...

[B4nan]

Ok so the problem is that you are working with the inverse side (the one without FK). ORM by default propagates changes to the owning side so things like this can work, but assign does not do that. If you do this instead of the assign, it will work:

const b = new B();
b.id = 'b';
b.as.add(a);

// the difference now is that `a.b` will be filled automatically for you, while with assign it was not
console.log(a.b === b); // true

[Dschoordsch]

In my use case I have an upsert with the list of all ids, so I would need to remove the missing ones and add the new ones. Definitely possible, but having this functionality in assign would be much more convenient. Also Collection.add wants to have the entity not just the id, doesn't it? This would require loading those first.

[B4nan]

In my use case I have an upsert with the list of all ids, so I would need to remove the missing ones and add the new ones.

Use Collection.set() then.

Also Collection.add wants to have the entity not just the id, doesn't it? This would require loading those first.

You can use em.getReference() to obtain the instance without loading anything.

[Dschoordsch]

Tried this, not as convenient (and confusing when to use what) and also still doesn't work for me.

[B4nan]

Hmm ok the problem is again a bit different, sorry for confusion. You can't work like this with 1:m collections, as they are the inverse side. You need to set the value to the owning side. It works automatically under the hood with add/set/remove methods (https://mikro-orm.io/docs/propagation/), but you need to have the owning side initialized. So it won't work with references (entities with only PK).

I will add validation for this (it is already there for M:N).

So to wrap up, you need query builder here, if you do not want to load the owning side entities. ORM always does changeset diffs only on owning sides, and if your owning side is not initialized, there is no changeset.

[Dschoordsch]

That's a bummer as this was one of the main reasons to switch from TypeORM, not needing to double check each modification from which side I have to do these and when I've got it wrong it just fails silently.
If I can use the model only for some updates and need to resort to querybuilders for other (simple ones as this) then there is no big advantage for me. The issue was also hidden at first because the context does not work for me either which caused the owning side to be still around making the TypeORM API appear really nice.

Would be nice if this would be optional and I could choose to load the owning side under the hood when using collection.set/add/remove or assign.

[B4nan]

That's a bummer as this was one of the main reasons to switch from TypeORM, not needing to double check each modification from which side I have to do these and when I've got it wrong it just fails silently.

As said, there is a validation missing. It is there for M:N already. Silent failures are definitely not a valid state.

Would be nice if this would be optional and I could choose to load the owning side under the hood when using collection.set/add/remove or assign.

Well, loading the owner is async operation, so don't expect that to happen automatically by calling synchronous API like Collection.add/set/remove or assign methods.

[Dschoordsch]

Well, loading the owner is async operation, so don't expect that to happen automatically by calling synchronous API like Collection.add/set/remove or assign methods.

I imagine it could just be a flag in the collection which would be checked when computing the changeset. Would also need to be checked when loading the primary side of a n:1 or m:n relation.

[B4nan]

I will need to think about this more, maybe we could fire the necessary update query if we see dirty collection on inverse side and we see that the owners are not loaded (I don't think we would have to load the owners this way). Currently the inverse side collection are never marked as dirty.

Just for you to understand, the problem is that the changeset that would trigger this is on the owning side (where the m:1 property is), so as long as it is not initialized there is no changeset for that entity, therefore no queries made.

[B4nan]

Will be fixed in v4 alpha 9.

[chasevida]

Hi there, I found this issue while trying to understand why updates within a collection where not being persisted. I think I've read the comments here correctly (and I think I understand them right) in which case my use case should work.

I have an entity that has a collection and when I update that entity with one of the collection members also containing updates, those updates should propagate down and all be persisted. This isn't working for me and it could totally be user error (I'm not an expert here).

I thought I'd create a super simple repo to reproduce the problem. Would appreciate any guidance but understand this is open source and your likely swamped so no pressure. Big thanks for the library.

Example repo - https://github.com/chasevida/mikro-orm-wrap

[B4nan]

mergeObjects has nothing to do with entities, that is for "scalar object props", e.g. json columns, not for relations. This issue is about persisting the collection (1:m, which was previously not supported unless all the items were loaded), not about updating its items (which sounds like what you are up to).

The assign helper does not work this way, it only handles the root entity and its properties. So it will handle updating what items are in a collection, but not how they should be updated. The code is quite simple, take a look yourself. I would not be opposed by allowing what you want (maybe under some flag, but probably enabled by default, like recursive: true).

https://github.com/mikro-orm/mikro-orm/blob/master/packages/core/src/entity/EntityAssigner.ts

[chasevida]

Thanks @B4nan for walking me through that, my confusion I think was because it worked in creating the entity that I then wrongly assumed I could update it the same way. Cheers for pointing me in the right direction, I'll go through the EntityAssigner and see if I can make a small tweak for the change with a flag like recursive: true as you suggested.

[lightning003]

@B4nan ... Continuing what @chasevida asked. How to achieve nested update for entity stored as collection. What other way I can achieve this? As I have a scenario of creating(this works) and updating (which pk are present - currently even passing object with primaryKey id is trying to insert it instead of update it) or deleting(which pk are now no longer present in req payload - orphanRemoval works) nested collections in one of the entity.
Scenario -
So if I have setup something like this. And I want to update b which already has some data, I am using const a = this.aRepository.findOne(id) and then wrap(a).assign(request.payload.a) but when I do persistAndFlush(a) - it updates the A's columns just fine but trying to insert the very same record in table B again instead of updating it and that results in duplicate value b_pkey error - it updates A's non-collection fields normally.
A {
@OneToMany({
entity: () => B,
mappedBy: (b) => b.a,
cascade: [Cascade.PERSIST, Cascade.REMOVE],
orphanRemoval: true,
})
b = new Collection<B>
}

[B4nan]

currently even passing object with primaryKey id is trying to insert it instead of update it

Yes, this ORM always worked like that - presence of a PK does not mean anything. Only way the ORM will issue update query (from flush operation) is when you have the entity already loaded and you mutate its state. Calling assign can never do that, it is sync method that mutates what you already have loaded.

assign helper will never work like this, because you want to issue update queries, while this ORM is change set based - it first needs to understand the state to be able to update it. Moreover this would never work for client side generated PKs (like UUID). For this reason I also don't see a way to implement the recursive flag at all - the method would need to be async or we would need to allow doing updates on references (not loaded entities) via UoW - which is something I was thinking about just recently, so just an early idea that will need a bit of experiments...

If you want such helper, write it yourself, it's not that hard :] See the EntityAssigner class, it is less than 150 loc (whitespace included).

[lightning003]

@B4nan ... I currently implemented the update by finding the collection entity separately using find and doing wrap().assign() on it. Then just doing persistAndFlush(a). Is it fine way to do it?

I also checked the EntityAssigner flow and I want to mention some observations. So I checked on EntityAssigner the collection entity are created by using em.create(updatedData, ...) and em.create end up returning the original entity (not loading data that is passed to it at all). As we are already loading/ trying to merge the entity and want to merge the data, cant this work like this (After this change, persistAndFlush(a) is working as expected, updating b entity properly).

if (exists && exists.__helper.__initialized && !options.refresh) {
     return Object.assign(exists, data);
}

instead of

if (exists && exists.__helper.__initialized && !options.refresh) {
     return exists;
}

[B4nan]

Nope, we can't change ORM internals because of how assign helper works. Definitely not such integral part like EntityFactory. The change needs to be in the EntityAssigner. Also in general using Object.assign won't work, as it will never handle things like references and identity look ups - instead the assign call should be propagated like if you would call it manually on the child property.

If you want to experiment with something you'd like to propose, it's best to clone the ORM and try what your changed do in the test suite. I can assure you this change would make a lot of tests fail.