Skip to content

Commit 78b44c2

Browse files
Copilothuangyiirene
andcommitted
Address code review feedback: remove unused imports/vars, improve timeout docs, add error logging, improve security docs
Co-authored-by: huangyiirene <7665279+huangyiirene@users.noreply.github.com>
1 parent d6049c9 commit 78b44c2

File tree

4 files changed

+54
-12
lines changed

4 files changed

+54
-12
lines changed

packages/foundation/core/src/formula-engine.ts

Lines changed: 14 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,6 @@ import {
1414
FormulaEvaluationOptions,
1515
FormulaError,
1616
FormulaErrorType,
17-
FormulaFieldValue,
1817
FormulaValue,
1918
FormulaDataType,
2019
FormulaMetadata,
@@ -70,7 +69,6 @@ export class FormulaEngine {
7069
): FormulaEvaluationResult {
7170
const startTime = Date.now();
7271
const timeout = options.timeout ?? this.config.max_execution_time ?? 1000;
73-
const strict = options.strict ?? true;
7472

7573
try {
7674
// Validate expression
@@ -177,6 +175,10 @@ export class FormulaEngine {
177175
options: FormulaEvaluationOptions
178176
): FormulaValue {
179177
// Check for blocked operations
178+
// NOTE: This is a basic check using string matching. It will have false positives
179+
// (e.g., a field named 'evaluation' contains 'eval') and can be bypassed
180+
// (e.g., using this['eval'] or globalThis['eval']).
181+
// For production security with untrusted formulas, use AST parsing or vm2.
180182
if (this.config.sandbox?.enabled) {
181183
const blockedOps = this.config.sandbox.blocked_operations || [];
182184
for (const op of blockedOps) {
@@ -273,25 +275,29 @@ export class FormulaEngine {
273275
/**
274276
* Execute function with timeout protection
275277
*
276-
* NOTE: This implements post-execution timeout validation, not pre-emptive interruption.
277-
* The function will run to completion and then check if it exceeded the timeout.
278-
* For true interruption, platform-specific mechanisms (Worker threads) would be needed.
278+
* NOTE: This synchronous implementation **cannot** pre-emptively interrupt execution.
279+
* The timeout check occurs AFTER the formula completes. Long-running formulas will
280+
* still block execution. This is a limitation of synchronous JavaScript.
281+
*
282+
* For true interruption, use platform-specific mechanisms like Worker threads or
283+
* migrate to an async/isolated runtime. Formulas should be written to be fast.
279284
*/
280285
private executeWithTimeout(
281286
func: Function,
282287
args: any[],
283288
timeout: number
284289
): unknown {
285-
// Simple synchronous execution (timeout would require async or worker threads)
286-
// For now, we execute directly. Advanced timeout requires platform-specific code.
290+
// Execute the formula synchronously
287291
const startTime = Date.now();
288292
const result = func(...args);
289293
const elapsed = Date.now() - startTime;
290294

295+
// Check timeout AFTER execution (cannot interrupt synchronous code)
291296
if (elapsed > timeout) {
292297
throw new FormulaError(
293298
FormulaErrorType.TIMEOUT,
294-
`Formula execution exceeded timeout of ${timeout}ms`,
299+
`Formula execution exceeded timeout of ${timeout}ms (actual: ${elapsed}ms). ` +
300+
`Note: Synchronous formulas cannot be interrupted. Write fast formulas or use async runtime.`,
295301
'',
296302
{ elapsed, timeout }
297303
);

packages/foundation/core/src/repository.ts

Lines changed: 15 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,22 @@ export class ObjectRepository {
178178
if (result.success) {
179179
record[fieldName] = result.value;
180180
} else {
181-
// In case of error, set to null and optionally log
181+
// In case of error, set to null and log for diagnostics
182182
record[fieldName] = null;
183-
// Could add logging here if needed
183+
// Formula evaluation should not throw here, but we need observability
184+
// This logging is intentionally minimal and side-effect free
185+
// eslint-disable-next-line no-console
186+
console.error(
187+
'[ObjectQL][FormulaEngine] Formula evaluation failed',
188+
{
189+
objectName: this.objectName,
190+
fieldName,
191+
recordId: formulaContext.record_id,
192+
formula: fieldConfig.formula,
193+
error: result.error,
194+
stack: result.stack,
195+
}
196+
);
184197
}
185198
}
186199
}

packages/foundation/core/test/formula-engine.test.ts

Lines changed: 0 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,8 +7,6 @@
77
import { FormulaEngine } from '../src/formula-engine';
88
import {
99
FormulaContext,
10-
FormulaErrorType,
11-
FormulaDataType,
1210
} from '@objectql/types';
1311

1412
describe('FormulaEngine', () => {

pnpm-lock.yaml

Lines changed: 25 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)