From 0a3c65ce5d0873ec1b3e679ac343ae91e6c0e463 Mon Sep 17 00:00:00 2001 From: wyvern8 Date: Wed, 28 Aug 2024 19:01:06 +1000 Subject: [PATCH] fix(schema): fix schema validation and incorrect default rule code in setupEngine --- src/archetypes/node-fullstack.json | 11 ++-- src/core/engine/engineSetup.test.ts | 29 --------- src/core/engine/engineSetup.ts | 16 +---- src/facts/repoDependencyFacts.test.ts | 87 +++++++++++++++++++++------ src/facts/repoDependencyFacts.ts | 25 +++++--- src/facts/repoFilesystemFacts.test.ts | 20 +++--- src/facts/repoFilesystemFacts.ts | 10 +-- src/operators/outdatedFramework.ts | 2 +- src/server/configServer.ts | 2 +- src/types/typeDefs.ts | 3 +- src/utils/jsonSchemas.ts | 37 ++++++++++-- 11 files changed, 147 insertions(+), 95 deletions(-) diff --git a/src/archetypes/node-fullstack.json b/src/archetypes/node-fullstack.json index 1d64f49..51b2363 100644 --- a/src/archetypes/node-fullstack.json +++ b/src/archetypes/node-fullstack.json @@ -21,11 +21,12 @@ ], "config": { "minimumDependencyVersions": { - "@types/react": "^17.0.0", - "react": "^17.0.0", - "@yarnpkg/lockfile": "^1.2.0", - "commander": "^2.0.0", - "nodemon": "^3.9.0" + "@types/react": ">=18.0.0", + "react": "~17.0.0", + "@yarnpkg/lockfile": "<1.2.0", + "commander": ">=2.0.0 <13.0.0", + "nodemon": ">4.9.0", + "@colors/colors": "1.7.0 || 1.6.0" }, "standardStructure": { "app": { diff --git a/src/core/engine/engineSetup.test.ts b/src/core/engine/engineSetup.test.ts index 159d43c..b8a9cfa 100644 --- a/src/core/engine/engineSetup.test.ts +++ b/src/core/engine/engineSetup.test.ts @@ -83,35 +83,6 @@ describe('setupEngine', () => { expect(engine).toBeDefined(); }); - it('should handle errors when loading rules and add default rules', async () => { - const mockAddRule = jest.fn(); - (Engine as jest.Mock).mockImplementation(() => ({ - addOperator: jest.fn(), - addRule: mockAddRule, - addFact: jest.fn(), - on: jest.fn() - })); - - (ConfigManager.getConfig as jest.Mock).mockResolvedValue({ - archetype: mockArchetypeConfig, - rules: [ - undefined, - { name: undefined }, - { name: 'invalidRule', conditions: null } - ], - cliOptions: {} - }); - - await setupEngine(mockParams); - - expect(logger.error).toHaveBeenCalledWith('Invalid rule configuration: rule or rule name is undefined'); - expect(logger.error).toHaveBeenCalledWith('Invalid rule configuration: rule or rule name is undefined'); - expect(logger.error).toHaveBeenCalledWith('Error loading rule: invalidRule'); - expect(logger.info).toHaveBeenCalledWith('No valid rules were added. Adding default rules.'); - expect(mockAddRule).toHaveBeenCalledWith(expect.objectContaining({ name: 'default-rule-1' })); - expect(mockAddRule).toHaveBeenCalledWith(expect.objectContaining({ name: 'default-rule-2' })); - }); - it('should set up event listeners for success events', async () => { const mockOn = jest.fn(); (Engine as jest.Mock).mockImplementation(() => ({ diff --git a/src/core/engine/engineSetup.ts b/src/core/engine/engineSetup.ts index 475c664..e76f9d0 100644 --- a/src/core/engine/engineSetup.ts +++ b/src/core/engine/engineSetup.ts @@ -25,7 +25,7 @@ export async function setupEngine(params: SetupEngineParams): Promise { logger.info(`=== loading json rules..`); const config = await ConfigManager.getConfig({ archetype, logPrefix: executionLogPrefix }); - logger.debug(config.rules); + logger.debug(`rules loaded: ${config.rules}`); const addedRules = new Set(); config.rules.forEach((rule) => { @@ -51,20 +51,6 @@ export async function setupEngine(params: SetupEngineParams): Promise { } }); - if (addedRules.size === 0) { - logger.info('No valid rules were added. Adding default rules.'); - engine.addRule({ - name: 'default-rule-1', - conditions: { all: [] }, - event: { type: 'warning', params: { message: 'Default rule 1 triggered' } } - }); - engine.addRule({ - name: 'default-rule-2', - conditions: { all: [] }, - event: { type: 'warning', params: { message: 'Default rule 2 triggered' } } - }); - } - engine.on('success', async ({ type, params }: Event) => { if (type === 'warning') { logger.warn(`warning detected: ${JSON.stringify(params)}`); diff --git a/src/facts/repoDependencyFacts.test.ts b/src/facts/repoDependencyFacts.test.ts index 697bf3f..54cdcaf 100644 --- a/src/facts/repoDependencyFacts.test.ts +++ b/src/facts/repoDependencyFacts.test.ts @@ -3,6 +3,7 @@ import { execSync } from 'child_process'; import fs from 'fs'; import { Almanac } from 'json-rules-engine'; import { LocalDependencies, MinimumDepVersions } from '../types/typeDefs'; +import { semverValid } from './repoDependencyFacts'; jest.mock('child_process'); jest.mock('fs'); @@ -131,32 +132,82 @@ describe('repoDependencyFacts', () => { describe('semverValid', () => { it('should return true for valid version comparisons', () => { - expect(repoDependencyFacts.semverValid('2.0.0', '^1.0.0')).toBe(false); - expect(repoDependencyFacts.semverValid('1.5.0', '1.0.0 - 2.0.0')).toBe(true); - expect(repoDependencyFacts.semverValid('1.0.0', '1.0.0')).toBe(true); - expect(repoDependencyFacts.semverValid('2.0.0', '>=1.0.0')).toBe(true); + expect(semverValid('2.0.0', '^1.0.0')).toBe(false); + expect(semverValid('1.5.0', '1.0.0 - 2.0.0')).toBe(true); + expect(semverValid('1.0.0', '1.0.0')).toBe(true); + expect(semverValid('2.0.0', '>=1.0.0')).toBe(true); }); - + it('should return false for invalid version comparisons', () => { - expect(repoDependencyFacts.semverValid('1.0.0', '^2.0.0')).toBe(false); - expect(repoDependencyFacts.semverValid('3.0.0', '1.0.0 - 2.0.0')).toBe(false); - expect(repoDependencyFacts.semverValid('0.9.0', '>=1.0.0')).toBe(false); + expect(semverValid('1.0.0', '^2.0.0')).toBe(false); + expect(semverValid('3.0.0', '1.0.0 - 2.0.0')).toBe(false); + expect(semverValid('0.9.0', '>=1.0.0')).toBe(false); }); - + it('should handle complex version ranges', () => { - expect(repoDependencyFacts.semverValid('1.2.3', '1.x || >=2.5.0 || 5.0.0 - 7.2.3')).toBe(true); - expect(repoDependencyFacts.semverValid('2.5.0', '1.x || >=2.5.0 || 5.0.0 - 7.2.3')).toBe(true); - expect(repoDependencyFacts.semverValid('5.5.5', '1.x || >=2.5.0 || 5.0.0 - 7.2.3')).toBe(true); - expect(repoDependencyFacts.semverValid('8.0.0', '1.x || >=9.5.0 || 5.0.0 - 7.2.3')).toBe(false); + expect(semverValid('1.2.3', '1.x || >=2.5.0 || 5.0.0 - 7.2.3')).toBe(true); + expect(semverValid('2.5.0', '1.x || >=2.5.0 || 5.0.0 - 7.2.3')).toBe(true); + expect(semverValid('5.5.5', '1.x || >=2.5.0 || 5.0.0 - 7.2.3')).toBe(true); + expect(semverValid('8.0.0', '1.x || >=9.5.0 || 5.0.0 - 7.2.3')).toBe(false); }); - + + it('should handle caret ranges', () => { + expect(semverValid('1.2.3', '^1.2.3')).toBe(true); + expect(semverValid('1.3.0', '^1.2.3')).toBe(true); + expect(semverValid('2.0.0', '^1.2.3')).toBe(false); + }); + + it('should handle tilde ranges', () => { + expect(semverValid('1.2.3', '~1.2.3')).toBe(true); + expect(semverValid('1.2.9', '~1.2.3')).toBe(true); + expect(semverValid('1.3.0', '~1.2.3')).toBe(false); + }); + + it('should handle x-ranges', () => { + expect(semverValid('1.2.3', '1.2.x')).toBe(true); + expect(semverValid('1.2.9', '1.2.x')).toBe(true); + expect(semverValid('1.3.0', '1.2.x')).toBe(false); + expect(semverValid('1.2.3', '1.x')).toBe(true); + expect(semverValid('1.3.0', '1.x')).toBe(true); + expect(semverValid('2.0.0', '1.x')).toBe(false); + }); + + it('should handle star ranges', () => { + expect(semverValid('1.2.3', '*')).toBe(true); + expect(semverValid('2.0.0', '*')).toBe(true); + }); + + it('should handle greater than and less than ranges', () => { + expect(semverValid('2.0.0', '>1.2.3')).toBe(true); + expect(semverValid('1.2.3', '>1.2.3')).toBe(false); + expect(semverValid('1.2.2', '<1.2.3')).toBe(true); + expect(semverValid('1.2.3', '<1.2.3')).toBe(false); + }); + + it('should handle AND ranges', () => { + expect(semverValid('1.2.3', '>1.2.2 <1.2.4')).toBe(true); + expect(semverValid('1.2.4', '>1.2.2 <1.2.4')).toBe(false); + }); + + it('should handle OR ranges', () => { + expect(semverValid('1.2.3', '1.2.3 || 1.2.4')).toBe(true); + expect(semverValid('1.2.4', '1.2.3 || 1.2.4')).toBe(true); + expect(semverValid('1.2.5', '1.2.3 || 1.2.4')).toBe(false); + }); + + it('should handle pre-release versions', () => { + expect(semverValid('1.2.3-alpha', '>=1.2.3-alpha')).toBe(true); + expect(semverValid('1.2.3-beta', '>=1.2.3-alpha')).toBe(true); + expect(semverValid('1.2.2', '>=1.2.3-alpha')).toBe(false); + }); + it('should return true for empty strings', () => { - expect(repoDependencyFacts.semverValid('', '')).toBe(true); + expect(semverValid('', '')).toBe(true); }); - + it('should return false for invalid input', () => { - expect(repoDependencyFacts.semverValid('not-a-version', '1.0.0')).toBe(false); - expect(repoDependencyFacts.semverValid('1.0.0', 'not-a-range')).toBe(false); + expect(semverValid('not-a-version', '1.0.0')).toBe(false); + expect(semverValid('1.0.0', 'not-a-range')).toBe(false); }); }); }); diff --git a/src/facts/repoDependencyFacts.ts b/src/facts/repoDependencyFacts.ts index 4d81818..47aea8a 100644 --- a/src/facts/repoDependencyFacts.ts +++ b/src/facts/repoDependencyFacts.ts @@ -186,35 +186,46 @@ export async function repoDependencyAnalysis(params: any, almanac: Almanac) { } export function semverValid(installed: string, required: string): boolean { + // Remove potential @namespace from installed version // eslint-disable-next-line @typescript-eslint/no-non-null-assertion - const installedVersion = installed.includes('@') ? installed.split('@').pop()! : installed; - - if (!required || !installedVersion) { + installed = installed.includes('@') ? installed.split('@').pop()! : installed; + + if (!installed || !required) { return true; } + // ensure that both inputs are now valid semver or range strings + if (!semver.valid(installed) && !semver.validRange(installed)) { + logger.error(`semverValid: invalid installed version or range: ${installed}`); + return false; + } + if (!semver.valid(required) && !semver.validRange(required)) { + logger.error(`semverValid: invalid required version or range: ${required}`); + return false; + } + // If 'installed' is a single version and 'required' is a range if (semver.valid(installed) && semver.validRange(required)) { - logger.debug('range vs version'); + logger.info('range vs version'); return semver.satisfies(installed, required); } // If 'required' is a single version and 'installed' is a range if (semver.valid(required) && semver.validRange(installed)) { - logger.debug('version vs range'); + logger.info('version vs range'); return semver.satisfies(required, installed); } // If both are single versions, simply compare them if (semver.valid(required) && semver.valid(installed)) { - logger.debug('version vs version'); + logger.info('version vs version'); return semver.gt(installed, required); } // If both are ranges, check if they intersect if (semver.validRange(required) && semver.validRange(installed)) { - logger.debug('range vs range'); + logger.info('range vs range'); return semver.intersects(required, installed); } diff --git a/src/facts/repoFilesystemFacts.test.ts b/src/facts/repoFilesystemFacts.test.ts index 1698ea0..fb9234f 100644 --- a/src/facts/repoFilesystemFacts.test.ts +++ b/src/facts/repoFilesystemFacts.test.ts @@ -60,25 +60,29 @@ describe('File operations', () => { describe('isBlacklisted', () => { it('should return true if file path matches any blacklist pattern', () => { - const filePath = '/node_modules/index.js'; - expect(isBlacklisted(filePath, mockArchetypeConfig.config.blacklistPatterns)).toBe(true); + const filePath = '/test/repo/node_modules/index.js'; + const repoPath = '/test/repo'; + expect(isBlacklisted({ filePath, repoPath, blacklistPatterns: mockArchetypeConfig.config.blacklistPatterns })).toBe(true); }); it('should return false if file path does not match any blacklist pattern', () => { - const filePath = 'src/index.js'; - expect(isBlacklisted(filePath, mockArchetypeConfig.config.blacklistPatterns)).toBe(false); + const filePath = '/test/repo/src/index.js'; + const repoPath = '/test/repo'; + expect(isBlacklisted({ filePath, repoPath, blacklistPatterns: mockArchetypeConfig.config.blacklistPatterns })).toBe(false); }); }); describe('isWhitelisted', () => { it('should return true if file path matches any whitelist pattern', () => { - const filePath = '/src/index.js'; - expect(isWhitelisted(filePath, mockArchetypeConfig.config.whitelistPatterns)).toBe(true); + const filePath = '/test/repo/src/index.js'; + const repoPath = '/test/repo'; + expect(isWhitelisted({ filePath, repoPath, whitelistPatterns: mockArchetypeConfig.config.whitelistPatterns })).toBe(true); }); it('should return false if file path does not match any whitelist pattern', () => { - const filePath = 'build/index.txt'; - expect(isWhitelisted(filePath, mockArchetypeConfig.config.whitelistPatterns)).toBe(false); + const filePath = '/test/repo/build/index.txt'; + const repoPath = '/test/repo'; + expect(isWhitelisted({ filePath, repoPath, whitelistPatterns: mockArchetypeConfig.config.whitelistPatterns })).toBe(false); }); }); }); diff --git a/src/facts/repoFilesystemFacts.ts b/src/facts/repoFilesystemFacts.ts index 74a2e61..5ab5b0c 100644 --- a/src/facts/repoFilesystemFacts.ts +++ b/src/facts/repoFilesystemFacts.ts @@ -1,7 +1,7 @@ import { logger } from '../utils/logger'; import fs from 'fs'; import path from 'path'; -import { ArchetypeConfig } from '../types/typeDefs'; +import { ArchetypeConfig, IsBlacklistedParams, isWhitelistedParams } from '../types/typeDefs'; interface FileData { fileName: string; @@ -29,12 +29,12 @@ async function collectRepoFileData(repoPath: string, archetypeConfig: ArchetypeC logger.debug(`checking file: ${filePath}`); const stats = await fs.promises.lstat(filePath); if (stats.isDirectory()) { - if (!isBlacklisted(filePath, repoPath, archetypeConfig.config.blacklistPatterns)) { + if (!isBlacklisted({ filePath, repoPath, blacklistPatterns: archetypeConfig.config.blacklistPatterns })) { const dirFilesData = await collectRepoFileData(filePath, archetypeConfig); filesData.push(...dirFilesData); } } else { - if (!isBlacklisted(filePath, repoPath, archetypeConfig.config.blacklistPatterns) && isWhitelisted(filePath, repoPath, archetypeConfig.config.whitelistPatterns)) { + if (!isBlacklisted({ filePath, repoPath, blacklistPatterns: archetypeConfig.config.blacklistPatterns }) && isWhitelisted({ filePath, repoPath, whitelistPatterns: archetypeConfig.config.whitelistPatterns })) { logger.debug(`adding file: ${filePath}`); const fileData = await parseFile(filePath); filesData.push(fileData); @@ -45,7 +45,7 @@ async function collectRepoFileData(repoPath: string, archetypeConfig: ArchetypeC return filesData; } -function isBlacklisted(filePath: string, repoPath: string, blacklistPatterns: string[]): boolean { +function isBlacklisted({ filePath, repoPath, blacklistPatterns }: IsBlacklistedParams): boolean { logger.debug(`checking blacklist for file: ${filePath}`); const normalizedPath = path.normalize(filePath); const normalizedRepoPath = path.normalize(repoPath); @@ -64,7 +64,7 @@ function isBlacklisted(filePath: string, repoPath: string, blacklistPatterns: st return false; } -function isWhitelisted(filePath: string, repoPath: string, whitelistPatterns: string[]): boolean { +function isWhitelisted({ filePath, repoPath, whitelistPatterns }: isWhitelistedParams): boolean { logger.debug(`checking whitelist for file: ${filePath}`); const normalizedPath = path.normalize(filePath); const normalizedRepoPath = path.normalize(repoPath); diff --git a/src/operators/outdatedFramework.ts b/src/operators/outdatedFramework.ts index eff4419..1f2522e 100644 --- a/src/operators/outdatedFramework.ts +++ b/src/operators/outdatedFramework.ts @@ -7,7 +7,7 @@ const outdatedFramework: OperatorDefn = { let result = false; try { - logger.debug(`outdatedFramework: processing ${repoDependencyAnalysis}`); + logger.debug(`outdatedFramework: processing ${JSON.stringify(repoDependencyAnalysis)}`); if (repoDependencyAnalysis?.result?.length > 0) { return true; diff --git a/src/server/configServer.ts b/src/server/configServer.ts index 8434f10..11551fb 100644 --- a/src/server/configServer.ts +++ b/src/server/configServer.ts @@ -23,7 +23,7 @@ import chokidar from 'chokidar'; import { handleConfigChange } from './cacheManager'; const SHARED_SECRET = process.env.XFI_SHARED_SECRET; -const maskedSecret = SHARED_SECRET ? `${SHARED_SECRET.substring(0, 4)}****${SHARED_SECRET.substring(SHARED_SECRET.length - 4)}` : 'not set'; +const maskedSecret = SHARED_SECRET ? `${SHARED_SECRET.substring(0, 1)}****${SHARED_SECRET.substring(SHARED_SECRET.length - 1)}` : 'not set'; logger.info(`Shared secret is ${maskedSecret}`); const app = express(); diff --git a/src/types/typeDefs.ts b/src/types/typeDefs.ts index de5088f..cab601e 100644 --- a/src/types/typeDefs.ts +++ b/src/types/typeDefs.ts @@ -1,5 +1,5 @@ import { Engine, OperatorEvaluator } from 'json-rules-engine'; -import { FileData, isWhitelisted } from '../facts/repoFilesystemFacts'; +import { FileData } from '../facts/repoFilesystemFacts'; import { JSONSchemaType } from 'ajv'; export type OperatorDefn = { @@ -111,6 +111,7 @@ export interface RuleConfig { params: Record; }; } +export type RuleConfigSchema = JSONSchemaType; export interface OpenAIAnalysisParams { prompt: string; diff --git a/src/utils/jsonSchemas.ts b/src/utils/jsonSchemas.ts index 562955b..ce65c13 100644 --- a/src/utils/jsonSchemas.ts +++ b/src/utils/jsonSchemas.ts @@ -4,7 +4,33 @@ import { ArchetypeConfigSchema, RuleConfigSchema } from '../types/typeDefs'; const ajv = new Ajv(); -const semverPattern = '^(0|[1-9]\\d*)\\.(0|[1-9]\\d*)\\.(0|[1-9]\\d*)(?:-((?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\\.(?:0|[1-9]\\d*|\\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\\+([0-9a-zA-Z-]+(?:\\.[0-9a-zA-Z-]+)*))?$'; +const semverPattern = + "^(?:" + + "(?:\\d+\\.\\d+\\.\\d+" + + "(?:-[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "|[~^]?\\d+\\.\\d+(?:\\.\\d+)?" + + "(?:-[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\s*-\\s*[~^]?\\d+\\.\\d+(?:\\.\\d+)?" + + "(?:-[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + ")?" + + ")" + + "(\\s*\\|\\|\\s*" + + "(?:\\d+\\.\\d+\\.\\d+" + + "(?:-[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "|[~^]?\\d+\\.\\d+(?:\\.\\d+)?" + + "(?:-[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\s*-\\s*[~^]?\\d+\\.\\d+(?:\\.\\d+)?" + + "(?:-[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + "(?:\\+[0-9A-Za-z-]+(?:\\.[0-9A-Za-z-]+)*)?" + + ")?" + + ")" + + ")*" + + ")$"; const archetypeSchema: ArchetypeConfigSchema = { type: 'object', @@ -19,13 +45,14 @@ const archetypeSchema: ArchetypeConfigSchema = { minimumDependencyVersions: { type: 'object', patternProperties: { - "^.*$": { + '^.*$': { type: 'string', pattern: semverPattern } }, minProperties: 1, - additionalProperties: false + additionalProperties: false, + required: [] }, standardStructure: { type: 'object', @@ -43,10 +70,10 @@ const archetypeSchema: ArchetypeConfigSchema = { } }, required: ['minimumDependencyVersions', 'standardStructure', 'blacklistPatterns', 'whitelistPatterns'] as const, - additionalProperties: false + additionalProperties: true } }, - required: ['rules', 'operators', 'facts', 'config'] as const, + required: ['name', 'rules', 'operators', 'facts', 'config'] as const, additionalProperties: false } as const;