fix(core): fix folder-based discovery for multiple entities in single file

[xmajzel]

Describe the bug

After upgrading from v5 to v6, initializing ORM doesn't work, throwing following stack trace:

TypeError: Cannot read properties of undefined (reading 'length')
    at MetadataDiscovery.defineBaseEntityProperties (.../node_modules/@mikro-orm/core/metadata/MetadataDiscovery.js:733:75)
    at .../node_modules/@mikro-orm/core/metadata/MetadataDiscovery.js:90:39
    at Array.forEach (<anonymous>)
    at MetadataDiscovery.processDiscoveredEntities (.../node_modules/@mikro-orm/core/metadata/MetadataDiscovery.js:90:18)
    at MetadataDiscovery.discover (.../node_modules/@mikro-orm/core/metadata/MetadataDiscovery.js:47:14)
    at async MikroORM.discoverEntities (.../node_modules/@mikro-orm/core/MikroORM.js:172:25)
    at async Function.init (.../node_modules/@mikro-orm/core/MikroORM.js:44:9)
    at async Function.handleMigrationCommand (.../node_modules/@mikro-orm/cli/commands/MigrationCommandFactory.js:83:21)

We went together with @Ruby184 through code and found out that that it was caused by this.metadata.set(name, Utils.copy(MetadataStorage.getMetadata(name, path), false)); in MetadataDiscovery.discoverDirectories function.

• This is problematic when there are multiple entities in one file.
• As a key in local this.metadata is const name = this.namingStrategy.getClassName(filename); used, which overwrites local metadata, when iterating multiple entities in file.
  • Overriding applies if the name derived from the file name is the same as the name of the class entity.

Resolution:

Global metadata initialisation (MetadataStorage.getMetadata(entity.meta.className, filepath)) is important if the user uses EntitySchema and TsMorphMetadataProvider for entities definition, so as a solution we moved this code to the MetadataDiscovery.getSchema function and refactored the MetadataDiscovery.prepare function to return EntitySchema even if it is located in EntitySchema.REGISTRY.

• See the code for details

However, we are not sure, that setting metadata locally is necessary

  private getSchema<T>(entity: Constructor<T> | EntitySchema<T>, filepath?: string): EntitySchema<T> {
    if (entity instanceof EntitySchema) {
      if (filepath) {
        const meta = Utils.copy(MetadataStorage.getMetadata(entity.meta.className, filepath), false);
        // Is following line neccessary?
        this.metadata.set(entity.meta.className, meta);
      }
      return entity;
    }
...

because they will be overwritten latter in the MetadataDiscovery.discoverDirectories function as follows:

...

const meta = schema.init().meta;
this.metadata.set(meta.className, meta);

...

Reproduction

• We added clear test for reproduction

What driver are you using?

None

MikroORM version

6.2.1

Node.js version

v20.11.1

Operating system

Mac OS Sonoma 14.4.1 (23E224)

Validations

• Read the Contributing Guidelines.
• Read the docs.
• Check that there isn't already an issue that reports the same bug to avoid creating a duplicate.
• Check that this is a concrete bug. For Q&A open a GitHub Discussion or join our Slack.
• The provided reproduction is a minimal reproducible example of the bug.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.74%. Comparing base (0fa6c0b) to head (86d71c5).
Report is 5 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5464      +/-   ##
==========================================
- Coverage   99.75%   99.74%   -0.01%     
==========================================
  Files         260      260              
  Lines       17904    17904              
  Branches     3796     3796              
==========================================
- Hits        17860    17859       -1     
- Misses         44       45       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[B4nan]

thanks!

[B4nan]

However, we are not sure, that setting metadata locally is necessary

Tests are passing without it, so I guess not anymore - before your patch, skipping this line broke a lot of stuff. So let's remove that, worst case we can revert.