Fix add-in downgrade issue

In some cases when add-ins are downgraded the add-in database may be left in
an invalid status. This happened because the add-in scanner did not properly
compare the versions of the old and the new add-in, so it did not uninstall
the old add-in when versions were different. This resulted on the old and
new descriptions of the add-in to be kept in the database, and it might
generate add-in dependency errors.

This also happened when upgrading, but since the engine always loads
add-ins with the highest version it doesn't matter if there is one with
lower version also registered.

The solution is to cache information about the old add-in version, and when
comparing ids, use that information.

Added unit tests.

6 files changed, 156 insertions, 21 deletions

diff --git a/Mono.Addins/Mono.Addins.Database/AddinRegistryUpdater.cs b/Mono.Addins/Mono.Addins.Database/AddinRegistryUpdater.cs
index 517a348..760c4e8 100644
--- a/Mono.Addins/Mono.Addins.Database/AddinRegistryUpdater.cs
+++ b/Mono.Addins/Mono.Addins.Database/AddinRegistryUpdater.cs
@@ -25,6 +25,7 @@
 // THE SOFTWARE.
 using System;
 using System.IO;
+using System.Linq;
 
 namespace Mono.Addins.Database
 {
@@ -34,6 +35,10 @@ namespace Mono.Addins.Database
 		AddinScanFolderInfo currentFolderInfo;
 		AddinScanResult scanResult;
 
+		// Folder info object that already existed. It is usually the same as currentFolderInfo, since
+		// folder info objects are reused, but not always.
+		AddinScanFolderInfo oldFolderInfo;
+
 		public AddinRegistryUpdater (AddinDatabase database, AddinScanResult scanResult): base (database)
 		{
 			this.database = database;
@@ -71,6 +76,7 @@ namespace Mono.Addins.Database
 			bool sharedFolder = domain == AddinDatabase.GlobalDomain;
 			bool isNewFolder = folderInfo == null;
 			bool folderHasIndex = dirScanDataIndex != null;
+			bool oldFolderInfoWasSet = false;
 
 			if (isNewFolder) {
 				// No folder info. It is the first time this folder is scanned.
@@ -81,8 +87,14 @@ namespace Mono.Addins.Database
 			} else if (folderInfo.FolderHasScanDataIndex != folderHasIndex) {
 				// A scan data index appeared or disappeared. The information in folderInfo is not reliable.
 				// Update the folder info and regenerate everything.
+
+				// Keep a copy of the old folder info, to be used to retrieve the old status of the add-ins.
+				oldFolderInfo = folderInfo;
+				oldFolderInfoWasSet = true;
+				folderInfo = new AddinScanFolderInfo (oldFolderInfo);
 				scanResult.RegenerateRelationData = true;
 				folderInfo.Reset ();
+
 				scanResult.RegisterModifiedFolderInfo (folderInfo);
 				folderInfo.FolderHasScanDataIndex = folderHasIndex;
 			}
@@ -119,14 +131,44 @@ namespace Mono.Addins.Database
 			if (monitor.LogLevel > 1)
 				monitor.Log ("Checking: " + path);
 
+			currentFolderInfo = folderInfo;
+
 			if (dirScanDataIndex != null) {
 				// Instead of scanning the folder, just register the files in the index
-				foreach (var file in dirScanDataIndex.Files)
-					RegisterFileToScan (monitor, file.FileName, folderInfo, file);
+				if (oldFolderInfo != null && !oldFolderInfo.FolderHasScanDataIndex)
+				{
+					// There was no scan index in the previous scan but there is one in this scan.
+					// The old folder info doesn't contain the info for all files, just for the files in this folder.
+					// Since the new scan has an index, it can contain references to files not directly in this folder,
+					// so for those files we need to find their corresponding old folder info.
+
+					// We group by folder so that we only need to query for folderInfo once per folder
+					foreach (var folder in dirScanDataIndex.Files.GroupBy(f => Path.GetDirectoryName(f.FileName)))
+					{
+						AddinScanFolderInfo oldFolderInfoForIncludedFolder;
+						if (folder.Key != path)
+						{
+							// The file does not belong to this folder, so we need to get the folderInfo from
+							// the right folder
+							database.GetFolderInfoForPath(monitor, folder.Key, out oldFolderInfoForIncludedFolder);
+						}
+						else
+						{
+							// The file belongs to the folder being visited, so oldFolderInfo is correct
+							oldFolderInfoForIncludedFolder = oldFolderInfo;
+						}
+						foreach(var file in folder)
+							RegisterFileToScan(monitor, file.FileName, file, oldFolderInfoForIncludedFolder);
+					}
+				}
+				else
+				{
+					foreach (var file in dirScanDataIndex.Files)
+						RegisterFileToScan(monitor, file.FileName, file, oldFolderInfo);
+				}
 				foreach (var file in dirScanDataIndex.Assemblies)
 					scanResult.AssemblyIndex.AddAssemblyLocation (file);
 			} else {
-				currentFolderInfo = folderInfo;
 
 				base.OnVisitFolder (monitor, path, domain, recursive);
 
@@ -136,23 +178,26 @@ namespace Mono.Addins.Database
 					scanResult.ChangesFound = true;
 					if (scanResult.CheckOnly)
 						return;
-					database.DeleteFolderInfo (monitor, folderInfo);
+					database.DeleteFolderInfo (monitor, oldFolderInfo ?? currentFolderInfo);
 				}
 			}
 
 			// Look for deleted add-ins.
 
-			UpdateDeletedAddins (monitor, folderInfo);
+			UpdateDeletedAddins (monitor, oldFolderInfo ?? currentFolderInfo);
+
+			if (oldFolderInfoWasSet)
+				oldFolderInfo = null;
 		}
 
 		protected override void OnVisitAddinManifestFile (IProgressStatus monitor, string file)
 		{
-			RegisterFileToScan (monitor, file, currentFolderInfo, null);
+			RegisterFileToScan (monitor, file, null, oldFolderInfo);
 		}
 
 		protected override void OnVisitAssemblyFile (IProgressStatus monitor, string file)
 		{
-			RegisterFileToScan (monitor, file, currentFolderInfo, null);
+			RegisterFileToScan (monitor, file, null, oldFolderInfo);
 			scanResult.AssemblyIndex.AddAssemblyLocation (file);
 		}
 
