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

Connection Reuse is broken in a Lambda environment: #4804

Merged
merged 3 commits into from Oct 18, 2019
Merged

Connection Reuse is broken in a Lambda environment: #4804

merged 3 commits into from Oct 18, 2019

Conversation

phong-innomizetech
Copy link
Contributor

  • Fix issue find metadata does not exist with webpack

- Fix issue find metadata does not exist with webpack
@@ -480,7 +480,8 @@ export class Connection {
*/
protected findMetadata(target: Function|EntitySchema<any>|string): EntityMetadata|undefined {
return this.entityMetadatas.find(metadata => {
if (metadata.target === target)
// @ts-ignore
Copy link
Contributor

Choose a reason for hiding this comment

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

no

@@ -480,7 +480,8 @@ export class Connection {
*/
protected findMetadata(target: Function|EntitySchema<any>|string): EntityMetadata|undefined {
return this.entityMetadatas.find(metadata => {
if (metadata.target === target)
// @ts-ignore
if (metadata.target.name === target.name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

target can be a string

@@ -480,7 +480,8 @@ export class Connection {
*/
protected findMetadata(target: Function|EntitySchema<any>|string): EntityMetadata|undefined {
return this.entityMetadatas.find(metadata => {
if (metadata.target === target)
// @ts-ignore
if (metadata.target.name === target.name) {

This comment was marked as abuse.

craigcartmell and others added 2 commits October 2, 2019 12:38
@craigcartmell
Copy link

Pushed a couple of fixes to @phong-innomizetech 's original commit.

@craigcartmell
Copy link

Original monkeypatch is here which I used for reference: #3427 (comment)

@Benjamin-Dobell
Copy link
Contributor

Benjamin-Dobell commented Jan 30, 2020

@pleerock This PR seems to be responsible for a lot of open issues at the moment.

It doesn't fix Webpack support, but instead breaks it. This is trivially verifiable by looking at the line:

https://github.com/typeorm/typeorm/pull/4804/files#diff-9ada25e7bb500fdf98eeb214f7c94e9aR483

What this does is compare runtime names of functions, that's almost certainly going to fail in production, as production code is typically minified. Minified names are by no means guaranteed to be unique, so they shouldn't ever be used in a lookup.

In my case, I've got two classes that were both minified to e, so when I tried getRepository(SomeModel) it returns the wrong repository.

The previous functionality on the other hand was correct, in that it compared functions themselves, not their runtime names. The code for this comparison still exists, but is only executed if a match isn't found on the runtime name (which won't occur).

@Benjamin-Dobell
Copy link
Contributor

Actually, even in non-minified code, there's no guarantee class (function) names are unique. They probably shouldn't be involved in the lookup at all.

@Benjamin-Dobell
Copy link
Contributor

@pleerock My apologies, I see this has been discussed in #4967.

Benjamin-Dobell added a commit to Benjamin-Dobell/typeorm that referenced this pull request Jan 30, 2020
0xMillz added a commit to 0xMillz/typeorm that referenced this pull request Feb 6, 2020
pleerock pushed a commit that referenced this pull request Feb 13, 2020
imnotjames added a commit to imnotjames/typeorm that referenced this pull request Oct 2, 2020
in typeorm#4958 we found that the PR typeorm#4804 caused issues in a variety
of situations.  this adds a test to prevent the issue from recurring
where similar classes get mixed up in our metadata resolver
imnotjames added a commit that referenced this pull request Oct 7, 2020
in #4958 we found that the PR #4804 caused issues in a variety
of situations.  this adds a test to prevent the issue from recurring
where similar classes get mixed up in our metadata resolver
zaro pushed a commit to zaro/typeorm that referenced this pull request Jan 12, 2021
in typeorm#4958 we found that the PR typeorm#4804 caused issues in a variety
of situations.  this adds a test to prevent the issue from recurring
where similar classes get mixed up in our metadata resolver
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

6 participants