Skip to content

fix(xml): XML deserialization to handle empty nodes #146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
lazulit3 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(xml): XML deserialization to handle empty nodes #146

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6bad90f
test(xml): add empty playlist case to XML test data
lazulit3 Apr 1, 2025
905da8e
fix(xml): XML deserialization to handle empty nodes
lazulit3 Mar 31, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion data/xml/database.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@
</TRACK>
</COLLECTION>
<PLAYLISTS>
<NODE Type="0" Name="ROOT" Count="2">
<NODE Type="0" Name="ROOT" Count="3">
<NODE Name="Folder" Type="0" Count="1">
<NODE Name="Sub Playlist" Type="1" KeyType="0" Entries="2">
<TRACK Key="5"/>
Expand All @@ -72,6 +72,7 @@
<TRACK Key="5"/>
<TRACK Key="6"/>
</NODE>
<NODE Name="EmptyPlaylist" Type="1" KeyType="0" Entries="0" />
</NODE>
</PLAYLISTS>
</DJ_PLAYLISTS>
136 changes: 36 additions & 100 deletions src/xml.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,6 @@
//! - <https://pyrekordbox.readthedocs.io/en/stable/formats/xml.html>
type NaiveDate = String; //Replace with "use chrono::naive::NaiveDate;"
use serde::{de::Error, ser::Serializer, Deserialize, Serialize};
use std::borrow::Cow;

/// The XML root element of a rekordbox XML file.
#[derive(Debug, PartialEq, Clone, Serialize, Deserialize)]
Expand Down Expand Up @@ -251,110 +250,47 @@ impl<'de> Deserialize<'de> for PlaylistGenericNode {
where
D: serde::Deserializer<'de>,
{
struct PlaylistGenericNodeVisitor;

impl<'de> serde::de::Visitor<'de> for PlaylistGenericNodeVisitor {
type Value = PlaylistGenericNode;

fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
formatter.write_str("struct PlaylistGenericNode")
}

fn visit_map<A>(self, mut map: A) -> Result<Self::Value, A::Error>
where
A: serde::de::MapAccess<'de>,
{
let mut node_type = None;
let mut name = None;
let mut count = None;
let mut key_type = None;
let mut entries = None;

while let Some(key) = map.next_key::<Cow<'_, str>>()? {
match key.as_ref() {
"@Name" => name = map.next_value::<Cow<'_, str>>()?.into(),
"@Type" => node_type = map.next_value::<Cow<'_, str>>()?.into(),
"@Count" => count = map.next_value::<usize>()?.into(),
"@KeyType" => key_type = map.next_value::<Cow<'_, str>>()?.into(),
"@Entries" => entries = map.next_value::<usize>()?.into(),
unknown => {
return Err(A::Error::unknown_field(
unknown,
&["@Name", "@Type", "@Count", "@KeyType", "@Entries"],
));
}
}
// stores a node with fields for a playlist or folder
#[derive(Deserialize)]
struct Node {
#[serde(rename = "@Name")]
name: String,
// indicates playlist or folder
#[serde(rename = "@Type")]
node_type: String,
// appears on playlists only
#[serde(rename = "@KeyType", default)]
key_type: Option<String>,
// child nodes in a folder
#[serde(rename = "NODE", default)]
nodes: Vec<PlaylistGenericNode>,
// tracks in a playlist
#[serde(rename = "TRACK", default)]
tracks: Vec<PlaylistTrack>,
}
Comment on lines +254 to +270
Copy link
Author

@lazulit3 lazulit3 Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this does not deserialize Count or Entries because this iteration does not actually use these.


match node_type.as_deref() {
Some("0") => {
if let (Some(n), Some(_c)) = (&name, count) {
let nodes = {
// Create anonymous type
#[derive(serde::Deserialize)]
struct Nodes {
#[serde(rename = "NODE")]
content: Vec<PlaylistGenericNode>,
}
let de = serde::de::value::MapAccessDeserializer::new(map);
Nodes::deserialize(de)?.content
};
// FIXME: Should we check if nodes.len() == count here?
Copy link
Author

@lazulit3 lazulit3 Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I restore the FIXME comments regarding validation that the amount of parsed children matches Entries or Count?

I could also add this validation, but I was unsure what type of error to return (or would we rather panic?)

Copy link
Collaborator

@Swiftb0y Swiftb0y Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah please add it if feasible. Maybe just add a (non-fatal) warning though, as we don't control the XML schema, we want to be maximally compatible

return Ok(PlaylistGenericNode::Folder(PlaylistFolderNode {
name: n.to_string(),
nodes,
}));
}
}
Some("1") => {
if let (Some(n), Some(_c), Some(t)) = (&name, entries, &key_type) {
let tracks = {
// Create anonymous type
#[derive(serde::Deserialize)]
struct Tracks {
#[serde(rename = "TRACK")]
content: Vec<PlaylistTrack>,
}
let de = serde::de::value::MapAccessDeserializer::new(map);
Tracks::deserialize(de)?.content
};
// FIXME: Should we check if nodes.len() == count here?
return Ok(PlaylistGenericNode::Playlist(PlaylistPlaylistNode {
name: n.to_string(),
keytype: t.to_string(),
tracks,
}));
}
}
Some(unknown) => {
return Err(A::Error::unknown_variant(unknown, &["0", "1"]))
}
None => (),
}
}
let node = Node::deserialize(deserializer)?;

match node_type.as_deref() {
Some("0") => {
if name.is_none() {
Err(A::Error::missing_field("@Name"))
} else {
Err(A::Error::missing_field("@Count"))
}
}
Some("1") => {
if name.is_none() {
Err(A::Error::missing_field("@Name"))
} else if entries.is_none() {
Err(A::Error::missing_field("@Entries"))
} else {
Err(A::Error::missing_field("@KeyType"))
}
}
_ => Err(A::Error::missing_field("@Type")),
match node.node_type.as_str() {
// Folder node
"0" => Ok(PlaylistGenericNode::Folder(PlaylistFolderNode {
name: node.name,
nodes: node.nodes,
})),
// Playlist node
"1" => {
if let Some(key_type) = node.key_type {
Ok(PlaylistGenericNode::Playlist(PlaylistPlaylistNode {
name: node.name,
keytype: key_type,
tracks: node.tracks,
}))
} else {
Err(D::Error::missing_field("@KeyType"))
Comment on lines +274 to +289
Copy link
Author

@lazulit3 lazulit3 Apr 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This does not actually validate that the Entries or Count attributes exist on the nodes. Would we prefer this to be stricter?

Copy link
Collaborator

@Swiftb0y Swiftb0y Aug 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm unfortunately very unfamiliar with the XML code, so I would prefer to stick as close as possible to the previous implementation (which was quite strict from what I can tell).

}
}
t => Err(D::Error::unknown_variant(t, &["0", "1"])),
}

deserializer.deserialize_map(PlaylistGenericNodeVisitor)
}
}

Expand Down