zlib: improve zlib errors

- Use assert to check mode in the Zlib constructor since it should only be passed by us. - Introduce checkRangesOrGetDefault() and checkFiniteNumber() to simplify type and range checking for numeric arguments - Instead of `ERR_INVALID_OPT_VALUE`, throw `ERR_OUT_OF_RANGE` and `ERR_INVALID_ARG_TYPE` with descriptions of the expected ranges or types to make the errors more user-friendly. - Add message tests for the changed errors
nodejs · Feb 18, 2018 · 22c7c8d · 22c7c8d
1 parent 472cde6
commit 22c7c8d
Show file tree

Hide file tree

Showing 4 changed files with 321 additions and 130 deletions.
diff --git a/lib/zlib.js b/lib/zlib.js
@@ -156,6 +156,56 @@ function flushCallback(level, strategy, callback) {
   }
 }
 
+// 1. Returns true for finite numbers
+// 2. Returns false for undefined and NaN
+// 3. Throws ERR_INVALID_ARG_TYPE for non-numbers
+// 4. Throws ERR_OUT_OF_RANGE for infinite numbers
+function checkFiniteNumber(number, name) {
+  if (Number.isFinite(number)) {
+    return true;  // is a valid number
+  } else {
+    // undefined or NaN
+    if (number === undefined || Number.isNaN(number)) {
+      return false;
+    }
+
+    // Other non-numbers
+    if (typeof number !== 'number') {
+      const err = new errors.TypeError('ERR_INVALID_ARG_TYPE', name,
+                                       'number', number);
+      Error.captureStackTrace(err, checkFiniteNumber);
+      throw err;
+    }
+
+    // Infinite numbers
+    const err = new errors.RangeError('ERR_OUT_OF_RANGE', name,
+                                      'a finite number', number);
+    Error.captureStackTrace(err, checkFiniteNumber);
+    throw err;
+  }
+}
+
+function checkRanges(number, name, lower, upper) {
+  if (number < lower || number > upper) {
+    const err = new errors.RangeError(
+      'ERR_OUT_OF_RANGE', name, `>= ${lower} and <= ${upper}`, number);
+    Error.captureStackTrace(err, checkRanges);
+    throw err;
+  }
+}
+
+// 1. Returns number for finite numbers >= lower and <= upper
+// 2. Returns def for undefined and NaN
+// 3. Throws ERR_INVALID_ARG_TYPE for non-numbers
+// 4. Throws ERR_OUT_OF_RANGE for infinite numbers or numbers > upper or < lower
+function checkRangesOrGetDefault(number, name, lower, upper, def) {
+  if (checkFiniteNumber(number, name, lower, upper)) {
+    checkRanges(number, name, lower, upper);
+    return number;
+  }
+  return def;
+}
+
 // the Zlib class they all inherit from
 // This thing manages the queue of requests, and returns
 // true or false if there is anything in the queue when
@@ -170,95 +220,53 @@ function Zlib(opts, mode) {
   var strategy = Z_DEFAULT_STRATEGY;
   var dictionary;
 
-  if (typeof mode !== 'number')
-    throw new errors.TypeError('ERR_INVALID_ARG_TYPE', 'mode', 'number');
-  if (mode < DEFLATE || mode > UNZIP)
-    throw new errors.RangeError('ERR_OUT_OF_RANGE', 'mode');
+  // The Zlib class is not exported to user land, the mode should only be
+  // passed in by us.
+  assert(typeof mode === 'number');
+  assert(mode >= DEFLATE && mode <= UNZIP);
 
   if (opts) {
     chunkSize = opts.chunkSize;
-    if (chunkSize !== undefined && !Number.isNaN(chunkSize)) {
-      if (chunkSize < Z_MIN_CHUNK || !Number.isFinite(chunkSize))
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'chunkSize',
-                                    chunkSize);
-    } else {
+    if (!checkFiniteNumber(chunkSize, 'options.chunkSize')) {
       chunkSize = Z_DEFAULT_CHUNK;
+    } else if (chunkSize < Z_MIN_CHUNK) {
+      throw new errors.RangeError('ERR_OUT_OF_RANGE', 'options.chunkSize',
+                                  `>= ${Z_MIN_CHUNK}`, chunkSize);
     }
 
-    flush = opts.flush;
-    if (flush !== undefined && !Number.isNaN(flush)) {
-      if (flush < Z_NO_FLUSH || flush > Z_BLOCK || !Number.isFinite(flush))
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE', 'flush', flush);
-    } else {
-      flush = Z_NO_FLUSH;
-    }
+    flush = checkRangesOrGetDefault(
+      opts.flush, 'options.flush',
+      Z_NO_FLUSH, Z_BLOCK, Z_NO_FLUSH);
 
-    finishFlush = opts.finishFlush;
-    if (finishFlush !== undefined && !Number.isNaN(finishFlush)) {
-      if (finishFlush < Z_NO_FLUSH || finishFlush > Z_BLOCK ||
-          !Number.isFinite(finishFlush)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'finishFlush',
-                                    finishFlush);
-      }
-    } else {
-      finishFlush = Z_FINISH;
-    }
+    finishFlush = checkRangesOrGetDefault(
+      opts.finishFlush, 'options.finishFlush',
+      Z_NO_FLUSH, Z_BLOCK, Z_FINISH);
 
-    windowBits = opts.windowBits;
-    if (windowBits !== undefined && !Number.isNaN(windowBits)) {
-      if (windowBits < Z_MIN_WINDOWBITS || windowBits > Z_MAX_WINDOWBITS ||
-          !Number.isFinite(windowBits)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'windowBits',
-                                    windowBits);
-      }
-    } else {
-      windowBits = Z_DEFAULT_WINDOWBITS;
-    }
+    windowBits = checkRangesOrGetDefault(
+      opts.windowBits, 'options.windowBits',
+      Z_MIN_WINDOWBITS, Z_MAX_WINDOWBITS, Z_DEFAULT_WINDOWBITS);
 
-    level = opts.level;
-    if (level !== undefined && !Number.isNaN(level)) {
-      if (level < Z_MIN_LEVEL || level > Z_MAX_LEVEL ||
-          !Number.isFinite(level)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'level', level);
-      }
-    } else {
-      level = Z_DEFAULT_COMPRESSION;
-    }
+    level = checkRangesOrGetDefault(
+      opts.level, 'options.level',
+      Z_MIN_LEVEL, Z_MAX_LEVEL, Z_DEFAULT_COMPRESSION);
 
-    memLevel = opts.memLevel;
-    if (memLevel !== undefined && !Number.isNaN(memLevel)) {
-      if (memLevel < Z_MIN_MEMLEVEL || memLevel > Z_MAX_MEMLEVEL ||
-          !Number.isFinite(memLevel)) {
-        throw new errors.RangeError('ERR_INVALID_OPT_VALUE',
-                                    'memLevel', memLevel);
-      }
-    } else {
-      memLevel = Z_DEFAULT_MEMLEVEL;
-    }
+    memLevel = checkRangesOrGetDefault(
+      opts.memLevel, 'options.memLevel',
+      Z_MIN_MEMLEVEL, Z_MAX_MEMLEVEL, Z_DEFAULT_MEMLEVEL);
 
-    strategy = opts.strategy;
-    if (strategy !== undefined && !Number.isNaN(strategy)) {
-      if (strategy < Z_DEFAULT_STRATEGY || strategy > Z_FIXED ||
-          !Number.isFinite(strategy)) {
-        throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
-                                   'strategy', strategy);
-      }
-    } else {
-      strategy = Z_DEFAULT_STRATEGY;
-    }
+    strategy = checkRangesOrGetDefault(
+      opts.strategy, 'options.strategy',
+      Z_DEFAULT_STRATEGY, Z_FIXED, Z_DEFAULT_STRATEGY);
 
     dictionary = opts.dictionary;
     if (dictionary !== undefined && !isArrayBufferView(dictionary)) {
       if (isAnyArrayBuffer(dictionary)) {
         dictionary = Buffer.from(dictionary);
       } else {
-        throw new errors.TypeError('ERR_INVALID_OPT_VALUE',
-                                   'dictionary',
-                                   dictionary);
+        throw new errors.TypeError(
+          'ERR_INVALID_ARG_TYPE', 'options.dictionary',
+          ['Buffer', 'TypedArray', 'DataView', 'ArrayBuffer'],
+          dictionary);
       }
     }
 
@@ -310,14 +318,8 @@ Object.defineProperty(Zlib.prototype, '_closed', {
 });
 
 Zlib.prototype.params = function params(level, strategy, callback) {
-  if (level < Z_MIN_LEVEL || level > Z_MAX_LEVEL)
-    throw new errors.RangeError('ERR_INVALID_ARG_VALUE', 'level', level);
-
-  if (strategy !== undefined &&
-      (strategy < Z_DEFAULT_STRATEGY || strategy > Z_FIXED ||
-       !Number.isFinite(strategy))) {
-    throw new errors.TypeError('ERR_INVALID_ARG_VALUE', 'strategy', strategy);
-  }
+  checkRangesOrGetDefault(level, 'level', Z_MIN_LEVEL, Z_MAX_LEVEL);
+  checkRangesOrGetDefault(strategy, 'strategy', Z_DEFAULT_STRATEGY, Z_FIXED);
 
   if (this._level !== level || this._strategy !== strategy) {
     this.flush(Z_SYNC_FLUSH,