Nullable columns in typescript interface declaration

[information-security]

Problem
I can not set a nullable column to null using .update function after upgrading sequelize to 6.6.2.
Executing user.update( { email: null });, results in following error:

TS2322: Type 'null' is not assignable to type 'string | Literal | Fn | Col | undefined'. 

user.ts(11, 3): The expected type comes from property 'email' which is declared here on type
{
  id?: number | Literal | Fn | Col | undefined;
  email?: string | Literal | Fn | Col | undefined;
  password?: string | Literal | Fn | Col | undefined;
  ... 14 more ...;
  last_login_date?: Literal | ... 3 more ... | undefined;
}

Hence, I believe null type must be added into attributes declaration.

Example
Consider following schema as an example:

CREATE TABLE user {
  `email` varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL
}

Output
The generated interface would be like below:

export interface userAttributes {
 
  email?: string;
}

export class user extends Model<userAttributes, userCreationAttributes> implements userAttributes {

    email?: string;

    static initModel(sequelize: Sequelize.Sequelize): typeof user {

        user.init({
            email: {
  type: DataTypes.STRING(255),
  allowNull: true 
},
        });
        return user;
    }
}

Expected output

export interface userAttributes {
 
  email?: string|null;
}

export class user extends Model<userAttributes, userCreationAttributes> implements userAttributes {

    email?: string|null;

    static initModel(sequelize: Sequelize.Sequelize): typeof user {

        user.init({
            email: {
  type: DataTypes.STRING(255),
  allowNull: true 
},
        });
        return user;
    }
}

Disclaimer
I might be misunderstanding something here. So if there are any solutions to this problem please let me know. Additionally, PR#547 might be a workaround to this problem.

Environment
sequelize version: 6.6.2
sequelize-auto version: 0.8.4
dialect: mysql
node version: 16.6.1
npm version: 7.21.1
CMD: sequelize-auto -o "./src/models" -l ts -e mysql

[kurochin143]

I run into this problem too.

I made more changes to #547. Now by default, all nullable columns will use null instead of ? as it is more logical.

There's no need to use the typeOverrides option, but it is in the branch already when I added the null changes so I can't remove it.

[information-security]

@kurochin143 Thanks for your reply and all your efforts on preparing the PR.

I tested your PR and I can see that you have replaced the optional tag with | null type definition. This will incur additional challenges when having multiple nullable columns that we just want to modify a few of them and leave the rest intact.

Consider following scenario:

CREATE TABLE user {
  `email` varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL
  `address` varchar(255) CHARACTER SET utf8mb4 COLLATE utf8mb4_0900_ai_ci DEFAULT NULL
}

now assume that we have a record in DB as below:

email | address
'[email protected]' | 'Some address'

If I want to use .update function to only set the address to NULL and leave the email column intact I must provide all the fields on the .update function since none of them are optional now:
user.update( { address: null, email: '[email protected]' });

This will require an additional row fetch from the DB which would be redundant in most of the scenarios. What do you think if you just add | null to the nullable columns and let the column remain as optional?

[kurochin143]

The .update function uses something similar to Partial<userAttributes>, it turns all fields to optional.

You should still be able to pass user.update({ address: null });

[information-security]

My bad! You are absolutely correct about the parameters of the .update function.

However, I still believe this is a breaking change and should be avoided for this PR. For example, we are setting fields to undefined throughout our code in order to make sure that the returning object would not contain that specific data (e.g. password fields). On the other hand, we do not want to use separate types for that purpose. The user object would get filled in our DB layer and would be passed to upper layers having each layer doing its processing on the data. In the end, the very same object would be returned by our API endpoints. We can absolutely set the fields to null but here comes two major problems:

1. Our API would return those fields with a null value which is redundant. We do not want to remove all null values from the output since some of them are required.
2. We need to make a lot of modifications to our codebase.

We might also need to write additional interfaces/types to overcome (1) which is again a lot of unnecessary code that would be hard to maintain. Please correct me if I am wrong.

[kurochin143]

I assume that only a handful of fields are needed to be optional. You can override the fields that you want optional using typeOverrides that's also in the PR.

There are also ways to make some fields optional with typescript wrappers, which is what I use. Like https://stackoverflow.com/questions/43159887/make-a-single-property-optional-in-typescript.

With that wrapper, you can maintain strict typings of every function return type, rather than having optional fields that will never really be undefined for that particular function call.

I think that sequelize-auto generated class should reflect the type of the database rather than the upper layer. Though, sequelize can exclude any field. But with sequelize exclusion, every field would have been optional.

But if this is not good enough then I can add another option nullableFieldType with options "NULL", "OPTIONAL", "NULL_AND_OPTIONAL"

[kurochin143]

Ok, it is a breaking change so I added the nullableFieldType option in the typeOverrides option. This way, users have the option to revert back to the current output.

Instruction is in #547.

I still stand with my previous comment of using null and wrapper.

[information-security]

I think that sequelize-auto generated class should reflect the type of the database rather than the upper layer. Though, sequelize can exclude any field. But with sequelize exclusion, every field would have been optional.

I totally agree with you that the generated classes by sequelize-auto should reflect the actual types of the DB. Now that you have added the nullableFieldType option, we can have a smooth transition toward that.

Thanks for all your help.

@steveschmitt Could you please review/merge the #547 PR?