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.

Sign up for GitHub

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

Deprecated models.init pattern for initializing models #22358

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

Deprecated models.init pattern for initializing models #22358

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
b31fd6c
Deprecated `models.init` pattern for initializing models
daniellockyer Mar 3, 2025
837e45f
abc
daniellockyer Mar 12, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions ghost/core/core/boot.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,12 +88,6 @@ async function initCore({ghostServer, config, frontend}) {
require('./shared/url-utils');
debug('End: Load urlUtils');

// Models are the heart of Ghost - this is a syncronous operation
debug('Begin: models');
const models = require('./server/models');
models.init();
debug('End: models');

// Settings are a core concept we use settings to store key-value pairs used in critical pathways as well as public data like the site title
debug('Begin: settings');
const settings = require('./server/services/settings/settings-service');
Expand Down
2 changes: 0 additions & 2 deletions ghost/core/core/cli/generate-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,6 @@ module.exports = class DataGeneratorCommand extends Command {
const models = require('../server/models');
const knex = require('../server/data/db/connection');

models.init();

context.models = models;
context.m = models;
context.knex = knex;
Expand Down
2 changes: 0 additions & 2 deletions ghost/core/core/cli/repl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ module.exports = class REPL extends Command {
const models = require('../server/models');
const knex = require('../server/data/db/connection');

models.init();

context.models = models;
context.m = models;
context.knex = knex;
Expand Down
3 changes: 0 additions & 3 deletions ghost/core/core/server/data/migrations/hooks/init/before.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,3 @@
const models = require('../../../../models');

module.exports = function before() {
models.init();
return Promise.resolve();
};
13 changes: 8 additions & 5 deletions ghost/core/core/server/models/base/listeners.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
const moment = require('moment-timezone');
const _ = require('lodash');
const models = require('../../models');
const logging = require('@tryghost/logging');
const errors = require('@tryghost/errors');
const {sequence} = require('@tryghost/promise');

const {Post: PostModel} = require('../post');
const {Settings: SettingsModel} = require('../settings');

// Listen to settings.timezone.edited and settings.notifications.edited to bind extra logic to settings, similar to the bridge and member service
const events = require('../../lib/common/events');

Expand Down Expand Up @@ -36,12 +38,13 @@ events.on('settings.timezone.edited', function (settingModel, options) {
* We lock the target row on fetch by using the `forUpdate` option.
* Read more in models/post.js - `onFetching`
*/
return models.Base.transaction(async function (transacting) {

return PostModel.transaction(async function (transacting) {
options.transacting = transacting;
options.forUpdate = true;

try {
const results = await models.Post.findAll(_.merge({filter: 'status:scheduled'}, options));
const results = await PostModel.findAll(_.merge({filter: 'status:scheduled'}, options));
if (!results.length) {
return;
}
Expand All @@ -67,7 +70,7 @@ events.on('settings.timezone.edited', function (settingModel, options) {
}

try {
await models.Post.edit(post.toJSON(), _.merge({id: post.id}, options));
await PostModel.edit(post.toJSON(), _.merge({id: post.id}, options));
} catch (err) {
logging.error(new errors.InternalServerError({
err
Expand Down Expand Up @@ -111,7 +114,7 @@ events.on('settings.notifications.edited', function (settingModel) {
return;
}

return models.Settings.edit({
return SettingsModel.edit({
key: 'notifications',
value: JSON.stringify(allNotifications)
}, options).catch(function (err) {
Expand Down
100 changes: 76 additions & 24 deletions ghost/core/core/server/models/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,35 +1,87 @@
/**
* Dependencies
*/

const _ = require('lodash');
const debug = require('@tryghost/debug')('models');
const glob = require('glob');
/* eslint-disable max-lines */

// enable event listeners
require('./base/listeners');

/**
* Expose all models
*/
exports = module.exports;
module.exports = {
// `base` file does not export a Base model
Base: require('./base'),

function init() {
const baseNow = Date.now();
exports.Base = require('./base');
debug(`${Date.now() - baseNow}ms - Base.js require`);

let modelsFiles = glob.sync('!(index).js', {cwd: __dirname});
modelsFiles.forEach((model) => {
const name = model.replace(/.js$/, '');
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to add this to a test? e.g. that all ghost/core/core/server/models/*.js are available/in the require cache?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good shout! I'll add that once I figure out this browser test failure 😆

Copy link
Member

Choose a reason for hiding this comment

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

The rabbit gave a pretty good starting point 🐇🧙‍♂️ #22358 (comment)

const modelNow = Date.now();
_.extend(exports, require('./' + name));
debug(`${Date.now() - modelNow}ms - ${model} require`);
});
}
...require('./action'),
...require('./author'),
...require('./api-key'),
...require('./benefit'),
...require('./collection-post'),
...require('./collection'),
...require('./comment-like'),
...require('./comment-report'),
...require('./comment'),
...require('./custom-theme-setting'),
...require('./donation-payment-event'),
...require('./email-batch'),
...require('./email-recipient-failure'),
...require('./email-recipient'),
...require('./email-spam-complaint-event'),
...require('./email'),
...require('./integration'),
...require('./invite'),
...require('./job'),
...require('./label'),
...require('./mail-event'),
...require('./member-cancel-event'),
...require('./member-click-event'),
...require('./member-created-event'),
...require('./member-email-change-event'),
...require('./member-feedback'),
...require('./member-login-event'),
...require('./member-newsletter'),
...require('./member-paid-subscription-event'),
...require('./member-payment-event'),
...require('./member-product-event'),
...require('./member-status-event'),
...require('./member-stripe-customer'),
...require('./member-subscribe-event'),
...require('./member'),
...require('./mention'),
...require('./milestone'),
...require('./mobiledoc-revision'),
...require('./newsletter'),
...require('./offer-redemption'),
...require('./offer'),
...require('./permission'),
...require('./post-revision'),
...require('./post'),
...require('./posts-meta'),
...require('./product'),
...require('./recommendation-click-event'),
...require('./recommendation-subscribe-event'),
...require('./recommendation'),
...require('./redirect'),
...require('./role'),
...require('./session'),
...require('./settings'),
...require('./single-use-token'),
...require('./snippet'),
...require('./stripe-customer-subscription'),
...require('./stripe-price'),
...require('./stripe-product'),
...require('./subscription-created-event'),
...require('./suppression'),
...require('./tag-public'),
...require('./tag'),
...require('./user'),
...require('./webhook')
};

Comment on lines +9 to +77
Copy link
Contributor

@coderabbitai coderabbitai bot Mar 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Add test for model availability as suggested in past review

Per the past review comment by vikaspotluri123, it would be valuable to add a test verifying that all models from ghost/core/core/server/models/*.js are properly exported from this module.

🏁 Script executed:

#!/bin/bash
# Let's analyze model files to ensure all are exported
echo "Checking model files vs exports..."

# Get all model files in the directory
model_files=$(ls ghost/core/core/server/models/*.js | grep -v "index.js" | sed 's/.*\///' | sed 's/\.js$//')

# Get all models explicitly required in index.js
exported_models=$(grep -o "require('./[^']*" ghost/core/core/server/models/index.js | sed "s/require('\.\///" | sort)

# Count and compare
file_count=$(echo "$model_files" | wc -l)
export_count=$(echo "$exported_models" | wc -l)

echo "Found $file_count model files and $export_count exports in index.js"

# Find any model files not explicitly exported
for model in $model_files; do
  if ! echo "$exported_models" | grep -q "$model"; then
    echo "WARNING: Model file $model.js exists but is not explicitly exported in index.js"
  fi
done

Length of output: 66573

Attention: Unexported Model Detected

The test script verifies that while all model files are accounted for, the file role-utils.js (present in ghost/core/core/server/models/) isn’t explicitly required in index.js. Please review this discrepancy and decide whether to:

  • Export it: If role-utils.js should be accessible via the module, add a corresponding ...require('./role-utils') in index.js.
  • Exclude it: If exporting role-utils.js is intentional, update the test to ignore this file.

This update aligns with the previous suggestion by vikaspotluri123 to add a test for model availability.

Copy link
Member

Choose a reason for hiding this comment

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

If we want this check to be run as part of the test suite, it would need to follow the existing unit tests (written in javascript, using BDD)

Copy link
Contributor

Choose a reason for hiding this comment

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

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look at an existing model test to understand the structure and BDD pattern
echo "==== Examining an existing model test ===="
cat ghost/core/test/unit/server/models/base/index.test.js

echo -e "\n\n==== Check if there's already a test for models/index.js ===="
find ghost/core/test/unit/server/models -name "index.test.js"

echo -e "\n\n==== Checking role-utils.js to understand its purpose ===="
head -n 20 ghost/core/core/server/models/role-utils.js

Length of output: 7182

@vikaspotluri123 I'll create a proper BDD-style unit test for checking that all models are exported correctly. Based on examining the existing test patterns in the codebase, here's what I would propose:

const should = require('should');
const fs = require('fs');
const path = require('path');
const models = require('../../../../core/server/models');

describe('Models: index', function () {
    it('exports all model files correctly', function () {
        // Get all JS files in the models directory excluding index.js
        const modelsDir = path.resolve(__dirname, '../../../../core/server/models');
        const modelFiles = fs.readdirSync(modelsDir)
            .filter(file => file.endsWith('.js') && file !== 'index.js');
        
        // Files that aren't models or are handled specially
        const excludedFiles = ['role-utils.js']; // Utility file, not a model
        const specialCases = {
            'base.js': 'Base' // base.js is exported as Base, not via spread operator
        };
        
        // Check each model file
        modelFiles.forEach((file) => {
            if (excludedFiles.includes(file)) {
                return; // Skip excluded files
            }
            
            const modelName = file.replace('.js', '');
            
            if (specialCases[file]) {
                // Handle special cases
                const exportName = specialCases[file];
                should.exist(models[exportName], `Special case ${file} should be exported as models.${exportName}`);
            } else {
                // Normal model files
                // Check if requiring the file produces exports that exist in models
                const modelExports = require(`../../../../core/server/models/${modelName}`);
                
                Object.keys(modelExports).forEach((exportKey) => {
                    // Find this export in the models object
                    const hasMatch = Object.values(models).some(modelValue => 
                        modelValue === modelExports[exportKey]);
                        
                    should(hasMatch).be.true(`Model export ${exportKey} from ${file} should be included in models index`);
                });
            }
        });
    });
    
    it('has a deprecated init function', function() {
        should.exist(models.init);
        models.init.should.be.a.Function();
        
        // In non-production, it should log a warning
        const originalConsoleWarn = console.warn;
        const originalNodeEnv = process.env.NODE_ENV;
        
        let warningLogged = false;
        try {
            console.warn = function(message) {
                if (message.includes('models.init() is deprecated')) {
                    warningLogged = true;
                }
            };
            
            process.env.NODE_ENV = 'development';
            models.init();
            warningLogged.should.be.true('A deprecation warning should be logged in non-production');
        } finally {
            console.warn = originalConsoleWarn;
            process.env.NODE_ENV = originalNodeEnv;
        }
    });
});

This follows the existing BDD patterns in the codebase and properly handles special cases like role-utils.js, which appears to be a utility file rather than a model. The test verifies that all model exports are properly included in the main models object.

Copy link
Member Author

Choose a reason for hiding this comment

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

😆

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!

/**
* Expose `init`
* @deprecated: remove this once we've removed it from everywhere
*/

exports.init = init;
module.exports.init = function init() {
if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line no-console
console.warn('@deprecated: models.init() is deprecated. Models are now automatically required.');
}
};
2 changes: 1 addition & 1 deletion ghost/core/core/server/models/user.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ const errors = require('@tryghost/errors');
const security = require('@tryghost/security');
const {pipeline} = require('@tryghost/promise');
const validatePassword = require('../lib/validate-password');
const permissions = require('../services/permissions');
const urlUtils = require('../../shared/url-utils');
const activeStates = ['active', 'warn-1', 'warn-2', 'warn-3', 'warn-4'];
const ASSIGNABLE_ROLES = ['Administrator', 'Editor', 'Author', 'Contributor'];
Expand Down Expand Up @@ -907,6 +906,7 @@ User = ghostBookshelf.Model.extend({
// @NOTE: your role is not the same than the role you try to change (!)
// e.g. admin can assign admin role to a user, but not owner

const permissions = require('../services/permissions');
return permissions.canThis(context).assign.role(role)
.then(() => {
if (hasUserPermission && hasApiKeyPermission) {
Expand Down
4 changes: 0 additions & 4 deletions ghost/core/core/server/services/update-check/run-update-check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,6 @@ if (parentPort) {
(async () => {
const updateCheck = require('./');

// INIT required services
const models = require('../../models');
models.init();

const permissions = require('../permissions');
await permissions.init();

Expand Down
2 changes: 0 additions & 2 deletions ghost/core/core/server/web/api/testmode/jobs/graceful-job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@ const internalContext = {context: {internal: true}};
(async () => {
const models = require('../../../../models');

await models.init();

postParentPortMessage(`Fetching tags`);
const tags = await models.Tag.findPage(internalContext);

Expand Down
2 changes: 1 addition & 1 deletion ghost/core/core/shared/config/env/config.testing-browser.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
"port": 9417
},
"logging": {
"level": "fatal"
"level": "info"
},
"spam": {
"user_login": {
Expand Down
5 changes: 0 additions & 5 deletions ghost/core/test/integration/url_service.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,16 +2,11 @@ const should = require('should');
const sinon = require('sinon');
const testUtils = require('../utils');
const configUtils = require('../utils/configUtils');
const models = require('../../core/server/models');
const UrlService = require('../../core/server/services/url/UrlService');

describe('Integration: services/url/UrlService', function () {
let urlService;

before(function () {
models.init();
});

before(testUtils.teardownDb);
before(testUtils.setup('users:roles', 'posts'));
after(testUtils.teardownDb);
Expand Down
2 changes: 0 additions & 2 deletions ghost/core/test/regression/api/admin/schedules.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ describe('Schedules API', function () {
let request;

before(function () {
models.init();

// @NOTE: mock the post scheduler, otherwise it will auto publish the post
sinon.stub(SchedulingDefault.prototype, '_pingUrl').resolves();
});
Expand Down
1 change: 0 additions & 1 deletion ghost/core/test/regression/models/model_settings.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const models = require('../../../core/server/models');
const SETTINGS_LENGTH = 94;

describe('Settings Model', function () {
before(models.init);
afterEach(testUtils.teardownDb);

describe('defaults', function () {
Expand Down
7 changes: 0 additions & 7 deletions ghost/core/test/unit/api/cache-invalidation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,8 @@ const path = require('path');

const glob = require('glob');

const models = require('../../../core/server/models');

describe('API', function () {
describe('Cache Invalidation', function () {
before(async function () {
// Initialise models - Utilised by various endpoints to reference static fields (i.e models.Post.allowedFormats) when required in
models.init();
});

it('Controller actions explicitly declare cacheInvalidate header', async function () {
const controllersRootPath = path.join(__dirname, '../../../core/server/api/endpoints');
const controllerPaths = glob.sync('*.js', {
Expand Down
4 changes: 0 additions & 4 deletions ghost/core/test/unit/api/canary/session.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@ const sessionController = require('../../../../core/server/api/endpoints/session
const sessionServiceMiddleware = require('../../../../core/server/services/auth/session');

describe('Session controller', function () {
before(function () {
models.init();
});

afterEach(function () {
sinon.restore();
});
Expand Down
4 changes: 0 additions & 4 deletions ghost/core/test/unit/api/canary/utils/validators/input/pages.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@ const validators = require('../../../../../../../core/server/api/endpoints/utils
const models = require('../../../../../../../core/server/models');

describe('Unit: endpoints/utils/validators/input/pages', function () {
before(function () {
return models.init();
});

beforeEach(function () {
const memberFindPageStub = sinon.stub(models.Member, 'findPage').returns(Promise.reject());
memberFindPageStub.withArgs({filter: 'label:vip', limit: 1}).returns(Promise.resolve());
Expand Down
4 changes: 0 additions & 4 deletions ghost/core/test/unit/api/canary/utils/validators/input/posts.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,6 @@ const validators = require('../../../../../../../core/server/api/endpoints/utils
const models = require('../../../../../../../core/server/models');

describe('Unit: endpoints/utils/validators/input/posts', function () {
before(function () {
models.init();
});

beforeEach(function () {
const memberFindPageStub = sinon.stub(models.Member, 'findPage').returns(Promise.reject());
memberFindPageStub.withArgs({filter: 'label:vip', limit: 1}).returns(Promise.resolve());
Expand Down
5 changes: 0 additions & 5 deletions ghost/core/test/unit/frontend/helpers/authors.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,9 @@ const should = require('should');
const sinon = require('sinon');
const urlService = require('../../../../core/server/services/url');
const authorsHelper = require('../../../../core/frontend/helpers/authors');
const models = require('../../../../core/server/models');
const testUtils = require('../../../utils');

describe('{{authors}} helper', function () {
before(function () {
models.init();
});

beforeEach(function () {
sinon.stub(urlService, 'getUrlByResourceId');
});
Expand Down
5 changes: 0 additions & 5 deletions ghost/core/test/unit/frontend/helpers/get.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ const loggingLib = require('@tryghost/logging');

// Stuff we are testing
const get = require('../../../../core/frontend/helpers/get');
const models = require('../../../../core/server/models');
const proxy = require('../../../../core/frontend/services/proxy');
const api = require('../../../../core/server/api').endpoints;

Expand All @@ -17,10 +16,6 @@ describe('{{#get}} helper', function () {
let locals = {};
let logging;

before(function () {
models.init();
});

beforeEach(function () {
fn = sinon.spy();
inverse = sinon.spy();
Expand Down
6 changes: 1 addition & 5 deletions ghost/core/test/unit/frontend/helpers/ghost_head.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ const _ = require('lodash');
const moment = require('moment');
const testUtils = require('../../../utils');
const configUtils = require('../../../utils/configUtils');
const models = require('../../../../core/server/models');
const imageLib = require('../../../../core/server/lib/image');
const routing = require('../../../../core/frontend/services/routing');
const urlService = require('../../../../core/server/services/url');
Expand Down Expand Up @@ -358,9 +357,6 @@ describe('{{ghost_head}} helper', function () {
};

before(function () {
// @TODO: remove when visibility is refactored out of models
models.init();

keyStub = sinon.stub().resolves('xyz');
const dataService = {
getFrontendKey: keyStub
Expand Down Expand Up @@ -1547,7 +1543,7 @@ describe('{{ghost_head}} helper', function () {
}));

rendered.should.match(/data-datasource="analytics_events_json_v1"/);
});
});
});
describe('respects values from excludes: ', function () {
it('when excludes is empty', async function () {
Expand Down
Loading
Loading