@@ -172,17 +217,17 @@ namespace Mono.Addins.Database
 			}
 		}
 
-		void RegisterFileToScan (IProgressStatus monitor, string file, AddinScanFolderInfo folderInfo, AddinScanData scanData)
+		void RegisterFileToScan (IProgressStatus monitor, string file, AddinScanData scanData, AddinScanFolderInfo oldFolderInfo)
 		{
-			AddinFileInfo finfo = folderInfo.GetAddinFileInfo (file);
+			AddinFileInfo finfo = (oldFolderInfo ?? currentFolderInfo).GetAddinFileInfo (file);
 			bool added = false;
 
-			if (finfo != null && (!finfo.IsAddin || finfo.Domain == folderInfo.GetDomain (finfo.IsRoot)) && !finfo.HasChanged (FileSystem, scanData?.MD5) && !scanResult.RegenerateAllData) {
+			if (finfo != null && (!finfo.IsAddin || finfo.Domain == currentFolderInfo.GetDomain (finfo.IsRoot)) && !finfo.HasChanged (FileSystem, scanData?.MD5) && !scanResult.RegenerateAllData) {
 				if (finfo.ScanError) {
 					// Always schedule the file for scan if there was an error in a previous scan.
 					// However, don't set ChangesFound=true, in this way if there isn't any other
 					// change in the registry, the file won't be scanned again.
-					scanResult.AddFileToScan (file, folderInfo, scanData);
+					scanResult.AddFileToScan (file, currentFolderInfo, finfo, scanData);
 					added = true;
 				}
 
@@ -201,7 +246,7 @@ namespace Mono.Addins.Database
 			scanResult.ChangesFound = true;
 
 			if (!scanResult.CheckOnly && !added)
-				scanResult.AddFileToScan (file, folderInfo, scanData);
+				scanResult.AddFileToScan (file, currentFolderInfo, finfo, scanData);
 		}
 	}
 }
diff --git a/Mono.Addins/Mono.Addins.Database/AddinScanFolderInfo.cs b/Mono.Addins/Mono.Addins.Database/AddinScanFolderInfo.cs
index 944922d..a3a8900 100644
--- a/Mono.Addins/Mono.Addins.Database/AddinScanFolderInfo.cs
+++ b/Mono.Addins/Mono.Addins.Database/AddinScanFolderInfo.cs
@@ -58,6 +58,16 @@ namespace Mono.Addins.Database
 			this.folder = folder;
 		}
 		
+		public AddinScanFolderInfo (AddinScanFolderInfo other)
+		{
+			files = new Hashtable (other.files);
+			folder = other.folder;
+			fileName = other.fileName;
+			domain = other.domain;
+			sharedFolder = other.sharedFolder;
+			FolderHasScanDataIndex = other.FolderHasScanDataIndex;
+		}
+		
 		public string FileName {
 			get { return fileName; }
 		}
