fix: deprecate maxExtension in favour of maxExtensionMinutes (#1402)

feywind · web-flow · commit 46b83ba14e95 · 2021-09-27T13:24:08.000-04:00
* fix: deprecate maxExtension in favour of maxExtensionMinutes

Background: maxExtension was incorrectly interpreted as seconds in the LeaseManager. This means that if users were relying on client side lease extension in subscribers, they would not actually extend leases for 60 minutes, but only 60 seconds. Also if the user passed in values, they'd be interpreted incorrectly. This PR deprecates the old field in favour of a new one that's explicitly named like the defaults, but also converts the old value if needed.

* fix: only allow one of maxExtension or maxExtensionMinutes
diff --git a/src/lease-manager.ts b/src/lease-manager.ts
@@ -21,8 +21,11 @@ import {defaultOptions} from './default-options';
 export interface FlowControlOptions {
   allowExcessMessages?: boolean;
   maxBytes?: number;
-  maxExtension?: number;
   maxMessages?: number;
+  maxExtensionMinutes?: number;
+
+  /** @deprecated Use maxExtensionMinutes. */
+  maxExtension?: number;
 }
 
 /**
@@ -36,7 +39,7 @@ export interface FlowControlOptions {
  * @property {number} [maxBytes=104857600] The desired amount of memory to
  *     allow message data to consume. (Default: 100MB) It's possible that this
  *     value will be exceeded, since messages are received in batches.
- * @property {number} [maxExtension=60] The maximum duration (in seconds)
+ * @property {number} [maxExtensionMinutes=60] The maximum duration (in minutes)
  *      to extend the message deadline before redelivering.
  * @property {number} [maxMessages=1000] The desired number of messages to allow
  *     in memory before pausing the message stream. Unless allowExcessMessages
@@ -179,13 +182,34 @@ export class LeaseManager extends EventEmitter {
    * Sets options for the LeaseManager.
    *
    * @param {FlowControlOptions} [options] The options.
+   *
+   * @throws {RangeError} If both maxExtension and maxExtensionMinutes are set.
+   *
    * @private
    */
   setOptions(options: FlowControlOptions): void {
+    // Convert the old deprecated maxExtension to avoid breaking clients,
+    // but allow only one.
+    if (
+      options.maxExtension !== undefined &&
+      options.maxExtensionMinutes !== undefined
+    ) {
+      throw new RangeError(
+        'Only one of "maxExtension" or "maxExtensionMinutes" may be set for subscriber lease management options'
+      );
+    }
+    if (
+      options.maxExtension !== undefined &&
+      options.maxExtensionMinutes === undefined
+    ) {
+      options.maxExtensionMinutes = options.maxExtension / 60;
+      delete options.maxExtension;
+    }
+
     const defaults: FlowControlOptions = {
       allowExcessMessages: true,
       maxBytes: defaultOptions.subscription.maxOutstandingBytes,
-      maxExtension: defaultOptions.subscription.maxExtensionMinutes,
+      maxExtensionMinutes: defaultOptions.subscription.maxExtensionMinutes,
       maxMessages: defaultOptions.subscription.maxOutstandingMessages,
     };
 
@@ -229,9 +253,10 @@ export class LeaseManager extends EventEmitter {
     const deadline = this._subscriber.ackDeadline;
 
     for (const message of this._messages) {
-      const lifespan = (Date.now() - message.received) / 1000;
+      // Lifespan here is in minutes.
+      const lifespan = (Date.now() - message.received) / (60 * 1000);
 
-      if (lifespan < this._options.maxExtension!) {
+      if (lifespan < this._options.maxExtensionMinutes!) {
         message.modAck(deadline);
       } else {
         this.remove(message);
diff --git a/test/lease-manager.ts b/test/lease-manager.ts
@@ -224,11 +224,31 @@ describe('LeaseManager', () => {
         });
       });
 
-      it('should remove any messages that pass the maxExtension value', () => {
-        const maxExtension = (expectedTimeout - 100) / 1000;
+      it('should properly convert any legacy maxExtension values', () => {
+        const maxExtension = 60 * 1000;
+        leaseManager.setOptions({maxExtension});
+        // eslint-disable-next-line @typescript-eslint/no-explicit-any
+        const options = (leaseManager as any)._options;
+        assert.strictEqual(options.maxExtensionMinutes, maxExtension / 60);
+        assert.strictEqual(options.maxExtension, undefined);
+      });
+
+      it('should not allow both maxExtension and maxExtensionMinutes', () => {
+        assert.throws(() => {
+          leaseManager.setOptions({
+            maxExtension: 10,
+            maxExtensionMinutes: 10,
+          });
+        });
+      });
+
+      it('should remove any messages that pass the maxExtensionMinutes value', () => {
+        const maxExtensionSeconds = (expectedTimeout - 100) / 1000;
         const badMessages = [new FakeMessage(), new FakeMessage()];
 
-        leaseManager.setOptions({maxExtension});
+        leaseManager.setOptions({
+          maxExtensionMinutes: maxExtensionSeconds / 60,
+        });
         badMessages.forEach(message =>
           leaseManager.add(message as {} as Message)
         );
