Sanitize MBID usage: do not trust MBID coming from files' metadata

src/Makefile.am

@@ -55,6 +55,7 @@ lms_SOURCES = \
 	$(srcdir)/main/main.cpp					\
 	$(srcdir)/metadata/AvFormat.cpp				\
 	$(srcdir)/metadata/AvFormat.hpp				\
+	$(srcdir)/metadata/MetaData.cpp				\
 	$(srcdir)/metadata/MetaData.hpp				\
 	$(srcdir)/metadata/TagLibParser.cpp			\
 	$(srcdir)/metadata/TagLibParser.hpp			\
@@ -159,6 +160,8 @@ lms_SOURCES = \
 	$(srcdir)/utils/StreamLogger.hpp			\
 	$(srcdir)/utils/Utils.cpp				\
 	$(srcdir)/utils/Utils.hpp				\
+	$(srcdir)/utils/UUID.cpp				\
+	$(srcdir)/utils/UUID.hpp				\
 	$(srcdir)/utils/WtLogger.cpp				\
 	$(srcdir)/utils/WtLogger.hpp
 

src/api/subsonic/SubsonicResource.cpp

@@ -934,8 +934,9 @@ handleGetArtistInfoRequestCommon(RequestContext& context, bool id3)
 		if (!artist)
 			throw RequestedDataNotFoundError {};
 
-		if (!artist->getMBID().empty())
+		std::optional<UUID> artistMBID {artist->getMBID()};
-			artistInfoNode.createChild("musicBrainzId").setValue(artist->getMBID());
+		if (artistMBID)
+			artistInfoNode.createChild("musicBrainzId").setValue(artistMBID->getAsString());
 	}
 
 	auto similarArtistsId {ServiceProvider<Similarity::Searcher>::get()->getSimilarArtists(context.dbSession, id.value, count)};

src/api/subsonic/SubsonicResponse.cpp

@@ -45,7 +45,7 @@ ResponseFormatToMimeType(ResponseFormat format)
 }
 
 void
-Response::Node::setValue(const std::string& value)
+Response::Node::setValue(std::string_view value)
 {
 	if (!_children.empty() || !_childrenArrays.empty())
 		throw LmsException {"Node already has children"};

src/api/subsonic/SubsonicResponse.hpp

@@ -182,7 +182,7 @@ class Response
 				void setAttribute(std::string_view key, std::string_view value);
 
 				// A Node has either a value or some children
-				void setValue(const std::string& value);
+				void setValue(std::string_view value);
 				Node& createChild(const std::string& key);
 				Node& createArrayChild(const std::string& key);
 

src/database/Artist.cpp

@@ -32,10 +32,10 @@
 namespace Database
 {
 
-Artist::Artist(const std::string& name, const std::string& MBID)
+Artist::Artist(const std::string& name, const std::optional<UUID>& MBID)
 : _name {std::string(name, 0 , _maxNameLength)},
 _sortName {_name},
-_MBID {MBID}
+_MBID {MBID ? MBID->getAsString() : ""}
 {
 
 }
@@ -50,10 +50,10 @@ Artist::getByName(Session& session, const std::string& name)
 }
 
 Artist::pointer
-Artist::getByMBID(Session& session, const std::string& mbid)
+Artist::getByMBID(Session& session, const UUID& mbid)
 {
 	session.checkSharedLocked();
-	return session.getDboSession().find<Artist>().where("mbid = ?").bind(mbid);
+	return session.getDboSession().find<Artist>().where("mbid = ?").bind(std::string {mbid.getAsString()});
 }
 
 Artist::pointer
@@ -64,7 +64,7 @@ Artist::getById(Session& session, IdType id)
 }
 
 Artist::pointer
