migration:create changes non-nullable to nullable, changes it back and back and forth for enum and custom type properties with default value

[ddadaal]

Describe the bug
Using mysql driver, for enum properties with default value and custom type properties with default values, repeated migration:create runs creates new migration files which removes the properties' not null, adds not null, removes it, ands it back and back and forth.

To Reproduce
A repo with clearer description and steps to reproduce: https://github.com/ddadaal/mikro-orm-migration-changing-nonnullable-to-nullable

Expected behavior
No new migration files should be generated after the first migration:create, since there is no change at all in the entity.

Versions

Dependency	Version
node	v14.15.1
typescript	4.2.3
mikro-orm	4.4.4
mysql-driver	4.4.4.

[B4nan]

Afaik in your custom type, "DECIMAL(19, 4)" should be "DECIMAL(19,4)" (no space) - or maybe it should be numeric, not sure right now, but you need to use the actual type name as the DB stores it, one of those is just an alias.

Also, this has nothing to do with migrations, this is schema generator/diffing issue, migrations are just a layer on top of it.

Btw mapping decimal to numbers is not very good idea, you can loose precision. You should map to strings or use some wrapper. In fact your custom type will most probably return strings as it is written now.

[ddadaal]

Thanks for the advice, but actually in my production code, DECIMAL(19, 4) works fine,

and I am mapping it into a bignumber.js instance like so:

import { Type } from "@mikro-orm/core";
import { BigNumber as Decimal } from "bignumber.js";

const precision = 4;

const moneyColumnType = `DECIMAL(19, ${precision})`;

Decimal.set({ DECIMAL_PLACES: precision });

export { Decimal };

export class DecimalType extends Type<Decimal | undefined, string | undefined> {
  convertToDatabaseValue(value: Decimal | string | undefined): string | undefined {
    if (!value) { return value;}
    return value.toString();
  }

  convertToJSValue(value: string | undefined): Decimal | undefined {
    if (!value) { return undefined; }
    return new Decimal(value);
  }

  getColumnType(): string {
    return moneyColumnType;
  }
}

And it seems to work fine as well. How the type is mapped is not the point 😄️, but I can update the code if necessary.

So should I report the issue to umzug?

[B4nan]

Thanks for the advice, but actually in my production code, DECIMAL(19, 4) works fine,

Have you tried it? This is not about "it works for creating the column", this is about diffing the column type, and for that the type needs to be normalized to what the information schema stores. And there I believe it has to be without the space - someone already reported this very same issue and without the space it was working as expected.

Umzug has nothing to do with this, neither have the migrations - as I mentioned, this is about schema diffing inside SchemaGenerator.

[ddadaal]

I added some tests in the reproduce repo to test the mapping and updating of the bignumber.js instance and DECIMAL(19, 4) db column, and I think it to some extent proves that the mapping should work as expected.

And I am sorry I got confused back then and kept thinking it was about migration. I fully understand it's about SchemaGenerator now.

[ddadaal]

Removing the space and using defaultRaw fixes the issue about the custom type in my case. Thanks! I am sorry that I still misunderstood you and thought it was about mapping the value to DB.

Here are the code lines that I found related to enum issue:

Setting defaultRaw to AEnum.A can make migration generate correct result, but fails to create the schema when the schema is not already created, for in the create schema SQL the default value is not quoted resulting in a SQL syntax error.

mikro-orm/packages/knex/src/schema/SchemaHelper.ts

Lines 224 to 231 in 288eee4

	if (info.defaultValue && prop.defaultRaw) {
	const defaultValue = info.defaultValue.toString().replace(/\([?\d]+\)/, '').toLowerCase();
	const propDefault = prop.defaultRaw.toString().toLowerCase();
	const same = propDefault === info.defaultValue.toString().toLowerCase();
	const equal = same || propDefault === defaultValue;
	return equal || Object.keys(defaultValues).map(t => t.replace(/\([?\d]+\)/, '').toLowerCase()).includes(defaultValue);
	}

If set the default to AEnum.A, in the code, the diffing compares the defaultValue of db column with defaultRaw of property. The column prop from db has defaultValue of A, but the defaultRaw of property is 'A', which are not equal.

{
  name: 'a_enum',
  type: 'enum',
  ...
  defaultValue: 'A',
  ...
} {
  name: 'aEnum',
  ...
  defaultRaw: "'A'"
}

But I think the root cause should be inside this function, or knex which actually changes the column:

mikro-orm/packages/knex/src/schema/SchemaGenerator.ts

Lines 435 to 455 in 288eee4

	private configureColumn<T>(meta: EntityMetadata<T>, prop: EntityProperty<T>, col: ColumnBuilder, columnName: string, pkProp = prop, alter?: IsSame) {
	const nullable = (alter && this.platform.requiresNullableForAlteringColumn()) || prop.nullable!;
	const sameNullable = alter && 'sameNullable' in alter && alter.sameNullable;
	const indexed = 'index' in prop ? prop.index : (![ReferenceType.SCALAR, ReferenceType.EMBEDDED].includes(prop.reference) && this.helper.indexForeignKeys());
	const index = indexed && !alter?.sameIndex;
	const indexName = this.getIndexName(meta, prop, 'index', [columnName]);
	const uniqueName = this.getIndexName(meta, prop, 'unique', [columnName]);
	const sameDefault = alter && 'sameDefault' in alter ? alter.sameDefault : !prop.defaultRaw;
	const composite = prop.fieldNames.length > 1;
	Utils.runIfNotEmpty(() => col.nullable(), !sameNullable && nullable);
	Utils.runIfNotEmpty(() => col.notNullable(), !sameNullable && !nullable);
	Utils.runIfNotEmpty(() => col.primary(), prop.primary && !meta.compositePK);
	Utils.runIfNotEmpty(() => col.unsigned(), pkProp.unsigned);
	Utils.runIfNotEmpty(() => col.index(indexName), !composite && index);
	Utils.runIfNotEmpty(() => col.unique(uniqueName), !composite && prop.unique && (!prop.primary || meta.compositePK));
	Utils.runIfNotEmpty(() => col.defaultTo(prop.defaultRaw ? this.knex.raw(prop.defaultRaw) : null), !sameDefault);
	Utils.runIfNotEmpty(() => col.comment(prop.comment!), prop.comment);
	return col;
	}

For the enum property in the original code without the workaround, the result of diffing is only the default value is different, so only

mikro-orm/packages/knex/src/schema/SchemaGenerator.ts

Line 451 in 288eee4

Utils.runIfNotEmpty(() => col.defaultTo(prop.defaultRaw ? this.knex.raw(prop.defaultRaw) : null), !sameDefault);

is executed, which, however, removes non null (or not add non null back) in the final result somehow.