BREAKING CHANGE: Handle non-trailing slashes correctly (#1512)

* router - remove do not merge double slashes by default
* use server.pre(restify.plugins.pre.dedupeSlashes()) to deal with double slashes
* refactor test to cover multiple slashes cases

Author: hekike

lib/router.js

@@ -101,7 +101,7 @@ function compileURL(options) {
             return false;
         }
 
-        pattern += '\\/+';
+        pattern += '\\/';
 
         if (frag.charAt(0) === ':') {
             var label = frag;
@@ -131,7 +131,7 @@ function compileURL(options) {
     }
 
     if (!options.strict) {
-        pattern += '[\\/]*';
+        pattern += '[\\/]?';
     }
 
     if (pattern === '^') {

test/plugins/dedupeSlashes.test.js

@@ -76,17 +76,23 @@ describe('dedupe forward slashes in URL', function() {
             });
         });
 
-        it(
-            'should remove duplicate slashes ' + 'including trailing slashes',
-            function(done) {
-                CLIENT.get('//foo//bar//', function(err, _, res, data) {
-                    assert.ifError(err);
-                    assert.equal(res.statusCode, 200);
-                    assert.equal(data, '/foo/bar/');
-                    done();
-                });
-            }
-        );
+        // eslint-disable-next-line
+        it('should remove duplicate slashes including trailing slashes', function(done) {
+            CLIENT.get('//foo//bar//', function(err, _, res, data) {
+                assert.ifError(err);
+                assert.equal(res.statusCode, 200);
+                assert.equal(data, '/foo/bar/');
+                done();
+            });
+        });
+        it('should merge multiple slashes', function(done) {
+            CLIENT.get('//////foo///bar///////', function(err, _, res, data) {
+                assert.ifError(err);
+                assert.equal(res.statusCode, 200);
+                assert.equal(data, '/foo/bar/');
+                done();
+            });
+        });
     });
 
     describe('strict routing', function() {
@@ -131,24 +137,22 @@ describe('dedupe forward slashes in URL', function() {
         });
 
         it('should remove duplicate slashes', function(done) {
-            CLIENT.get('//foo//bar//', function(err, _, res, data) {
+            CLIENT.get('//////foo///bar///////', function(err, _, res, data) {
                 assert.ifError(err);
                 assert.equal(res.statusCode, 200);
                 assert.equal(data, '/foo/bar/');
                 done();
             });
         });
 
-        it(
-            'should remove duplicate slashes ' + 'including trailing slashes',
-            function(done) {
-                CLIENT.get('//foo//bar//', function(err, _, res, data) {
-                    assert.ifError(err);
-                    assert.equal(res.statusCode, 200);
-                    assert.equal(data, '/foo/bar/');
-                    done();
-                });
-            }
-        );
+        // eslint-disable-next-line
+        it('should remove duplicate slashes including trailing slashes', function(done) {
+            CLIENT.get('//foo//bar//', function(err, _, res, data) {
+                assert.ifError(err);
+                assert.equal(res.statusCode, 200);
+                assert.equal(data, '/foo/bar/');
+                done();
+            });
+        });
     });
 });

test/router.test.js

@@ -205,12 +205,31 @@ test('Strict routing distinguishes trailing slash', function(t) {
     server.get('/no-trailing', noop);
 
     var trailing = server.router.routes.GET[0];
-    t.ok(trailing.path.test('/trailing/'));
-    t.notOk(trailing.path.test('/trailing'));
+    t.ok(trailing.path.test('/trailing/'), 'Single trailing slash is ok');
+    t.notOk(trailing.path.test('/trailing'), 'No trailing slash is not ok');
+    t.notOk(
+        trailing.path.test('/trailing//'),
+        'Double trailing slash is not ok'
+    );
+    t.notOk(
+        trailing.path.test('//trailing/'),
+        'Double heading slash is not ok'
+    );
 
     var noTrailing = server.router.routes.GET[1];
-    t.ok(noTrailing.path.test('/no-trailing'));
-    t.notOk(noTrailing.path.test('/no-trailing/'));
+    t.ok(noTrailing.path.test('/no-trailing', 'No trailing slash is ok'));
+    t.notOk(
+        noTrailing.path.test('/no-trailing/'),
+        'Single trailing slash is not ok'
+    );
+    t.notOk(
+        noTrailing.path.test('/no-trailing//'),
+        'Double trailing slash is not ok'
+    );
+    t.notOk(
+        noTrailing.path.test('//no-trailing'),
+        'Double heading slash is not ok'
+    );
 
     t.end();
 });
@@ -223,14 +242,25 @@ test('Default non-strict routing ignores trailing slash(es)', function(t) {
     server.get('/no-trailing', noop);
 
     var trailing = server.router.routes.GET[0];
-    t.ok(trailing.path.test('/trailing/'));
-    t.ok(trailing.path.test('//trailing//'));
-    t.ok(trailing.path.test('/trailing'));
+    t.ok(trailing.path.test('/trailing/', 'Single trailing slash is ok'));
+    t.ok(trailing.path.test('/trailing'), 'No trailing slash is not ok');
+    t.notOk(
+        trailing.path.test('/trailing//'),
+        'Double trailing slash is not ok'
+    );
+    t.notOk(trailing.path.test('//trailing'), 'Double heading slash is not ok');
 
     var noTrailing = server.router.routes.GET[1];
-    t.ok(noTrailing.path.test('/no-trailing'));
-    t.ok(noTrailing.path.test('//no-trailing//'));
-    t.ok(noTrailing.path.test('/no-trailing/'));
+    t.ok(noTrailing.path.test('/no-trailing', 'No trailing slash is ok'));
+    t.ok(noTrailing.path.test('/no-trailing/'), 'Single trailing slash is ok');
+    t.notOk(
+        noTrailing.path.test('/no-trailing//'),
+        'Double trailing slash is not ok'
+    );
+    t.notOk(
+        noTrailing.path.test('//no-trailing'),
+        'Double heading slash is not ok'
+    );
 
     t.end();
 });