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

Property 'fieldName' option and ORM 'namingStrategy' do not affect Embeddable for MongoDB #4371

Closed
funduck opened this issue May 20, 2023 · 7 comments
Milestone

Comments

@funduck
Copy link
Contributor

funduck commented May 20, 2023

Describe the bug
Use MongoDB driver
Use @Property({fieldName: 'something'}) in embedded entity or choose UnderscoreNamingStrategy for ORM
In database embedded entity will have same naming as in code.

To Reproduce
Steps to reproduce the behavior:

import { Embeddable, Embedded, Entity, MikroORM, PrimaryKey, Property, UnderscoreNamingStrategy } from '@mikro-orm/core';
import { MongoDriver } from '@mikro-orm/mongodb';

@Embeddable()
class E {
  @Property()
  camelCase: string = 'c';

  @Property({fieldName: 'withALIAS'})
  withAlias: string = 'w';
}

@Entity()
class A {
  @PrimaryKey()
  _id = '1';

  @Property()
  complexName = 'n';

  @Embedded({entity: () => E, object: true})
  emBedded = new E()
}

let orm: MikroORM<MongoDriver>;

describe('GH issue 0', () => {
  beforeAll(async () => {
    orm = await MikroORM.init({
      entities: [A],
      clientUrl: 'mongodb://localhost:27017/mikro-orm-test',
      driver: MongoDriver,
      namingStrategy: UnderscoreNamingStrategy,
    });
    await orm.schema.clearDatabase();

    await orm.em.persistAndFlush(new A());
    orm.em.clear();
  });

  afterAll(async () => {
    await orm.close();
  });

  test('Ensure that embedded entity has underscore naming and fieldName is applied', async () => {
    const collection = orm.em.getDriver().getConnection().getDb().collection('a');
    expect(await collection.findOne({},{projection:{_id:0}})).toEqual({
      complex_name: 'n',
      em_bedded: {
        camel_case: 'c',
        withALIAS: 'w'
      }
    });
  });
});

Expected behavior
Renaming should propagate to the depth of embedded documents.

Versions

Dependency Version
node 18
typescript 5
mikro-orm 5.7
your-driver MongoDB
@funduck
Copy link
Contributor Author

funduck commented May 20, 2023

@B4nan I'm preparing PR but I'm not sure how object options should work.
In test tests/features/embeddables/nested-embeddables.mongo.test.ts I see that first @Embedded has object: true and it's effect propagates to nested embedded entities despite they dont have object: true option.
I expected that object option works on every level independently. It is natural: we traverse properties see Embedded and include it depending on it's object property, and then go deeper and transform nested embeddeds too.

In example , which behavior is expected?

@Entity()
export class User {
  @PrimaryKey()
  id!: number;

  @Property()
  name!: string;

  @Embedded(() => Profile, { object: true, nullable: true })
  profile?: Profile;
}

@Embeddable()
export class Profile {
  @Property()
  username: string;

  @Embedded(() => Identity)
  identity: Identity;

  constructor(username: string, identity: Identity) {
    this.username = username;
    this.identity = identity;
  }
}

@Embeddable()
export class Identity {
  @Property()
  email: string;

  constructor(email: string) {
    this.email = email;
  }
}

becomes (because Profile includes Identity in inline mode by default)

User {
  id: '',
  name: '',
  profile: {
    username: '',
    identity_email: '',
  }
}

or (because root include was in object mode and all nested includes should be in object mode too)

User {
  id: '',
  name: '',
  profile: {
    username: '',
    identity: {
      email: '',
    }
  }
}

@B4nan
Copy link
Member

B4nan commented May 21, 2023

or (because root include was in object mode and all nested includes should be in object mode too)

It should be transitive.

@B4nan
Copy link
Member

B4nan commented May 21, 2023

This is probably a duplicate of #2165.

@funduck
Copy link
Contributor Author

funduck commented May 21, 2023

Oh from #2165 it looks like the problem is also in sqlite driver, example in issue uses sqlite.
And this issue is exactly in mongo driver.
In the end, both drivers do not apply prop.fieldName in embedded entities.

I'm not sure, but may it is possible to move the responsibility of renaming from drivers to core? Now I see that driver has to check meta and correctly apply it on reads and writes. Is there a reason why core cant do renaming?

@B4nan
Copy link
Member

B4nan commented May 21, 2023

Nope, that issue is about mongo, the reproduction was added by me as the same problem is with SQL drivers, but the fix will need to be handled on both ends.

@B4nan
Copy link
Member

B4nan commented May 21, 2023

IIRC its done this way so its not handled twice - the query builder already handles the conversion and is used internally too. Maybe it would be best to unify this in v6.

FYI there is also #2361 which is kinda connected to all of this.

B4nan added a commit that referenced this issue Oct 22, 2023
B4nan added a commit that referenced this issue Oct 22, 2023
@B4nan B4nan added this to the 6.0 milestone Oct 23, 2023
B4nan added a commit that referenced this issue Oct 23, 2023
…ded properties