@@ -269,8 +279,13 @@ namespace Mono.Addins.Database
 
 		public bool HasChanged (AddinFileSystemExtension fs, string md5)
 		{
-			if (md5 != null && ScanDataMD5 != null)
+			// Special case: if an md5 is stored, this method can only return a valid result
+			// if compared with another md5. If no md5 is provided for comparison, then always consider
+			// the file to be changed.
+
+			if (ScanDataMD5 != null)
 				return md5 != ScanDataMD5;
+
 			return fs.GetLastWriteTime (File) != LastScan;
 		}
 		
diff --git a/Mono.Addins/Mono.Addins.Database/AddinScanResult.cs b/Mono.Addins/Mono.Addins.Database/AddinScanResult.cs
index 85b02e2..1917ce3 100644
--- a/Mono.Addins/Mono.Addins.Database/AddinScanResult.cs
+++ b/Mono.Addins/Mono.Addins.Database/AddinScanResult.cs
@@ -86,11 +86,12 @@ namespace Mono.Addins.Database
 				RemovedAddins.Add (addinId);
 		}
 		
-		public void AddFileToScan (string file, AddinScanFolderInfo folderInfo, AddinScanData scanData)
+		public void AddFileToScan (string file, AddinScanFolderInfo folderInfo, AddinFileInfo oldFileInfo, AddinScanData scanData)
 		{
 			FileToScan di = new FileToScan ();
 			di.File = file;
 			di.AddinScanFolderInfo = folderInfo;
+			di.OldFileInfo = oldFileInfo;
 			di.ScanDataMD5 = scanData?.MD5;
 			FilesToScan.Add (di);
 			RegisterModifiedFolderInfo (folderInfo);
@@ -119,6 +120,7 @@ namespace Mono.Addins.Database
 	{
 		public string File;
 		public AddinScanFolderInfo AddinScanFolderInfo;
+		public AddinFileInfo OldFileInfo;
 		public string ScanDataMD5;
 	}
 
diff --git a/Mono.Addins/Mono.Addins.Database/AddinScanner.cs b/Mono.Addins/Mono.Addins.Database/AddinScanner.cs
index bab121f..3c389c4 100644
--- a/Mono.Addins/Mono.Addins.Database/AddinScanner.cs
+++ b/Mono.Addins/Mono.Addins.Database/AddinScanner.cs
@@ -188,7 +188,7 @@ namespace Mono.Addins.Database
 					
 					if (config.LocalId.Length == 0) {
 						// Generate an internal id for this add-in
-						config.LocalId = database.GetUniqueAddinId (file, (fi != null ? fi.AddinId : null), config.Namespace, config.Version);
+						config.LocalId = database.GetUniqueAddinId (file, fi?.AddinId, config.Namespace, config.Version);
 						config.HasUserId = false;
 					}
 					
@@ -232,11 +232,11 @@ namespace Mono.Addins.Database
 					
 					// If the scanned file results in an add-in version different from the one obtained from
 					// previous scans, the old add-in needs to be uninstalled.
-					if (fi != null && fi.IsAddin && fi.AddinId != config.AddinId) {
-						database.UninstallAddin (monitor, folderInfo.Domain, fi.AddinId, fi.File, scanResult);
+					if (fileToScan.OldFileInfo != null && fileToScan.OldFileInfo.IsAddin && fileToScan.OldFileInfo.AddinId != config.AddinId) {
+						database.UninstallAddin (monitor, folderInfo.Domain, fileToScan.OldFileInfo.AddinId, fileToScan.OldFileInfo.File, scanResult);
 						
 						// If the add-in version has changed, regenerate everything again since old data can't be reused
-						if (Addin.GetIdName (fi.AddinId) == Addin.GetIdName (config.AddinId))
+						if (Addin.GetIdName (fileToScan.OldFileInfo.AddinId) == Addin.GetIdName (config.AddinId))
 							scanResult.RegenerateRelationData = true;
 					}
 					
diff --git a/Mono.Addins/Mono.Addins.Description/ModuleDescription.cs b/Mono.Addins/Mono.Addins.Description/ModuleDescription.cs
index dfbd169..6184449 100644
--- a/Mono.Addins/Mono.Addins.Description/ModuleDescription.cs
+++ b/Mono.Addins/Mono.Addins.Description/ModuleDescription.cs
@@ -391,7 +391,7 @@ namespace Mono.Addins.Description
 			writer.WriteValue ("DataFiles", NormalizePaths (DataFiles));
 			writer.WriteValue ("Dependencies", Dependencies);
 			writer.WriteValue ("Extensions", Extensions);
-			writer.WriteValue ("IgnorePaths", NormalizePaths (ignorePaths));
+			writer.WriteValue ("IgnorePaths", NormalizePaths (IgnorePaths));
 		}
 		
 		internal override void Read (BinaryXmlReader reader)
diff --git a/Test/UnitTests/TestScanDataFileGeneration.cs b/Test/UnitTests/TestScanDataFileGeneration.cs
index bb5c713..8fb64a7 100644
--- a/Test/UnitTests/TestScanDataFileGeneration.cs
+++ b/Test/UnitTests/TestScanDataFileGeneration.cs
@@ -103,8 +103,6 @@ namespace UnitTests
 			Assert.Contains ("SimpleApp.Ext2,0.1.0", addins);
 			Assert.Contains ("SimpleApp.Ext3,0.1.0", addins);
 			Assert.Contains ("SimpleApp.Ext4,0.1.0", addins);
-
-			AddinEngine engine = new AddinEngine ();
 		}
 
 		[Test]
