Skip to content
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

refactor: only merge simple objects in ORMUtils.mergeDeep #7774

Merged

Conversation

imnotjames
Copy link
Contributor

@imnotjames imnotjames commented Jun 21, 2021

Description of change

based on the work of #5097

fixes #5096
fixes #5762
fixes #6181
fixes #5487
fixes #5321

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • npm run lint passes with this change
  • npm run test passes with this change
  • This pull request links relevant issues as Fixes #0000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change
  • The new commits follow conventions explained in CONTRIBUTING.md

@francescoferraioli
Copy link
Contributor

@imnotjames what is the plan with this PR? When do you think it'll be ready for review?

@imnotjames
Copy link
Contributor Author

imnotjames commented Jun 28, 2021

@imnotjames what is the plan with this PR? When do you think it'll be ready for review?

Tests to confirm this doesn't break behaviors for mongo & other locations

I've been focused on some other areas of the codebase instead.

@imnotjames
Copy link
Contributor Author

so - use cases were the weird mongo edge case & usages of createValueMap.

I think all of those are hit now - I replaced the mongo edge case with a more specific cloneSubject helper within the subject executor & added a few tests.

@imnotjames imnotjames force-pushed the refactor/5096/limited-merge-deep branch from a6a48f7 to 1d79b15 Compare June 29, 2021 23:13
@imnotjames
Copy link
Contributor Author

Hmm.. failure looks to be related to this change. Bummer! Gotta figure that out.

@imnotjames imnotjames force-pushed the refactor/5096/limited-merge-deep branch from 1d79b15 to eb5a2b3 Compare July 3, 2021 04:00
@imnotjames imnotjames changed the title Refactor/5096/limited merge deep refactor: only merge simple objects in ORMUtils.mergeDeep Jul 3, 2021
@imnotjames imnotjames force-pushed the refactor/5096/limited-merge-deep branch 3 times, most recently from f32d9e3 to ec6c614 Compare July 5, 2021 02:31
The mergeDeep functionality was causing issues where it would
try to merge objects that were not safe to merge.  to correct this
we now only merge plain objects - such as those returned
by createValueMap.

there were three cases where the mongo driver had a split off
code path where it passed in entities, so we've updated to instead
update that to createValueMap as well
@imnotjames imnotjames force-pushed the refactor/5096/limited-merge-deep branch from ec6c614 to 7e36579 Compare July 5, 2021 05:24
add basic support of array embedded in ColumnMetadata so we can
use getEntityValueMap and getEntityValue with mongo
@imnotjames imnotjames force-pushed the refactor/5096/limited-merge-deep branch from 7e36579 to b9a5071 Compare July 5, 2021 05:29
@imnotjames imnotjames marked this pull request as ready for review July 5, 2021 05:30
@imnotjames imnotjames merged commit 08f9a1c into typeorm:master Jul 5, 2021
@imnotjames imnotjames deleted the refactor/5096/limited-merge-deep branch July 5, 2021 06:29
MerlinB pushed a commit to MerlinB/typeorm that referenced this pull request Jul 20, 2021
…7774)

The mergeDeep functionality was causing issues where it would
try to merge objects that were not safe to merge.  to correct this
we now only merge plain objects - such as those returned
by createValueMap.

there were three cases where the mongo driver had a split off
code path where it passed in entities, so we've updated to instead
update that to getEntityValueMap as well

adds basic support of array embedded in ColumnMetadata so we can
use getEntityValueMap with mongo

fixes typeorm#5096
fixes typeorm#5762
fixes typeorm#6181
fixes typeorm#5487
fixes typeorm#5321
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment