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

[RFC] Unification of createdAt, updatedAt, and deletedAt #14303

Open
ephys opened this issue Dec 23, 2021 · 4 comments
Open

[RFC] Unification of createdAt, updatedAt, and deletedAt #14303

ephys opened this issue Dec 23, 2021 · 4 comments
Assignees
Labels
RFC Request for comments regarding breaking or large changes

Comments

@ephys
Copy link
Member

ephys commented Dec 23, 2021

Configuring the timestamp fields is something I've seen users struggle with multiple times.
I believe this is due to their API being separate from the other attributes.

I propose to move their configuration to the attributes definition instead of options to simplify their API design.

This is already something I've seen users do, including myself, to control the column name.

API Design

Instead of renaming timestamp attributes through ModelOptions.timestamps, I propose to extend the ModelAttributes interface to accept a new TimestampAttributeColumnOptions interface on top of the existing two options.

This interface would accept a new autoTimestamp option, named after ModelOptions.timestamps, which would accept values createdAt, updatedAt, and deletedAt. Letting sequelize know that it should update these generated attributes accordingly.
As discussed in follow-up comments, only a small subset of options would be allowed when using autoTimestamp. The interface would look somewhat like this:

interface TimestampAttributeColumnOptions<M extends Model = Model> {
  autoTimestamp: 'createdAt' | 'deletedAt' | 'updatedAt',
  field?: string;
  comment?: string;
  get?(this: M): unknown;
  set?(this: M, val: unknown): void;

  // we could also optionally add `type?: typeof DataTypes.DATE`
  // if we end up implementing https://gist.github.com/ephys/50a6a05be7322c64645be716fea6186a#support-for-alternative-js-types
}

Using this solution, configuring timestamp attributes would be done in the same way as the other attributes:
Changing the attribute name would be done by renaming the key used in ModelAttributes. Changing the column name would be done by changing the field option.

Its usage would look like this:

UserModel.init({
  createdAt: {
    field: 'created_at',
    autoTimestamp: 'createdAt',
  },
})

I would deprecate and later drop ModelOptions.createdAt, ModelOptions.updatedAt, ModelOptions.deletedAt, as renaming the Attribute would be done through ModelAttributeColumnOptions
Whether each timestamp is enabled or not would be moved to ModelOptions.timestamps, which would have the following signature:

interface ModelOptions {
  // undefined or true: same behavior as v6 (createdAt/updatedAt enabled, deletedAt enabled if paranoid is true)
  // false: every timestamp disabled
  // or per-timestamp configuration
  timestamps?: boolean | {
    createdAt?: boolean,
    updatedAt?: boolean,
    deletedAt?: boolean,
  },
}

We would need to add some validation to TimestampAttributeColumnOptions.timestamp to ensure the user is not using one that's actually disabled.

Prior Art

They're decorators but it's basically the same idea.

sequelize-typescript enables the timestamp option if @CreatedAt / @UpdatedAt is used. And enables paranoid mode if @DeletedAt is used.

I would argue that it's safer, at least to beging with, to throw an error if these timestamp values are used but they are disabled in options or paranoid is not explicitely set to true.

Related issues

@ephys ephys added the RFC Request for comments regarding breaking or large changes label Dec 23, 2021
@ephys ephys self-assigned this Dec 23, 2021
@papb
Copy link
Member

papb commented Jan 18, 2022

Hello :) Taking a look on a few things here before "retiring"...

Configuring the timestamp fields is something I've seen users struggle with multiple times.

Me too 😅

I believe this is due to their API being separate from the other attributes.

This may be true, but I am not so sure. There could be other reasons, such as the confusing underscored option or even confusing documentation (although it was me who wrote most of the current manual content 😅)

To me, it's more natural that their configuration happens with a different API indeed, since those three timestamp fields are very special fields:

  • Their values are computed by Sequelize under the hood
  • Their values are used under the hood for some operations as well
  • They aren't supposed to be as customizable as standard fields (i.e. you shouldn't place a validator on them, shouldn't place a unique constraint, put boolean or something crazy as the datatype, etc.)

We could make type & allowNull optional given they must be specific values anyway

Optional, but would they be overridable?

Also, from a TypeScript point of view, it would be messy to type this correctly. We would have to detect the presence of the timestamp key on the model definition and apply different rules on it (such as disallowing unique:). This conditional type is doable but I think it would give an unfriendly error message to the developer.

However, from the prior art you linked, indeed they seem to manage to do it somehow. Please take this just as a heads-up to be careful, in order not to introduce new confusions while you fix old ones :)

@ephys
Copy link
Member Author

ephys commented Jan 18, 2022

Taking a look on a few things here before "retiring"...

Aw that's too bad, it's really nice seeing you again :) Still if you'll always be welcome whenever if you ever want to stop by, even if it's for tiny things or some insight :)

Thank you for taking the time to give your insight on this! I'll try to address your concerns as best I can then give some extra informations as to why I think it'd be a good change. I've also updated the initial RFC based on your feedback :)

There could be other reasons, such as the confusing underscored

You're right that this is also a pain point, I've added it to https://github.com/sequelize/meetings/issues/17 because I think it deserves some clarification too :)

To me, it's more natural that their configuration happens with a different API indeed

You gave me some pretty good points. Especially the fact that they're not supposed to be as configurable.
I still think it'd be good to move it to ModelAttributes but it should not share the same interface as ModelAttributeColumnOptions.

And as you raised, type & allowNull should not just be optional, they should be plainly forbidden.

I made a quick TypeScript test to see how it would hold up. I think the result is decent:

// Timestamp attributes only allow a subset of options
interface TimestampAttributeColumnOptions<M extends Model = Model> {
  autoTimestamp: 'createdAt' | 'deletedAt' | 'updatedAt',
  field?: string;
  comment?: string;
  get?(this: M): unknown;
  set?(this: M, val: unknown): void;

  // we could also optionally add `type?: typeof DataTypes.DATE`. See `Customizable JS types` below as to why
}

interface ModelAttributeColumnOptions<M extends Model = Model> extends ColumnOptions {
  autoTimestamp?: never;
  // ... other options unchanged
}

/**
 * Interface for Attributes provided for all columns in a model
 */
type ModelAttributes<M extends Model = Model, TAttributes = any> = {
  /**
   * The description of a database column
   */
  [name in keyof TAttributes]: DataType | ModelAttributeColumnOptions<M> | TimestampAttributeColumnOptions<M>;
}

We end up with this:

image
image

We should of course also have runtime validation for non-ts users, and proper documentation.


I also have extra reasons why I'd like to implement this API in Sequelize:

Customizable JS types

I'd like to offer an API that lets users choose how a DataType is represented in JavaScript (https://gist.github.com/ephys/50a6a05be7322c64645be716fea6186a#support-for-alternative-js-types). Users could request that Sequelize populates their DATE attribute with a string, a Temporal instance, or Date instance.

If we unify these 2 APIs, this new API can also be used in a unified manner:

User.init({
  id: {
    type: DataTypes.NUMBER,
    primaryKey: true,
  },
  createdAt: {
    type: DataTypes.DATE({ jsType: Temporal.Instant }), // we'd only allow DataTypes.DATE here
    autoTimestamp: 'createdAt',
    field: 'created_at',
  },
}, {});

Decorators (#14298)

In TypeScript, users are already going to declare their timestamp attributes in their model for type safety. When using a decorator, we can get the attribute name from the public class field name:

import { Model, HasManyAttribute, HasMany } from "sequelize";

class User extends Model {
  @Attribute(DataType.NUMBER, { primaryKey: true })
  id: number;

  @Attribute(DataType.DATE({ jsType: Temporal.Instant }), { autoTimestamp: 'createdAt' })
  // or @TimestampAttribute, or @CreatedAtAttribute
  myCreatedAt: Temporal.Instant; // Model option 'createdAt' will be set to 'myCreatedAt'
}

Now this is already doable with the current API (as https://github.com/RobinBuschmann/sequelize-typescript#createdat-updatedat-deletedat proves it), but I think it could be cleaner to have the decorator and non-decorator approaches mirror each-other.

Please take this just as a heads-up to be careful, in order not to introduce new confusions while you fix old ones :)

Absolutely, and thank you for taking the time to review this! :)

@papb
Copy link
Member

papb commented Jan 18, 2022

@ephys That's awesome!! I agree with everything, I like every single point you raised 😃 Excellent work, thank you very much! 🚀

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has been open for 14 days without activity. It will be closed if no further activity occurs within the next 14 days. If this is still an issue, just leave a comment or remove the "stale" label. 🙂

@github-actions github-actions bot added the stale label Apr 14, 2022
@ephys ephys removed the stale label Apr 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
RFC Request for comments regarding breaking or large changes
Projects
Status: No status
Development

No branches or pull requests

2 participants