@@ -233,6 +231,81 @@ namespace UnitTests
 
 			engine.Shutdown ();
 		}
+
+		[Test]
+		[TestCase (true, true, TestName = "DowngradeAddins - with scan data")]
+		[TestCase (true, false, TestName = "DowngradeAddins - from scan to no scan data")]
+		[TestCase (false, true, TestName = "DowngradeAddins - from no scan to scan data")]
+		[TestCase (false, false, TestName = "DowngradeAddins - with no scan data")]
+		public void DowngradeAddins (bool hasScaIndexBefore, bool hasScanIndexAfter)
+		{
+			// Tests that the database is properly updated when add-ins are downgraded.
+
+			var dir = Util.GetSampleDirectory ("ScanDataFilesTest");
+
+			AddinRegistry registry;
+
+			if (hasScaIndexBefore) {
+				// Generate the scan data files before initializing the engine
+				registry = GetRegistry (dir);
+				registry.GenerateAddinScanDataFiles (new ConsoleProgressStatus (true), recursive: true);
+				registry.Dispose ();
+			}
+
+			AddinEngine engine = new AddinEngine ();
+			engine.Initialize (Path.Combine (dir, "Config"), Path.Combine (dir, "UserAddins"), null, Path.Combine (dir, "App"));
+			registry = engine.Registry;
+
+			var addins = registry.GetAddins ().Select (a => a.Id).ToArray ();
+			Assert.AreEqual (4, addins.Length);
+			Assert.Contains ("SimpleApp.Ext1,0.1.0", addins);
+			Assert.Contains ("SimpleApp.Ext2,0.1.0", addins);
+			Assert.Contains ("SimpleApp.Ext3,0.1.0", addins);
+			Assert.Contains ("SimpleApp.Ext4,0.1.0", addins);
+			engine.Shutdown ();
+
+			// Downgrade add-ins
+
+			SetAddinVersions (dir, "0.1.0", "0.0.1");
+
+			if (hasScanIndexAfter) {
+				// Regenerate the data files
+				registry = GetRegistry (dir);
+				registry.GenerateAddinScanDataFiles (new ConsoleProgressStatus (true), recursive: true);
+				registry.Dispose ();
+			} else {
+				CleanAddinData (dir);
+			}
+
+			engine = new AddinEngine ();
+			engine.Initialize (Path.Combine (dir, "Config"), Path.Combine (dir, "UserAddins"), null, Path.Combine (dir, "App"));
+			registry = engine.Registry;
+			registry.Update ();
+
+			addins = registry.GetAddins ().Select (a => a.Id).ToArray ();
+			Assert.AreEqual (4, addins.Length);
+			Assert.Contains ("SimpleApp.Ext1,0.0.1", addins);
+			Assert.Contains ("SimpleApp.Ext2,0.0.1", addins);
+			Assert.Contains ("SimpleApp.Ext3,0.0.1", addins);
+			Assert.Contains ("SimpleApp.Ext4,0.0.1", addins);
+			engine.Shutdown ();
+		}
+
+		void SetAddinVersions (string path, string oldVersion, string newVersion)
+		{
+			foreach (var file in Directory.GetFiles (path, "*.xml"))
+				File.WriteAllText (file, File.ReadAllText (file).Replace (oldVersion, newVersion));
+			foreach (var dir in Directory.GetDirectories (path))
+				SetAddinVersions (dir, oldVersion, newVersion);
+		}
+
+		void CleanAddinData (string path)
+		{
+			foreach (var file in Directory.GetFiles (path, "*.addindata"))
+				File.Delete (file);
+			foreach (var dir in Directory.GetDirectories (path))
+				CleanAddinData (dir);
+		}
 	}
 
 	class FileList: MarshalByRefObject