From efede83271bb5ffa860c8ca32e72349db8d72b09 Mon Sep 17 00:00:00 2001 From: Alex Christoffer Rasmussen Date: Wed, 21 Jan 2026 16:07:21 +0100 Subject: [PATCH] Fix: automation (#1943) * Fix(automation): Handles undefined values in conditions (#1933) * Fix(automation): Handles undefined values in conditions Addresses an issue where conditions with the 'not_equals' operator incorrectly evaluated undefined values. This change ensures that empty string comparisons correctly identify missing values in automation rules. * Fixes automation "not equals" logic Simplifies the 'not_equals' condition evaluation in automations by reusing the 'equals' condition, improving code readability and consistency. Fixes #1932 * Test(automation): Expanding filter condition testing Expanding test cases to cover various data types and edge case for each operators. Unexpected behavior have been mark with a TO_DO. * Fix(automation): Handles default filter case - Ensures that their is a default scenario. - Fixing Deepsource issue "No default cases in switch statements JS-0047" * Test(automation): Remove abstraction in test * move to test.each * extract a isEquivalent function * lowercase contains test --------- Co-authored-by: Philippe Allard-Rousse --- .../__tests__/automation.service.test.ts | 506 ++++++++++++++---- .../api-data/automation/automation.service.ts | 22 +- .../api-data/automation/automation.utils.ts | 32 ++ 3 files changed, 452 insertions(+), 108 deletions(-) diff --git a/apps/server/src/api-data/automation/__tests__/automation.service.test.ts b/apps/server/src/api-data/automation/__tests__/automation.service.test.ts index e0ea64709..22461ed15 100644 --- a/apps/server/src/api-data/automation/__tests__/automation.service.test.ts +++ b/apps/server/src/api-data/automation/__tests__/automation.service.test.ts @@ -9,6 +9,7 @@ import * as oscClient from '../clients/osc.client.js'; import * as httpClient from '../clients/http.client.js'; import { makeOSCAction, makeHTTPAction } from './testUtils.js'; +import { RuntimeState } from '../../../stores/runtimeState.js'; beforeAll(() => { vi.mock('../../../classes/data-provider/DataProvider.js', () => { @@ -100,117 +101,440 @@ describe('testConditions()', () => { expect(result).toBe(true); }); - describe('equals operator', () => { - it('should compare two equal values', () => { - const mockStore = makeRuntimeStateData({ clock: 10 }); - const result = testConditions([{ field: 'clock', operator: 'equals', value: '10' }], 'all', mockStore); - expect(result).toBe(true); - }); - - it('string comparisons should be case insensitive', () => { - const mockStore = makeRuntimeStateData({ eventNow: { title: 'Title' } as PlayableEvent }); - const result = testConditions( - [{ field: 'eventNow.title', operator: 'equals', value: 'title' }], - 'all', - mockStore, - ); - expect(result).toBe(true); - }); - - it('should check if a value does not exist', () => { - const mockStore = makeRuntimeStateData({ eventNow: null }); - const result = testConditions([{ field: 'eventNow.title', operator: 'equals', value: '' }], 'all', mockStore); - expect(result).toBe(true); - }); - - it('should handle trueness boolean comparisons', () => { - const mockStore = makeRuntimeStateData({ eventNow: { countToEnd: true } as PlayableEvent }); + describe('equals', () => { + test.each([ + { + description: 'number and string number', + value: '10', + state: 10, + }, + { + description: 'string and string', + value: 'Title', + state: 'Title', + }, + { + description: 'null and empty string', + value: '', + state: null, + }, + { + description: 'undefined and empty string', + value: '', + state: undefined, + }, + { + description: 'boolean true and string true', + value: 'true', + state: true, + }, + { + description: 'boolean false equals string false', + value: 'false', + state: false, + }, + { + description: 'not case sensitive for strings', + value: 'title', + state: 'Title', + }, + { + description: 'not case sensitive for booleans', + value: 'TRUE', + state: true, + }, + ])('$description', ({ value, state }) => { expect( - testConditions([{ field: 'eventNow.countToEnd', operator: 'equals', value: 'true' }], 'all', mockStore), + testConditions([{ field: 'test.path', operator: 'equals', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), ).toBe(true); expect( - testConditions([{ field: 'eventNow.countToEnd', operator: 'equals', value: 'false' }], 'all', mockStore), - ).toBe(false); - }); - - it('should handle falseness boolean comparisons', () => { - const mockStore = makeRuntimeStateData({ eventNow: { countToEnd: false } as PlayableEvent }); - expect( - testConditions([{ field: 'eventNow.countToEnd', operator: 'equals', value: 'false' }], 'all', mockStore), - ).toBe(true); - expect( - testConditions([{ field: 'eventNow.countToEnd', operator: 'equals', value: 'true' }], 'all', mockStore), + testConditions([{ field: 'test.path', operator: 'not_equals', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), ).toBe(false); }); }); - describe('not_equals operator', () => { - it('should check if two values are different', () => { - const mockStore = makeRuntimeStateData({ clock: 10 }); - const result = testConditions([{ field: 'clock', operator: 'not_equals', value: '11' }], 'all', mockStore); - expect(result).toBe(true); + describe('not_equals', () => { + test.each([ + { + description: 'boolean true and string false', + value: 'false', + state: true, + }, + { + description: 'boolean false and string true', + value: 'true', + state: false, + }, + { + description: 'number and non-numeric string', + value: 'lighting', + state: 10, + }, + { + description: 'number and different number', + value: '100', + state: 10, + }, + { + description: '10 and 11', + value: '11', + state: 10, + }, + { + description: '10 and 5', + value: '5', + state: 10, + }, + { + description: '10 and 1', + value: '1', + state: 10, + }, + { + description: 'string and substring', + value: 'lighting', + state: 'testing-lighting-10', + }, + { + description: 'string and different string', + value: 'sound', + state: 'testing-lighting-10', + }, + { + description: 'null and non-empty string', + value: 'not-empty', + state: null, + }, + { + description: 'undefined and non-empty string', + value: 'not-empty', + state: undefined, + }, + { + description: 'undefined and " " string', + value: ' ', + state: undefined, + }, + { + description: 'boolean false and empty string', + value: '', + state: false, + }, + { + description: '0 and empty string', + value: '', + state: 0, + }, + ])('$description', ({ value, state }) => { + expect( + testConditions([{ field: 'test.path', operator: 'not_equals', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(true); + expect( + testConditions([{ field: 'test.path', operator: 'equals', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(false); }); }); - describe('greater_than operator', () => { - it('should check if the given value is smaller', () => { - const mockStore = makeRuntimeStateData({ clock: 10 }); - const result = testConditions([{ field: 'clock', operator: 'greater_than', value: '9' }], 'all', mockStore); - expect(result).toBe(true); - }); - it('should handle values which are not numbers', () => { - const mockStore = makeRuntimeStateData({ clock: 10 }); - const result = testConditions([{ field: 'clock', operator: 'greater_than', value: 'testing' }], 'all', mockStore); - expect(result).toBe(false); + describe('greater_than', () => { + test.each([ + { + description: '10 > 5', + value: '5', + state: 10, + }, + { + description: '10 > 1', + value: '1', + state: 10, + }, + { + description: '100 > 5', + value: '5', + state: 100, + }, + { + description: '11 > 10', + value: '10', + state: 11, + }, + { + description: '0 > -1', + value: '-1', + state: 0, + }, + { + description: '-100 > -150', + value: '-150', + state: -100, + }, + ])('$description', ({ value, state }) => { + expect( + testConditions([{ field: 'test.path', operator: 'greater_than', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(true); + expect( + testConditions([{ field: 'test.path', operator: 'less_than', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(false); }); }); - describe('less_than operator', () => { - it('should check if the given value is larger', () => { - const mockStore = makeRuntimeStateData({ clock: 10 }); - const result = testConditions([{ field: 'clock', operator: 'less_than', value: '11' }], 'all', mockStore); - expect(result).toBe(true); - }); - it('should handle values which are not numbers', () => { - const mockStore = makeRuntimeStateData({ clock: 10 }); - const result = testConditions([{ field: 'clock', operator: 'less_than', value: 'testing' }], 'all', mockStore); - expect(result).toBe(false); + describe('less_than', () => { + test.each([ + { + description: '10 > 5', + value: '10', + state: 5, + }, + { + description: '10 > 1', + value: '10', + state: 1, + }, + { + description: '100 > 5', + value: '100', + state: 5, + }, + { + description: '11 > 10', + value: '11', + state: 10, + }, + { + description: '0 > -1', + value: '0', + state: -1, + }, + { + description: '-100 > -150', + value: '-100', + state: -150, + }, + ])('$description', ({ value, state }) => { + expect( + testConditions([{ field: 'test.path', operator: 'less_than', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(true); + expect( + testConditions([{ field: 'test.path', operator: 'greater_than', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(false); }); }); - describe('contains operator', () => { - it('should check if value contains given string', () => { - const result = testConditions( - [{ field: 'eventNow.title', operator: 'contains', value: 'lighting' }], - 'all', - makeRuntimeStateData({ eventNow: makeOntimeEvent({ title: 'test-lighting' }) as PlayableEvent }), - ); - expect(result).toBe(true); - - const result2 = testConditions( - [{ field: 'eventNow.title', operator: 'contains', value: 'sound' }], - 'all', - makeRuntimeStateData({ eventNow: makeOntimeEvent({ title: 'test-lighting' }) as PlayableEvent }), - ); - expect(result2).toBe(false); + describe('invalid and edge case for greater/less_than', () => { + test.each([ + { + description: 'number and non-numeric string', + value: 'lighting', + state: 10, + }, + { + description: '0 and empty string', + value: '', + state: 0, + }, + { + description: 'equal values', + value: '10', + state: 10, + }, + { + description: 'boolean false and empty string', + value: '', + state: false, + }, + { + description: 'undefined and non-empty string', + value: 'not-empty', + state: undefined, + }, + { + description: 'null and non-empty string', + value: 'not-empty', + state: null, + }, + { + description: 'string and different string', + value: 'sound', + state: 'testing-lighting-10', + }, + { + description: 'string and substring', + value: 'lighting', + state: 'testing-lighting-10', + }, + { + description: 'boolean true and uppercase TRUE', + value: 'TRUE', + state: true, + }, + { + description: 'string and lowercase string', + value: 'title', + state: 'Title', + }, + { + description: 'boolean false and string true', + value: 'true', + state: false, + }, + { + description: 'boolean false and string false', + value: 'false', + state: false, + }, + { + description: 'boolean true and string false', + value: 'false', + state: true, + }, + { + description: 'string and string', + value: 'Title', + state: 'Title', + }, + { + description: 'null and empty string', + value: '', + state: null, + }, + { + description: 'undefined and empty string', + value: '', + state: undefined, + }, + { + description: 'boolean true and string true', + value: 'true', + state: true, + }, + ])('$description', ({ value, state }) => { + expect( + testConditions([{ field: 'test.path', operator: 'less_than', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(false); + expect( + testConditions([{ field: 'test.path', operator: 'greater_than', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(false); }); }); - describe('not_contains operator', () => { - it('should check if value does not contain given string', () => { - const result = testConditions( - [{ field: 'eventNow.title', operator: 'not_contains', value: 'lighting' }], - 'all', - makeRuntimeStateData({ eventNow: makeOntimeEvent({ title: 'test-lighting' }) as PlayableEvent }), - ); - expect(result).toBe(false); + describe('contains', () => { + test.each([ + { + description: 'substring in string', + value: 'lighting', + state: 'testing-lighting-10', + }, + { + description: 'number in string', + value: '10', + state: 'testing-lighting-10', + }, + { + description: 'digit in string', + value: '1', + state: 'testing-lighting-10', + }, + { + description: 'empty in empty', + value: '', + state: '', + }, + { + description: 'empty in not-empty', + value: '', + state: '12345', // TODO: is this intended + }, + { + description: 'string equals string', + value: 'Title', + state: 'Title', + }, + { + description: 'case sensitive', + value: 'title', + state: 'Title', + }, + { + description: 'number does contain string', + value: '10', + state: 2105, + }, + ])('$description', ({ value, state }) => { + expect( + testConditions([{ field: 'test.path', operator: 'contains', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(true); + expect( + testConditions([{ field: 'test.path', operator: 'not_contains', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(false); + }); + }); - const result2 = testConditions( - [{ field: 'eventNow.title', operator: 'not_contains', value: 'sound' }], - 'all', - makeRuntimeStateData({ eventNow: makeOntimeEvent({ title: 'test-lighting' }) as PlayableEvent }), - ); - expect(result2).toBe(true); + describe('not_contains', () => { + test.each([ + { + description: 'number does not contain substring', + value: '456', + state: 12345, + }, + { + description: 'substring not in string', + value: 'sound', + state: 'testing-lighting-10', + }, + { + description: 'string and undefined', + value: 'sound', + state: undefined, + }, + { + description: 'string and null', + value: 'sound', + state: null, + }, + { + description: 'boolean true and string true', + value: 'true', + state: true, + }, + { + description: 'boolean false and string false', + value: 'false', + state: false, + }, + ])('$description', ({ value, state }) => { + expect( + testConditions([{ field: 'test.path', operator: 'not_contains', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(true); + expect( + testConditions([{ field: 'test.path', operator: 'contains', value }], 'all', { + test: { path: state }, + } as unknown as RuntimeState), + ).toBe(false); }); }); diff --git a/apps/server/src/api-data/automation/automation.service.ts b/apps/server/src/api-data/automation/automation.service.ts index 87b0aec6d..e109efe2f 100644 --- a/apps/server/src/api-data/automation/automation.service.ts +++ b/apps/server/src/api-data/automation/automation.service.ts @@ -17,7 +17,7 @@ import { isOntimeCloud } from '../../setup/environment.js'; import { emitOSC } from './clients/osc.client.js'; import { emitHTTP } from './clients/http.client.js'; import { getAutomationsEnabled, getAutomations, getAutomationTriggers } from './automation.dao.js'; -import { isBooleanEquals, isGreaterThan, isLessThan } from './automation.utils.js'; +import { isContained, isEquivalent, isGreaterThan, isLessThan } from './automation.utils.js'; import { toOntimeAction } from './clients/ontime.client.js'; /** @@ -92,29 +92,17 @@ export function testConditions( // we use loose equality to be able to check for converted values (eg '10' == 10) switch (operator) { case 'equals': - // handle the case where we are comparing boolean strings - if (typeof fieldValue === 'boolean') { - return isBooleanEquals(fieldValue, lowerCasedValue); - } - // make string comparisons case insensitive - if (typeof fieldValue === 'string') { - return fieldValue.toLowerCase() === lowerCasedValue; - } - // overload the edge case where we use empty string to check if a value does not exist - if (value === '' && fieldValue === undefined) { - return true; - } - return fieldValue == value; + return isEquivalent(fieldValue, lowerCasedValue); case 'not_equals': - return fieldValue != value; + return !isEquivalent(fieldValue, lowerCasedValue); case 'greater_than': return isGreaterThan(fieldValue, value); case 'less_than': return isLessThan(fieldValue, value); case 'contains': - return typeof fieldValue === 'string' && fieldValue.includes(value); + return isContained(fieldValue, lowerCasedValue); case 'not_contains': - return typeof fieldValue === 'string' && !fieldValue.includes(value); + return !isContained(fieldValue, lowerCasedValue); default: { operator satisfies never; return false; diff --git a/apps/server/src/api-data/automation/automation.utils.ts b/apps/server/src/api-data/automation/automation.utils.ts index 357b2138d..2eea318a8 100644 --- a/apps/server/src/api-data/automation/automation.utils.ts +++ b/apps/server/src/api-data/automation/automation.utils.ts @@ -148,6 +148,38 @@ const quickAliases: AliasesDefinition = { startedAt: { key: 'timer.startedAt', cb: (value: string) => formatDisplayFromString(value) }, }; +export function isEquivalent(a: unknown, b: string): boolean { + // handle the case where we are comparing boolean strings + if (typeof a === 'boolean') { + return isBooleanEquals(a, b); + } + // make string comparisons case insensitive + if (typeof a === 'string') { + return a.toLowerCase() === b; + } + // overload the edge case where we use empty string to check if a value does not exist + // this also avoids the case where 0 == '' + if (b === '') { + return a === null || a === undefined; + } + return a == b; +} + +export function isContained(a: unknown, b: string): boolean { + // handle the case where we are comparing boolean strings + if (typeof a === 'boolean') { + return false; + } + // make string comparisons case insensitive + if (typeof a === 'string') { + return a.toLowerCase().includes(b); + } + if (typeof a === 'number') { + return a.toString().toLowerCase().includes(b); + } + return false; +} + /** * Utility encapsulates logic for comparing two strings which may encode numbers * @example isGreaterThan('10', '5') // true