Merge pull request #74 from log4js-node/fix-allow-zero-backups-and-ze…

…ro-daysToKeep

fix: allow for zero backups and zero daysToKeep

README.md

@@ -55,7 +55,7 @@ When filename size >= maxSize then:
   * `compress` \<boolean\> - defaults to `false` - compress the backup files using gzip (files will have `.gz` extension).
   * `keepFileExt` \<boolean\> - defaults to `false` - keep the file original extension. e.g.: `abc.log -> abc.2013-08-30.log`.
   * `alwaysIncludePattern` \<boolean\> - defaults to `false` - extend the initial file with the pattern
-  * `daysToKeep` \<integer\> - defaults to `MAX_SAFE_INTEGER` - the number of old files that matches the pattern to keep (excluding the hot file)
+  * `daysToKeep` \<integer\> - defaults to `1` - the number of old files that matches the pattern to keep (excluding the hot file)
 
 
 This returns a `WritableStream`. When the current time, formatted as `pattern`, changes then the current file will be renamed to `filename.formattedDate` where `formattedDate` is the result of processing the date through the pattern, and a new file will begin to be written. Streamroller uses [date-format](http://github.com/nomiddlename/date-format) to format dates, and the `pattern` should use the date-format format. e.g. with a `pattern` of `".yyyy-MM-dd"`, and assuming today is August 29, 2013 then writing to the stream today will just write to `filename`. At midnight (or more precisely, at the next file write after midnight), `filename` will be renamed to `filename.2013-08-29` and a new `filename` will be created. If `options.alwaysIncludePattern` is true, then the initial file will be `filename.2013-08-29` and no renaming will occur at midnight, but a new file will be written to with the name `filename.2013-08-30`.

lib/DateRollingFileStream.js

@@ -13,9 +13,15 @@ class DateRollingFileStream extends RollingFileWriteStream {
     if (!pattern) {
       pattern = 'yyyy-MM-dd';
     }
-    if (options.daysToKeep) {
-      options.numToKeep = options.daysToKeep;
+    if (!options.daysToKeep && options.daysToKeep !== 0) {
+      options.daysToKeep = 1;
+    } else if (options.daysToKeep < 0) {
+      throw new Error(`options.daysToKeep (${options.daysToKeep}) should be >= 0`);
+    } else if (options.daysToKeep >= Number.MAX_SAFE_INTEGER) {
+      // to cater for numToKeep (include the hot file) at Number.MAX_SAFE_INTEGER
+      throw new Error(`options.daysToKeep (${options.daysToKeep}) should be < Number.MAX_SAFE_INTEGER`);
     }
+    options.numToKeep = options.daysToKeep + 1;
     if (pattern.startsWith('.')) {
       pattern = pattern.substring(1);
     }

lib/RollingFileStream.js

@@ -9,12 +9,17 @@ class RollingFileStream extends RollingFileWriteStream {
     if (size) {
       options.maxSize = size;
     }
-    if (!backups) {
+    if (!backups && backups !== 0) {
       backups = 1;
+    } else if (backups < 0) {
+      throw new Error(`backups (${backups}) should be >= 0`);
+    } else if (backups >= Number.MAX_SAFE_INTEGER) {
+      // to cater for numToKeep (include the hot file) at Number.MAX_SAFE_INTEGER
+      throw new Error(`backups (${backups}) should be < Number.MAX_SAFE_INTEGER`);
     }
-    options.numToKeep = backups;
+    options.numToKeep = backups + 1;
     super(filename, options);
-    this.backups = this.options.numToKeep;
+    this.backups = backups;
     this.size = this.options.maxSize;
   }
 

lib/RollingFileWriteStream.js

@@ -250,7 +250,7 @@ class RollingFileWriteStream extends Writable {
     debug("_clean: existing files are: ", existingFileDetails);
     if (this._tooManyFiles(existingFileDetails.length)) {
       const fileNamesToRemove = existingFileDetails
-        .slice(0, existingFileDetails.length - this.options.numToKeep - 1)
+        .slice(0, existingFileDetails.length - this.options.numToKeep)
         .map(f => path.format({ dir: this.fileObject.dir, base: f.filename }));
       await deleteFiles(fileNamesToRemove);
     }

test/DateRollingFileStream-test.js

@@ -460,6 +460,28 @@ describe("DateRollingFileStream", function() {
     });
   });
 
+  describe("with invalid number of daysToKeep", () => {
+    it("should complain about negative daysToKeep", () => {
+      const daysToKeep = -1;
+      (() => {
+        new DateRollingFileStream(
+          path.join(__dirname, "daysToKeep.log"),
+          { daysToKeep: daysToKeep }
+        );
+      }).should.throw(`options.daysToKeep (${daysToKeep}) should be >= 0`);
+    });
+
+    it("should complain about daysToKeep >= Number.MAX_SAFE_INTEGER", () => {
+      const daysToKeep = Number.MAX_SAFE_INTEGER;
+      (() => {
+        new DateRollingFileStream(
+          path.join(__dirname, "daysToKeep.log"),
+          { daysToKeep: daysToKeep }
+        );
+      }).should.throw(`options.daysToKeep (${daysToKeep}) should be < Number.MAX_SAFE_INTEGER`);
+    });
+  });
+
   describe("with daysToKeep option", function() {
     let stream;
     var daysToKeep = 4;
@@ -539,7 +561,7 @@ describe("DateRollingFileStream", function() {
         stream.write("New file message\n", "utf8", done);
       });
 
-      it("should be 4 files left from original 3", async function() {
+      it("should be 5 files left from original 11", async function() {
         const files = await fs.readdir(__dirname);
         var logFiles = files.filter(
           file => file.indexOf("compressedDaysToKeep.log") > -1

test/RollingFileStream-test.js

@@ -119,6 +119,30 @@ describe("RollingFileStream", function() {
     });
   });
 
+  describe("with invalid number of backups", () => {
+    it("should complain about negative backups", () => {
+      const backups = -1;
+      (() => {
+        new RollingFileStream(
+          path.join(__dirname, "test-rolling-file-stream"),
+          1024,
+          backups
+        );
+      }).should.throw(`backups (${backups}) should be >= 0`);
+    });
+
+    it("should complain about backups >= Number.MAX_SAFE_INTEGER", () => {
+      const backups = Number.MAX_SAFE_INTEGER;
+      (() => {
+        new RollingFileStream(
+          path.join(__dirname, "test-rolling-file-stream"),
+          1024,
+          backups
+        );
+      }).should.throw(`backups (${backups}) should be < Number.MAX_SAFE_INTEGER`);
+    });
+  });
+
   describe("writing less than the file size", function() {
     before(async function() {
       await remove("test-rolling-file-stream-write-less");

test/RollingFileWriteStream-test.js

@@ -472,15 +472,15 @@ describe("RollingFileWriteStream", () => {
     });
   });
 
-  describe("with 5 maxSize and 3 files limit", () => {
+  describe("with 5 maxSize and 3 backups limit", () => {
     const fileObj = generateTestFile();
     let s;
 
     before(async () => {
       fakeNow = new Date(2012, 8, 12, 10, 37, 11);
       s = new RollingFileWriteStream(fileObj.path, {
         maxSize: 5,
-        numToKeep: 3
+        numToKeep: 4
       });
       const flows = Array.from(Array(38).keys()).map(i => () => {
         fakeNow = new Date(2012, 8, 12 + parseInt(i / 5), 10, 37, 11);
@@ -543,7 +543,7 @@ describe("RollingFileWriteStream", () => {
     });
   });
 
-  describe("with 5 maxSize and 3 files limit, rotating daily", () => {
+  describe("with 5 maxSize and 3 backups limit, rotating daily", () => {
     const fileObj = generateTestFile();
     let s;
 
@@ -552,7 +552,7 @@ describe("RollingFileWriteStream", () => {
       s = new RollingFileWriteStream(fileObj.path, {
         maxSize: 5,
         pattern: "yyyy-MM-dd",
-        numToKeep: 3
+        numToKeep: 4
       });
       const flows = Array.from(Array(38).keys()).map(i => () => {
         fakeNow = new Date(2012, 8, 12 + parseInt(i / 10), 10, 37, 11);