ID3v2: Properly handle solidus character (U+002F) in text frames

CHANGELOG.md

@@ -24,11 +24,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - **ogg_pager**: `paginate` now works with a collection of packets.
 - **lofty_attr**: The `lofty_attr::LoftyFile` derive proc macro is now exported as `lofty::LoftyFile`.
 - **TaggedFile**: All methods have been split out into a new trait, `TaggedFileExt`.
+- **Accessor**: All methods returning string values now return `Cow<str>`.
+  - This is an unfortunate change that needed to be made in order to accommodate the handling of the different
+    possible text separators between ID3v2 versions.
 
 ### Removed
 - **ogg_pager**: Removed `Page::new`, now pages can only be created through `ogg_pager::paginate` or
                  `Packets::paginate`.
 
+### Fixed
+- **ID3v2**: The `'/'` character is no longer used as a separator ([issue](https://github.com/Serial-ATA/lofty-rs/issues/82))
+
 ## [0.9.0] - 2022-10-30
 
 ### Added

examples/tag_reader.rs

@@ -23,10 +23,10 @@ fn main() {
 	};
 
 	println!("--- Tag Information ---");
-	println!("Title: {}", tag.title().unwrap_or("None"));
-	println!("Artist: {}", tag.artist().unwrap_or("None"));
-	println!("Album: {}", tag.album().unwrap_or("None"));
-	println!("Genre: {}", tag.genre().unwrap_or("None"));
+	println!("Title: {}", tag.title().as_deref().unwrap_or("None"));
+	println!("Artist: {}", tag.artist().as_deref().unwrap_or("None"));
+	println!("Album: {}", tag.album().as_deref().unwrap_or("None"));
+	println!("Genre: {}", tag.genre().as_deref().unwrap_or("None"));
 
 	// import keys from https://docs.rs/lofty/latest/lofty/enum.ItemKey.html
 	println!(

src/ape/tag/mod.rs

@@ -8,6 +8,7 @@ use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
 use crate::traits::{Accessor, TagExt};
 
+use std::borrow::Cow;
 use std::convert::TryInto;
 use std::fs::{File, OpenOptions};
 use std::io::Write;
@@ -19,11 +20,11 @@ macro_rules! impl_accessor {
 	($($name:ident => $($key:literal)|+;)+) => {
 		paste::paste! {
 			$(
-				fn $name(&self) -> Option<&str> {
+				fn $name(&self) -> Option<Cow<'_, str>> {
 					$(
 						if let Some(i) = self.get_key($key) {
 							if let ItemValue::Text(val) = i.value() {
-								return Some(val)
+								return Some(Cow::Borrowed(val));
 							}
 						}
 					)+

src/id3/v1/tag.rs

@@ -4,6 +4,7 @@ use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
 use crate::traits::{Accessor, TagExt};
 
+use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
 use std::io::Write;
 use std::path::Path;
@@ -14,8 +15,12 @@ macro_rules! impl_accessor {
 	($($name:ident,)+) => {
 		paste::paste! {
 			$(
-				fn $name(&self) -> Option<&str> {
-					self.$name.as_deref()
+				fn $name(&self) -> Option<Cow<'_, str>> {
+					if let Some(item) = self.$name.as_deref() {
+						return Some(Cow::Borrowed(item));
+					}
+
+					None
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {
@@ -87,12 +92,12 @@ pub struct ID3v1Tag {
 impl Accessor for ID3v1Tag {
 	impl_accessor!(title, artist, album,);
 
-	fn genre(&self) -> Option<&str> {
+	fn genre(&self) -> Option<Cow<'_, str>> {
 		if let Some(g) = self.genre {
 			let g = g as usize;
 
 			if g < GENRES.len() {
-				return Some(GENRES[g]);
+				return Some(Cow::Borrowed(GENRES[g]));
 			}
 		}
 
@@ -126,8 +131,8 @@ impl Accessor for ID3v1Tag {
 		self.track_number = None;
 	}
 
-	fn comment(&self) -> Option<&str> {
-		self.comment.as_deref()
+	fn comment(&self) -> Option<Cow<'_, str>> {
+		self.comment.as_deref().map(Cow::Borrowed)
 	}
 
 	fn set_comment(&mut self, value: String) {

src/id3/v2/tag.rs

@@ -24,17 +24,8 @@ macro_rules! impl_accessor {
 	($($name:ident => $id:literal;)+) => {
 		paste::paste! {
 			$(
-				fn $name(&self) -> Option<&str> {
-					if let Some(f) = self.get($id) {
-						if let FrameValue::Text {
-							ref value,
-							..
-						} = f.content() {
-							return Some(value)
-						}
-					}
-
-					None
+				fn $name(&self) -> Option<Cow<'_, str>> {
+					self.get_text($id)
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {
@@ -160,6 +151,27 @@ impl ID3v2Tag {
 			.find(|f| f.id_str().eq_ignore_ascii_case(id))
 	}
 
+	/// Gets the text for a frame
+	///
+	/// If the tag is [`ID3v2Version::V4`], this will allocate if the text contains any
+	/// null (`'\0'`) text separators to replace them with a slash (`'/'`).
+	pub fn get_text(&self, id: &str) -> Option<Cow<'_, str>> {
+		let frame = self.get(id);
+		if let Some(Frame {
+			value: FrameValue::Text { value, .. },
+			..
+		}) = frame
+		{
+			if !value.contains('\0') || self.original_version != ID3v2Version::V4 {
+				return Some(Cow::Borrowed(value.as_str()));
+			}
+
+			return Some(Cow::Owned(value.replace('\0', "/")));
+		}
+
+		None
+	}
+
 	/// Inserts a [`Frame`]
 	///
 	/// This will replace any frame of the same id (**or description!** See [`EncodedTextFrame`])
@@ -375,13 +387,13 @@ impl Accessor for ID3v2Tag {
 		self.remove("TDRC");
 	}
 
-	fn comment(&self) -> Option<&str> {
+	fn comment(&self) -> Option<Cow<'_, str>> {
 		if let Some(Frame {
 			value: FrameValue::Comment(LanguageFrame { content, .. }),
 			..
 		}) = self.get("COMM")
 		{
-			return Some(content);
+			return Some(Cow::Borrowed(content));
 		}
 
 		None
@@ -531,7 +543,7 @@ impl From<ID3v2Tag> for Tag {
 					}),
 				) => {
 					let item_key = ItemKey::from_key(TagType::ID3v2, description);
-					for c in content.split(&['\0', '/'][..]) {
+					for c in content.split('\0') {
 						tag.items.push(TagItem::new(
 							item_key.clone(),
 							ItemValue::Text(c.to_string()),
@@ -547,7 +559,7 @@ impl From<ID3v2Tag> for Tag {
 					}),
 				) => {
 					let item_key = ItemKey::from_key(TagType::ID3v2, description);
-					for c in content.split(&['\0', '/'][..]) {
+					for c in content.split('\0') {
 						tag.items.push(TagItem::new(
 							item_key.clone(),
 							ItemValue::Locator(c.to_string()),
@@ -562,7 +574,7 @@ impl From<ID3v2Tag> for Tag {
 						| FrameValue::UnSyncText(LanguageFrame { content, .. })
 						| FrameValue::Text { value: content, .. }
 						| FrameValue::UserText(EncodedTextFrame { content, .. }) => {
-							for c in content.split(&['\0', '/'][..]) {
+							for c in content.split('\0') {
 								tag.items.push(TagItem::new(
 									item_key.clone(),
 									ItemValue::Text(c.to_string()),
@@ -603,7 +615,7 @@ impl From<Tag> for ID3v2Tag {
 					let mut s = String::with_capacity(iter.size_hint().0);
 					s.push_str(&first);
 					iter.for_each(|i| {
-						s.push('/');
+						s.push('\0');
 						s.push_str(&i);
 					});
 
@@ -1135,7 +1147,7 @@ mod tests {
 		use crate::traits::Accessor;
 		let mut tag = ID3v2Tag::default();
 
-		tag.set_artist(String::from("foo/bar\0baz"));
+		tag.set_artist(String::from("foo\0bar\0baz"));
 
 		let tag: Tag = tag.into();
 		let collected_artists = tag.get_strings(&ItemKey::TrackArtist).collect::<Vec<_>>();
@@ -1161,7 +1173,7 @@ mod tests {
 		));
 
 		let tag: ID3v2Tag = tag.into();
-		assert_eq!(tag.artist(), Some("foo/bar/baz"))
+		assert_eq!(tag.artist().as_deref(), Some("foo/bar/baz"))
 	}
 
 	#[test]

src/iff/aiff/tag.rs

@@ -5,6 +5,7 @@ use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
 use crate::traits::{Accessor, TagExt};
 
+use std::borrow::Cow;
 use std::convert::TryFrom;
 use std::fs::{File, OpenOptions};
 use std::io::{Read, Seek, SeekFrom, Write};
@@ -77,8 +78,8 @@ pub struct AIFFTextChunks {
 }
 
 impl Accessor for AIFFTextChunks {
-	fn artist(&self) -> Option<&str> {
-		self.author.as_deref()
+	fn artist(&self) -> Option<Cow<'_, str>> {
+		self.author.as_deref().map(Cow::Borrowed)
 	}
 	fn set_artist(&mut self, value: String) {
 		self.author = Some(value)
@@ -87,8 +88,8 @@ impl Accessor for AIFFTextChunks {
 		self.author = None
 	}
 
-	fn title(&self) -> Option<&str> {
-		self.name.as_deref()
+	fn title(&self) -> Option<Cow<'_, str>> {
+		self.name.as_deref().map(Cow::Borrowed)
 	}
 	fn set_title(&mut self, value: String) {
 		self.name = Some(value)
@@ -97,15 +98,15 @@ impl Accessor for AIFFTextChunks {
 		self.name = None
 	}
 
-	fn comment(&self) -> Option<&str> {
+	fn comment(&self) -> Option<Cow<'_, str>> {
 		if let Some(ref anno) = self.annotations {
 			if !anno.is_empty() {
-				return anno.first().map(String::as_str);
+				return anno.first().map(String::as_str).map(Cow::Borrowed);
 			}
 		}
 
 		if let Some(ref comm) = self.comments {
-			return comm.first().map(|c| c.text.as_str());
+			return comm.first().map(|c| c.text.as_str()).map(Cow::Borrowed);
 		}
 
 		None

src/iff/wav/tag/mod.rs

@@ -6,6 +6,7 @@ use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
 use crate::traits::{Accessor, TagExt};
 
+use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
 use std::io::Write;
 use std::path::Path;
@@ -16,8 +17,8 @@ macro_rules! impl_accessor {
 	($($name:ident => $key:literal;)+) => {
 		paste::paste! {
 			$(
-				fn $name(&self) -> Option<&str> {
-					self.get($key)
+				fn $name(&self) -> Option<Cow<'_, str>> {
+					self.get($key).map(Cow::Borrowed)
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {

src/mp4/ilst/mod.rs

@@ -14,6 +14,7 @@ use crate::traits::{Accessor, TagExt};
 use atom::{AdvisoryRating, Atom, AtomData};
 use r#ref::AtomIdentRef;
 
+use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
 use std::io::Write;
 use std::path::Path;
@@ -30,10 +31,10 @@ macro_rules! impl_accessor {
 	($($name:ident => $const:ident;)+) => {
 		paste::paste! {
 			$(
-				fn $name(&self) -> Option<&str> {
+				fn $name(&self) -> Option<Cow<'_, str>> {
 					if let Some(atom) = self.atom(&$const) {
 						if let Some(AtomData::UTF8(val) | AtomData::UTF16(val)) = atom.data().next() {
-							return Some(val)
+							return Some(Cow::Borrowed(val));
 						}
 					}
 

src/ogg/tag.rs

@@ -8,6 +8,7 @@ use crate::tag::item::{ItemKey, ItemValue, TagItem};
 use crate::tag::{Tag, TagType};
 use crate::traits::{Accessor, TagExt};
 
+use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
 use std::io::Write;
 use std::path::Path;
@@ -18,8 +19,8 @@ macro_rules! impl_accessor {
 	($($name:ident => $key:literal;)+) => {
 		paste::paste! {
 			$(
-				fn $name(&self) -> Option<&str> {
-					self.get($key)
+				fn $name(&self) -> Option<Cow<'_, str>> {
+					self.get($key).map(Cow::Borrowed)
 				}
 
 				fn [<set_ $name>](&mut self, value: String) {

src/tag/mod.rs

@@ -9,6 +9,7 @@ use crate::probe::Probe;
 use crate::traits::{Accessor, TagExt};
 use item::{ItemKey, ItemValue, TagItem};
 
+use std::borrow::Cow;
 use std::fs::{File, OpenOptions};
 use std::io::Write;
 use std::path::Path;
@@ -17,9 +18,9 @@ macro_rules! impl_accessor {
 	($($item_key:ident => $name:tt),+) => {
 		paste::paste! {
 			$(
-				fn $name(&self) -> Option<&str> {
+				fn $name(&self) -> Option<Cow<'_, str>> {
 					if let Some(ItemValue::Text(txt)) = self.get_item_ref(&ItemKey::$item_key).map(TagItem::value) {
-						return Some(&*txt)
+						return Some(Cow::Borrowed(txt))
 					}
 
 					None
@@ -678,7 +679,7 @@ mod tests {
 		let mut tag = Tag::new(TagType::ID3v2);
 		tag.set_title(String::from("Foo title"));
 
-		assert_eq!(tag.title(), Some("Foo title"));
+		assert_eq!(tag.title().as_deref(), Some("Foo title"));
 
 		tag.set_title(String::new());
 		assert_eq!(tag.title(), None);

src/traits.rs

@@ -1,3 +1,5 @@
+use std::borrow::Cow;
+
 // This defines the `Accessor` trait, used to define unified getters/setters for commonly
 // accessed tag values.
 //
@@ -110,11 +112,11 @@ macro_rules! accessor_trait {
 }
 
 accessor_trait! {
-	[artist]<&str, String>, [title]<&str, String>,
-	[album]<&str, String>,  [genre]<&str, String>,
-	[track]<u32>,			[track total]<u32>,
-	[disk]<u32>,		  	[disk total]<u32>,
-	[year]<u32>,			[comment]<&str, String>,
+	[artist]<Cow<'_, str>, String>, [title      ]<Cow<'_, str>, String>,
+	[album ]<Cow<'_, str>, String>, [genre      ]<Cow<'_, str>, String>,
+	[track ]<u32>,                  [track total]<u32>,
+	[disk  ]<u32>,                  [disk total ]<u32>,
+	[year  ]<u32>,                  [comment    ]<Cow<'_, str>, String>,
 }
 
 use crate::tag::Tag;