fix(core): allow assigning null to embeddable property

[francisco-sanchez-molina]

When you have entity like this:

@Entity()
class Document extends BaseEntity<Document> {
...
  @Embedded({
    entity: () => Summary,
    nullable: true,
  })
  summary!: Summary;
...
}

and do this:

document.assign({summary: null}, { mergeObjects: true });

it explode with:

Cannot convert undefined or null to object
    at Function.keys (<anonymous>)
    at Function.assignEmbeddable (***/node_modules/@mikro-orm/core/entity/EntityAssigner.js:108:16)

thank you very much for your great work!

[B4nan]

Thanks for the fix! Please add a testcase too, otherwise we might fall into same issue during future refactorings easily.

[B4nan]

i'd rather keep this as simple else branch (as noted above, if we return early, there is no need for new branching at all), or add a validation if the value is not object. doing it like this, it would silently ignore badly formed data - we should rather fail in such case.

also this will definitely need some test cases to keep the 100% branch coverage

[francisco-sanchez-molina]

@B4nan thank you very much for your quick response!
I'll tell you the reasons that made me do it this way:

• In the first if I understand that null allows to delete from the database, so I assign null but only if the property is nullable, to prevent deleting something if it is not configured for it.
• In the second if, the object type check allows to discard two cases, on the one hand null which is not nullable and on the other hand undefined, which is an ignored value and respects the previous value.

These are the reasons, do you think it is preferable to make the changes you are talking about?

Thanks a lot!

[B4nan]

As I said, we should not ignore malformed input data, we should throw instead. e.g. we could throw when user tries to set null/undefined and the property is not nullable, but ignoring the request to set a property to null, just because it is not nullable, that sounds wrong to me.

[francisco-sanchez-molina]

You are absolutely right! I have updated it

[B4nan]

i'd rather not check theh prop.nullable here, also we should think about undefined too (it is possible to force usage of undefined instead of null globally)

[francisco-sanchez-molina]

done ;)

[B4nan]

Some tests are failing

Summary of all failing tests
FAIL tests/EntityAssigner.mysql.test.ts
  ● EntityAssignerMySql › assign() should update entity values [mysql]

    TypeError: Cannot read property 'nullable' of undefined

      28 |       let value = data[prop as keyof EntityData<T>];
      29 | 
    > 30 |       if (!props[prop].nullable && (value === undefined || value === null)) {
         |                        ^
      31 |         throw new Error(`You must pass a non-${value} value to the property ${prop} of entity ${entity.constructor.name}.`);
      32 |       }
      33 | 

      at packages/core/src/entity/EntityAssigner.ts:30:24
          at Array.forEach (<anonymous>)
      at Function.assign (packages/core/src/entity/EntityAssigner.ts:23:23)
      at WrappedEntity.assign (packages/core/src/entity/WrappedEntity.ts:61:27)
      at Object.<anonymous> (tests/EntityAssigner.mysql.test.ts:20:16)
          at runMicrotasks (<anonymous>)

FAIL tests/EntityAssigner.mongo.test.ts
  ● EntityAssignerMongo › #assign() should update entity values

    TypeError: Cannot read property 'nullable' of undefined

      28 |       let value = data[prop as keyof EntityData<T>];
      29 | 
    > 30 |       if (!props[prop].nullable && (value === undefined || value === null)) {
         |                        ^
      31 |         throw new Error(`You must pass a non-${value} value to the property ${prop} of entity ${entity.constructor.name}.`);
      32 |       }
      33 | 

      at packages/core/src/entity/EntityAssigner.ts:30:24
          at Array.forEach (<anonymous>)
      at Function.assign (packages/core/src/entity/EntityAssigner.ts:23:23)
      at Book.assign (packages/core/src/entity/BaseEntity.ts:36:27)
      at Object.<anonymous> (tests/EntityAssigner.mongo.test.ts:20:10)
          at runMicrotasks (<anonymous>)