Nullable default value for Enum types

[falahati]

Description of change

This PR is the result of a conversation at #5371 regarding a bug with TypeOrm and MySQL resulting in the generation of an invalid table create query for enum fields with a default value of null
This happens simply due to treating null as another value for the enum and therefore, escaping and passing it as a string to the database.
The fix was easy, we are already checking for undefined before searching in enum and escaping the value in case there was no default value specified, now we are also checking for null in case the passed default value is special and should not be treated like others.
This is done by simply replacing value !== undefined with value != null therefore checking for both conditions.

Pull-Request Checklist

• Code is up-to-date with the master branch
• npm run lint passes with this change
• npm run test passes with this change
• This pull request links relevant issues as Fixes #0000
• There are new or updated unit tests validating the change
• Documentation has been updated to reflect this change
• The new commits follow conventions explained in CONTRIBUTING.md

This is a small change and so I did the changes on my browser without setting up the project for test, build, and lint. But honestly, I don't think it can break anything! Feel free to reject it if it doesn't fits the standards!

Thank you for the great library.

[pleerock]

!= doesn't feel reliable

[falahati]

you think defaultValue !== undefined && defaultValue !== null is more descriptive? cuz != is reliable, its just not as descriptive since we might have both these values here and each might mean a different thing in context.
I will change it if you think that it will help.

[nebkat]

Perhaps we could just place the null check before all the others? Then null is always NULL and the rest stays the same. Would apply the same fix for sets too.

Suggested change

	if ((columnMetadata.type === "enum" || columnMetadata.type === "simple-enum") && defaultValue != null) {
	if (defaultValue === null) {
	return `NULL`;
	} else if ((columnMetadata.type === "enum" || columnMetadata.type === "simple-enum") && defaultValue !== undefined) {

[pleerock]

what is the point to set default: null, if when you set nullable: true database already sets you a null by default?

[falahati]

what is the point to set default: null, if when you set nullable: true database already sets you a null by default?

true as I never saw one that doesn't automatically set the default to null when the column is nullable; however in many projects, I prefer to explicitly mention it in design and write it in code as it makes the design more obvious to the person implementing it.
Having a default value indicates to the programmer that you don't need to pass a value to this column to create a row.

Regardless, the problem here with typeorm is not the usefulness of the default: null value, rather, the discussion is about the inconsistency of typeorm behavior when a default: null value is used on an enum column.

[pleerock]

I prefer to explicitly mention it in design and write it in code as it makes the design more obvious to the person implementing it.

from other side, it might be confusing if you put both nullable: true and default: null. Other programmer might ask you "why did you use both if nullable already does null? What is the point? (Like I did) Can I remove one?"
Another problem is what will happen if you use nullable: false and default: null? Personally I don't know if database even allow such behaviour. If we make default: null to lead to no behaviour, it can also confuse because undefined served as a thing leading to no behaviour while null in TypeORM always does something else regarding to nulls.

Not sure what is the best approach here, maybe remove null support in defaults at all, but this will be a breaking change.

[falahati]

I prefer to explicitly mention it in design and write it in code as it makes the design more obvious to the person implementing it.

from other side, it might be confusing if you put both nullable: true and default: null. Other programmer might ask you "why did you use both if nullable already does null? What is the point? (Like I did) Can I remove one?"
Another problem is what will happen if you use nullable: false and default: null? Personally I don't know if database even allow such behaviour. If we make default: null to lead to no behaviour, it can also confuse because undefined served as a thing leading to no behaviour while null in TypeORM always does something else regarding to nulls.

Not sure what is the best approach here, maybe remove null support in defaults at all, but this will be a breaking change.

Well, I don't understand the other side's argument.
You see, I am not aware of such a standard that says every database needs to set the default value of a nullable field to null and if there is one, I don't know if all databases comply with that. Hence why I said, "Every database that I saw...". I assume that the majority of developers are also not sure about the standards of database design and therefore, nobody should find a default: nullable anything out of the ordinary. Not to mention that this type of descriptive design is not something that someone decides to remove since that decision should come with all sort of knowledge that I pointed out before.

So unless you do have this information, this confusion would never happen. But then if you have this knowledge, you can simply deduce that default: null is there just to be explicit. If anything, it helps with developers that are not aware of such behavior by the database engine.

My understanding is that the TypeOrm core should not limit the null value since it makes a lot of confusion without such knowledge that I mentioned.
However, drivers for each database engine are free to ignore it in the final create table query generation process. Or just does what it does now, pass it along; since again, I don't remember any database engine that I saw has any problem with an additional DEFAULT NULL statement in the field creation script. If they are ok with that, we should be too.

TypeOrm should just do what someone expects from an ORM to do; translate the queries, map objects, and provide an easy way to work with a database. Reducing or adding to the meaning of messages is not the property of a good translator.

[nebkat]

All other column types (except 'set' which has the same problem) accept default: null, so unless you want to explicitly prevent it from ever being set as a default value (default?: Exclude<any, null>?, which itself would be a much larger breaking change), I think this should be allowed.

from other side, it might be confusing if you put both nullable: true and default: null. Other programmer might ask you "why did you use both if nullable already does null? What is the point? (Like I did) Can I remove one?"

You could extend this logic to something like ColumnOptions primary and nullable. A programmer could ask why is {primary: true, nullable: false} defined, but the answer is surely quite obvious here. The extra information doesn't do any harm, and I don't think the library should ever implicitly prevent redundant information for the sake of enforcing conciseness.

Another problem is what will happen if you use nullable: false and default: null? Personally I don't know if database even allow such behaviour. If we make default: null to lead to no behaviour, it can also confuse because undefined served as a thing leading to no behaviour while null in TypeORM always does something else regarding to nulls.

It should do what { primary: true, nullable: true } does, which is throw a PrimaryColumnCannotBeNullableError, in this case a NonNullableColumnCannotDefaultNullError. Even without an error, the user has clearly contradicted themselves so it is their own fault if the database doesn't accept it.

default: null, wouldn't be leading to no behaviour, since it would explicitly set the column default to NULL. undefined would continue to do nothing (i.e. return undefined from normalizeDefault).

---

I would almost have said that default: null should automatically set nullable: true if it is undefined, but it is probably better not to do this in case of situations like:

function calculateDefault() {
    try {
        return doSomething();
    } catch (e) {
        return null;
    }

// ...

@Column({
    default: calculateDefault() // never actually meant for this to be null
})