From d05a592b610a4ca8167852046acb5767fa538b01 Mon Sep 17 00:00:00 2001 From: Konrad Kollnig <5175206+kasnder@users.noreply.github.com> Date: Mon, 13 Jul 2026 15:47:27 +0200 Subject: [PATCH] Deduplicate analysis policy config and appid SQL predicate Extract the CURRENT_ANALYSIS_VERSION/STALE_ANALYSIS_DAYS/ PROCESSING_TIMEOUT_MINUTES env parsing into lib/analysisPolicy.js, and the appid ~ pattern / length SQL predicate into lib/appId.js, so they're each defined once instead of duplicated across models/Apps.js and the reporting scripts. Also document that apps.analysed is stamped on failed analyses too (it's really "last attempt time"), in a code comment and a migration comment. Pure refactor plus comments; no behavior change. Co-Authored-By: Claude Fable 5 --- lib/analysisPolicy.js | 15 +++++++++++++++ lib/appId.js | 12 +++++++++++- migrations/000_apps.sql | 2 ++ models/Apps.js | 32 +++++++++++++++++++------------- scripts/priority-report.js | 11 ++++++++--- scripts/queue-status.js | 10 +++++++--- 6 files changed, 62 insertions(+), 20 deletions(-) create mode 100644 lib/analysisPolicy.js diff --git a/lib/analysisPolicy.js b/lib/analysisPolicy.js new file mode 100644 index 0000000..fe8deb4 --- /dev/null +++ b/lib/analysisPolicy.js @@ -0,0 +1,15 @@ +// Analysis scheduling policy, driven by environment variables. Centralised +// here so models/Apps.js and the reporting scripts read identical defaults. +// +// Callers that load .env themselves (e.g. scripts using dotenv.config()) +// must require this module AFTER dotenv.config() has run, since the values +// below are parsed once at require time. +const CURRENT_ANALYSIS_VERSION = parseInt(process.env.CURRENT_ANALYSIS_VERSION || process.env.ANALYSIS_VERSION || '4', 10); +const STALE_ANALYSIS_DAYS = parseInt(process.env.STALE_ANALYSIS_DAYS || '180', 10); +const PROCESSING_TIMEOUT_MINUTES = parseInt(process.env.PROCESSING_TIMEOUT_MINUTES || '120', 10); + +module.exports = { + CURRENT_ANALYSIS_VERSION, + STALE_ANALYSIS_DAYS, + PROCESSING_TIMEOUT_MINUTES +}; diff --git a/lib/appId.js b/lib/appId.js index a6c6987..d7f7099 100644 --- a/lib/appId.js +++ b/lib/appId.js @@ -18,9 +18,19 @@ function isSameAppId(a, b) { && a.toLowerCase() === b.toLowerCase(); } +// SQL predicate that matches the same validity rules as isValidAppId(), +// for use in queries against the appid column. patternParamIndex and +// lengthParamIndex are the 1-based positional parameter indices to bind +// APP_ID_PATTERN_SOURCE and MAX_APP_ID_LENGTH to (e.g. appIdSqlPredicate(2, 3) +// produces a predicate using $2 and $3). +function appIdSqlPredicate(patternParamIndex, lengthParamIndex) { + return `appid ~ $${patternParamIndex} AND appid !~ '[.]$' AND length(appid) <= $${lengthParamIndex}`; +} + module.exports = { APP_ID_PATTERN_SOURCE, MAX_APP_ID_LENGTH, isSameAppId, - isValidAppId + isValidAppId, + appIdSqlPredicate }; diff --git a/migrations/000_apps.sql b/migrations/000_apps.sql index a486f4b..4a51ffb 100644 --- a/migrations/000_apps.sql +++ b/migrations/000_apps.sql @@ -9,6 +9,8 @@ CREATE TABLE IF NOT EXISTS apps ( details json, analysis json, analysisversion integer, + -- For failed analyses this is really "last attempt time": updateAnalysis + -- (models/Apps.js) stamps analysed = NOW() on failures too, not only on success. analysed timestamp without time zone, added timestamp without time zone NOT NULL DEFAULT NOW() ); diff --git a/models/Apps.js b/models/Apps.js index f5f4fff..9c88fa1 100644 --- a/models/Apps.js +++ b/models/Apps.js @@ -2,8 +2,16 @@ const { Pool } = require('pg'); const { APP_ID_PATTERN_SOURCE, MAX_APP_ID_LENGTH, - isValidAppId + isValidAppId, + appIdSqlPredicate } = require('../lib/appId'); +// index.js runs dotenv.config() before requiring this module (via server.js), +// so process.env is already populated by the time analysisPolicy is read. +const { + CURRENT_ANALYSIS_VERSION, + STALE_ANALYSIS_DAYS, + PROCESSING_TIMEOUT_MINUTES +} = require('../lib/analysisPolicy'); const pool = new Pool( process.env.DATABASE_URL ? { connectionString: process.env.DATABASE_URL } @@ -59,9 +67,7 @@ const countQueue = async (added) => { FROM apps WHERE status = 'queued' AND added < $1 - AND appid ~ $2 - AND appid !~ '[.]$' - AND length(appid) <= $3 + AND ${appIdSqlPredicate(2, 3)} `, [added, APP_ID_PATTERN_SOURCE, MAX_APP_ID_LENGTH]); return result.rows[0].count; } else { @@ -69,9 +75,7 @@ const countQueue = async (added) => { SELECT COUNT(*) FROM apps WHERE status = 'queued' - AND appid ~ $1 - AND appid !~ '[.]$' - AND length(appid) <= $2 + AND ${appIdSqlPredicate(1, 2)} `, [APP_ID_PATTERN_SOURCE, MAX_APP_ID_LENGTH]); return result.rows[0].count; } @@ -93,9 +97,9 @@ const popularityExpression = ` ELSE 0 END`; -const currentAnalysisVersion = parseInt(process.env.CURRENT_ANALYSIS_VERSION || process.env.ANALYSIS_VERSION || '4', 10); -const staleAnalysisDays = parseInt(process.env.STALE_ANALYSIS_DAYS || '180', 10); -const processingTimeoutMinutes = parseInt(process.env.PROCESSING_TIMEOUT_MINUTES || '120', 10); +const currentAnalysisVersion = CURRENT_ANALYSIS_VERSION; +const staleAnalysisDays = STALE_ANALYSIS_DAYS; +const processingTimeoutMinutes = PROCESSING_TIMEOUT_MINUTES; const nextApp = async () => { const client = await pool.connect(); @@ -111,9 +115,7 @@ const nextApp = async () => { const candidate = await client.query(` SELECT appid FROM apps - WHERE appid ~ $4 - AND appid !~ '[.]$' - AND length(appid) <= $5 + WHERE ${appIdSqlPredicate(4, 5)} AND ( status = 'queued' OR ( @@ -185,6 +187,10 @@ const updateAnalysis = async (appId, analysis, analysisVersion) => { // display and existing HTTP responses depend on it) AND set the new // scheduling columns. processing_started is cleared now that the lock // is resolved. + // + // Note: analysed is stamped with NOW() even when status ends up + // 'failed', so for failed apps this column really means "time of the + // last analysis attempt", not "time of the last successful analysis". const result = await client.query( `UPDATE apps SET analysis = $1, diff --git a/scripts/priority-report.js b/scripts/priority-report.js index b0257d0..5e5ae57 100644 --- a/scripts/priority-report.js +++ b/scripts/priority-report.js @@ -7,11 +7,16 @@ const { Client } = require('pg'); dotenv.config({ path: path.join(__dirname, '..', '.env') }); dotenv.config({ path: path.join(__dirname, '..', 'analyser', '.env') }); +// Required after dotenv.config() above, since analysisPolicy reads +// process.env at require time. +const { + CURRENT_ANALYSIS_VERSION: currentAnalysisVersion, + STALE_ANALYSIS_DAYS: staleAnalysisDays, + PROCESSING_TIMEOUT_MINUTES: processingTimeoutMinutes +} = require('../lib/analysisPolicy'); + const limitArg = process.argv.find((arg) => arg.startsWith('--limit=')); const limit = limitArg ? Math.max(1, parseInt(limitArg.split('=')[1], 10) || 20) : 20; -const currentAnalysisVersion = parseInt(process.env.CURRENT_ANALYSIS_VERSION || process.env.ANALYSIS_VERSION || '4', 10); -const staleAnalysisDays = parseInt(process.env.STALE_ANALYSIS_DAYS || '180', 10); -const processingTimeoutMinutes = parseInt(process.env.PROCESSING_TIMEOUT_MINUTES || '120', 10); const reviewsExpr = ` CASE diff --git a/scripts/queue-status.js b/scripts/queue-status.js index 64f26ae..58a7ab3 100644 --- a/scripts/queue-status.js +++ b/scripts/queue-status.js @@ -7,9 +7,13 @@ const { Client } = require('pg'); dotenv.config({ path: path.join(__dirname, '..', '.env') }); dotenv.config({ path: path.join(__dirname, '..', 'analyser', '.env') }); -const currentAnalysisVersion = parseInt(process.env.CURRENT_ANALYSIS_VERSION || process.env.ANALYSIS_VERSION || '4', 10); -const staleAnalysisDays = parseInt(process.env.STALE_ANALYSIS_DAYS || '180', 10); -const processingTimeoutMinutes = parseInt(process.env.PROCESSING_TIMEOUT_MINUTES || '120', 10); +// Required after dotenv.config() above, since analysisPolicy reads +// process.env at require time. +const { + CURRENT_ANALYSIS_VERSION: currentAnalysisVersion, + STALE_ANALYSIS_DAYS: staleAnalysisDays, + PROCESSING_TIMEOUT_MINUTES: processingTimeoutMinutes +} = require('../lib/analysisPolicy'); const reviewsExpr = ` CASE