fix(core): Fix returning stale data in Role Update Event #3154
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Previously it returned
role
which was fetched before(!) applying the update. See definition ofrole
in line 253 and then the event bus publishing on line 281. I see thatpatchEntity
is mutating the existing object instead of cloning which should be fine for the fields, but then theupdatedAt
is wrong since its not being updated bypatchEntity
. Event subscribers that depend on theupdatedAt
timestamp are receiving a wrongupdatedAt
and or other auto-updated columns.Also removed the reference returned by
pathEntity
to make it visually clear that it actually mutates the object instead of returning a new one. This way its unambigious.Note: Have not look at many other services where this might also be the case.
Question (Edit: solved and reverted)
Edit: Not going through
findOne
fails therole.e2e-spec.ts
tests so I reverted it. Leaving it for history but the following can be ignored now.I did remove the manual fetch since TypeORM reloads by default. I couldnt come up with an explanation why we are disabling reloading but then refetch manually anyway? I found the commit which disabled the reload 5 years ago but it doesnt make sense to me. The commit says:
Which makes sense, for cases where the return is not used, but in this case we really do want the new value and are refetching it manually via
this.findOne(ctx, role.id)
. Am I missing something? Maybe something with relations?findOne
does add thechannels
relation in the query but is that relevant? 🤔Breaking changes
No
Checklist
📌 Always:
👍 Most of the time: