assign(...) uses the updated primary key in the where clause rather than the original key

[rat-matheson]

Describe the bug
When I update the primary key of an entity, the generated query uses the updated key in the where clause rather than the original key

To Reproduce
Steps to reproduce the behavior:

1. Create an entity
2. Update the primary key
3. The generated query will try to match based on the updated primary key rather than the original key

(see example in additional context)

Expected behavior
The original key should be used

Additional context

let entity = this.em.findOneOrFail(UserEntity, { id: 'DW9q1j2U50qVIqxHU3N1q', deletedDate:0 });

this.em.assign(entity, {
    deletedDate:new Date(),
    deletedBy:'someUser'
});

return this.em.flush();

Generated query

update `user` set `aud_deleted_date` = 1654379985669, `aud_deleted_by` = 'RclzZWUCD435rE-5XU3mA' where `id` = 'DW9q1j2U50qVIqxHU3N1q' and `aud_deleted_date` = 1654379985669 

Expected query

update `user` set `aud_deleted_date` = 1654379985669, `aud_deleted_by` = 'RclzZWUCD435rE-5XU3mA' where `id` = 'DW9q1j2U50qVIqxHU3N1q' and `aud_deleted_date` = 0

I set up an event listener to see what is actually getting passed to the query generator

beforeUpdate

beforeUpdate ChangeSet {
  entity: UserEntity {
    id: 'DW9q1j2U50qVIqxHU3N1q',
    deletedDate: 2022-06-04T21:59:45.669Z,
    email: 'oOtkCdyLogga/7n5nnz8kYheYAeNR0MqST2vS+zlhvk=.1654379980993',
    fullName: null,
    username: 'dfdfdfd',
    isBlacklisted: false,
    deletedBy: 'RclzZWUCD435rE-5XU3mA'
  },
  type: 'update',
  payload: {
    deletedDate: 1654379985669,
    deletedBy: 'RclzZWUCD435rE-5XU3mA'
  },
  name: 'UserEntity',
  rootName: 'UserEntity',
  collection: 'user',
  schema: undefined,
  originalEntity: {
    id: 'DW9q1j2U50qVIqxHU3N1q',
    deletedDate: 0,
    email: 'oOtkCdyLogga/7n5nnz8kYheYAeNR0MqST2vS+zlhvk=.1654379980993',
    fullName: null,
    username: 'dfdfdfd',
    isBlacklisted: false,
    deletedBy: null
  },
  primaryKey: {
    id: 'DW9q1j2U50qVIqxHU3N1q',
    deletedDate: 2022-06-04T21:59:45.669Z
  }
} 

afterUpdate

afterUpdate ChangeSet {
  entity: UserEntity {
    id: 'DW9q1j2U50qVIqxHU3N1q',
    deletedDate: 2022-06-04T21:59:45.669Z,
    email: 'oOtkCdyLogga/7n5nnz8kYheYAeNR0MqST2vS+zlhvk=.1654379980993',
    fullName: null,
    username: 'dfdfdfd',
    isBlacklisted: false,
    deletedBy: 'RclzZWUCD435rE-5XU3mA'
  },
  type: 'update',
  payload: {
    deletedDate: 1654379985669,
    deletedBy: 'RclzZWUCD435rE-5XU3mA'
  },
  name: 'UserEntity',
  rootName: 'UserEntity',
  collection: 'user',
  schema: undefined,
  originalEntity: {
    id: 'DW9q1j2U50qVIqxHU3N1q',
    deletedDate: 0,
    email: 'oOtkCdyLogga/7n5nnz8kYheYAeNR0MqST2vS+zlhvk=.1654379980993',
    fullName: null,
    username: 'dfdfdfd',
    isBlacklisted: false,
    deletedBy: nul
  },
  primaryKey: {
    id: 'DW9q1j2U50qVIqxHU3N1q',
    deletedDate: 2022-06-04T21:59:45.669Z
  },
  persisted: true
}

Versions

Dependency	Version
node	?
typescript	?
mikro-orm	5.1.3
your-driver	@mikro-orm/sqlite@5.1.3

[B4nan]

I think that's a wontfix, you will need to use a QB for such things. We dont really support changing PKs, PK is what forms the identity key, if you would change it, things will break anyway. Changing PKs is an antipatter if you ask me.

edit: Btw its not about assign, if something, this is about the how we process updates in UoW. After giving it a second thought, I have no issues with fixing this by using the original PK, I think it makes sense in the end. Still I would expect weird issues after you do such change, but if you won't flush again, it shouldn't matter.

[rat-matheson]

As a rule of thumb, I agree. Don't change the primary key. But only as a rule of thumb. Once you start doing things like logical deletes, there are reasonable use cases for bending the rules.

I've described my use case here. It is an edge case that requires soft deletes, audit history, and re-insert/restore. This is a required feature for sensitive data where deletions can be reviewed and the original entity re-added. To achieve this, the deletedDate is part of the primary key since once it is set, the data is logically removed but the id can be re-added at a later date with a null (or 0) deletedDate.

I'm old enough to know that better ways can exist so perhaps you have some ideas? That said, I'm been at this for many years and seen this soft delete pattern used in FAANG for sensitive data that had these audit requirements. I don't think there is anything fundamentally wrong with thinking about soft deletes this way. Moving to an audit table might be better but has the cost of added complexity, especially if you have to add a transaction. Having a new id for each insert is a fine approach but fails for the re-insert/restore use case.

One other idea I had for using Mikro Orm was to make deletedDate a primary key in the DB but just a property in the mikro-orm layer and then use filters to ensure that undeleted data was never selected. This works pretty well until you get to em.assign. At that point, there is no way to specific that we only want to update the record that has deletedDate = 0. So if there is a way to universally apply a filter to assign, that would work too.

[B4nan]

Let's see how it works: 32ab215

[rat-matheson]

Read through the commit. Looks like it fixes the issue I had. Thanks so much. I was expecting that maybe you would have delegated the fix to me (which would have taken a long time for me to ramp up). I'm incredibly impressed at the turn around time.

I spent more time yesterday thinking about it but still can't think of a simpler way to handle the soft deletes with the extra conditions I mentioned.

[rat-matheson]

Thanks for this fix. Just tried out the new version. It's working well so far but I'll keep you posted if I encounter any issues