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

Fix transform embeddeds with no columns but with nested embeddeds (mongodb) #4131

Merged
merged 2 commits into from
May 30, 2019
Merged

Fix transform embeddeds with no columns but with nested embeddeds (mongodb) #4131

merged 2 commits into from
May 30, 2019

Conversation

hoshinokanade
Copy link
Contributor

Issue type:

[ ] question
[x] bug report
[ ] feature request
[ ] documentation issue

Database system/driver:

[ ] cordova
[x] mongodb
[ ] mssql
[ ] mysql / mariadb
[ ] oracle
[ ] postgres
[ ] cockroachdb
[ ] sqlite
[ ] sqljs
[ ] react-native
[ ] expo

TypeORM version:

[x] latest
[ ] @next
[ ] 0.2.17 (or put your version here)

Steps to reproduce or a small repository showing the problem:

export class Post {
  @Column()
  title: string

  @Column(type => ExtraInfo)
  extraInfo: ExtraInfo
}
export class ExtraInfo {
  @Column(type => EditHistory)
  lastEdit: EditHistory
}
export class EditHistory {
  @Column()
  message: string
}
...
await repo.save(somePost);
const [post] = await repo.find();

// UnhandledPromiseRejectionWarning: TypeError: Cannot read property 'lastEdit' of undefined
at \src\query-builder\transformer\DocumentToEntityTransformer.ts
at addEmbeddedValuesRecursively

// extraInfo is not created!

After hours of trial without looking at the source code, I got a workaround.

export class ExtraInfo {
  @Column(type => EditHistory)
  lastEdit: EditHistory

  @Column()
   justNothing: number = 1
}
// clear the collection, insert another post with that additional field
await repo.save(anotherPost);
const [post] = await repo.find();
// works perfectly

This workaround does not make any sense it should not be an intentional design so it might be a bug indeed...

In addEmbeddedValuesRecursively, I discovered that the field may not be created if embedded.columns is empty array.
Then, when the foreach clause is skipped and it tries the enter into next recursion of addEmbeddedValuesRecursively the first argument may be undefined,
which cause the entity in that recursion to be undefined,
eventually causing unexpected error as we still got some columns deep deep inside.

Here I suggest to fix it by an extra check, so that it won't break any existing behavior.

…o transform an embedded column that has no columns but only with nested embedded columns

- provide a test case to this issue
- write changelog for this fix
@Kononnable Kononnable requested a review from rustamwin May 27, 2019 16:36
@@ -13,6 +13,10 @@ feel free to ask us and community.
* support sql.js v1.0 ([#4104](https://github.com/typeorm/typeorm/issues/4104))
* added support for `orUpdate` in SQLlite ([#4097](https://github.com/typeorm/typeorm/pull/4097))

### Bug fixes

* fixed transform embeddeds with no columns but with nested embeddeds (mongodb) ([#4131](https://github.com/typeorm/typeorm/pull/44131))
Copy link
Contributor

@rustamwin rustamwin May 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
* fixed transform embeddeds with no columns but with nested embeddeds (mongodb) ([#4131](https://github.com/typeorm/typeorm/pull/44131))
* fixed transform embeddeds with no columns but with nested embeddeds (mongodb) ([#4131](https://github.com/typeorm/typeorm/pull/4131))

Copy link
Contributor

@rustamwin rustamwin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

@pleerock pleerock merged commit 4307a59 into typeorm:master May 30, 2019
@pleerock
Copy link
Member

Thanks guys!

@hoshinokanade
Copy link
Contributor Author

Thanks @rustamwin for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants