fix(xml): XML deserialization to handle empty nodes #146
fix(xml): XML deserialization to handle empty nodes #146lazulit3 wants to merge 2 commits intoHolzhaus:mainfrom
Conversation
Updates the XML test data to include an empty playlist.
Fixes a bug where XML parsing would panic if there exists either of: - a folder with no children - a playlist with no tracks This updates the approach to deserialization to use a more general struct that can contain the fields of a playlist or folder, and the visitor implementation is removed.
lazulit3
left a comment
lazulit3
left a comment
There was a problem hiding this comment.
Some notes & discussion included below.
| let de = serde::de::value::MapAccessDeserializer::new(map); | ||
| Nodes::deserialize(de)?.content | ||
| }; | ||
| // FIXME: Should we check if nodes.len() == count here? |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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
| #[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>, | ||
| } |
There was a problem hiding this comment.
Note that this does not deserialize Count or Entries because this iteration does not actually use these.
| 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")) |
There was a problem hiding this comment.
This does not actually validate that the Entries or Count attributes exist on the nodes. Would we prefer this to be stricter?
There was a problem hiding this comment.
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).
Swiftb0y
left a comment
Swiftb0y
left a comment
There was a problem hiding this comment.
hey there, I'm sorry for the long delay. Are you still interested in merging this PR?
| 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")) |
There was a problem hiding this comment.
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).
| let de = serde::de::value::MapAccessDeserializer::new(map); | ||
| Nodes::deserialize(de)?.content | ||
| }; | ||
| // FIXME: Should we check if nodes.len() == count here? |
There was a problem hiding this comment.
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
2 participants
This PR fixes a bug where XML parsing would panic if the exported XML contains an edge case where there is (either of):
The bug is described at:
Approach
This removes the visitor map implementation. Instead, a node is first deserialized into a general
Nodetype (which can contain fields for either a folder or playlist). After identifying which type the node is, the specific type is constructed and returned.Discussion
I am not sure if this approach is advantageous, but I struggled to wrap my head around the visitor map.
This PR is against
main, but I can open a PR againstxml-improvementsas well (if desired).Testing
I added a test case for an empty playlist, and this seems to resolve the issue. Additionally, this branch parses my rekordbox export (with my real data) successfully.