This is breaking mainly for SQL drivers, where the default naming strategy is underscoring, and will now applied to the embedded properties too. You can restore to the old behaviour by implementing custom naming strategy, overriding the `propertyToColumnName` method. It now has a second boolean parameter to indicate if the property is defined inside a JSON object context.

```ts
import { UnderscoreNamingStrategy } from '@mikro-orm/core';

class CustomNamingStrategy extends UnderscoreNamingStrategy {

   propertyToColumnName(propertyName: string, object?: boolean): string {
      if (object) {
         return propertyName;
      }

      return super.propertyToColumnName(propertyName, object);
   }

}
```

Closes #4371
Closes #2165
Closes #2361
B4nan added a commit that referenced this issue Oct 23, 2023
…ded properties (#4866)

This is breaking mainly for SQL drivers, where the default naming
strategy is underscoring, and will now applied to the embedded
properties too. You can restore to the old behaviour by implementing
custom naming strategy, overriding the `propertyToColumnName` method. It
now has a second boolean parameter to indicate if the property is
defined inside a JSON object context.

```ts
import { UnderscoreNamingStrategy } from '@mikro-orm/core';

class CustomNamingStrategy extends UnderscoreNamingStrategy {

   propertyToColumnName(propertyName: string, object?: boolean): string {
      if (object) {
         return propertyName;
      }

      return super.propertyToColumnName(propertyName, object);
   }

}
```

Closes #4371
Closes #2165
Closes #2361
@B4nan
Copy link
Member

B4nan commented Oct 23, 2023

Closing as fixed in v6 via #4866

@B4nan B4nan closed this as completed Oct 23, 2023
B4nan added a commit that referenced this issue Oct 23, 2023
…ded properties

This is breaking mainly for SQL drivers, where the default naming strategy is underscoring, and will now applied to the embedded properties too. You can restore to the old behaviour by implementing custom naming strategy, overriding the `propertyToColumnName` method. It now has a second boolean parameter to indicate if the property is defined inside a JSON object context.

```ts
import { UnderscoreNamingStrategy } from '@mikro-orm/core';

class CustomNamingStrategy extends UnderscoreNamingStrategy {

   propertyToColumnName(propertyName: string, object?: boolean): string {
      if (object) {
         return propertyName;
      }

      return super.propertyToColumnName(propertyName, object);
   }

}
```

Closes #4371
Closes #2165
Closes #2361
B4nan added a commit that referenced this issue Oct 25, 2023
…ded properties (#4866)

This is breaking mainly for SQL drivers, where the default naming
strategy is underscoring, and will now applied to the embedded
properties too. You can restore to the old behaviour by implementing
custom naming strategy, overriding the `propertyToColumnName` method. It
now has a second boolean parameter to indicate if the property is
defined inside a JSON object context.

```ts
import { UnderscoreNamingStrategy } from '@mikro-orm/core';

class CustomNamingStrategy extends UnderscoreNamingStrategy {

   propertyToColumnName(propertyName: string, object?: boolean): string {
      if (object) {
         return propertyName;
      }

      return super.propertyToColumnName(propertyName, object);
   }

}
```

Closes #4371
Closes #2165
Closes #2361
B4nan added a commit that referenced this issue Nov 2, 2023
…ded properties (#4866)

This is breaking mainly for SQL drivers, where the default naming
strategy is underscoring, and will now applied to the embedded
properties too. You can restore to the old behaviour by implementing
custom naming strategy, overriding the `propertyToColumnName` method. It
now has a second boolean parameter to indicate if the property is
defined inside a JSON object context.

```ts
import { UnderscoreNamingStrategy } from '@mikro-orm/core';

class CustomNamingStrategy extends UnderscoreNamingStrategy {

   propertyToColumnName(propertyName: string, object?: boolean): string {
      if (object) {
         return propertyName;
      }

      return super.propertyToColumnName(propertyName, object);
   }

}
```

Closes #4371
Closes #2165
Closes #2361
B4nan added a commit that referenced this issue Nov 5, 2023
…ded properties (#4866)

This is breaking mainly for SQL drivers, where the default naming
strategy is underscoring, and will now applied to the embedded
properties too. You can restore to the old behaviour by implementing
custom naming strategy, overriding the `propertyToColumnName` method. It
now has a second boolean parameter to indicate if the property is
defined inside a JSON object context.

```ts
import { UnderscoreNamingStrategy } from '@mikro-orm/core';

class CustomNamingStrategy extends UnderscoreNamingStrategy {

   propertyToColumnName(propertyName: string, object?: boolean): string {
      if (object) {
         return propertyName;
      }

      return super.propertyToColumnName(propertyName, object);
   }

}
```

Closes #4371
Closes #2165
Closes #2361
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

No branches or pull requests

2 participants