Skip to content

fixing allowNull, onDelete, onChange, deferrable #13

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

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

obecny
Copy link
Contributor

@obecny obecny commented Aug 12, 2020

fixes and new things for postgresql

  1. fixing allowNull
  2. adding onDelete and onChange for foreign keys
  3. adding deferrable information to be used later in ts model for foreign keys.

For point 3 I had to comment out the last piece for deferrable in ModelBuilder.ts, can you please give me a hint how this should be handled - I suspect I need to build some ts property - although I'm not sure how

@spinlud
Copy link
Owner

spinlud commented Aug 13, 2020

@obecny Hi, thanks a lot for the contribution!
I am on vacation now, will look into that when I am back 🙂

@obecny
Copy link
Contributor Author

obecny commented Oct 14, 2020

@spinlud hey any update on that, thx

@spinlud
Copy link
Owner

spinlud commented Oct 17, 2020

Hi @obecny, sry for the long delay.
I am afraid I am not very familiar with deferred constraints in Postgre, thus I am not sure how much can I help here.

I can be wrong but probably you need to import these types from Sequelize?

import { ModelAttributeColumnOptions, ModelAttributeColumnReferencesOptions } from 'sequelize';

// [...]

const props: Partial<ModelAttributeColumnOptions> = {
                ...col.originName && col.name !== col.originName && { field: col.originName },
                ...col.primaryKey && { primaryKey: col.primaryKey },
                ...col.autoIncrement && { autoIncrement: col.autoIncrement },
                ...col.allowNull && { allowNull: col.allowNull },
                ...col.dataType && { type: col.dataType },
                ...col.comment && { comment: col.comment },
                ...col.defaultValue && { defaultValue: col.defaultValue },
                ...col.onUpdate && { onUpdate: col.onUpdate },
                ...col.onDelete && { onDelete: col.onDelete },
                // @TODO fix this by creating a typescript definition for model
                ...col.references && { references: col.references as ModelAttributeColumnReferencesOptions },
            };

// [...]

Some questions:

Can we extended the onUpdate and onDelete behaviours to all the dialects (probably needed a query update)?

Do you think will be possible to add tests for deferrable constraints in Postgre?

@@ -11,6 +11,7 @@
"clean": "rm -fr build",
"lint": "eslint --fix --ext .ts output/*.ts",
"build": "npm run clean && tsc",
"prepare": "npm run build",
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is redundant, it's the same as build

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is not
https://docs.npmjs.com/misc/scripts#description
This is needed so when you simply call npm install it will build the packages correctly, otherwise the build is not run. This is especially visible when you use the package using github path.

...col.onUpdate && { onUpdate: col.onUpdate },
...col.onDelete && { onDelete: col.onDelete },
// @TODO fix this by creating a typescript definition for model
// ...col.references && { references: col.references as ModelAttributeColumnReferencesOptions },
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably the following type must be imported?

import { ModelAttributeColumnOptions, ModelAttributeColumnReferencesOptions } from 'sequelize';

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not about importing types. This is about reading references directly from postgres tables instead of taking it from file definition - this was just a try

@@ -22,6 +22,20 @@ export interface ITableMetadata {
comment?: string;
}

export enum CONSTRAINT_TYPES {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these types common to all the Dialects?
If I assuming correctly these are constraint for foreign keys, so we should probably name it something like FOREIGN_KEY_CONSTRAINTS. Also for enum we should probably use more verbose naming, using constant case:

export enum FOREIGN_KEY_CONSTRAINTS {
    CASCADE = 'CASCADE',
    SET_DEFAULT = 'SET DEFAULT',
    SET_NULL = 'SET NULL',
    RESTRICT = 'RESTRICT',
    NO_ACTION = 'NO ACTION',
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about other dialects, this is for postgresql. The keys are what is coming from postgresql already and then how it should be mapped so you can't change it

a = 'NO ACTION',
}

export interface IColumnMetadataReference {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We probably don't need to create this interface, this type is already defined in sequelize:

/**
 * References options for the column's attributes
 */
export interface ModelAttributeColumnReferencesOptions {
  /**
   * If this column references another table, provide it here as a Model, or a string
   */
  model?: string | typeof Model;

  /**
   * The column of the foreign table that this column references
   */
  key?: string;

  /**
   * When to check for the foreign key constraing
   *
   * PostgreSQL only
   */
  deferrable?: Deferrable;
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants