Preserve plugin metadata insertion order

cpp/src/tests/api/interface/database_interface_test.h

@@ -964,31 +964,17 @@ TEST_P(DatabaseInterfaceTest,
 
   // Plugin entries are unordered.
   std::stringstream expectedContent;
-  if (content.find(blankDifferentEsm) < content.find(blankEsm)) {
-    expectedContent << "plugins:" << endl
-                    << "  - name: '" << blankDifferentEsm << "'" << endl
-                    << "    dirty:" << endl
-                    << "      - crc: 0x7D22F9DF" << endl
-                    << "        util: 'TES4Edit'" << endl
-                    << "        udr: 4" << endl
-                    << "  - name: '" << blankEsm << "'" << endl
-                    << "    tag:" << endl
-                    << "      - Actors.ACBS" << endl
-                    << "      - Actors.AIData" << endl
-                    << "      - -C.Water";
-  } else {
-    expectedContent << "plugins:" << endl
-                    << "  - name: '" << blankEsm << "'" << endl
-                    << "    tag:" << endl
-                    << "      - Actors.ACBS" << endl
-                    << "      - Actors.AIData" << endl
-                    << "      - -C.Water" << endl
-                    << "  - name: '" << blankDifferentEsm << "'" << endl
-                    << "    dirty:" << endl
-                    << "      - crc: 0x7D22F9DF" << endl
-                    << "        util: 'TES4Edit'" << endl
-                    << "        udr: 4";
-  }
+  expectedContent << "plugins:" << endl
+                  << "  - name: '" << blankEsm << "'" << endl
+                  << "    tag:" << endl
+                  << "      - Actors.ACBS" << endl
+                  << "      - Actors.AIData" << endl
+                  << "      - -C.Water" << endl
+                  << "  - name: '" << blankDifferentEsm << "'" << endl
+                  << "    dirty:" << endl
+                  << "      - crc: 0x7D22F9DF" << endl
+                  << "        util: 'TES4Edit'" << endl
+                  << "        udr: 4";
 
   EXPECT_EQ(expectedContent.str(), content);
 }

src/database/mod.rs

