env variables for ACTION_HISTORY limits (#1262)

* Introduce ACTION_HISTORY env variables
* fix: env vars must be integers
* Use AppSettings and introduce minValue and maxValue
* Update README.md
* Start testing AppSettings

Co-authored-by: vviers <vincent.viers@beta.gouv.fr>

README.md

@@ -262,6 +262,8 @@ Grist can be configured in many ways. Here are the main environment variables it
 | APP_STATIC_URL                     | url prefix for static resources                                                                                                                                                                                                                                                                                                                               |
 | APP_STATIC_INCLUDE_CUSTOM_CSS      | set to "true" to include custom.css (from APP_STATIC_URL) in static pages                                                                                                                                                                                                                                                                                     |
 | APP_UNTRUSTED_URL                  | URL at which to serve/expect plugin content.                                                                                                                                                                                                                                                                                                                  |
+| GRIST_ACTION_HISTORY_MAX_ROWS | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1000. ⚠️ A too low value may make the "[Work on a copy](https://support.getgrist.com/newsletters/2021-06/#work-on-a-copy)" feature [malfunction](https://github.com/gristlabs/grist-core/issues/1121#issuecomment-2248112023) |
+| GRIST_ACTION_HISTORY_MAX_BYTES | Maximum number of rows allowed in ActionHistory before pruning (up to a 1.25 grace factor). Defaults to 1Gb. ⚠️ A too low value may make the "[Work on a copy](https://support.getgrist.com/newsletters/2021-06/#work-on-a-copy)" feature [malfunction](https://github.com/gristlabs/grist-core/issues/1121#issuecomment-2248112023) |
 | GRIST_ADAPT_DOMAIN                 | set to "true" to support multiple base domains (careful, host header should be trustworthy)                                                                                                                                                                                                                                                                   |
 | GRIST_APP_ROOT                     | directory containing Grist sandbox and assets (specifically the sandbox and static subdirectories).                                                                                                                                                                                                                                                           |
 | GRIST_BACKUP_DELAY_SECS            | wait this long after a doc change before making a backup                                                                                                                                                                                                                                                                                                      |

app/server/lib/ActionHistoryImpl.ts

@@ -12,14 +12,28 @@ import mapValues = require('lodash/mapValues');
 import {ActionGroupOptions, ActionHistory, ActionHistoryUndoInfo, asActionGroup,
         asMinimalActionGroup} from './ActionHistory';
 import {ISQLiteDB, ResultRow} from './SQLiteDB';
+import { appSettings } from './AppSettings';
+
+const section = appSettings.section('history').section('action');
 
 // History will from time to time be pruned back to within these limits
 // on rows and the maximum total number of bytes in the "body" column.
 // Pruning is done when the history has grown above these limits, to
 // the specified factor.
-const ACTION_HISTORY_MAX_ROWS = 1000;
+const ACTION_HISTORY_MAX_ROWS = section.flag('maxRows').requireInt({
-const ACTION_HISTORY_MAX_BYTES = 1000 * 1000 * 1000;  // 1 GB.
+  envVar: 'GRIST_ACTION_HISTORY_MAX_ROWS',
-const ACTION_HISTORY_GRACE_FACTOR = 1.25;  // allow growth to 1250 rows / 1.25 GB.
+  defaultValue: 1000,
+
+  minValue: 1,
+});
+
+const ACTION_HISTORY_MAX_BYTES = section.flag('maxBytes').requireInt({
+  envVar: 'GRIST_ACTION_HISTORY_MAX_BYTES',
+  defaultValue: 1e9, // 1 GB.
+  minValue: 1,  // 1 B.
+});
+
+const ACTION_HISTORY_GRACE_FACTOR = 1.25;  // allow growth to 1.25 times the above limits.
 const ACTION_HISTORY_CHECK_PERIOD = 10;    // number of actions between size checks.
 
 /**

app/server/lib/AppSettings.ts

@@ -99,7 +99,7 @@ export class AppSettings {
   /**
    * As for readInt() but fail if nothing was found.
    */
-  public requireInt(query: AppSettingQuery): number {
+  public requireInt(query: AppSettingQueryInt): number {
     const result = this.readInt(query);
     if (result === undefined) {
       throw new Error(`missing environment variable: ${query.envVar}`);
@@ -122,9 +122,19 @@ export class AppSettings {
    * As for read() but type (and store, and report) the result as
    * an integer (well, a number).
    */
-  public readInt(query: AppSettingQuery): number|undefined {
+  public readInt(query: AppSettingQueryInt): number|undefined {
     this.readString(query);
     const result = this.getAsInt();
+
+    if (result !== undefined) {
+      if (query.minValue !== undefined && result < query.minValue) {
+        throw new Error(`value ${result} is less than minimum ${query.minValue}`);
+      }
+      if (query.maxValue !== undefined && result > query.maxValue) {
+        throw new Error(`value ${result} is greater than maximum ${query.maxValue}`);
+      }
+    }
+
     this._value = result;
     return result;
   }
@@ -213,11 +223,39 @@ export const appSettings = new AppSettings('grist');
  * environment variables and default values.
  */
 export interface AppSettingQuery {
-  envVar: string|string[];  // environment variable(s) to check.
+  /**
-  preferredEnvVar?: string; // "Canonical" environment variable to suggest.
+   * Environment variable(s) to check.
-                            // Should be in envVar (though this is not checked).
+   */
-  defaultValue?: JSONValue; // value to use if variable(s) unavailable.
+  envVar: string|string[];
-  censor?: boolean;   // should the value of the setting be obscured when printed.
+  /**
+   * "Canonical" environment variable to suggest. Should be in envVar (though this is not checked).
+   */
+  preferredEnvVar?: string;
+  /**
+   * Value to use if the variable(s) is/are unavailable.
+   */
+  defaultValue?: JSONValue;
+  /**
+   * When set to true, the value is obscured when printed.
+   */
+  censor?: boolean;
+}
+
+export interface AppSettingQueryInt extends AppSettingQuery {
+  /**
+   * Value to use if variable(s) unavailable.
+   */
+  defaultValue?: number;
+  /**
+   * Minimum value allowed. Raises an error if the value is lower than this.
+   * If the value is undefined, the setting is not checked.
+   */
+  minValue?: number;
+  /**
+   * Maximum value allowed. Raises an error if the value is greater than this.
+   * If the value is undefined, the setting is not checked.
+   */
+  maxValue?: number;
 }
 
 /**

app/server/lib/OIDCConfig.ts

@@ -141,6 +141,7 @@ export class OIDCConfig {
     });
     const httpTimeout = section.flag('httpTimeout').readInt({
       envVar: 'GRIST_OIDC_SP_HTTP_TIMEOUT',
+      minValue: 0, // 0 means no timeout
     });
     this._namePropertyKey = section.flag('namePropertyKey').readString({
       envVar: 'GRIST_OIDC_SP_PROFILE_NAME_ATTR',

test/server/lib/AppSettings.ts

@@ -0,0 +1,96 @@
+import { AppSettings } from 'app/server/lib/AppSettings';
+import { EnvironmentSnapshot } from '../testUtils';
+
+import { assert } from 'chai';
+
+describe('AppSettings', () => {
+  let appSettings: AppSettings;
+  let env: EnvironmentSnapshot;
+  beforeEach(() => {
+    appSettings = new AppSettings('test');
+    env = new EnvironmentSnapshot();
+  });
+
+  afterEach(() => {
+    env.restore();
+  });
+
+  describe('for integers', () => {
+    function testIntMethod(method: 'readInt' | 'requireInt') {
+      it('should throw an error if the value is less than the minimum', () => {
+        process.env.TEST = '4';
+        assert.throws(() => {
+          appSettings[method]({ envVar: 'TEST', minValue: 5 });
+        }, 'value 4 is less than minimum 5');
+      });
+
+      it('should throw an error if the value is greater than the maximum', () => {
+        process.env.TEST = '6';
+        assert.throws(() => {
+          appSettings[method]({ envVar: 'TEST', maxValue: 5 });
+        }, 'value 6 is greater than maximum 5');
+      });
+
+      it('should throw if the value is NaN', () => {
+        process.env.TEST = 'not a number';
+        assert.throws(() => appSettings[method]({ envVar: 'TEST' }), 'not a number does not look like a number');
+      });
+
+      it('should throw if the default value is not finite', () => {
+        assert.throws(
+          () => appSettings[method]({ envVar: 'TEST', defaultValue: Infinity }),
+          'Infinity does not look like a number'
+        );
+      });
+
+      it('should throw if the default value is not within the range', () => {
+        assert.throws(
+          () => appSettings[method]({
+            envVar: 'TEST',
+            defaultValue: 6,
+            minValue: 7,
+            maxValue: 9,
+          }),
+          'value 6 is less than minimum 7'
+        );
+      });
+
+      it('should return the default value if it is within the range', () => {
+        const result = appSettings[method]({
+          envVar: 'TEST',
+          defaultValue: 5,
+          minValue: 5,
+          maxValue: 12
+        });
+        assert.strictEqual(result, 5);
+      });
+
+      it('should return the value if it is within the range', () => {
+        process.env.TEST = '5';
+        assert.strictEqual(appSettings[method]({ envVar: 'TEST', minValue: 5 }), 5);
+      });
+
+      it('should return the integer value of a float', () => {
+        process.env.TEST = '5.9';
+        assert.strictEqual(appSettings[method]({ envVar: 'TEST' }), 5);
+      });
+    }
+
+    describe('readInt()', () => {
+      testIntMethod('readInt');
+
+      it('should return undefined when no value nor default value is passed', () => {
+        const result = appSettings.readInt({ envVar: 'TEST', maxValue: 5 });
+        assert.isUndefined(result);
+      });
+    });
+
+    describe('requireInt()', () => {
+      testIntMethod('requireInt');
+
+      it('should throw if env variable is not set and no default value is passed', () => {
+        assert.throws(() => appSettings.requireInt({ envVar: 'TEST' }), 'missing environment variable: TEST');
+      });
+    });
+  });
+});