VorbisComments: Verify keys prior to insertion

CHANGELOG.md

@@ -9,11 +9,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 ### Added
 - **Properties**: Expose channel mask (only supported for WAV and MPEG for now) ([PR](https://github.com/Serial-ATA/lofty-rs/pull/155))
 - **ItemKey**: `InitialKey` mapping for Vorbis Comments ([PR](https://github.com/Serial-ATA/lofty-rs/pull/156))
+- **VorbisComments**: `VorbisComments::push` to allow for a non-replacing insertion
 
 ### Changed
 - **APE**/**ID3v1**/**ID3v2**/**Tag**:
   - Allow empty strings as values instead of removing the corresponding item when empty ([PR](https://github.com/Serial-ATA/lofty-rs/pull/134))
   - Separated the trait `SplitAndMergeTag` into `SplitTag` and `MergeTag` to prevent any unexpected or undefined behavior at runtime ([#143](https://github.com/Serial-ATA/lofty-rs/pull/143))
+- **VorbisComments**:
+  - Keys will now be verified according to spec before insertion
+  - Getters will now case-insensitively search for keys
+  - `TRACKNUM` will now be considered in the `Accessor::*track` methods
 
 ### Fixed
 - **ID3v2**:

src/ogg/tag.rs

@@ -26,7 +26,7 @@ macro_rules! impl_accessor {
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {
-					self.insert(String::from($key), value, true)
+					self.insert(String::from($key), value)
 				}
 
 				fn [<remove_ $name>](&mut self) {
@@ -78,19 +78,19 @@ impl VorbisComments {
 	}
 
 	/// Gets an item by key
-	///
-	/// NOTE: This is case-sensitive
 	pub fn get(&self, key: &str) -> Option<&str> {
+		if !verify_key(key) {
+			return None;
+		}
+
 		self.items
 			.iter()
-			.find(|(k, _)| k == key)
+			.find(|(k, _)| k.eq_ignore_ascii_case(key))
 			.map(|(_, v)| v.as_str())
 	}
 
 	/// Gets all items with the key
 	///
-	/// NOTE: This is case-sensitive
-	///
 	/// # Examples
 	///
 	/// ```rust
@@ -99,39 +99,77 @@ impl VorbisComments {
 	/// let mut vorbis_comments = VorbisComments::default();
 	///
 	/// // Vorbis comments allows multiple fields with the same key, such as artist
-	/// vorbis_comments.insert(String::from("ARTIST"), String::from("Foo artist"), false);
-	/// vorbis_comments.insert(String::from("ARTIST"), String::from("Bar artist"), false);
-	/// vorbis_comments.insert(String::from("ARTIST"), String::from("Baz artist"), false);
+	/// vorbis_comments.push(String::from("ARTIST"), String::from("Foo artist"));
+	/// vorbis_comments.push(String::from("ARTIST"), String::from("Bar artist"));
+	/// vorbis_comments.push(String::from("ARTIST"), String::from("Baz artist"));
 	///
 	/// let all_artists = vorbis_comments.get_all("ARTIST").collect::<Vec<&str>>();
 	/// assert_eq!(all_artists, vec!["Foo artist", "Bar artist", "Baz artist"]);
 	/// ```
 	pub fn get_all<'a>(&'a self, key: &'a str) -> impl Iterator<Item = &'a str> + Clone + '_ {
 		self.items
 			.iter()
-			.filter_map(move |(k, v)| (k == key).then_some(v.as_str()))
+			.filter_map(move |(k, v)| (k.eq_ignore_ascii_case(key)).then_some(v.as_str()))
 	}
 
 	/// Inserts an item
 	///
-	/// If `replace_all` is true, it will remove all items with the key before insertion
-	pub fn insert(&mut self, key: String, value: String, replace_all: bool) {
-		if replace_all {
-			self.items.retain(|(k, _)| k != &key);
+	/// This is the same as [`VorbisComments::push`], except it will remove any items with the same key.
+	///
+	/// NOTE: This will do nothing if the key is invalid. This specification is available [here](https://xiph.org/vorbis/doc/v-comment.html#vectorformat).
+	///
+	/// ```rust
+	/// use lofty::ogg::VorbisComments;
+	///
+	/// let mut tag = VorbisComments::default();
+	/// tag.insert(String::from("TITLE"), String::from("Title 1"));
+	/// tag.insert(String::from("TITLE"), String::from("Title 2"));
+	///
+	/// // We only retain the last title inserted
+	/// let mut titles = tag.get_all("TITLE");
+	/// assert_eq!(titles.next(), Some("Title 2"));
+	/// assert_eq!(titles.next(), None);
+	/// ```
+	pub fn insert(&mut self, key: String, value: String) {
+		if !verify_key(&key) {
+			return;
 		}
 
+		self.items.retain(|(k, _)| !k.eq_ignore_ascii_case(&key));
 		self.items.push((key, value))
 	}
 
-	/// Removes all items with a key, returning an iterator
+	/// Appends an item
+	///
+	/// NOTE: This will do nothing if the key is invalid. This specification is available [here](https://xiph.org/vorbis/doc/v-comment.html#vectorformat).
+	///
+	/// ```rust
+	/// use lofty::ogg::VorbisComments;
+	///
+	/// let mut tag = VorbisComments::default();
+	/// tag.push(String::from("TITLE"), String::from("Title 1"));
+	/// tag.push(String::from("TITLE"), String::from("Title 2"));
 	///
-	/// NOTE: This is case-sensitive
+	/// // We retain both titles
+	/// let mut titles = tag.get_all("TITLE");
+	/// assert_eq!(titles.next(), Some("Title 1"));
+	/// assert_eq!(titles.next(), Some("Title 2"));
+	/// ```
+	pub fn push(&mut self, key: String, value: String) {
+		if !verify_key(&key) {
+			return;
+		}
+
+		self.items.push((key, value))
+	}
+
+	/// Removes all items with a key, returning an iterator
 	pub fn remove(&mut self, key: &str) -> impl Iterator<Item = String> + '_ {
 		// TODO: drain_filter
 		let mut split_idx = 0_usize;
 
 		for read_idx in 0..self.items.len() {
-			if self.items[read_idx].0 == key {
+			if self.items[read_idx].0.eq_ignore_ascii_case(key) {
 				self.items.swap(split_idx, read_idx);
 				split_idx += 1;
 			}
@@ -141,6 +179,17 @@ impl VorbisComments {
 	}
 }
 
+// A case-insensitive field name that may consist of ASCII 0x20 through 0x7D, 0x3D ('=') excluded.
+// ASCII 0x41 through 0x5A inclusive (A-Z) is to be considered equivalent to ASCII 0x61 through 0x7A inclusive (a-z).
+fn verify_key(key: &str) -> bool {
+	if key.is_empty() {
+		return false;
+	}
+
+	key.bytes()
+		.all(|byte| (0x20..=0x7D).contains(&byte) && byte != 0x3D)
+}
+
 impl OggPictureStorage for VorbisComments {
 	fn pictures(&self) -> &[(Picture, PictureInformation)] {
 		&self.pictures
@@ -157,19 +206,24 @@ impl Accessor for VorbisComments {
 	);
 
 	fn track(&self) -> Option<u32> {
-		if let Some(item) = self.get("TRACKNUMBER") {
+		if let Some(item) = self
+			.get("TRACKNUMBER")
+			.map_or_else(|| self.get("TRACKNUM"), Some)
+		{
 			return item.parse::<u32>().ok();
 		}
 
 		None
 	}
 
 	fn set_track(&mut self, value: u32) {
-		self.insert(String::from("TRACKNUMBER"), value.to_string(), true);
+		self.remove_track();
+		self.insert(String::from("TRACKNUMBER"), value.to_string());
 	}
 
 	fn remove_track(&mut self) {
 		let _ = self.remove("TRACKNUMBER");
+		let _ = self.remove("TRACKNUM");
 	}
 
 	fn track_total(&self) -> Option<u32> {
@@ -184,7 +238,7 @@ impl Accessor for VorbisComments {
 	}
 
 	fn set_track_total(&mut self, value: u32) {
-		self.insert(String::from("TRACKTOTAL"), value.to_string(), true);
+		self.insert(String::from("TRACKTOTAL"), value.to_string());
 		let _ = self.remove("TOTALTRACKS");
 	}
 
@@ -202,7 +256,7 @@ impl Accessor for VorbisComments {
 	}
 
 	fn set_disk(&mut self, value: u32) {
-		self.insert(String::from("DISCNUMBER"), value.to_string(), true);
+		self.insert(String::from("DISCNUMBER"), value.to_string());
 	}
 
 	fn remove_disk(&mut self) {
@@ -221,7 +275,7 @@ impl Accessor for VorbisComments {
 	}
 
 	fn set_disk_total(&mut self, value: u32) {
-		self.insert(String::from("DISCTOTAL"), value.to_string(), true);
+		self.insert(String::from("DISCTOTAL"), value.to_string());
 		let _ = self.remove("TOTALDISCS");
 	}
 
@@ -241,7 +295,7 @@ impl Accessor for VorbisComments {
 	fn set_year(&mut self, value: u32) {
 		// DATE is the preferred way of storing the year, but it is still possible we will
 		// encounter YEAR
-		self.insert(String::from("DATE"), value.to_string(), true);
+		self.insert(String::from("DATE"), value.to_string());
 		let _ = self.remove("YEAR");
 	}
 
@@ -524,13 +578,13 @@ mod tests {
 
 		expected_tag.set_vendor(String::from("Lavf58.76.100"));
 
-		expected_tag.insert(String::from("ALBUM"), String::from("Baz album"), false);
-		expected_tag.insert(String::from("ARTIST"), String::from("Bar artist"), false);
-		expected_tag.insert(String::from("COMMENT"), String::from("Qux comment"), false);
-		expected_tag.insert(String::from("DATE"), String::from("1984"), false);
-		expected_tag.insert(String::from("GENRE"), String::from("Classical"), false);
-		expected_tag.insert(String::from("TITLE"), String::from("Foo title"), false);
-		expected_tag.insert(String::from("TRACKNUMBER"), String::from("1"), false);
+		expected_tag.push(String::from("ALBUM"), String::from("Baz album"));
+		expected_tag.push(String::from("ARTIST"), String::from("Bar artist"));
+		expected_tag.push(String::from("COMMENT"), String::from("Qux comment"));
+		expected_tag.push(String::from("DATE"), String::from("1984"));
+		expected_tag.push(String::from("GENRE"), String::from("Classical"));
+		expected_tag.push(String::from("TITLE"), String::from("Foo title"));
+		expected_tag.push(String::from("TRACKNUMBER"), String::from("1"));
 
 		let file_cont = crate::tag::utils::test_utils::read_path("tests/tags/assets/test.vorbis");
 		let parsed_tag = read_tag(&file_cont);