-Artist::create(Session& session, const std::string& name, const std::string& MBID)
+Artist::create(Session& session, const std::string& name, const std::optional<UUID>& MBID)
 {
 	session.checkUniqueLocked();
 

src/database/Artist.hpp

@@ -26,6 +26,8 @@
 #include <Wt/WDateTime.h>
 #include <Wt/Dbo/Dbo.h>
 
+#include "utils/UUID.hpp"
+
 #include "TrackArtistLink.hpp"
 #include "Types.hpp"
 
@@ -46,10 +48,10 @@ class Artist : public Wt::Dbo::Dbo<Artist>
 		using pointer = Wt::Dbo::ptr<Artist>;
 
 		Artist() {}
-		Artist(const std::string& name, const std::string& MBID = "");
+		Artist(const std::string& name, const std::optional<UUID>& MBID = {});
 
 		// Accessors
-		static pointer			getByMBID(Session& session, const std::string& MBID);
+		static pointer			getByMBID(Session& session, const UUID& MBID);
 		static pointer			getById(Session& session, IdType id);
 		static std::vector<pointer>	getByName(Session& session, const std::string& name);
 		static std::vector<pointer> 	getByClusters(Session& session,
@@ -69,7 +71,7 @@ class Artist : public Wt::Dbo::Dbo<Artist>
 
 		// Accessors
 		const std::string& getName(void) const { return _name; }
-		const std::string& getMBID(void) const { return _MBID; }
+		std::optional<UUID> getMBID(void) const { return readAs<UUID>(_MBID); }
 
 		std::vector<Wt::Dbo::ptr<Release>>	getReleases(const std::set<IdType>& clusterIds = {}) const; // if non empty, get the releases that match all these clusters
 		std::size_t				getReleaseCount() const;
@@ -83,11 +85,11 @@ class Artist : public Wt::Dbo::Dbo<Artist>
 		// size is the max number of cluster per cluster type
 		std::vector<std::vector<Wt::Dbo::ptr<Cluster>>> getClusterGroups(std::vector<Wt::Dbo::ptr<ClusterType>> clusterTypes, std::size_t size) const;
 
-		void setMBID(const std::string& mbid) { _MBID = mbid; }
+		void setMBID(const std::optional<UUID>& mbid) { _MBID = mbid ? mbid->getAsString() : ""; }
 		void setSortName(const std::string& sortName);
 
 		// Create
-		static pointer create(Session& session, const std::string& name, const std::string& MBID = "");
+		static pointer create(Session& session, const std::string& name, const std::optional<UUID>& UUID = {});
 
 		template<class Action>
 			void persist(Action& a)

src/database/Release.cpp

@@ -31,9 +31,9 @@
 namespace Database
 {
 
-Release::Release(const std::string& name, const std::string& MBID)
+Release::Release(const std::string& name, const std::optional<UUID>& MBID)
-: _name(std::string(name, 0 , _maxNameLength)),
+: _name {std::string(name, 0 , _maxNameLength)},
-_MBID(MBID)
+_MBID {MBID ? MBID->getAsString() : ""}
 {
 
 }
@@ -48,11 +48,11 @@ Release::getByName(Session& session, const std::string& name)
 }
 
 Release::pointer
-Release::getByMBID(Session& session, const std::string& mbid)
+Release::getByMBID(Session& session, const UUID& mbid)
 {
 	session.checkSharedLocked();
 
-	return session.getDboSession().find<Release>().where("mbid = ?").bind(mbid);
+	return session.getDboSession().find<Release>().where("mbid = ?").bind(std::string {mbid.getAsString()});
 }
 
 Release::pointer
@@ -64,7 +64,7 @@ Release::getById(Session& session, IdType id)
 }
 
 Release::pointer
-Release::create(Session& session, const std::string& name, const std::string& MBID)
+Release::create(Session& session, const std::string& name, const std::optional<UUID>& MBID)
 {
 	session.checkSharedLocked();
 

src/database/Release.hpp

@@ -23,6 +23,7 @@
 
 #include <Wt/Dbo/WtSqlTraits.h>
 
+#include "utils/UUID.hpp"
 #include "TrackArtistLink.hpp"
 #include "Types.hpp"
 
@@ -43,11 +44,11 @@ class Release : public Wt::Dbo::Dbo<Release>
 		using pointer = Wt::Dbo::ptr<Release>;
 
 		Release() {}
-		Release(const std::string& name, const std::string& MBID = "");
+		Release(const std::string& name, const std::optional<UUID>& MBID = {});
 
 		// Accessors
 		static std::size_t		getCount(Session& session);
-		static pointer			getByMBID(Session& session, const std::string& MBID);
+		static pointer			getByMBID(Session& session, const UUID& MBID);
 		static std::vector<pointer>	getByName(Session& session, const std::string& name);
 		static pointer			getById(Session& session, IdType id);
 		static std::vector<pointer>	getAllOrphans(Session& session); // no track related
@@ -75,7 +76,7 @@ class Release : public Wt::Dbo::Dbo<Release>
 		std::vector<std::vector<Wt::Dbo::ptr<Cluster>>> getClusterGroups(std::vector<Wt::Dbo::ptr<ClusterType>> clusterTypes, std::size_t size) const;
 
 		// Create
-		static pointer create(Session& session, const std::string& name, const std::string& MBID = "");
+		static pointer create(Session& session, const std::string& name, const std::optional<UUID>& MBID = {});
 
 		// Utility functions
 		std::optional<int> getReleaseYear(bool originalDate = false) const; // 0 if unknown or various
@@ -88,7 +89,7 @@ class Release : public Wt::Dbo::Dbo<Release>
 
 		// Accessors
 		std::string			getName() const		{ return _name; }
-		std::string			getMBID() const		{ return _MBID; }
+		std::optional<UUID>		getMBID() const		{ return readAs<UUID>(_MBID); }
 		std::optional<std::size_t>	getTotalTrackNumber() const;
 		std::optional<std::size_t>	getTotalDiscNumber() const;
 		std::chrono::milliseconds	getDuration() const;
@@ -99,7 +100,7 @@ class Release : public Wt::Dbo::Dbo<Release>
 		bool hasVariousArtists() const;
 		std::vector<pointer>		getSimilarReleases(std::optional<std::size_t> offset = {}, std::optional<std::size_t> count = {}) const;
 
-		void setMBID(std::string mbid) { _MBID = mbid; }
+		void setMBID(const std::optional<UUID>& mbid) { _MBID = mbid ? mbid->getAsString() : ""; }
 
 		template<class Action>
 			void persist(Action& a)

src/database/Session.cpp

@@ -40,7 +40,7 @@
 
 namespace Database {
 
-#define LMS_DATABASE_VERSION	11
+#define LMS_DATABASE_VERSION	12
 
 using Version = std::size_t;
 
@@ -145,6 +145,12 @@ CREATE TABLE IF NOT EXISTS "track_bookmark" (
 			ScanSettings::get(*this).modify()->addAudioFileExtension(".m4b");
 			ScanSettings::get(*this).modify()->addAudioFileExtension(".alac");
 		}
+		else if (version == 11)
+		{
+			// Sanitize bad MBID, need to rescan the whole files
+			// Just increment the scan version of the settings to make the next scheduled scan rescan everything
+			ScanSettings::get(*this).modify()->incScanVersion();
+		}
 		else
 		{
 			LMS_LOG(DB, ERROR) << "Database version " << version << " cannot be handled using migration";

src/database/Track.cpp

@@ -88,12 +88,12 @@ Track::getById(Session& session, IdType id)
 }
 
 Track::pointer
-Track::getByMBID(Session& session, const std::string& mbid)
+Track::getByMBID(Session& session, const UUID& mbid)
 {
 	session.checkSharedLocked();
 
 	return session.getDboSession().find<Track>()
-		.where("mbid = ?").bind(mbid);
+		.where("mbid = ?").bind(std::string {mbid.getAsString()});
 }
 
 Track::pointer

src/database/Track.hpp

@@ -28,6 +28,8 @@
 #include <Wt/Dbo/Dbo.h>
 #include <Wt/WDateTime.h>
 
+#include "utils/UUID.hpp"
+
 #include "TrackArtistLink.hpp"
 #include "Types.hpp"
 
@@ -54,7 +56,7 @@ class Track : public Wt::Dbo::Dbo<Track>
 		// Find utility functions
 		static pointer getByPath(Session& session, const std::filesystem::path& p);
 		static pointer getById(Session& session, IdType id);
-		static pointer getByMBID(Session& session, const std::string& MBID);
+		static pointer getByMBID(Session& session, const UUID& MBID);
 		static std::vector<pointer>	getSimilarTracks(Session& session,
 							const std::set<IdType>& trackIds,
 							std::optional<std::size_t> offset = {},
@@ -91,7 +93,7 @@ class Track : public Wt::Dbo::Dbo<Track>
 		void setYear(int year)						{ _year = year; }
 		void setOriginalYear(int year)					{ _originalYear = year; }
 		void setHasCover(bool hasCover)					{ _hasCover = hasCover; }
-		void setMBID(const std::string& MBID)				{ _MBID = MBID; }
+		void setMBID(const std::optional<UUID>& MBID)			{ _MBID = MBID ? MBID->getAsString() : ""; }
 		void setCopyright(const std::string& copyright)			{ _copyright = std::string(copyright, 0, _maxCopyrightLength); }
 		void setCopyrightURL(const std::string& copyrightURL)		{ _copyrightURL = std::string(copyrightURL, 0, _maxCopyrightURLLength); }
 		void clearArtistLinks();
@@ -111,7 +113,7 @@ class Track : public Wt::Dbo::Dbo<Track>
 		Wt::WDateTime				getLastWriteTime() const	{ return _fileLastWrite; }
 		Wt::WDateTime				getAddedTime() const		{ return _fileAdded; }
 		bool					hasCover() const		{ return _hasCover; }
-		const std::string&			getMBID() const			{ return _MBID; }
+		std::optional<UUID>			getMBID() const			{ return readAs<UUID>(_MBID); }
 		std::optional<std::string>		getCopyright() const;
 		std::optional<std::string>		getCopyrightURL() const;
 		std::vector<Wt::Dbo::ptr<Artist>>	getArtists(TrackArtistLink::Type type = TrackArtistLink::Type::Artist) const;

src/metadata/AvFormat.cpp

@@ -29,35 +29,54 @@ namespace MetaData
 
 using MetadataMap = std::map<std::string, std::string>;
 
-std::optional<std::string>
+template <typename T>
-findFirstValueOf(const MetadataMap& metadataMap, std::initializer_list<std::string> tags)
+std::optional<T>
+findFirstValueOfAs(const MetadataMap& metadataMap, std::initializer_list<std::string> tags)
 {
 	auto it = std::find_first_of(std::cbegin(metadataMap), std::cend(metadataMap), std::cbegin(tags), std::cend(tags), [](const auto& it, const auto& str) { return it.first == str; });
 	if (it == std::cend(metadataMap))
 		return std::nullopt;
 
-	return stringTrim(it->second);
+	return readAs<T>(stringTrim(it->second));
 }
 
+template <>
+std::optional<std::vector<UUID>>
+findFirstValueOfAs(const MetadataMap& metadataMap, std::initializer_list<std::string> tags)
+{
+	std::optional<std::string> str {findFirstValueOfAs<std::string>(metadataMap, tags)};
+	if (!str)
+		return std::nullopt;
+
+	std::vector<std::string> strUuids = splitString(*str, "/");
+	std::vector<UUID> res;
+
+	for (const std::string strUuid : strUuids)
+	{
+		std::optional<UUID> uuid {readAs<UUID>(strUuid)};
+		if (!uuid)
+			return std::nullopt;
+
+		res.push_back(std::move(*uuid));
+	}
+
+	return res;
+}
+
+
 static
 std::optional<Album>
 getAlbum(const MetadataMap& metadataMap)
 {
 	std::optional<Album> res;
 
-	auto album {findFirstValueOf(metadataMap, {"ALBUM"})};
+	auto album {findFirstValueOfAs<std::string>(metadataMap, {"ALBUM"})};
 	if (!album)
 		return res;
 
-	res = Album{*album, ""};
+	auto albumMBID {findFirstValueOfAs<UUID>(metadataMap, {"MUSICBRAINZ ALBUM ID", "MUSICBRAINZ_ALBUMID", "MUSICBRAINZ/ALBUM ID"})};
 
-	auto albumMBID {findFirstValueOf(metadataMap, {"MUSICBRAINZ ALBUM ID", "MUSICBRAINZ_ALBUMID", "MUSICBRAINZ/ALBUM ID"})};
+	return Album{*album, albumMBID};
-	if (!albumMBID)
-		return res;
-
-	res->musicBrainzAlbumID = *albumMBID;
-
-	return res;
 }
 
 static
@@ -66,17 +85,13 @@ getAlbumArtists(const MetadataMap& metadataMap)
 {
 	std::vector<Artist> res;
 
-	auto name {findFirstValueOf(metadataMap, {"ALBUM_ARTIST"})};
+	auto name {findFirstValueOfAs<std::string>(metadataMap, {"ALBUM_ARTIST"})};
 	if (!name)
 		return res;
 
-	Artist artist {*name, ""};
+	auto mbid {findFirstValueOfAs<UUID>(metadataMap, {"MUSICBRAINZ ALBUM ARTIST ID", "MUSICBRAINZ/ALBUM ARTIST ID"})};
 
-	auto mbid {findFirstValueOf(metadataMap, {"MUSICBRAINZ ALBUM ARTIST ID", "MUSICBRAINZ/ALBUM ARTIST ID"})};
+	return {Artist {*name, mbid} };
-	if (mbid)
-		artist.musicBrainzArtistID = *mbid;
-
-	return {std::move(artist)};
 }
 
 static
@@ -95,21 +110,14 @@ getArtists(const MetadataMap& metadataMap)
 		artistNames = {metadataMap.find("ARTIST")->second};
 	}
 
-	std::vector<std::string> artistMBIDs;
+	auto artistMBIDs {findFirstValueOfAs<std::vector<UUID>>(metadataMap, {"MUSICBRAINZ ARTIST ID", "MUSICBRAINZ_ARTISTID", "MUSICBRAINZ/ARTIST ID"})};
-	{
-		auto mbids {findFirstValueOf(metadataMap, {"MUSICBRAINZ ARTIST ID", "MUSICBRAINZ_ARTISTID", "MUSICBRAINZ/ARTIST ID"})};
-		if (mbids)
-			artistMBIDs = splitString(*mbids, "/");
-	}
 
 	for (std::size_t i {}; i < artistNames.size(); ++i)
 	{
-		Artist artist{std::move(artistNames[i]), ""};
+		if (artistMBIDs && artistNames.size() == artistMBIDs->size())
-
+			artists.emplace_back(Artist {artistNames[i], (*artistMBIDs)[i]});
-		if (artistNames.size() == artistMBIDs.size())
+		else
-			artist.musicBrainzArtistID = std::move(artistMBIDs[i]);
+			artists.emplace_back(Artist {artistNames[i], {}});
-
-		artists.emplace_back(std::move(artist));
 	}
 
 	return artists;
@@ -191,14 +199,14 @@ AvFormat::parse(const std::filesystem::path& p, bool debug)
 			}
 			else if (tag == "ACOUSTID ID")
 			{
-				track.acoustID = value;
+				track.acoustID = readAs<UUID>(value);
 			}
 			else if (tag == "MUSICBRAINZ RELEASE TRACK ID"
 					|| tag == "MUSICBRAINZ_RELEASETRACKID"
 					|| tag == "MUSICBRAINZ_TRACKID"
 					|| tag == "MUSICBRAINZ/TRACK ID")
 			{
-				track.musicBrainzTrackID = value;
+				track.musicBrainzTrackID = readAs<UUID>(value);
 			}
 			else if (_clusterTypeNames.find(tag) != _clusterTypeNames.end())
 			{

src/metadata/MetaData.cpp

@@ -0,0 +1,30 @@
+/*
+ * Copyright (C) 2018 Emeric Poupon
+ *
+ * This file is part of LMS.
+ *
+ * LMS is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * LMS is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with LMS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "MetaData.hpp"
+
+#include "utils/Utils.hpp"
+
+namespace MetaData
+{
+
+
+
+} // namespace MetaData
+

src/metadata/MetaData.hpp

@@ -26,6 +26,9 @@
 #include <set>
 #include <vector>
 
+#include "utils/Utils.hpp"
+#include "utils/UUID.hpp"
+
 namespace MetaData
 {
 	using Clusters = std::map<std::string /* type */, std::set<std::string> /* names */>;
@@ -33,13 +36,13 @@ namespace MetaData
 	struct Artist
 	{
 		std::string name;
-		std::string musicBrainzArtistID;
+		std::optional<UUID> musicBrainzArtistID;
 	};
 
 	struct Album
 	{
 		std::string name;
-		std::string musicBrainzAlbumID;
+		std::optional<UUID> musicBrainzAlbumID;
 	};
 
 	struct AudioStream
@@ -52,8 +55,8 @@ namespace MetaData
 		std::vector<Artist>		artists;
 		std::vector<Artist>		albumArtists;
 		std::string			title;
-		std::string			musicBrainzTrackID;
+		std::optional<UUID>		musicBrainzTrackID;
-		std::string			musicBrainzRecordID;
+		std::optional<UUID>		musicBrainzRecordID;
 		std::optional<Album>		album;
 		Clusters			clusters;
 		std::chrono::milliseconds 	duration {};
@@ -65,7 +68,7 @@ namespace MetaData
 		std::optional<int>		originalYear;
 		bool				hasCover {false};
 		std::vector<AudioStream>	audioStreams;
-		std::string			acoustID;
+		std::optional<UUID>		acoustID;
 		std::string			copyright;
 		std::string			copyrightURL;
 	};

src/metadata/TagLibParser.cpp

@@ -33,10 +33,11 @@
 namespace MetaData
 {
 
-std::vector<std::string>
+template<typename T>
-getPropertyValuesFirstMatch(const TagLib::PropertyMap& properties, const std::set<std::string>& keys)
+std::vector<T>
+getPropertyValuesFirstMatchAs(const TagLib::PropertyMap& properties, const std::set<std::string>& keys)
 {
-	std::vector<std::string> res;
+	std::vector<T> res;
 
 	for (const std::string& key : keys)
 	{
@@ -45,7 +46,15 @@ getPropertyValuesFirstMatch(const TagLib::PropertyMap& properties, const std::se
 			continue;
 
 		res.reserve(values.size());
-		std::transform(std::cbegin(values), std::cend(values), std::back_inserter(res), [](const auto& value) { return stringTrim(value.to8Bit(true)); });
+
+		for (const auto& value : values)
+		{
+			auto val {readAs<T>(stringTrim(value.to8Bit(true)))};
+			if (!val)
+				continue;
+
+			res.emplace_back(std::move(*val));
+		}
 
 		break;
 	}
@@ -53,10 +62,11 @@ getPropertyValuesFirstMatch(const TagLib::PropertyMap& properties, const std::se
 	return res;
 }
 
-std::vector<std::string>
+template <typename T>
-getPropertyValues(const TagLib::PropertyMap& properties, const std::string& key)
+std::vector<T>
+getPropertyValuesAs(const TagLib::PropertyMap& properties, const std::string& key)
 {
-	return getPropertyValuesFirstMatch(properties, {std::move(key)});
+	return getPropertyValuesFirstMatchAs<T>(properties, {std::move(key)});
 }
 
 static
@@ -78,24 +88,24 @@ getArtists(const TagLib::PropertyMap& properties)
 {
 	std::vector<Artist> res;
 
-	std::vector<std::string> artistNames {getPropertyValues(properties, "ARTISTS")};
+	std::vector<std::string> artistNames {getPropertyValuesAs<std::string>(properties, "ARTISTS")};
 	if (artistNames.empty())
-		artistNames = getPropertyValues(properties, "ARTIST");
+		artistNames = getPropertyValuesAs<std::string>(properties, "ARTIST");
 
 	if (artistNames.empty())
 		return res;
 
-	const std::vector<std::string> artistsMBID {getPropertyValuesFirstMatch(properties, {"MUSICBRAINZ_ARTISTID", "MUSICBRAINZ ARTIST ID"})};
+	const std::vector<UUID> artistsMBID {getPropertyValuesFirstMatchAs<UUID>(properties, {"MUSICBRAINZ_ARTISTID", "MUSICBRAINZ ARTIST ID"})};
 
 	if (artistNames.size() == artistsMBID.size())
 	{
 		std::transform(std::cbegin(artistNames), std::cend(artistNames), std::cbegin(artistsMBID), std::back_inserter(res),
-				[&](const std::string& name, const std::string& mbid) { return Artist{name, mbid}; });
+				[&](const std::string& name, const UUID& mbid) { return Artist {name, mbid}; });
 	}
 	else
 	{
 		std::transform(std::cbegin(artistNames), std::cend(artistNames), std::back_inserter(res),
-				[&](const std::string& name) { return Artist{name, ""}; });
+				[&](const std::string& name) { return Artist{name, {}}; });
 	}
 
 	return res;
@@ -107,21 +117,21 @@ getAlbumArtists(const TagLib::PropertyMap& properties)
 {
 	std::vector<Artist> res;
 
-	std::vector<std::string> artistNames {getPropertyValues(properties, "ALBUMARTIST")};
+	std::vector<std::string> artistNames {getPropertyValuesAs<std::string>(properties, "ALBUMARTIST")};
 	if (artistNames.empty())
 		return res;
 
-	const std::vector<std::string> artistsMBID {getPropertyValuesFirstMatch(properties, {"MUSICBRAINZ_ALBUMARTISTID", "MUSICBRAINZ ALBUM ARTIST ID"})};
+	const std::vector<UUID> artistsMBID {getPropertyValuesFirstMatchAs<UUID>(properties, {"MUSICBRAINZ_ALBUMARTISTID", "MUSICBRAINZ ALBUM ARTIST ID"})};
 
 	if (artistNames.size() == artistsMBID.size())
 	{
 		std::transform(std::cbegin(artistNames), std::cend(artistNames), std::cbegin(artistsMBID), std::back_inserter(res),
-				[&](const std::string& name, const std::string& mbid) { return Artist{name, mbid}; });
+				[&](const std::string& name, const UUID& mbid) { return Artist{name, mbid}; });
 	}
 	else
 	{
 		std::transform(std::cbegin(artistNames), std::cend(artistNames), std::back_inserter(res),
-				[&](const std::string& name) { return Artist{name, ""}; });
+				[&](const std::string& name) { return Artist{name, {}}; });
 	}
 
 	return res;
@@ -131,20 +141,16 @@ static
 std::optional<Album>
 getAlbum(const TagLib::PropertyMap& properties)
 {
-	std::optional<Album> res;
+	std::vector<std::string> albumName {getPropertyValuesAs<std::string>(properties, "ALBUM")};
-
-	std::vector<std::string> albumName {getPropertyValues(properties, "ALBUM")};
 	if (albumName.empty())
-		return res;
+		return std::nullopt;
 
-	std::vector<std::string> albumMBID {getPropertyValuesFirstMatch(properties, {"MUSICBRAINZ_ALBUMID", "MUSICBRAINZ ALBUM ID"})};
+	const std::vector<UUID> albumMBID {getPropertyValuesFirstMatchAs<UUID>(properties, {"MUSICBRAINZ_ALBUMID", "MUSICBRAINZ ALBUM ID"})};
 
-	res = Album{std::move(albumName.front()), ""};
+	if (albumMBID.empty())
-
+		return Album {std::move(albumName.front()), {}};
-	if (!albumMBID.empty())
+	else
-		res->musicBrainzAlbumID = std::move(albumMBID.front());
+		return Album {std::move(albumName.front()), albumMBID.front()};
-
-	return res;
 }
 
 std::optional<Track>
@@ -231,13 +237,13 @@ TagLibParser::parse(const std::filesystem::path& p, bool debug)
 			else if (tag == "MUSICBRAINZ_RELEASETRACKID"
 				|| tag == "MUSICBRAINZ RELEASE TRACK ID")
 			{
-				track.musicBrainzTrackID = value;
+				track.musicBrainzTrackID = readAs<UUID>(value);
 			}
 			else if (tag == "MUSICBRAINZ_TRACKID"
 				|| tag == "MUSICBRAINZ TRACK ID")
-				track.musicBrainzRecordID = value;
+				track.musicBrainzRecordID = readAs<UUID>(value);
 			else if (tag == "ACOUSTID_ID")
-				track.acoustID = value;
+				track.acoustID = readAs<UUID>(value);
 			else if (tag == "TRACKTOTAL")
 			{
 				auto totalTrack = readAs<std::size_t>(value);

src/scanner/MediaScanner.cpp

@@ -92,9 +92,9 @@ getOrCreateArtists(Session& session, const std::vector<MetaData::Artist>& artist
 		Artist::pointer artist;
 
 		// First try to get by MBID
-		if (!artistInfo.musicBrainzArtistID.empty())
+		if (artistInfo.musicBrainzArtistID)
 		{
-			artist = Artist::getByMBID(session, artistInfo.musicBrainzArtistID);
+			artist = Artist::getByMBID(session, *artistInfo.musicBrainzArtistID);
 			if (!artist)
 				artist = Artist::create(session, artistInfo.name, artistInfo.musicBrainzArtistID);
 
@@ -107,7 +107,8 @@ getOrCreateArtists(Session& session, const std::vector<MetaData::Artist>& artist
 		{
 			for (const Artist::pointer& sameNamedArtist : Artist::getByName(session, artistInfo.name))
 			{
-				if (sameNamedArtist->getMBID().empty())
+				// Do not fallback on artist that is correctly tagged
+				if (!sameNamedArtist->getMBID())
 				{
 					artist = sameNamedArtist;
 					break;
@@ -132,9 +133,9 @@ getOrCreateRelease(Session& session, const MetaData::Album& album)
 	Release::pointer release;
 
 	// First try to get by MBID
-	if (!album.musicBrainzAlbumID.empty())
+	if (album.musicBrainzAlbumID)
 	{
-		release = Release::getByMBID(session, album.musicBrainzAlbumID);
+		release = Release::getByMBID(session, *album.musicBrainzAlbumID);
 		if (!release)
 			release = Release::create(session, album.name, album.musicBrainzAlbumID);
 
@@ -146,7 +147,8 @@ getOrCreateRelease(Session& session, const MetaData::Album& album)
 	{
 		for (const Release::pointer& sameNamedRelease : Release::getByName(session, album.name))
 		{
-			if (sameNamedRelease->getMBID().empty())
+			// do not fallback on properly tagged releases
+			if (!sameNamedRelease->getMBID())
 			{
 				release = sameNamedRelease;
 				break;
@@ -810,9 +812,12 @@ MediaScanner::checkDuplicatedAudioFiles(ScanStats& stats)
 	const std::vector<Track::pointer> tracks = Database::Track::getMBIDDuplicates(_dbSession);
 	for (const Track::pointer& track : tracks)
 	{
-		LMS_LOG(DBUPDATER, INFO) << "Found duplicated MBID [" << track->getMBID() << "], file: " << track->getPath().string() << " - " << track->getName();
+		if (track->getMBID())
+		{
+			LMS_LOG(DBUPDATER, INFO) << "Found duplicated MBID [" << track->getMBID()->getAsString() << "], file: " << track->getPath().string() << " - " << track->getName();
 			stats.duplicates.emplace_back(ScanDuplicate {track->getPath(), DuplicateReason::SameMBID});
 		}
+	}
 
 	LMS_LOG(DBUPDATER, INFO) << "Checking duplicated audio files done!";
 }

src/similarity/features/AcousticBrainzUtils.cpp

@@ -33,12 +33,13 @@
 namespace AcousticBrainz
 {
 
-static std::string
+static
-getJsonData(const std::string& mbid)
+std::string
+getJsonData(const UUID& mbid)
 {
 	static const std::string defaultAPIURL = "https://acousticbrainz.org/api/v1/";
 
-	const std::string url {ServiceProvider<Config>::get()->getString("acousticbrainz-api-url", defaultAPIURL) + mbid + "/low-level"};
+	const std::string url {ServiceProvider<Config>::get()->getString("acousticbrainz-api-url", defaultAPIURL) + std::string {mbid.getAsString()} + "/low-level"};
 
 	boost::asio::io_service ioService;
 
@@ -77,7 +78,7 @@ getJsonData(const std::string& mbid)
 }
 
 std::string
-extractLowLevelFeatures(const std::string& mbid)
+extractLowLevelFeatures(const UUID& mbid)
 {
 	return getJsonData(mbid);
 }

src/similarity/features/AcousticBrainzUtils.hpp

@@ -19,12 +19,12 @@
 
 #pragma once
 
-#include <map>
-#include <set>
 #include <string>
 
+#include "utils/UUID.hpp"
+
 namespace AcousticBrainz
 {
-	std::string extractLowLevelFeatures(const std::string& MBID);
+	std::string extractLowLevelFeatures(const UUID& MBID);
 }
 

src/similarity/features/SimilarityFeaturesScannerAddon.cpp

@@ -40,7 +40,7 @@ hasAtLeastOneTrackWithFeatures(Database::Session& session)
 struct TrackInfo
 {
 	Database::IdType id;
-	std::string mbid;
+	std::optional<UUID> mbid;
 };
 
 static
@@ -119,7 +119,8 @@ FeaturesScannerAddon::preScanComplete()
 		if (_stopRequested)
 			return;
 
-		fetchFeatures(trackInfo.id, trackInfo.mbid);
+		if (trackInfo.mbid)
+			fetchFeatures(trackInfo.id, *trackInfo.mbid);
 	}
 
 	updateSearcher();
@@ -157,15 +158,15 @@ FeaturesScannerAddon::updateSearcher()
 }
 
 bool
-FeaturesScannerAddon::fetchFeatures(Database::IdType trackId, const std::string& MBID)
+FeaturesScannerAddon::fetchFeatures(Database::IdType trackId, const UUID& MBID)
 {
 	std::map<std::string, double> features;
 
-	LMS_LOG(DBUPDATER, DEBUG) << "Fetching low level features for track '" << MBID << "'";
+	LMS_LOG(DBUPDATER, DEBUG) << "Fetching low level features for track '" << MBID.getAsString() << "'";
 	const std::string data {AcousticBrainz::extractLowLevelFeatures(MBID)};
 	if (data.empty())
 	{
-		LMS_LOG(DBUPDATER, ERROR) << "Track " << trackId << ", MBID = '" << MBID << "': cannot extract features using AcousticBrainz";
+		LMS_LOG(DBUPDATER, ERROR) << "Track " << trackId << ", MBID = '" << MBID.getAsString() << "': cannot extract features using AcousticBrainz";
 		return false;
 	}
 

src/similarity/features/SimilarityFeaturesScannerAddon.hpp

@@ -22,8 +22,11 @@
 #include "database/Session.hpp"
 #include "scanner/MediaScannerAddon.hpp"
 
+#include "utils/UUID.hpp"
+
 #include "SimilarityFeaturesSearcher.hpp"
 
+
 namespace Database {
 	class Db;
 }
@@ -48,7 +51,7 @@ class FeaturesScannerAddon final : public Scanner::MediaScannerAddon
 		void trackToRemove(Database::IdType) override {}
 		void trackUpdated(Database::IdType trackId) override;
 
-		bool fetchFeatures(Database::IdType trackId, const std::string& MBID);
+		bool fetchFeatures(Database::IdType trackId, const UUID& MBID);
 
 		void updateSearcher();
 

src/utils/UUID.cpp

@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2020 Emeric Poupon
+ *
+ * This file is part of LMS.
+ *
+ * LMS is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * LMS is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with LMS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include "UUID.hpp"
+
+#include <regex>
+
+#include "Utils.hpp"
+
+static
+bool
+stringIsUUID(const std::string& str)
+{
+	static const std::regex re { R"([0-9a-fA-F]{8}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{4}\-[0-9a-fA-F]{12})"};
+
+	return std::regex_match(str, re);
+}
+
+
+template<>
+std::optional<UUID>
+readAs(const std::string& str)
+{
+	if (!stringIsUUID(str))
+		return std::nullopt;
+
+	return UUID {str};
+}
+

src/utils/UUID.hpp

@@ -0,0 +1,45 @@
+/*
+ * Copyright (C) 2020 Emeric Poupon
+ *
+ * This file is part of LMS.
+ *
+ * LMS is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * LMS is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with LMS.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#pragma once
+
+#include <string>
+#include <string_view>
+
+#include "Utils.hpp"
+
+class UUID
+{
+	public:
+
+		std::string_view getAsString() const { return _value; }
+
+	private:
+
+		template <typename UUID>
+		friend std::optional<UUID> readAs(const std::string& str);
+
+		UUID(std::string_view value) : _value {value} {}
+		std::string _value;
+};
+
+template<>
+std::optional<UUID>
+readAs(const std::string& str);
+

src/utils/Utils.cpp

@@ -20,6 +20,7 @@
 #include "Utils.hpp"
 
 #include <string>
+#include <regex>
 #include <sstream>
 #include <iomanip>
 #include <iostream>
@@ -28,7 +29,8 @@
 #include <boost/algorithm/string/join.hpp>
 #include <boost/algorithm/string.hpp>
 
-bool readList(const std::string& str, const std::string& separators, std::list<std::string>& results)
+bool
+readList(const std::string& str, const std::string& separators, std::list<std::string>& results)
 {
 	std::string curStr;
 

tools/metadata/LmsMetadata.cpp

@@ -34,8 +34,8 @@ std::ostream& operator<<(std::ostream& os, const MetaData::Artist& artist)
 {
 	os << artist.name;
 
-	if (!artist.musicBrainzArtistID.empty())
+	if (artist.musicBrainzArtistID)
-		os << " (" << artist.musicBrainzArtistID << ")";
+		os << " (" << artist.musicBrainzArtistID->getAsString() << ")";
 
 	return os;
 }
@@ -44,8 +44,8 @@ std::ostream& operator<<(std::ostream& os, const MetaData::Album& album)
 {
 	os << album.name;
 
-	if (!album.musicBrainzAlbumID.empty())
+	if (album.musicBrainzAlbumID)
-		os << " (" << album.musicBrainzAlbumID << ")";
+		os << " (" << album.musicBrainzAlbumID->getAsString() << ")";
 
 	return os;
 }
@@ -82,11 +82,11 @@ void parse(MetaData::Parser& parser, const std::filesystem::path& file)
 
 	std::cout << "Title: " << track->title << std::endl;
 
-	if (!track->musicBrainzTrackID.empty())
+	if (track->musicBrainzTrackID)
-		std::cout << "MB TrackID = " << track->musicBrainzTrackID << std::endl;
+		std::cout << "MB TrackID = " << track->musicBrainzTrackID->getAsString() << std::endl;
 
-	if (!track->musicBrainzRecordID.empty())
+	if (track->musicBrainzRecordID)
-		std::cout << "MB RecordID = " << track->musicBrainzRecordID << std::endl;
+		std::cout << "MB RecordID = " << track->musicBrainzRecordID->getAsString() << std::endl;
 
 	for (const auto& cluster : track->clusters)
 	{
@@ -122,8 +122,8 @@ void parse(MetaData::Parser& parser, const std::filesystem::path& file)
 	for (const auto& audioStream : track->audioStreams)
 		std::cout << "Audio stream: " << audioStream.bitRate << " bps" << std::endl;
 
-	if (!track->acoustID.empty())
+	if (track->acoustID)
-		std::cout << "AcoustID: " << track->acoustID << std::endl;
+		std::cout << "AcoustID: " << track->acoustID->getAsString() << std::endl;
 
 	if (!track->copyright.empty())
 		std::cout << "Copyright: " << track->copyright << std::endl;

tools/metadata/Makefile.am

@@ -3,11 +3,13 @@ bin_PROGRAMS = lms-metadata
 lms_metadata_SOURCES = \
 	$(srcdir)/LmsMetadata.cpp			\
 	$(top_srcdir)/src/av/AvInfo.cpp 		\
+	$(top_srcdir)/src/metadata/MetaData.cpp		\
 	$(top_srcdir)/src/metadata/AvFormat.cpp		\
 	$(top_srcdir)/src/metadata/TagLibParser.cpp	\
 	$(top_srcdir)/src/utils/Logger.cpp 		\
 	$(top_srcdir)/src/utils/StreamLogger.cpp 	\
-	$(top_srcdir)/src/utils/Utils.cpp
+	$(top_srcdir)/src/utils/Utils.cpp		\
+	$(top_srcdir)/src/utils/UUID.cpp
 
 lms_metadata_CXXFLAGS=-std=c++17 -I$(top_srcdir)/src -D_REENTRANT