@@ -131,7 +131,7 @@ impl Database {
 
         let mut doc = MetadataDocument::default();
 
-        for plugin in self.masterlist.plugins_iter() {
+        for plugin in self.masterlist.ordered_plugins_iter() {
             let mut minimal_plugin = PluginMetadata::with_same_name(plugin);
             minimal_plugin.set_tags(plugin.tags().to_vec());
             minimal_plugin.set_dirty_info(plugin.dirty_info().to_vec());
@@ -725,7 +725,7 @@ plugins:
         use super::*;
 
         #[test]
-        fn should_only_write_plugin_bash_tags_and_dirty_info() {
+        fn should_write_plugins_in_insertion_order_with_only_bash_tags_and_dirty_info() {
             let fixture = Fixture::new(GameType::Oblivion);
             let mut database = fixture.database();
             let output_path = fixture.inner.local_path.join("minimal.yaml");
@@ -741,20 +741,7 @@ plugins:
             let content = std::fs::read_to_string(output_path).unwrap();
 
             // Plugin entries are unordered.
-            let expected_content = if content.find(BLANK_DIFFERENT_ESM) < content.find(BLANK_ESM) {
-                "plugins:
-  - name: 'Blank - Different.esm'
-    dirty:
-      - crc: 0x7D22F9DF
-        util: 'TES4Edit'
-        udr: 4
-  - name: 'Blank.esm'
-    tag:
-      - Actors.ACBS
-      - Actors.AIData
-      - -C.Water"
-            } else {
-                "plugins:
+            let expected_content = "plugins:
   - name: 'Blank.esm'
     tag:
       - Actors.ACBS
@@ -764,8 +751,7 @@ plugins:
     dirty:
       - crc: 0x7D22F9DF
         util: 'TES4Edit'
-        udr: 4"
-            };
+        udr: 4";
 
             assert_eq!(expected_content, content);
         }

src/metadata/metadata_document.rs

@@ -1,6 +1,7 @@
 use std::{
     collections::{HashMap, HashSet},
     path::Path,
+    sync::Arc,
 };
 
 use saphyr::{LoadableYamlNode, MarkedYaml, YamlData};
@@ -29,8 +30,9 @@ pub(crate) struct MetadataDocument {
     bash_tags: Vec<String>,
     groups: Vec<Group>,
     messages: Vec<Message>,
-    plugins: HashMap<Filename, PluginMetadata>,
+    plugins: HashMap<Arc<Filename>, PluginMetadata>,
     regex_plugins: Vec<PluginMetadata>,
+    ordered_plugin_names: Vec<Arc<Filename>>,
 }
 
 impl MetadataDocument {
@@ -142,23 +144,25 @@ impl MetadataDocument {
             .into());
         };
 
-        let mut plugins: HashMap<Filename, PluginMetadata> = HashMap::new();
-        let mut regex_plugins: Vec<PluginMetadata> = Vec::new();
+        let mut plugins = HashMap::new();
+        let mut regex_plugins = Vec::new();
+        let mut ordered_plugin_names = Vec::new();
         for plugin_yaml in get_slice_value(&doc, "plugins", YamlObjectType::MetadataDocument)? {
             let plugin = PluginMetadata::try_from_yaml(plugin_yaml)?;
+            let filename = Arc::new(Filename::new(plugin.name().to_owned()));
+
             if plugin.is_regex_plugin() {
                 regex_plugins.push(plugin);
-            } else {
-                let filename = Filename::new(plugin.name().to_owned());
-                if let Some(old) = plugins.insert(filename, plugin) {
-                    return Err(ParseMetadataError::duplicate_entry(
-                        plugin_yaml.span.start,
-                        old.name().to_owned(),
-                        YamlObjectType::PluginMetadata,
-                    )
-                    .into());
-                }
+            } else if let Some(old) = plugins.insert(Arc::clone(&filename), plugin) {
+                return Err(ParseMetadataError::duplicate_entry(
+                    plugin_yaml.span.start,
+                    old.name().to_owned(),
+                    YamlObjectType::PluginMetadata,
+                )
+                .into());
             }
+
+            ordered_plugin_names.push(filename);
         }
 
         let messages = get_slice_value(&doc, "globals", YamlObjectType::MetadataDocument)?
@@ -208,6 +212,7 @@ impl MetadataDocument {
 
         self.plugins = plugins;
         self.regex_plugins = regex_plugins;
+        self.ordered_plugin_names = ordered_plugin_names;
         self.messages = messages;
         self.bash_tags = bash_tags;
         self.groups = groups;
@@ -249,7 +254,7 @@ impl MetadataDocument {
 
             emitter.begin_array();
 
-            for plugin in self.plugins_iter() {
+            for plugin in self.ordered_plugins_iter() {
                 if !plugin.has_name_only() {
                     plugin.emit_yaml(&mut emitter);
                 }
@@ -283,8 +288,14 @@ impl MetadataDocument {
         &self.messages
     }
 
-    pub(crate) fn plugins_iter(&self) -> impl Iterator<Item = &PluginMetadata> {
-        self.plugins.values().chain(self.regex_plugins.iter())
+    pub(crate) fn ordered_plugins_iter(&self) -> impl Iterator<Item = &PluginMetadata> {
+        self.ordered_plugin_names.iter().filter_map(|f| {
+            self.plugins.get(f).or_else(|| {
+                self.regex_plugins
+                    .iter()
+                    .find(|r| r.name() == f.as_ref().as_str())
+            })
+        })
     }
 
     pub(crate) fn find_plugin(
@@ -332,24 +343,38 @@ impl MetadataDocument {
     }
 
     pub(crate) fn set_plugin_metadata(&mut self, plugin_metadata: PluginMetadata) {
+        let filename = Arc::new(Filename::new(plugin_metadata.name().to_owned()));
+
         if plugin_metadata.is_regex_plugin() {
             self.regex_plugins.push(plugin_metadata);
+            self.ordered_plugin_names.push(filename);
         } else {
-            self.plugins.insert(
-                Filename::new(plugin_metadata.name().to_owned()),
-                plugin_metadata,
-            );
+            let old_value = self.plugins.insert(Arc::clone(&filename), plugin_metadata);
+            if old_value.is_none() {
+                self.ordered_plugin_names.push(filename);
+            }
         }
     }
 
     pub(crate) fn remove_plugin_metadata(&mut self, plugin_name: &str) {
-        let removed = self.plugins.remove(&Filename::new(plugin_name.to_owned()));
+        let filename = Filename::new(plugin_name.to_owned());
+        let mut was_removed = self.plugins.remove(&filename).is_some();
 
         // Only remove regex plugins if no specific plugin was removed, because
         // they're mutually exclusive.
-        if removed.is_none() {
-            self.regex_plugins
-                .retain(|p| !unicase::eq(p.name(), plugin_name));
+        if !was_removed {
+            self.regex_plugins.retain(|p| {
+                let equal = unicase::eq(p.name(), plugin_name);
+                if equal {
+                    was_removed = true;
+                }
+                !equal
+            });
+        }
+
+        if was_removed {
+            self.ordered_plugin_names
+                .retain(|f| f.as_ref() != &filename);
         }
     }
 
@@ -359,6 +384,7 @@ impl MetadataDocument {
         self.messages.clear();
         self.plugins.clear();
         self.regex_plugins.clear();
+        self.ordered_plugin_names.clear();
     }
 }
 
@@ -370,6 +396,7 @@ impl std::default::Default for MetadataDocument {
             messages: Vec::default(),
             plugins: HashMap::default(),
             regex_plugins: Vec::default(),
+            ordered_plugin_names: Vec::default(),
         }
     }
 }
@@ -649,14 +676,17 @@ plugins:
             let mut metadata_list = MetadataDocument::default();
             metadata_list.load(&path).unwrap();
 
-            let plugin_names: Vec<_> = metadata_list
-                .plugins_iter()
-                .map(PluginMetadata::name)
-                .collect();
-            assert!(plugin_names.contains(&"Blank.esm"));
-            assert!(plugin_names.contains(&"Blank.esp"));
-            assert!(plugin_names.contains(&"Blank.+\\.esp"));
-            assert!(plugin_names.contains(&"Blank.+(Different)?.*\\.esp"));
+            let mut plugin_names_iter = metadata_list
+                .ordered_plugins_iter()
+                .map(PluginMetadata::name);
+            assert_eq!("Blank.esm", plugin_names_iter.next().unwrap());
+            assert_eq!("Blank.+\\.esp", plugin_names_iter.next().unwrap());
+            assert_eq!(
+                "Blank.+(Different)?.*\\.esp",
+                plugin_names_iter.next().unwrap()
+            );
+            assert_eq!("Blank.esp", plugin_names_iter.next().unwrap());
+            assert_eq!(None, plugin_names_iter.next());
 
             assert_eq!(&["C.Climate", "Relev"], metadata_list.bash_tags());
 
@@ -1003,14 +1033,18 @@ plugins:
             metadata.load_from_str(METADATA_LIST_YAML).unwrap();
 
             assert!(!metadata.messages().is_empty());
-            assert!(metadata.plugins_iter().next().is_some());
             assert!(!metadata.bash_tags().is_empty());
+            assert!(!metadata.plugins.is_empty());
+            assert!(!metadata.regex_plugins.is_empty());
+            assert!(!metadata.ordered_plugin_names.is_empty());
 
             metadata.clear();
 
             assert!(metadata.messages().is_empty());
-            assert!(metadata.plugins_iter().next().is_none());
             assert!(metadata.bash_tags().is_empty());
+            assert!(metadata.plugins.is_empty());
+            assert!(metadata.regex_plugins.is_empty());
+            assert!(metadata.ordered_plugin_names.is_empty());
         }
 
         #[test]
@@ -1049,7 +1083,7 @@ plugins:
         }
 
         #[test]
-        fn add_plugin_should_store_specific_plugin_metadata() {
+        fn set_plugin_metadata_should_store_specific_plugin_metadata() {
             let mut metadata = MetadataDocument::default();
 
             let name = "Blank.esp";
@@ -1064,7 +1098,33 @@ plugins:
         }
 
         #[test]
-        fn add_plugin_should_store_given_regex_plugin_metadata() {
+        fn set_plugin_metadata_should_append_the_specific_plugin_filename_to_ordered_plugin_names_if_it_is_new()
+         {
+            let mut metadata = MetadataDocument::default();
+            metadata.load_from_str(METADATA_LIST_YAML).unwrap();
+
+            let name = "Blank - Other.esp";
+            metadata.set_plugin_metadata(PluginMetadata::new(name).unwrap());
+
+            assert_eq!(name, metadata.ordered_plugin_names.last().unwrap().as_str());
+        }
+
+        #[test]
+        fn set_plugin_metadata_should_not_append_the_specific_plugin_filename_to_ordered_plugin_names_if_it_is_not_new()
+         {
+            let mut metadata = MetadataDocument::default();
+            metadata.load_from_str(METADATA_LIST_YAML).unwrap();
+
+            let ordered_plugin_names = metadata.ordered_plugin_names.clone();
+
+            let name = "Blank.esp";
+            metadata.set_plugin_metadata(PluginMetadata::new(name).unwrap());
+
+            assert_eq!(ordered_plugin_names, metadata.ordered_plugin_names);
+        }
+
+        #[test]
+        fn set_plugin_metadata_should_store_given_regex_plugin_metadata() {
             let mut metadata = MetadataDocument::default();
 
             let mut plugin = PluginMetadata::new(".+Dependent\\.esp").unwrap();
@@ -1078,6 +1138,17 @@ plugins:
             assert_eq!("group1", plugin.group().unwrap());
         }
 
+        #[test]
+        fn set_plugin_metadata_should_append_the_regex_plugin_filename_to_ordered_plugin_names() {
+            let mut metadata = MetadataDocument::default();
+            metadata.load_from_str(METADATA_LIST_YAML).unwrap();
+
+            let name = ".+Dependent\\.esp";
+            metadata.set_plugin_metadata(PluginMetadata::new(name).unwrap());
+
+            assert_eq!(name, metadata.ordered_plugin_names.last().unwrap().as_str());
+        }
+
         #[test]
         fn remove_plugin_metadata_should_remove_the_given_plugin_specific_metadata() {
             let mut metadata = MetadataDocument::default();
@@ -1089,6 +1160,12 @@ plugins:
             metadata.remove_plugin_metadata(name);
 
             assert!(metadata.find_plugin(name).unwrap().is_none());
+
+            assert!(
+                !metadata
+                    .ordered_plugin_names
+                    .contains(&Arc::new(Filename::new(name.to_owned())))
+            );
         }
 
         #[test]
@@ -1112,6 +1189,12 @@ plugins:
             metadata.remove_plugin_metadata(regex_name);
 
             assert!(metadata.find_plugin(name).unwrap().is_none());
+
+            assert!(
+                !metadata
+                    .ordered_plugin_names
+                    .contains(&Arc::new(Filename::new(name.to_owned())))
+            );
         }
 
         #[test]
@@ -1135,6 +1218,12 @@ plugins:
             metadata.remove_plugin_metadata("blank.*\\.esp");
 
             assert!(metadata.find_plugin(name).unwrap().is_none());
+
+            assert!(
+                !metadata
+                    .ordered_plugin_names
+                    .contains(&Arc::new(Filename::new(name.to_owned())))
+            );
         }
 
         #[test]
@@ -1145,13 +1234,15 @@ plugins:
             let name = "Blank.+\\.esp";
             assert!(metadata.find_plugin(name).unwrap().is_some());
 
-            metadata.remove_plugin_metadata(name);
-
-            assert!(metadata.find_plugin(name).unwrap().is_some());
-
             metadata.remove_plugin_metadata("Blank - Different.esp");
 
             assert!(metadata.find_plugin(name).unwrap().is_some());
+
+            assert!(
+                metadata
+                    .ordered_plugin_names
+                    .contains(&Arc::new(Filename::new(name.to_owned())))
+            );
         }
     }