From ecdae702ad021307ca94eec3ec8cdde3123924bf Mon Sep 17 00:00:00 2001 From: funkecoder23 <12570656+FunkeCoder23@users.noreply.github.com> Date: Sun, 4 Feb 2024 23:23:57 -0500 Subject: [PATCH 1/2] Remove alter param from database sync Re: [sequelize docs](https://sequelize.org/docs/v6/core-concepts/model-basics/#synchronization-in-production) Alter is a bad idea in production. Also print error message instead of sequelize error object as it's a bit verbose --- src/node/consumer/src/lib/repository.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node/consumer/src/lib/repository.js b/src/node/consumer/src/lib/repository.js index 96a75a4..a71793f 100644 --- a/src/node/consumer/src/lib/repository.js +++ b/src/node/consumer/src/lib/repository.js @@ -1,7 +1,7 @@ import moment from 'moment'; import { Sequelize, Op, DataTypes, fn, col, literal } from 'sequelize'; import { databaseConfig } from './config.js'; -import {logger} from "./logger.js"; +import { logger } from "./logger.js"; import * as Promises from './promises.js'; const database = new Sequelize( @@ -185,9 +185,9 @@ Subtitle.belongsTo(File, { foreignKey: 'fileId', constraints: false }); export function connect() { if (databaseConfig.ENABLE_SYNC) { - return database.sync({ alter: true }) + return database.sync() .catch(error => { - console.error('Failed syncing database: ', error); + console.error('Failed syncing database: ', error.message); throw error; }); } From 287507d8e088892981fe77947878b7e8b1bb21a8 Mon Sep 17 00:00:00 2001 From: purple_emily Date: Mon, 5 Feb 2024 10:43:20 +0000 Subject: [PATCH 2/2] Add alter param back to database sync. Default to false. Make toggleable from .env file. --- deployment/docker/.env.example | 1 + src/node/consumer/src/lib/config.js | 2 +- src/node/consumer/src/lib/repository.js | 17 +++++++++-------- 3 files changed, 11 insertions(+), 9 deletions(-) diff --git a/deployment/docker/.env.example b/deployment/docker/.env.example index ee06905..32953d2 100644 --- a/deployment/docker/.env.example +++ b/deployment/docker/.env.example @@ -27,6 +27,7 @@ MAX_SINGLE_TORRENT_CONNECTIONS=10 TORRENT_TIMEOUT=30000 UDP_TRACKERS_ENABLED=true CONSUMER_REPLICAS=3 +AUTO_CREATE_AND_APPLY_MIGRATIONS=false # Fix for #66 - toggle on for development # Producer RabbitMqConfiguration__Host=rabbitmq diff --git a/src/node/consumer/src/lib/config.js b/src/node/consumer/src/lib/config.js index 9249956..c99e39a 100644 --- a/src/node/consumer/src/lib/config.js +++ b/src/node/consumer/src/lib/config.js @@ -24,7 +24,7 @@ export const databaseConfig = { POSTGRES_DATABASE: process.env.POSTGRES_DATABASE || 'knightcrawler', POSTGRES_USERNAME: process.env.POSTGRES_USERNAME || 'postgres', POSTGRES_PASSWORD: process.env.POSTGRES_PASSWORD || 'postgres', - ENABLE_SYNC: true + AUTO_CREATE_AND_APPLY_MIGRATIONS: parseBool(process.env.AUTO_CREATE_AND_APPLY_MIGRATIONS, false) } // Combine the environment variables into a connection string diff --git a/src/node/consumer/src/lib/repository.js b/src/node/consumer/src/lib/repository.js index a71793f..34c2b8c 100644 --- a/src/node/consumer/src/lib/repository.js +++ b/src/node/consumer/src/lib/repository.js @@ -184,14 +184,15 @@ File.hasMany(Subtitle, { foreignKey: 'fileId', constraints: false }); Subtitle.belongsTo(File, { foreignKey: 'fileId', constraints: false }); export function connect() { - if (databaseConfig.ENABLE_SYNC) { - return database.sync() - .catch(error => { - console.error('Failed syncing database: ', error.message); - throw error; - }); - } - return Promise.resolve(); + return database.sync({ alter: databaseConfig.AUTO_CREATE_AND_APPLY_MIGRATIONS }) + .catch(error => { + console.error('Failed syncing database: ', error.message); + throw error; + }); + // I'm not convinced this code is needed. If anyone can see where it leads, or what it does, please inform me. + // For now, I'm commenting it out. I don't think we ever reach this at the moment anyway as the previous ENABLE_SYNC + // was always on. + // return Promise.resolve(); } export function getProvider(provider) {