From 52a0f624919f49e0d481b8960e2a29c1d1dc7054 Mon Sep 17 00:00:00 2001 From: emeric Date: Sat, 8 Feb 2020 12:38:28 +0100 Subject: [PATCH] Sanitize MBID usage: do not trust MBID coming from files' metadata --- src/Makefile.am | 3 + src/api/subsonic/SubsonicResource.cpp | 5 +- src/api/subsonic/SubsonicResponse.cpp | 2 +- src/api/subsonic/SubsonicResponse.hpp | 2 +- src/database/Artist.cpp | 10 +-- src/database/Artist.hpp | 12 +-- src/database/Release.cpp | 12 +-- src/database/Release.hpp | 11 +-- src/database/Session.cpp | 8 +- src/database/Track.cpp | 4 +- src/database/Track.hpp | 8 +- src/metadata/AvFormat.cpp | 74 ++++++++++--------- src/metadata/MetaData.cpp | 30 ++++++++ src/metadata/MetaData.hpp | 13 ++-- src/metadata/TagLibParser.cpp | 66 +++++++++-------- src/scanner/MediaScanner.cpp | 21 ++++-- .../features/AcousticBrainzUtils.cpp | 9 ++- .../features/AcousticBrainzUtils.hpp | 6 +- .../SimilarityFeaturesScannerAddon.cpp | 11 +-- .../SimilarityFeaturesScannerAddon.hpp | 5 +- src/utils/UUID.cpp | 45 +++++++++++ src/utils/UUID.hpp | 45 +++++++++++ src/utils/Utils.cpp | 4 +- tools/metadata/LmsMetadata.cpp | 20 ++--- tools/metadata/Makefile.am | 4 +- 25 files changed, 298 insertions(+), 132 deletions(-) create mode 100644 src/metadata/MetaData.cpp create mode 100644 src/utils/UUID.cpp create mode 100644 src/utils/UUID.hpp diff --git a/src/Makefile.am b/src/Makefile.am index 08572294..b89df341 100644 --- a/src/Makefile.am +++ b/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 diff --git a/src/api/subsonic/SubsonicResource.cpp b/src/api/subsonic/SubsonicResource.cpp index 733720ba..f78118aa 100644 --- a/src/api/subsonic/SubsonicResource.cpp +++ b/src/api/subsonic/SubsonicResource.cpp @@ -934,8 +934,9 @@ handleGetArtistInfoRequestCommon(RequestContext& context, bool id3) if (!artist) throw RequestedDataNotFoundError {}; - if (!artist->getMBID().empty()) - artistInfoNode.createChild("musicBrainzId").setValue(artist->getMBID()); + std::optional artistMBID {artist->getMBID()}; + if (artistMBID) + artistInfoNode.createChild("musicBrainzId").setValue(artistMBID->getAsString()); } auto similarArtistsId {ServiceProvider::get()->getSimilarArtists(context.dbSession, id.value, count)}; diff --git a/src/api/subsonic/SubsonicResponse.cpp b/src/api/subsonic/SubsonicResponse.cpp index bf1fdb12..b6b2431d 100644 --- a/src/api/subsonic/SubsonicResponse.cpp +++ b/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"}; diff --git a/src/api/subsonic/SubsonicResponse.hpp b/src/api/subsonic/SubsonicResponse.hpp index b3c0ef38..f153e6f7 100644 --- a/src/api/subsonic/SubsonicResponse.hpp +++ b/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); diff --git a/src/database/Artist.cpp b/src/database/Artist.cpp index 50dec877..88ee6cb1 100644 --- a/src/database/Artist.cpp +++ b/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& 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().where("mbid = ?").bind(mbid); + return session.getDboSession().find().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& MBID) { session.checkUniqueLocked(); diff --git a/src/database/Artist.hpp b/src/database/Artist.hpp index 9357c666..078932c5 100644 --- a/src/database/Artist.hpp +++ b/src/database/Artist.hpp @@ -26,6 +26,8 @@ #include #include +#include "utils/UUID.hpp" + #include "TrackArtistLink.hpp" #include "Types.hpp" @@ -46,10 +48,10 @@ class Artist : public Wt::Dbo::Dbo using pointer = Wt::Dbo::ptr; Artist() {} - Artist(const std::string& name, const std::string& MBID = ""); + Artist(const std::string& name, const std::optional& 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 getByName(Session& session, const std::string& name); static std::vector getByClusters(Session& session, @@ -69,7 +71,7 @@ class Artist : public Wt::Dbo::Dbo // Accessors const std::string& getName(void) const { return _name; } - const std::string& getMBID(void) const { return _MBID; } + std::optional getMBID(void) const { return readAs(_MBID); } std::vector> getReleases(const std::set& 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 // size is the max number of cluster per cluster type std::vector>> getClusterGroups(std::vector> clusterTypes, std::size_t size) const; - void setMBID(const std::string& mbid) { _MBID = mbid; } + void setMBID(const std::optional& 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 = {}); template void persist(Action& a) diff --git a/src/database/Release.cpp b/src/database/Release.cpp index bbf26aaa..a4857f15 100644 --- a/src/database/Release.cpp +++ b/src/database/Release.cpp @@ -31,9 +31,9 @@ namespace Database { -Release::Release(const std::string& name, const std::string& MBID) -: _name(std::string(name, 0 , _maxNameLength)), -_MBID(MBID) +Release::Release(const std::string& name, const std::optional& MBID) +: _name {std::string(name, 0 , _maxNameLength)}, +_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().where("mbid = ?").bind(mbid); + return session.getDboSession().find().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& MBID) { session.checkSharedLocked(); diff --git a/src/database/Release.hpp b/src/database/Release.hpp index c2c6bbf1..fdaa3ae5 100644 --- a/src/database/Release.hpp +++ b/src/database/Release.hpp @@ -23,6 +23,7 @@ #include +#include "utils/UUID.hpp" #include "TrackArtistLink.hpp" #include "Types.hpp" @@ -43,11 +44,11 @@ class Release : public Wt::Dbo::Dbo using pointer = Wt::Dbo::ptr; Release() {} - Release(const std::string& name, const std::string& MBID = ""); + Release(const std::string& name, const std::optional& 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 getByName(Session& session, const std::string& name); static pointer getById(Session& session, IdType id); static std::vector getAllOrphans(Session& session); // no track related @@ -75,7 +76,7 @@ class Release : public Wt::Dbo::Dbo std::vector>> getClusterGroups(std::vector> 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& MBID = {}); // Utility functions std::optional getReleaseYear(bool originalDate = false) const; // 0 if unknown or various @@ -88,7 +89,7 @@ class Release : public Wt::Dbo::Dbo // Accessors std::string getName() const { return _name; } - std::string getMBID() const { return _MBID; } + std::optional getMBID() const { return readAs(_MBID); } std::optional getTotalTrackNumber() const; std::optional getTotalDiscNumber() const; std::chrono::milliseconds getDuration() const; @@ -99,7 +100,7 @@ class Release : public Wt::Dbo::Dbo bool hasVariousArtists() const; std::vector getSimilarReleases(std::optional offset = {}, std::optional count = {}) const; - void setMBID(std::string mbid) { _MBID = mbid; } + void setMBID(const std::optional& mbid) { _MBID = mbid ? mbid->getAsString() : ""; } template void persist(Action& a) diff --git a/src/database/Session.cpp b/src/database/Session.cpp index 62446009..0c3aab24 100644 --- a/src/database/Session.cpp +++ b/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"; diff --git a/src/database/Track.cpp b/src/database/Track.cpp index f28d9d9c..65503bbf 100644 --- a/src/database/Track.cpp +++ b/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() - .where("mbid = ?").bind(mbid); + .where("mbid = ?").bind(std::string {mbid.getAsString()}); } Track::pointer diff --git a/src/database/Track.hpp b/src/database/Track.hpp index 898eacc3..880a6057 100644 --- a/src/database/Track.hpp +++ b/src/database/Track.hpp @@ -28,6 +28,8 @@ #include #include +#include "utils/UUID.hpp" + #include "TrackArtistLink.hpp" #include "Types.hpp" @@ -54,7 +56,7 @@ class Track : public Wt::Dbo::Dbo // 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 getSimilarTracks(Session& session, const std::set& trackIds, std::optional offset = {}, @@ -91,7 +93,7 @@ class Track : public Wt::Dbo::Dbo 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& 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 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 getMBID() const { return readAs(_MBID); } std::optional getCopyright() const; std::optional getCopyrightURL() const; std::vector> getArtists(TrackArtistLink::Type type = TrackArtistLink::Type::Artist) const; diff --git a/src/metadata/AvFormat.cpp b/src/metadata/AvFormat.cpp index d36e6b89..1e53d6c8 100644 --- a/src/metadata/AvFormat.cpp +++ b/src/metadata/AvFormat.cpp @@ -29,35 +29,54 @@ namespace MetaData using MetadataMap = std::map; -std::optional -findFirstValueOf(const MetadataMap& metadataMap, std::initializer_list tags) +template +std::optional +findFirstValueOfAs(const MetadataMap& metadataMap, std::initializer_list 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(stringTrim(it->second)); } +template <> +std::optional> +findFirstValueOfAs(const MetadataMap& metadataMap, std::initializer_list tags) +{ + std::optional str {findFirstValueOfAs(metadataMap, tags)}; + if (!str) + return std::nullopt; + + std::vector strUuids = splitString(*str, "/"); + std::vector res; + + for (const std::string strUuid : strUuids) + { + std::optional uuid {readAs(strUuid)}; + if (!uuid) + return std::nullopt; + + res.push_back(std::move(*uuid)); + } + + return res; +} + + static std::optional getAlbum(const MetadataMap& metadataMap) { std::optional res; - auto album {findFirstValueOf(metadataMap, {"ALBUM"})}; + auto album {findFirstValueOfAs(metadataMap, {"ALBUM"})}; if (!album) return res; - res = Album{*album, ""}; + auto albumMBID {findFirstValueOfAs(metadataMap, {"MUSICBRAINZ ALBUM ID", "MUSICBRAINZ_ALBUMID", "MUSICBRAINZ/ALBUM ID"})}; - auto albumMBID {findFirstValueOf(metadataMap, {"MUSICBRAINZ ALBUM ID", "MUSICBRAINZ_ALBUMID", "MUSICBRAINZ/ALBUM ID"})}; - if (!albumMBID) - return res; - - res->musicBrainzAlbumID = *albumMBID; - - return res; + return Album{*album, albumMBID}; } static @@ -66,17 +85,13 @@ getAlbumArtists(const MetadataMap& metadataMap) { std::vector res; - auto name {findFirstValueOf(metadataMap, {"ALBUM_ARTIST"})}; + auto name {findFirstValueOfAs(metadataMap, {"ALBUM_ARTIST"})}; if (!name) return res; - Artist artist {*name, ""}; + auto mbid {findFirstValueOfAs(metadataMap, {"MUSICBRAINZ ALBUM ARTIST ID", "MUSICBRAINZ/ALBUM ARTIST ID"})}; - auto mbid {findFirstValueOf(metadataMap, {"MUSICBRAINZ ALBUM ARTIST ID", "MUSICBRAINZ/ALBUM ARTIST ID"})}; - if (mbid) - artist.musicBrainzArtistID = *mbid; - - return {std::move(artist)}; + return {Artist {*name, mbid} }; } static @@ -95,21 +110,14 @@ getArtists(const MetadataMap& metadataMap) artistNames = {metadataMap.find("ARTIST")->second}; } - std::vector artistMBIDs; - { - auto mbids {findFirstValueOf(metadataMap, {"MUSICBRAINZ ARTIST ID", "MUSICBRAINZ_ARTISTID", "MUSICBRAINZ/ARTIST ID"})}; - if (mbids) - artistMBIDs = splitString(*mbids, "/"); - } + auto artistMBIDs {findFirstValueOfAs>(metadataMap, {"MUSICBRAINZ ARTIST ID", "MUSICBRAINZ_ARTISTID", "MUSICBRAINZ/ARTIST ID"})}; for (std::size_t i {}; i < artistNames.size(); ++i) { - Artist artist{std::move(artistNames[i]), ""}; - - if (artistNames.size() == artistMBIDs.size()) - artist.musicBrainzArtistID = std::move(artistMBIDs[i]); - - artists.emplace_back(std::move(artist)); + if (artistMBIDs && artistNames.size() == artistMBIDs->size()) + artists.emplace_back(Artist {artistNames[i], (*artistMBIDs)[i]}); + else + artists.emplace_back(Artist {artistNames[i], {}}); } 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(value); } else if (tag == "MUSICBRAINZ RELEASE TRACK ID" || tag == "MUSICBRAINZ_RELEASETRACKID" || tag == "MUSICBRAINZ_TRACKID" || tag == "MUSICBRAINZ/TRACK ID") { - track.musicBrainzTrackID = value; + track.musicBrainzTrackID = readAs(value); } else if (_clusterTypeNames.find(tag) != _clusterTypeNames.end()) { diff --git a/src/metadata/MetaData.cpp b/src/metadata/MetaData.cpp new file mode 100644 index 00000000..ec188d3f --- /dev/null +++ b/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 . + */ + +#include "MetaData.hpp" + +#include "utils/Utils.hpp" + +namespace MetaData +{ + + + +} // namespace MetaData + diff --git a/src/metadata/MetaData.hpp b/src/metadata/MetaData.hpp index b720c23c..011a8729 100644 --- a/src/metadata/MetaData.hpp +++ b/src/metadata/MetaData.hpp @@ -26,6 +26,9 @@ #include #include +#include "utils/Utils.hpp" +#include "utils/UUID.hpp" + namespace MetaData { using Clusters = std::map /* names */>; @@ -33,13 +36,13 @@ namespace MetaData struct Artist { std::string name; - std::string musicBrainzArtistID; + std::optional musicBrainzArtistID; }; struct Album { std::string name; - std::string musicBrainzAlbumID; + std::optional musicBrainzAlbumID; }; struct AudioStream @@ -52,8 +55,8 @@ namespace MetaData std::vector artists; std::vector albumArtists; std::string title; - std::string musicBrainzTrackID; - std::string musicBrainzRecordID; + std::optional musicBrainzTrackID; + std::optional musicBrainzRecordID; std::optional album; Clusters clusters; std::chrono::milliseconds duration {}; @@ -65,7 +68,7 @@ namespace MetaData std::optional originalYear; bool hasCover {false}; std::vector audioStreams; - std::string acoustID; + std::optional acoustID; std::string copyright; std::string copyrightURL; }; diff --git a/src/metadata/TagLibParser.cpp b/src/metadata/TagLibParser.cpp index a2151c27..add1ab55 100644 --- a/src/metadata/TagLibParser.cpp +++ b/src/metadata/TagLibParser.cpp @@ -33,10 +33,11 @@ namespace MetaData { -std::vector -getPropertyValuesFirstMatch(const TagLib::PropertyMap& properties, const std::set& keys) +template +std::vector +getPropertyValuesFirstMatchAs(const TagLib::PropertyMap& properties, const std::set& keys) { - std::vector res; + std::vector 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(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 -getPropertyValues(const TagLib::PropertyMap& properties, const std::string& key) +template +std::vector +getPropertyValuesAs(const TagLib::PropertyMap& properties, const std::string& key) { - return getPropertyValuesFirstMatch(properties, {std::move(key)}); + return getPropertyValuesFirstMatchAs(properties, {std::move(key)}); } static @@ -78,24 +88,24 @@ getArtists(const TagLib::PropertyMap& properties) { std::vector res; - std::vector artistNames {getPropertyValues(properties, "ARTISTS")}; + std::vector artistNames {getPropertyValuesAs(properties, "ARTISTS")}; if (artistNames.empty()) - artistNames = getPropertyValues(properties, "ARTIST"); + artistNames = getPropertyValuesAs(properties, "ARTIST"); if (artistNames.empty()) return res; - const std::vector artistsMBID {getPropertyValuesFirstMatch(properties, {"MUSICBRAINZ_ARTISTID", "MUSICBRAINZ ARTIST ID"})}; + const std::vector artistsMBID {getPropertyValuesFirstMatchAs(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 res; - std::vector artistNames {getPropertyValues(properties, "ALBUMARTIST")}; + std::vector artistNames {getPropertyValuesAs(properties, "ALBUMARTIST")}; if (artistNames.empty()) return res; - const std::vector artistsMBID {getPropertyValuesFirstMatch(properties, {"MUSICBRAINZ_ALBUMARTISTID", "MUSICBRAINZ ALBUM ARTIST ID"})}; + const std::vector artistsMBID {getPropertyValuesFirstMatchAs(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 getAlbum(const TagLib::PropertyMap& properties) { - std::optional res; - - std::vector albumName {getPropertyValues(properties, "ALBUM")}; + std::vector albumName {getPropertyValuesAs(properties, "ALBUM")}; if (albumName.empty()) - return res; + return std::nullopt; - std::vector albumMBID {getPropertyValuesFirstMatch(properties, {"MUSICBRAINZ_ALBUMID", "MUSICBRAINZ ALBUM ID"})}; + const std::vector albumMBID {getPropertyValuesFirstMatchAs(properties, {"MUSICBRAINZ_ALBUMID", "MUSICBRAINZ ALBUM ID"})}; - res = Album{std::move(albumName.front()), ""}; - - if (!albumMBID.empty()) - res->musicBrainzAlbumID = std::move(albumMBID.front()); - - return res; + if (albumMBID.empty()) + return Album {std::move(albumName.front()), {}}; + else + return Album {std::move(albumName.front()), albumMBID.front()}; } std::optional @@ -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(value); } else if (tag == "MUSICBRAINZ_TRACKID" || tag == "MUSICBRAINZ TRACK ID") - track.musicBrainzRecordID = value; + track.musicBrainzRecordID = readAs(value); else if (tag == "ACOUSTID_ID") - track.acoustID = value; + track.acoustID = readAs(value); else if (tag == "TRACKTOTAL") { auto totalTrack = readAs(value); diff --git a/src/scanner/MediaScanner.cpp b/src/scanner/MediaScanner.cpp index e6d2dfaf..f928331f 100644 --- a/src/scanner/MediaScanner.cpp +++ b/src/scanner/MediaScanner.cpp @@ -92,9 +92,9 @@ getOrCreateArtists(Session& session, const std::vector& 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& 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,8 +812,11 @@ MediaScanner::checkDuplicatedAudioFiles(ScanStats& stats) const std::vector 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(); - stats.duplicates.emplace_back(ScanDuplicate {track->getPath(), DuplicateReason::SameMBID}); + 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!"; diff --git a/src/similarity/features/AcousticBrainzUtils.cpp b/src/similarity/features/AcousticBrainzUtils.cpp index 26f74f3b..7e460782 100644 --- a/src/similarity/features/AcousticBrainzUtils.cpp +++ b/src/similarity/features/AcousticBrainzUtils.cpp @@ -33,12 +33,13 @@ namespace AcousticBrainz { -static std::string -getJsonData(const std::string& mbid) +static +std::string +getJsonData(const UUID& mbid) { static const std::string defaultAPIURL = "https://acousticbrainz.org/api/v1/"; - const std::string url {ServiceProvider::get()->getString("acousticbrainz-api-url", defaultAPIURL) + mbid + "/low-level"}; + const std::string url {ServiceProvider::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); } diff --git a/src/similarity/features/AcousticBrainzUtils.hpp b/src/similarity/features/AcousticBrainzUtils.hpp index 8be80f93..70d8fdf6 100644 --- a/src/similarity/features/AcousticBrainzUtils.hpp +++ b/src/similarity/features/AcousticBrainzUtils.hpp @@ -19,12 +19,12 @@ #pragma once -#include -#include #include +#include "utils/UUID.hpp" + namespace AcousticBrainz { - std::string extractLowLevelFeatures(const std::string& MBID); + std::string extractLowLevelFeatures(const UUID& MBID); } diff --git a/src/similarity/features/SimilarityFeaturesScannerAddon.cpp b/src/similarity/features/SimilarityFeaturesScannerAddon.cpp index c0f3107e..20ecf2d3 100644 --- a/src/similarity/features/SimilarityFeaturesScannerAddon.cpp +++ b/src/similarity/features/SimilarityFeaturesScannerAddon.cpp @@ -40,7 +40,7 @@ hasAtLeastOneTrackWithFeatures(Database::Session& session) struct TrackInfo { Database::IdType id; - std::string mbid; + std::optional 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 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; } diff --git a/src/similarity/features/SimilarityFeaturesScannerAddon.hpp b/src/similarity/features/SimilarityFeaturesScannerAddon.hpp index 7bbc8900..38651e37 100644 --- a/src/similarity/features/SimilarityFeaturesScannerAddon.hpp +++ b/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(); diff --git a/src/utils/UUID.cpp b/src/utils/UUID.cpp new file mode 100644 index 00000000..82e3213d --- /dev/null +++ b/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 . + */ + +#include "UUID.hpp" + +#include + +#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 +readAs(const std::string& str) +{ + if (!stringIsUUID(str)) + return std::nullopt; + + return UUID {str}; +} + diff --git a/src/utils/UUID.hpp b/src/utils/UUID.hpp new file mode 100644 index 00000000..45a2ba4f --- /dev/null +++ b/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 . + */ + +#pragma once + +#include +#include + +#include "Utils.hpp" + +class UUID +{ + public: + + std::string_view getAsString() const { return _value; } + + private: + + template + friend std::optional readAs(const std::string& str); + + UUID(std::string_view value) : _value {value} {} + std::string _value; +}; + +template<> +std::optional +readAs(const std::string& str); + diff --git a/src/utils/Utils.cpp b/src/utils/Utils.cpp index c609c091..3a98ba67 100644 --- a/src/utils/Utils.cpp +++ b/src/utils/Utils.cpp @@ -20,6 +20,7 @@ #include "Utils.hpp" #include +#include #include #include #include @@ -28,7 +29,8 @@ #include #include -bool readList(const std::string& str, const std::string& separators, std::list& results) +bool +readList(const std::string& str, const std::string& separators, std::list& results) { std::string curStr; diff --git a/tools/metadata/LmsMetadata.cpp b/tools/metadata/LmsMetadata.cpp index a3019e5f..9828c7f9 100644 --- a/tools/metadata/LmsMetadata.cpp +++ b/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()) - os << " (" << artist.musicBrainzArtistID << ")"; + if (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()) - os << " (" << album.musicBrainzAlbumID << ")"; + if (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()) - std::cout << "MB TrackID = " << track->musicBrainzTrackID << std::endl; + if (track->musicBrainzTrackID) + std::cout << "MB TrackID = " << track->musicBrainzTrackID->getAsString() << std::endl; - if (!track->musicBrainzRecordID.empty()) - std::cout << "MB RecordID = " << track->musicBrainzRecordID << std::endl; + if (track->musicBrainzRecordID) + 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()) - std::cout << "AcoustID: " << track->acoustID << std::endl; + if (track->acoustID) + std::cout << "AcoustID: " << track->acoustID->getAsString() << std::endl; if (!track->copyright.empty()) std::cout << "Copyright: " << track->copyright << std::endl; diff --git a/tools/metadata/Makefile.am b/tools/metadata/Makefile.am index 027773ec..765c4941 100644 --- a/tools/metadata/Makefile.am +++ b/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