Circular Node Module Requiring Causes DataTypes to be Empty Object

[kyleguate]

What are you doing?

I'm trying to use sequelize with tables that contain columns with defaultValues, like:

column_title: {type: DataTypes.STRING(255), defaultValue: ''},

Expected Result

The data-types.js module required in sequelize/lib/utils.js returns a non-empty DataTypes object

Actual Result

The data-types.js module required in sequelize/lib/utils.js returns an empty DataTypes object which causes various errors to occur when running my app, including:

{ error: TypeError: Right-hand side of 'instanceof' is not an object
        at Object.defaultValueSchemable (/home/default/node_modules/sequelize/lib/utils.js:341:13)
        at Object.attributeToSQL (/home/default/node_modules/sequelize/lib/dialects/mysql/query-generator.js:334:120)
        at Object.attributesToSQL (/home/default/node_modules/sequelize/lib/dialects/mysql/query-generator.js:387:45)
        at QueryInterface.createTable (/home/default/node_modules/sequelize/lib/query-interface.js:276:40)
        at Promise.try.then.then (/home/default/node_modules/sequelize/lib/model.js:1127:39)
    From previous event:
        at Function.sync (/home/default/node_modules/sequelize/lib/model.js:1127:8)
        at Promise.each.model (/home/default/node_modules/sequelize/lib/sequelize.js:719:50)
    From previous event:
        at Promise.try.then.then (/home/default/node_modules/sequelize/lib/sequelize.js:719:22)
    From previous event:
        at Sequelize.sync (/home/default/node_modules/sequelize/lib/sequelize.js:706:8)

Root Cause

Node circular module dependency between sequelize/lib/utils.js and sequelize/lib/data-types.js
See screenshots below for details

Screenshots

Step 1) Notice Circular Node Module Dependencies - utils.js >> data-types.js >> utils.js

Step 2) Add Logging When Modules Are Required

Step 3) Add Logging When DataTypes is Exported

Step 4) Start App and Notice Empty Data Types Object

Step 5) Comment Out ./utils.js Module Requires to Eliminate Circular Dependency

Step 6) Restart App and Notice Data Types Is Exported Before utils.js Log Statement Prints

Step 7) Notice After All DataTypes from data-types.js Log Statment Are Printed We See utils.js Log Statement with Legit DataTypes Object

Proposed Resolution

Refactor utils.js to move string formatting into it's own module. Then data-types would just require that module since that's all it was using utils.js for.

Update
utils.js is only referenced once in all of data-types.js file, on line #27

Utils.warn(`${text}, '\n>> Check:', ${link}`);

in module utils.js, warn is exported as

exports.warn = logger.warn.bind(logger);

It seems as though logger is instantiated and exported as a singleton here, probably for convenience. One option could be to move logging into it's own module. Another (probably less painful) option is to use something else to log for the one line in data-types.js module

Node Circular Dependency Reading

http://blog.cloudmineinc.com/managing-cyclic-dependencies-in-node.js?hsFormKey=ae1b2c3916d0a2be7aafb5d841ce24c0
http://selfcontained.us/2012/05/08/node-js-circular-dependencies/

Dialect: mysql
Database version: 5.6.34
Sequelize version: 4.36.1
Tested with latest release: Yes

[SimonSchick]

This seems odd, can you post the actual code you are writing? iirc you shouldn't import sequelize submodules (and that seems like what you are doing).

[kyleguate]

@SimonSchick
I don't think I am, but I will provide an example to verify.

However, if you look at these two files you can clearly see circular module requiring going on:
utils.js
https://github.com/sequelize/sequelize/blob/master/lib/utils.js - line 3

data-types.js
https://github.com/sequelize/sequelize/blob/master/lib/data-types.js - line 12

So utils requires data-types, then data-types requires utils, then utils requires data-types, etc.

Do you see what I mean?

[sushantdhiman]

Please provide me a SSCCE. It will help me understand your problem quickly.

( What is SSCCE ? You can find template for Sequelize here )

A few good example of SSCCE

1. ResourceRequest timed out - connection not reestablished after db restarted #8014 (comment)
2. removeAttribute('id') results in undefined: null data value #7318 (comment)
3. toJSON converting booleans to "t" (true) or "f" (false) #8749 (comment)

Node is able to handle circular dependencies by deferring object construction, and if it was a real issue we should not be able to build this project on CI.

Please include a code sample that really reproduce this issue because it seems some usual userland issue

[kyleguate]

@sushantdhiman

I will dive deeper into my own codebase to see if there is an issue on my end.

I tried learning more about how node handles circular dependencies and the only real documentation I could find was this:
https://nodejs.org/api/modules.html#modules_cycles

Do you have any other references/links I could check out? Specifically around deferred object construction you mentioned? I really want to learn more for my own project.

[sushantdhiman]

@kyleguate

https://nodejs.org/api/modules.html#modules_cycles explains what I meant, if you require a module a inside b, where b is requiring a, then you initially get empty / unfinished exports object, when both module finished loading correct exports object for both modules is realized.

You are trying to include Utils in your project directly?, may be inside a model that's being imported by import call? Utils is a private API

Generally you can avoid cycling dependencies by

1. require them in actual method, suppose some function xyz of your module need that dependency then you should directly require inside xyz
2. Rather than having first / second module require each other, abstract shared dependency into a third module. Then requiring that third module in other two.

[kyleguate]

I did a global search in my codebase for references of require('sequelize and found this:

const SqlString = require('sequelize/lib/sql-string');

I added that a while ago when we were still on v3 to escape a column that was a reserved keyword as described here:
#1132 (comment)

We have since upgraded to v4 and also changed the column name to no longer be a reserved keyword, so this line is obsolete and should have been removed.

Once I removed that line I no longer experienced this issue.

Thanks for the feedback @SimonSchick and @sushantdhiman, this issue can be closed.

[sushantdhiman]

If you want to escape/quote something you should use this

const sequelize = /** get your sequelize instance */
const qi = sequelize.getQueryInterface();

// qi.quoteIdentifier, for quoting strings
// qi.escape         , for escaping strings