From dfdcaa893e07cb9e91ee5fb1ddd8955c2ddb60bc Mon Sep 17 00:00:00 2001
From: steve-p-com <github@steve-p.com>
Date: Mon, 18 Apr 2016 20:53:00 +0100
Subject: [PATCH] Change the way package distribution tags are processed and
 stored

---
 .gitignore               |  6 ++++
 conf/full.yaml           |  6 ----
 lib/config.js            |  2 --
 lib/local-storage.js     | 25 +++++----------
 lib/storage.js           | 27 ++++++----------
 lib/utils.js             | 68 +++++++++++++++++++++++++++-------------
 test/unit/st_merge.js    | 16 ++--------
 test/unit/tag_version.js | 23 ++++----------
 8 files changed, 77 insertions(+), 96 deletions(-)

diff --git a/.gitignore b/.gitignore
index 79595d190..fe652cd2d 100644
--- a/.gitignore
+++ b/.gitignore
@@ -9,3 +9,9 @@ bin/**
 test-storage*
 
 node_modules
+
+# Visual Studio Code
+.vscode/*
+.jscsrc
+.jshintrc
+jsconfig.json
diff --git a/conf/full.yaml b/conf/full.yaml
index b8c344181..4cc4d4b70 100644
--- a/conf/full.yaml
+++ b/conf/full.yaml
@@ -119,9 +119,3 @@ logs:
 # maximum size of uploaded json document
 # increase it if you have "request entity too large" errors
 #max_body_size: 1mb
-
-# Workaround for countless npm bugs. Must have for npm <1.14.x, but expect
-# it to be turned off in future versions. If `true`, latest tag is ignored,
-# and the highest semver is placed instead.
-#ignore_latest_tag: false
-
diff --git a/lib/config.js b/lib/config.js
index e9add9ec7..df0744d75 100644
--- a/lib/config.js
+++ b/lib/config.js
@@ -142,8 +142,6 @@ function Config(config) {
     self.server_id = Crypto.pseudoRandomBytes(6).toString('hex')
   }
 
-  if (self.ignore_latest_tag == null) self.ignore_latest_tag = false
-
   return self
 }
 
diff --git a/lib/local-storage.js b/lib/local-storage.js
index 52a03210d..e73bad50d 100644
--- a/lib/local-storage.js
+++ b/lib/local-storage.js
@@ -176,19 +176,7 @@ Storage.prototype.update_versions = function(name, newdata, callback) {
       }
     }
     for (var tag in newdata['dist-tags']) {
-      if (!Array.isArray(data['dist-tags'][tag]) || data['dist-tags'][tag].length != newdata['dist-tags'][tag].length) {
-        // backward compat
-        var need_change = true
-      } else {
-        for (var i=0; i<data['dist-tags'][tag].length; i++) {
-          if (data['dist-tags'][tag][i] != newdata['dist-tags'][tag][i]) {
-            var need_change = true
-            break
-          }
-        }
-      }
-
-      if (need_change) {
+      if (!data['dist-tags'][tag] || data['dist-tags'][tag] !== newdata['dist-tags'][tag]) {
         change = true
         data['dist-tags'][tag] = newdata['dist-tags'][tag]
       }
@@ -247,7 +235,7 @@ Storage.prototype.add_version = function(name, version, metadata, tag, callback)
     }
 
     data.versions[version] = metadata
-    Utils.tag_version(data, version, tag, self.config)
+    Utils.tag_version(data, version, tag)
     self.config.localList.add(name)
     cb()
   }, callback)
@@ -267,7 +255,7 @@ Storage.prototype.merge_tags = function(name, tags, callback) {
         return cb( Error[404]("this version doesn't exist") )
       }
 
-      Utils.tag_version(data, tags[t], t, self.config)
+      Utils.tag_version(data, tags[t], t)
     }
     cb()
   }, callback)
@@ -289,7 +277,7 @@ Storage.prototype.replace_tags = function(name, tags, callback) {
         return cb( Error[404]("this version doesn't exist") )
       }
 
-      Utils.tag_version(data, tags[t], t, self.config)
+      Utils.tag_version(data, tags[t], t)
     }
     cb()
   }, callback)
@@ -601,7 +589,7 @@ Storage.prototype.search = function(startkey, options) {
           if (err) return cb(err)
 
           var versions = Utils.semver_sort(Object.keys(data.versions))
-          var latest = versions[versions.length - 1]
+          var latest = data['dist-tags'] && data['dist-tags'].latest ? data['dist-tags'].latest : versions.pop()
 
           if (data.versions[latest]) {
             stream.push({
@@ -641,6 +629,9 @@ Storage.prototype._normalize_package = function(pkg) {
     if (!Utils.is_object(pkg[key])) pkg[key] = {}
   })
   if (typeof(pkg._rev) !== 'string') pkg._rev = '0-0000000000000000'
+
+  // normalize dist-tags
+  Utils.normalize_dist_tags(pkg)
 }
 
 Storage.prototype._write_package = function(name, json, callback) {
diff --git a/lib/storage.js b/lib/storage.js
index 3daace978..14bf349ee 100644
--- a/lib/storage.js
+++ b/lib/storage.js
@@ -307,16 +307,7 @@ Storage.prototype.get_package = function(name, options, callback) {
         if (whitelist.indexOf(i) === -1) delete result[i]
       }
 
-      if (self.config.ignore_latest_tag || !result['dist-tags'].latest) {
-        result['dist-tags'].latest = Utils.semver_sort(Object.keys(result.versions))
-      }
-
-      for (var i in result['dist-tags']) {
-        if (Array.isArray(result['dist-tags'][i])) {
-          result['dist-tags'][i] = result['dist-tags'][i][result['dist-tags'][i].length-1]
-          if (result['dist-tags'][i] == null) delete result['dist-tags'][i]
-        }
-      }
+      Utils.normalize_dist_tags(result)
 
       // npm can throw if this field doesn't exist
       result._attachments = {}
@@ -382,10 +373,8 @@ Storage.prototype.get_local = function(callback) {
   var getPackage = function(i) {
     self.local.get_package(locals[i], function(err, info) {
       if (!err) {
-        var latest = Array.isArray(info['dist-tags'].latest)
-                   ? Utils.semver_sort(info['dist-tags'].latest).pop()
-                   : info['dist-tags'].latest
-        if (info.versions[latest]) {
+        var latest = info['dist-tags'].latest
+        if (latest && info.versions[latest]) {
           packages.push(info.versions[latest])
         } else {
           self.logger.warn( { package: locals[i] }
@@ -521,10 +510,12 @@ Storage._merge_versions = function(local, up, config) {
 
   // refresh dist-tags
   for (var i in up['dist-tags']) {
-    var added = Utils.tag_version(local, up['dist-tags'][i], i, config || {})
-    if (i === 'latest' && added) {
-      // if remote has more fresh package, we should borrow its readme
-      local.readme = up.readme
+    if (local['dist-tags'][i] !== up['dist-tags'][i]) {
+      local['dist-tags'][i] = up['dist-tags'][i]
+      if (i === 'latest') {
+        // if remote has more fresh package, we should borrow its readme
+        local.readme = up.readme
+      }
     }
   }
 }
diff --git a/lib/utils.js b/lib/utils.js
index bf558115f..303998c34 100644
--- a/lib/utils.js
+++ b/lib/utils.js
@@ -80,28 +80,19 @@ module.exports.filter_tarball_urls = function(pkg, req, config) {
   return pkg
 }
 
-function can_add_tag(tag, config) {
-  if (!tag) return false
-  if (tag === 'latest' && config.ignore_latest_tag) return false
-  return true
-}
-
-module.exports.tag_version = function(data, version, tag, config) {
-  if (!can_add_tag(tag, config)) return false
-
-  switch (typeof(data['dist-tags'][tag])) {
-    case 'string':
-      data['dist-tags'][tag] = [ data['dist-tags'][tag] ]
-      break
-    case 'object': // array
-      break
-    default:
-      data['dist-tags'][tag] = []
-  }
-  if (data['dist-tags'][tag].indexOf(version) === -1) {
-    data['dist-tags'][tag].push(version)
-    data['dist-tags'][tag] = module.exports.semver_sort(data['dist-tags'][tag])
-    return data['dist-tags'][tag][data['dist-tags'][tag].length - 1] === version
+module.exports.tag_version = function(data, version, tag) {
+  if (tag) {
+    if (data['dist-tags'][tag] !== version) {
+      if (Semver.parse(version, true)) {
+        // valid version - store
+        data['dist-tags'][tag] = version
+        return true
+      }
+    }
+    Logger.logger.warn({ver: version, tag: tag}, 'ignoring bad version @{ver} in @{tag}')
+    if (tag && data['dist-tags'][tag]) {
+      delete data['dist-tags'][tag]
+    }
   }
   return false
 }
@@ -170,3 +161,36 @@ module.exports.semver_sort = function semver_sort(array) {
         .map(String)
 }
 
+// flatten arrays of tags
+module.exports.normalize_dist_tags = function (data) {
+  var sorted
+
+  if (!data['dist-tags'].latest) {
+    // overwrite latest with highest known version based on semver sort
+    sorted = module.exports.semver_sort(Object.keys(data.versions))
+    if (sorted && sorted.length) {
+        data['dist-tags'].latest = sorted.pop()
+    }
+  }
+
+  for (var tag in data['dist-tags']) {
+    if (Array.isArray(data['dist-tags'][tag])) {
+      if (data['dist-tags'][tag].length) {
+        // sort array
+        sorted = module.exports.semver_sort(data['dist-tags'][tag])
+        if (sorted.length) {
+            // use highest version based on semver sort
+            data['dist-tags'][tag] = sorted.pop()
+        }
+
+      } else {
+        delete data['dist-tags'][tag]
+      }
+    } else if (typeof data['dist-tags'][tag] === 'string') {
+      if (!Semver.parse(data['dist-tags'][tag], true)) {
+        // if the version is invalid, delete the dist-tag entry
+        delete data['dist-tags'][tag]
+      }
+    }
+  }
+}
diff --git a/test/unit/st_merge.js b/test/unit/st_merge.js
index d07988fd1..8c98ba453 100644
--- a/test/unit/st_merge.js
+++ b/test/unit/st_merge.js
@@ -20,24 +20,12 @@ describe('Merge', function() {
   it('dist-tags - compat', function() {
     var x = {
       versions: {},
-      'dist-tags': {q:'1.1.1',w:['2.2.2']},
+      'dist-tags': {q:'1.1.1',w:'2.2.2'},
     }
     merge(x, {'dist-tags':{q:'2.2.2',w:'3.3.3',t:'4.4.4'}})
     assert.deepEqual(x, {
       versions: {},
-      'dist-tags': {q:['1.1.1','2.2.2'],w:['2.2.2','3.3.3'],t:['4.4.4']},
-    })
-  })
-
-  it('dist-tags - sort', function() {
-    var x = {
-      versions: {},
-      'dist-tags': {w:['2.2.2','1.1.1','12.2.2','2.2.2-rc2']},
-    }
-    merge(x, {'dist-tags':{w:'3.3.3'}})
-    assert.deepEqual(x, {
-      versions: {},
-      'dist-tags': {w:["1.1.1","2.2.2-rc2","2.2.2","3.3.3","12.2.2"]},
+      'dist-tags': {q:'2.2.2',w:'3.3.3',t:'4.4.4'},
     })
   })
 
diff --git a/test/unit/tag_version.js b/test/unit/tag_version.js
index b8f2f1c46..036881d80 100644
--- a/test/unit/tag_version.js
+++ b/test/unit/tag_version.js
@@ -12,7 +12,7 @@ describe('tag_version', function() {
     assert(tag_version(x, '1.1.1', 'foo', {}))
     assert.deepEqual(x, {
       versions: {},
-      'dist-tags': {foo: ['1.1.1']},
+      'dist-tags': {foo: '1.1.1'},
     })
   })
 
@@ -21,35 +21,24 @@ describe('tag_version', function() {
       versions: {},
       'dist-tags': {foo: '1.1.0'},
     }
-    assert(tag_version(x, '1.1.1', 'foo', {}))
+    assert(tag_version(x, '1.1.1', 'foo'))
     assert.deepEqual(x, {
       versions: {},
-      'dist-tags': {foo: ['1.1.0', '1.1.1']},
+      'dist-tags': {foo: '1.1.1'},
     })
   })
 
   it('add fresh tag', function() {
     var x = {
       versions: {},
-      'dist-tags': {foo: ['1.1.0']},
+      'dist-tags': {foo: '1.1.0'},
     }
-    assert(tag_version(x, '1.1.1', 'foo', {}))
+    assert(tag_version(x, '1.1.1', 'foo'))
     assert.deepEqual(x, {
       versions: {},
-      'dist-tags': {foo: ['1.1.0', '1.1.1']},
+      'dist-tags': {foo: '1.1.1'},
     })
   })
 
-  it('add stale tag', function() {
-    var x = {
-      versions: {},
-      'dist-tags': {foo: ['1.1.2']},
-    }
-    assert(!tag_version(x, '1.1.1', 'foo', {}))
-    assert.deepEqual(x, {
-      versions: {},
-      'dist-tags': {foo: ['1.1.1', '1.1.2']},
-    })
-  })
 })