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(core): improve validation of wrong entity references #3085

Merged
merged 4 commits into from May 6, 2022

Conversation

hcharley
Copy link
Contributor

@hcharley hcharley commented May 4, 2022

I haven't created a bug ticket for this, but I did explain the problem in this Slack thread.

…ined" from obscuring the fromWrongReference MetadataError
@codecov-commenter
Copy link

codecov-commenter commented May 4, 2022

Codecov Report

Merging #3085 (bd9e026) into master (8da69d7) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #3085   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          193       193           
  Lines        11881     11881           
  Branches      2742      2742           
=========================================
  Hits         11881     11881           
Impacted Files Coverage Δ
packages/core/src/metadata/MetadataDiscovery.ts 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8da69d7...bd9e026. Read the comment docs.

Copy link
Member

@B4nan B4nan left a comment

Choose a reason for hiding this comment

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

let's simplify and more importantly, add a test case - this is coming from wrong decorator usage, right? so can be just a unit test in https://github.com/mikro-orm/mikro-orm/blob/master/tests/decorators.test.ts

packages/core/src/metadata/MetadataDiscovery.ts Outdated Show resolved Hide resolved
hcharley and others added 2 commits May 5, 2022 12:31
@hcharley
Copy link
Contributor Author

hcharley commented May 5, 2022

@B4nan I made your suggested change.

I'm going to hold off on writing a test for this until I can fix my own bug.

this is coming from wrong decorator usage, right?

I'm still running into the underlying error. So I do not know where this is coming from. Which is a part of the reason why I'm not in a position to write a test.

My best guess thus far is some production-only circular dependency issue. Idk 🤷

@hcharley
Copy link
Contributor Author

hcharley commented May 5, 2022

Dropping this in hopes this could possibly reproduce the issue later:

import { Field, ObjectType } from '@nestjs/graphql';

import {
  Collection,
  Entity,
  IdentifiedReference,
  ManyToOne,
  OneToMany,
} from '@mikro-orm/core';

import { MyBaseEntity } from '@my/entities';

@ObjectType()
@Entity()
export class OpsManagerService extends MyBaseEntity<OpsManagerService> {
  @OneToMany({
    entity: () => OpsManagerEnvironmentService,
    mappedBy: 'opsManagerService',
  })
  public opsManagerEnvironmentServices = new Collection<OpsManagerEnvironmentService>(
    this
  );
}

@ObjectType()
@Entity()
export class OpsManagerEnvironmentService extends MyBaseEntity<OpsManagerEnvironmentService> {
  @ManyToOne({
    entity: () => OpsManagerService,
    eager: true,
    wrappedReference: true,
    inversedBy: 'opsManagerEnvironmentServices',
  })
  @Field(() => OpsManagerService, { nullable: false })
  opsManagerService: IdentifiedReference<OpsManagerService>;
}

@hcharley
Copy link
Contributor Author

hcharley commented May 5, 2022

Hmmm.... seems that Mikro is logging some inconsistencies between two class references. I'm console logging out everything that comes into the validate bidirectional functions, and I'm seeing two different path types:

One entity's path is the bundle: main.js

The other's path is a webpack path: webpack:/ops-manager/libs/domain/src/lib/ops-manager-environment-service/entities/ops-manager-environment-service.entity.ts

@B4nan
Copy link
Member

B4nan commented May 6, 2022

I was asking for a unit test case, not sure what that (very incomplete) entity definition should reproduce... Also hearing about webpack, the path to entity is irrelevant in there.

Let's merge, the change is definitely not problematic, it can't break anything, so I am fine merging without tests.

@B4nan B4nan changed the title fix(MetadataDiscovery): prevent "Cannot read property 'name' of undefined" from obscuring the fromWrongReference MetadataError fix(core): improve validation of wrong entity references May 6, 2022
@B4nan B4nan merged commit f5de135 into mikro-orm:master May 6, 2022
@hcharley
Copy link
Contributor Author

hcharley commented May 6, 2022

@B4nan I understood your request, but I didn't have time yesterday to task switch, as I am still attempting to debug the underlying error that I ran into. I'm only seeing it in production, and so I was just dropping notes above on what I think might be causing it.

Glad it was okay enough to merge.

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