diff --git a/CMakeLists.txt b/CMakeLists.txt index fea9730e7..f68856697 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -68,6 +68,8 @@ set(MX_CORE_RUNTIME_SOURCES ${PRIVATE_DIR}/mx/core/Decimal.cpp ${PRIVATE_DIR}/mx/core/Decimal.h ${PRIVATE_DIR}/mx/core/Error.h + ${PRIVATE_DIR}/mx/core/IdIntegrity.cpp + ${PRIVATE_DIR}/mx/core/IdIntegrity.h ${PRIVATE_DIR}/mx/core/Lexical.cpp ${PRIVATE_DIR}/mx/core/Lexical.h ${PRIVATE_DIR}/mx/core/NameToken.cpp diff --git a/src/include/mx/api/Diagnostics.h b/src/include/mx/api/Diagnostics.h index 37e538c05..561bf2316 100644 --- a/src/include/mx/api/Diagnostics.h +++ b/src/include/mx/api/Diagnostics.h @@ -30,7 +30,9 @@ enum class DiagnosticCode unmatchedSpanner, // a spanner endpoint had no matching endpoint invalidValue, // a value could not be read, so a default was used missingValueDefaulted, // a required value was missing, so a default was used - droppedData // data could not be read or written, so it was left out + droppedData, // data could not be read or written, so it was left out + duplicateId, // two elements claimed one ID, which must be unique + danglingIdReference // an ID reference did not name the ID it should have }; // A non-fatal problem noticed while producing a score or MusicXML document. diff --git a/src/include/mx/api/MusicXml.h b/src/include/mx/api/MusicXml.h index 0a3eeab1b..c9b48339a 100644 --- a/src/include/mx/api/MusicXml.h +++ b/src/include/mx/api/MusicXml.h @@ -55,16 +55,28 @@ class MusicXml // are represented by an error result. Result writeToFile(const std::string &filePath) const; + // Writes the document to a file and reports what had to be adjusted to + // keep the output valid, such as an id claimed by two elements. + Result writeToFile(const std::string &filePath, Diagnostics &diagnostics) const; + // Writes the document to a character stream. Result writeToStream(std::ostream &stream) const; - // TODO: document ID validity loophole - // + // Writes the document to a character stream and reports what had to be + // adjusted to keep the output valid. + Result writeToStream(std::ostream &stream, Diagnostics &diagnostics) const; + // This is an escape hatch in case mx::api does not do what you need and // you want to edit the core DOM directly. You will need to include the // private mx::core headers in your header search paths to do so. Not // recommended, try opening an issue first! // + // An id you set here follows the same rules as one that was parsed. The + // core model can keep an id a legal name, but it cannot keep it unique, + // so that is checked when the document is written. A duplicate is + // renamed and reported. A reference that does not name an id, or that + // names the wrong kind of element, is reported and left as it is. + // // The reference is only good for as long as this MusicXml is alive and // you have not moved it away: do not keep it past a std::move of this // object into another MusicXml or into intoScore, which destroys the diff --git a/src/private/mx/api/MusicXml.cpp b/src/private/mx/api/MusicXml.cpp index 3eb25f521..c074132ae 100644 --- a/src/private/mx/api/MusicXml.cpp +++ b/src/private/mx/api/MusicXml.cpp @@ -5,6 +5,7 @@ #include "mx/api/MusicXml.h" #include "mx/core/Attribution.h" #include "mx/core/Error.h" +#include "mx/core/IdIntegrity.h" #include "mx/core/ParseContext.h" #include "mx/core/generated/Document.h" #include "mx/impl/ScoreConversions.h" @@ -86,19 +87,28 @@ DiagnosticCode mirrorToApiDiagnosticCode(core::DiagnosticCode code) return DiagnosticCode::valueAdjusted; case core::DiagnosticCode::missingValueDefaulted: return DiagnosticCode::missingValueDefaulted; + case core::DiagnosticCode::duplicateId: + return DiagnosticCode::duplicateId; + case core::DiagnosticCode::danglingIdReference: + return DiagnosticCode::danglingIdReference; } return DiagnosticCode::invalidValue; } -// Every import repair leaves a usable document, so each one is a warning. -core::ParseContext parseContextReportingTo(Diagnostics &diagnostics) +// Every repair leaves a usable document, so each one is a warning. +core::DiagnosticHandler handlerReportingTo(Diagnostics &diagnostics) { - return core::ParseContext{[&diagnostics](const core::Diagnostic &diagnostic) { + return [&diagnostics](const core::Diagnostic &diagnostic) { Location location; location.xmlPath = diagnostic.path; diagnostics.add(Diagnostic{Severity::warning, mirrorToApiDiagnosticCode(diagnostic.code), std::move(location), diagnostic.message}); - }}; + }; +} + +core::ParseContext parseContextReportingTo(Diagnostics &diagnostics) +{ + return core::ParseContext{handlerReportingTo(diagnostics)}; } // Builds the error for a caught exception. Call it inside a catch block only: @@ -209,6 +219,10 @@ Result MusicXml::fromFile(const std::string &filePath, Diagnostics &di return mirrorToApiError(parsed.error()); } + // After the parse, so that a repaired id is reported before any + // collision the repair caused. + core::checkIds(xdoc, handlerReportingTo(diagnostics)); + return MusicXml{core::Document{std::move(parsed).value()}, true}; } catch (const std::bad_alloc &) @@ -250,6 +264,10 @@ Result MusicXml::fromStream(std::istream &stream, Diagnostics &diagnos return mirrorToApiError(parsed.error()); } + // After the parse, so that a repaired id is reported before any + // collision the repair caused. + core::checkIds(xdoc, handlerReportingTo(diagnostics)); + return MusicXml{core::Document{std::move(parsed).value()}, true}; } catch (const std::bad_alloc &) @@ -267,6 +285,12 @@ Result MusicXml::fromStream(std::istream &stream, Diagnostics &diagnos } Result MusicXml::writeToFile(const std::string &filePath) const +{ + Diagnostics diagnostics; + return writeToFile(filePath, diagnostics); +} + +Result MusicXml::writeToFile(const std::string &filePath, Diagnostics &diagnostics) const { try { @@ -280,6 +304,7 @@ Result MusicXml::writeToFile(const std::string &filePath) const { core::serialize(toWrite, xdoc); } + core::repairIds(xdoc, handlerReportingTo(diagnostics)); if (!xdoc.save_file(filePath.c_str(), " ")) { return ApiError{ResultCode::ioError, Location{}, "writeToFile: could not write '" + filePath + "'"}; @@ -301,6 +326,12 @@ Result MusicXml::writeToFile(const std::string &filePath) const } Result MusicXml::writeToStream(std::ostream &stream) const +{ + Diagnostics diagnostics; + return writeToStream(stream, diagnostics); +} + +Result MusicXml::writeToStream(std::ostream &stream, Diagnostics &diagnostics) const { try { @@ -314,6 +345,7 @@ Result MusicXml::writeToStream(std::ostream &stream) const { core::serialize(toWrite, xdoc); } + core::repairIds(xdoc, handlerReportingTo(diagnostics)); xdoc.save(stream, " "); return Result{}; } diff --git a/src/private/mx/core/IdIntegrity.cpp b/src/private/mx/core/IdIntegrity.cpp new file mode 100644 index 000000000..5b852fd20 --- /dev/null +++ b/src/private/mx/core/IdIntegrity.cpp @@ -0,0 +1,282 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mx/core/IdIntegrity.h" + +#include "mx/core/Lexical.h" +#include "mx/core/Token.h" +#include "mx/core/Xml.h" + +#include +#include +#include +#include +#include +#include + +namespace mx::core +{ + +const IdAttributeKind kIdDefines = IdAttributeKind::definition; +const IdAttributeKind kIdRefers = IdAttributeKind::reference; + +// xs:ID. All but the last four come from the optional-unique-id attribute +// group; bookmark, player, score-instrument and score-part declare their own +// and require it. +const IdAttribute kIdAttributes[] = { + {"accidental-mark", "id", kIdDefines, ""}, + {"accordion-registration", "id", kIdDefines, ""}, + {"arpeggiate", "id", kIdDefines, ""}, + {"articulations", "id", kIdDefines, ""}, + {"barline", "id", kIdDefines, ""}, + {"beam", "id", kIdDefines, ""}, + {"bracket", "id", kIdDefines, ""}, + {"clef", "id", kIdDefines, ""}, + {"coda", "id", kIdDefines, ""}, + {"credit", "id", kIdDefines, ""}, + {"credit-image", "id", kIdDefines, ""}, + {"credit-symbol", "id", kIdDefines, ""}, + {"credit-words", "id", kIdDefines, ""}, + {"damp", "id", kIdDefines, ""}, + {"damp-all", "id", kIdDefines, ""}, + {"dashes", "id", kIdDefines, ""}, + {"direction", "id", kIdDefines, ""}, + {"direction-type", "id", kIdDefines, ""}, + {"dynamics", "id", kIdDefines, ""}, + {"eyeglasses", "id", kIdDefines, ""}, + {"fermata", "id", kIdDefines, ""}, + {"figured-bass", "id", kIdDefines, ""}, + {"for-part", "id", kIdDefines, ""}, + {"frame", "id", kIdDefines, ""}, + {"glissando", "id", kIdDefines, ""}, + {"grouping", "id", kIdDefines, ""}, + {"harmony", "id", kIdDefines, ""}, + {"harp-pedals", "id", kIdDefines, ""}, + {"image", "id", kIdDefines, ""}, + {"key", "id", kIdDefines, ""}, + {"lyric", "id", kIdDefines, ""}, + {"measure", "id", kIdDefines, ""}, + {"measure-style", "id", kIdDefines, ""}, + {"metronome", "id", kIdDefines, ""}, + {"non-arpeggiate", "id", kIdDefines, ""}, + {"notations", "id", kIdDefines, ""}, + {"note", "id", kIdDefines, ""}, + {"octave-shift", "id", kIdDefines, ""}, + {"ornaments", "id", kIdDefines, ""}, + {"other-direction", "id", kIdDefines, ""}, + {"other-notation", "id", kIdDefines, ""}, + {"pedal", "id", kIdDefines, ""}, + {"percussion", "id", kIdDefines, ""}, + {"principal-voice", "id", kIdDefines, ""}, + {"print", "id", kIdDefines, ""}, + {"rehearsal", "id", kIdDefines, ""}, + {"scordatura", "id", kIdDefines, ""}, + {"segno", "id", kIdDefines, ""}, + {"slide", "id", kIdDefines, ""}, + {"slur", "id", kIdDefines, ""}, + {"sound", "id", kIdDefines, ""}, + {"staff-divide", "id", kIdDefines, ""}, + {"string-mute", "id", kIdDefines, ""}, + {"symbol", "id", kIdDefines, ""}, + {"technical", "id", kIdDefines, ""}, + {"tied", "id", kIdDefines, ""}, + {"time", "id", kIdDefines, ""}, + {"transpose", "id", kIdDefines, ""}, + {"tuplet", "id", kIdDefines, ""}, + {"wedge", "id", kIdDefines, ""}, + {"words", "id", kIdDefines, ""}, + {"bookmark", "id", kIdDefines, ""}, + {"player", "id", kIdDefines, ""}, + {"score-instrument", "id", kIdDefines, ""}, + {"score-part", "id", kIdDefines, ""}, + + // xs:IDREF, with the element each one has to name. The schema says so in + // its documentation; nothing in it enforces the target. + {"assess", "player", kIdRefers, "player"}, + {"instrument", "id", kIdRefers, "score-instrument"}, + {"instrument-change", "id", kIdRefers, "score-instrument"}, + {"instrument-link", "id", kIdRefers, "score-instrument"}, + {"midi-device", "id", kIdRefers, "score-instrument"}, + {"midi-instrument", "id", kIdRefers, "score-instrument"}, + {"other-listen", "player", kIdRefers, "player"}, + {"other-listening", "player", kIdRefers, "player"}, + {"part", "id", kIdRefers, "score-part"}, + {"play", "id", kIdRefers, "score-instrument"}, + {"sync", "player", kIdRefers, "player"}, + {"wait", "player", kIdRefers, "player"}, +}; + +using IdDefinitions = std::map; + +// An element name is enough to classify: no MusicXML element declares an +// identity attribute one way in one place and the other way somewhere else. +// IdSchemaDriftTest checks that against the schema. +const IdAttribute *classifyIdAttribute(const char *elementName) +{ + static const std::unordered_map byName = [] { + std::unordered_map out; + for (const IdAttribute &entry : kIdAttributes) + { + out.emplace(entry.element, &entry); + } + return out; + }(); + const auto found = byName.find(std::string_view{elementName}); + return found == byName.end() ? nullptr : found->second; +} + +/// The value the parsed document will hold. On the way out this is the +/// identity: a serialized tree holds Token values, which are repaired +/// already. +std::string repairedIdValue(const char *text) +{ + return Token::parse(text).value(); +} + +/// Visits every element carrying an identity attribute, in document order. +template void forEachIdAttribute(pugi::xml_node node, const Fn &fn) +{ + for (pugi::xml_node child = node.first_child(); child; child = child.next_sibling()) + { + if (child.type() != pugi::node_element) + { + continue; + } + if (const IdAttribute *entry = classifyIdAttribute(child.name())) + { + if (pugi::xml_attribute attribute = child.attribute(entry->attribute)) + { + fn(child, attribute, *entry); + } + } + forEachIdAttribute(child, fn); + } +} + +void reportDanglingIdReferences(pugi::xml_node root, const IdDefinitions &definitions, const DiagnosticHandler &handler) +{ + forEachIdAttribute(root, [&](pugi::xml_node el, pugi::xml_attribute attribute, const IdAttribute &entry) { + if (entry.kind != IdAttributeKind::reference) + { + return; + } + const std::string value = repairedIdValue(attribute.value()); + std::string message = "ID reference \""; + message += value; + message += "\" in attribute \""; + message += entry.attribute; + message += "\" "; + const auto found = definitions.find(value); + if (found == definitions.end()) + { + message += "does not match an ID in the document"; + } + else if (std::string_view{found->second.name()} != entry.target) + { + message += "names a <"; + message += found->second.name(); + message += ">, not a <"; + message += entry.target; + message += ">"; + } + else + { + return; + } + handler(Diagnostic{DiagnosticCode::danglingIdReference, nodePath(el), std::move(message)}); + }); +} + +std::string freshIdValue(const std::string &base, const std::set &taken) +{ + for (int suffix = 2;; ++suffix) + { + std::string candidate = base + "-" + formatInt(suffix); + if (taken.find(candidate) == taken.end()) + { + return candidate; + } + } +} + +std::span idAttributes() +{ + return std::span{kIdAttributes}; +} + +void checkIds(const pugi::xml_document &doc, const DiagnosticHandler &handler) +{ + if (!handler) + { + // Reading changes nothing, so there is nothing to do when nobody is + // listening. + return; + } + + IdDefinitions definitions; + forEachIdAttribute(doc, [&](pugi::xml_node el, pugi::xml_attribute attribute, const IdAttribute &entry) { + if (entry.kind != IdAttributeKind::definition) + { + return; + } + const auto [found, inserted] = definitions.emplace(repairedIdValue(attribute.value()), el); + if (!inserted) + { + std::string message = "duplicate ID \""; + message += found->first; + message += "\"; "; + message += nodePath(found->second); + message += " already uses it"; + handler(Diagnostic{DiagnosticCode::duplicateId, nodePath(el), std::move(message)}); + } + }); + + reportDanglingIdReferences(doc, definitions, handler); +} + +void repairIds(pugi::xml_document &doc, const DiagnosticHandler &handler) +{ + // Collect every id first, so a rename cannot land on one that appears + // further down the document. + std::set taken; + forEachIdAttribute(doc, [&](pugi::xml_node, pugi::xml_attribute attribute, const IdAttribute &entry) { + if (entry.kind == IdAttributeKind::definition) + { + taken.insert(repairedIdValue(attribute.value())); + } + }); + + IdDefinitions definitions; + forEachIdAttribute(doc, [&](pugi::xml_node el, pugi::xml_attribute attribute, const IdAttribute &entry) { + if (entry.kind != IdAttributeKind::definition) + { + return; + } + const std::string value = repairedIdValue(attribute.value()); + if (definitions.emplace(value, el).second) + { + return; + } + const std::string replacement = freshIdValue(value, taken); + attribute.set_value(replacement.c_str()); + taken.insert(replacement); + definitions.emplace(replacement, el); + if (handler) + { + std::string message = "duplicate ID \""; + message += value; + message += "\"; renamed to \""; + message += replacement; + message += "\""; + handler(Diagnostic{DiagnosticCode::duplicateId, nodePath(el), std::move(message)}); + } + }); + + if (handler) + { + reportDanglingIdReferences(doc, definitions, handler); + } +} + +} // namespace mx::core diff --git a/src/private/mx/core/IdIntegrity.h b/src/private/mx/core/IdIntegrity.h new file mode 100644 index 000000000..a1eb7deab --- /dev/null +++ b/src/private/mx/core/IdIntegrity.h @@ -0,0 +1,61 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +// Hand-written runtime for the generated mx::core model; never regenerated. + +#pragma once + +#include "mx/core/ParseContext.h" + +#include "pugixml.hpp" + +#include + +namespace mx::core +{ + +/// How the schema types an element's identity attribute. +enum class IdAttributeKind +{ + definition, // xs:ID: no other element in the document may use the value + reference // xs:IDREF: the value must be some element's xs:ID +}; + +struct IdAttribute +{ + const char *element; + const char *attribute; + IdAttributeKind kind; + /// For a reference, the element the value must name; empty otherwise. + /// MusicXML states this in prose only: XSD can require that an ID exist + /// but not that it belong to a particular element. + const char *target; +}; + +/// Every identity attribute in MusicXML 4.0, one entry per element name. +/// +/// To whoever extends this, human or coding agent: a MusicXML version that +/// adds an xs:ID or xs:IDREF attribute needs an entry here, and a new +/// reference's target has to be read out of the schema's documentation. +/// IdSchemaDriftTest re-derives these entries from the schema and fails while +/// the two disagree, but the schema does not state a target, so check that +/// one by hand. +std::span idAttributes(); + +/// Reports duplicate xs:ID values and xs:IDREF values that do not resolve. +/// Nothing is changed: the caller is owed fidelity to the file they handed +/// us, and renaming on import would break the parse/serialize round trip. +/// +/// Values are compared after Token repair, because that is what the parsed +/// document holds. Two ids that differ in the file can therefore collide +/// here, which is the collision reportIdRepair warns may happen. +void checkIds(const pugi::xml_document &doc, const DiagnosticHandler &handler); + +/// The same checks for a document about to be written, renaming duplicates +/// so we never emit a document that violates the schema. The first element +/// to claim an id keeps it and later ones are renamed; references are left +/// pointing at the first, because nothing says which was meant. +void repairIds(pugi::xml_document &doc, const DiagnosticHandler &handler); + +} // namespace mx::core diff --git a/src/private/mx/core/ParseContext.h b/src/private/mx/core/ParseContext.h index 9d599ad1c..7e865d194 100644 --- a/src/private/mx/core/ParseContext.h +++ b/src/private/mx/core/ParseContext.h @@ -18,13 +18,16 @@ namespace mx::core { -/// The repairs the lenient parsers make on import. The parsed document is -/// still valid; a diagnostic tells the caller what was changed. +/// What a diagnostic is about: the repairs the lenient parsers make on +/// import, and the document-wide ID rules no single value can enforce. The +/// document is usable either way; a diagnostic says what was found. enum class DiagnosticCode { invalidValue, valueAdjusted, missingValueDefaulted, + duplicateId, + danglingIdReference, }; struct Diagnostic diff --git a/src/private/mxtest/api/DiagnosticsTest.cpp b/src/private/mxtest/api/DiagnosticsTest.cpp index 98d371b25..a4f3a7542 100644 --- a/src/private/mxtest/api/DiagnosticsTest.cpp +++ b/src/private/mxtest/api/DiagnosticsTest.cpp @@ -349,4 +349,74 @@ TEST(tooManyConcurrentSpannersIsLocatedRefusal, Diagnostics) T_END +inline std::string diagnosticsIdXml(const std::string &partList, const std::string &partId) +{ + return R"( + + )" + + partList + R"( + + 1 + + +)"; +} + +inline std::string diagnosticsScorePart(const std::string &id) +{ + return "Music"; +} + +TEST(fromStreamReportsDuplicateId, Diagnostics) +{ + std::istringstream stream{diagnosticsIdXml(diagnosticsScorePart("P1") + diagnosticsScorePart("P1"), "P1")}; + Diagnostics diagnostics; + const auto document = MusicXml::fromStream(stream, diagnostics); + REQUIRE(document.ok()); + REQUIRE(diagnostics.all().size() == 1); + + const auto &diagnostic = diagnostics.all().front(); + CHECK(Severity::warning == diagnostic.severity); + CHECK(DiagnosticCode::duplicateId == diagnostic.code); + CHECK_EQUAL(std::string{"/score-partwise/part-list/score-part[2]"}, diagnostic.location.xmlPath); +} + +T_END + +TEST(fromStreamReportsDanglingIdReference, Diagnostics) +{ + std::istringstream stream{diagnosticsIdXml(diagnosticsScorePart("P1"), "P9")}; + Diagnostics diagnostics; + const auto document = MusicXml::fromStream(stream, diagnostics); + REQUIRE(document.ok()); + REQUIRE(diagnostics.all().size() == 1); + + const auto &diagnostic = diagnostics.all().front(); + CHECK(Severity::warning == diagnostic.severity); + CHECK(DiagnosticCode::danglingIdReference == diagnostic.code); + CHECK_EQUAL(std::string{"/score-partwise/part"}, diagnostic.location.xmlPath); +} + +T_END + +TEST(writeToStreamRenamesDuplicateId, Diagnostics) +{ + // Reading keeps the document as it was written; writing it out cannot, + // because a duplicate ID is not a MusicXML document. + std::istringstream stream{diagnosticsIdXml(diagnosticsScorePart("P1") + diagnosticsScorePart("P1"), "P1")}; + const auto document = MusicXml::fromStream(stream); + REQUIRE(document.ok()); + + Diagnostics diagnostics; + std::ostringstream written; + REQUIRE(document.value().writeToStream(written, diagnostics).ok()); + REQUIRE(diagnostics.all().size() == 1); + CHECK(DiagnosticCode::duplicateId == diagnostics.all().front().code); + CHECK(written.str().find("id=\"P1-2\"") != std::string::npos); +} + +T_END + #endif diff --git a/src/private/mxtest/core/IdIntegrityTest.cpp b/src/private/mxtest/core/IdIntegrityTest.cpp new file mode 100644 index 000000000..f7f2c19ae --- /dev/null +++ b/src/private/mxtest/core/IdIntegrityTest.cpp @@ -0,0 +1,198 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +// Document-wide ID rules: xs:ID values are unique and xs:IDREF values name +// an xs:ID of the right element. Reading reports; writing renames a +// duplicate so the document we emit is valid. + +#include "cpul/cpulTestHarness.h" + +#include "mx/core/IdIntegrity.h" + +#include "pugixml.hpp" + +#include +#include +#include + +using namespace mx::core; + +inline std::string idIntegrityScore(const std::string &partList, const std::string &parts) +{ + return "" + partList + "" + parts + ""; +} + +inline std::string idIntegrityScorePart(const std::string &id) +{ + return "Music"; +} + +inline std::string idIntegrityInstrumentPart(const std::string &partId, const std::string &instrumentId) +{ + return "Musicx"; +} + +inline std::vector idIntegrityCheck(const std::string &xml) +{ + pugi::xml_document doc; + CHECK(doc.load_string(xml.c_str())); + std::vector out; + checkIds(doc, [&out](const Diagnostic &diagnostic) { out.push_back(diagnostic); }); + return out; +} + +struct IdIntegrityRepaired +{ + std::string xml; + std::vector diagnostics; +}; + +inline IdIntegrityRepaired idIntegrityRepair(const std::string &xml) +{ + pugi::xml_document doc; + CHECK(doc.load_string(xml.c_str())); + IdIntegrityRepaired out; + repairIds(doc, [&out](const Diagnostic &diagnostic) { out.diagnostics.push_back(diagnostic); }); + std::ostringstream stream; + doc.save(stream, "", pugi::format_raw); + out.xml = stream.str(); + return out; +} + +inline bool idIntegrityContains(const std::string &haystack, const std::string &needle) +{ + return haystack.find(needle) != std::string::npos; +} + +TEST(CleanDocumentReportsNothing, IdIntegrity) +{ + const auto diagnostics = idIntegrityCheck( + idIntegrityScore(idIntegrityScorePart("P1"), "")); + CHECK_EQUAL(std::size_t{0}, diagnostics.size()); +} + +TEST(ReportsDuplicateId, IdIntegrity) +{ + const auto diagnostics = idIntegrityCheck(idIntegrityScore(idIntegrityScorePart("P1") + idIntegrityScorePart("P1"), + "")); + CHECK_EQUAL(std::size_t{1}, diagnostics.size()); + CHECK(diagnostics.at(0).code == DiagnosticCode::duplicateId); + CHECK_EQUAL(std::string{"/score-partwise/part-list/score-part[2]"}, diagnostics.at(0).path); + CHECK(idIntegrityContains(diagnostics.at(0).message, "duplicate ID \"P1\"")); + // The message names the element that got there first. + CHECK(idIntegrityContains(diagnostics.at(0).message, "score-part[1]")); +} + +TEST(ReportsDuplicateAcrossElementKinds, IdIntegrity) +{ + // xs:ID is unique across the document, not within one kind of element. + const auto diagnostics = idIntegrityCheck( + idIntegrityScore(idIntegrityScorePart("P1"), "")); + CHECK_EQUAL(std::size_t{1}, diagnostics.size()); + CHECK(diagnostics.at(0).code == DiagnosticCode::duplicateId); + CHECK(idIntegrityContains(diagnostics.at(0).path, "measure")); +} + +TEST(ReportsCollisionMadeByRepair, IdIntegrity) +{ + // "1P1" is not a legal NCName. Token repair drops the leading digit and + // lands on an id another element already holds, so two ids that differ + // in the file are one id in the parsed document. + const auto diagnostics = idIntegrityCheck(idIntegrityScore(idIntegrityScorePart("P1") + idIntegrityScorePart("1P1"), + "")); + CHECK_EQUAL(std::size_t{1}, diagnostics.size()); + CHECK(diagnostics.at(0).code == DiagnosticCode::duplicateId); + CHECK(idIntegrityContains(diagnostics.at(0).message, "duplicate ID \"P1\"")); +} + +TEST(ReportsDanglingReference, IdIntegrity) +{ + const auto diagnostics = idIntegrityCheck( + idIntegrityScore(idIntegrityScorePart("P1"), "")); + CHECK_EQUAL(std::size_t{1}, diagnostics.size()); + CHECK(diagnostics.at(0).code == DiagnosticCode::danglingIdReference); + CHECK_EQUAL(std::string{"/score-partwise/part"}, diagnostics.at(0).path); + CHECK(idIntegrityContains(diagnostics.at(0).message, "does not match an ID in the document")); +} + +TEST(ReportsReferenceToWrongElement, IdIntegrity) +{ + // The id exists, but a has to name a . XSD cannot say + // that, so the schema says it in prose and we check it here. + const auto diagnostics = idIntegrityCheck( + idIntegrityScore(idIntegrityInstrumentPart("P1", "I1"), "")); + CHECK_EQUAL(std::size_t{1}, diagnostics.size()); + CHECK(diagnostics.at(0).code == DiagnosticCode::danglingIdReference); + CHECK(idIntegrityContains(diagnostics.at(0).message, "names a , not a ")); +} + +TEST(ReferenceToRightElementIsQuiet, IdIntegrity) +{ + const auto diagnostics = idIntegrityCheck(idIntegrityScore( + idIntegrityInstrumentPart("P1", "I1"), "" + "1" + "")); + CHECK_EQUAL(std::size_t{0}, diagnostics.size()); +} + +TEST(ReadingChangesNothing, IdIntegrity) +{ + // Renaming on import would break the parse/serialize round trip, so the + // duplicate survives the read. + const std::string xml = idIntegrityScore(idIntegrityScorePart("P1") + idIntegrityScorePart("P1"), + ""); + pugi::xml_document doc; + CHECK(doc.load_string(xml.c_str())); + checkIds(doc, [](const Diagnostic &) {}); + const pugi::xml_node first = doc.document_element().child("part-list").child("score-part"); + CHECK_EQUAL(std::string{"P1"}, std::string{first.attribute("id").value()}); + CHECK_EQUAL(std::string{"P1"}, std::string{first.next_sibling("score-part").attribute("id").value()}); +} + +TEST(WriteRenamesDuplicate, IdIntegrity) +{ + const auto repaired = idIntegrityRepair(idIntegrityScore(idIntegrityScorePart("P1") + idIntegrityScorePart("P1"), + "")); + CHECK_EQUAL(std::size_t{1}, repaired.diagnostics.size()); + CHECK(repaired.diagnostics.at(0).code == DiagnosticCode::duplicateId); + CHECK(idIntegrityContains(repaired.diagnostics.at(0).message, "renamed to \"P1-2\"")); + CHECK(idIntegrityContains(repaired.xml, "")); + CHECK(idIntegrityContains(repaired.xml, "")); + // The first element keeps the id, so the reference still resolves. + CHECK(idIntegrityContains(repaired.xml, "")); +} + +TEST(WriteRenameSkipsAnIdUsedLater, IdIntegrity) +{ + const auto repaired = idIntegrityRepair( + idIntegrityScore(idIntegrityScorePart("P1") + idIntegrityScorePart("P1") + idIntegrityScorePart("P1-2"), + "")); + CHECK_EQUAL(std::size_t{1}, repaired.diagnostics.size()); + CHECK(idIntegrityContains(repaired.diagnostics.at(0).message, "renamed to \"P1-3\"")); + CHECK(idIntegrityContains(repaired.xml, "")); +} + +TEST(WriteReportsDanglingReferenceWithoutChangingIt, IdIntegrity) +{ + const auto repaired = idIntegrityRepair( + idIntegrityScore(idIntegrityScorePart("P1"), "")); + CHECK_EQUAL(std::size_t{1}, repaired.diagnostics.size()); + CHECK(repaired.diagnostics.at(0).code == DiagnosticCode::danglingIdReference); + CHECK(idIntegrityContains(repaired.xml, "")); +} + +TEST(WriteRenamesWithoutAHandler, IdIntegrity) +{ + // The rename keeps the output valid, so it happens whether or not + // anybody is listening. + const std::string xml = idIntegrityScore(idIntegrityScorePart("P1") + idIntegrityScorePart("P1"), + ""); + pugi::xml_document doc; + CHECK(doc.load_string(xml.c_str())); + repairIds(doc, DiagnosticHandler{}); + const pugi::xml_node second = + doc.document_element().child("part-list").child("score-part").next_sibling("score-part"); + CHECK_EQUAL(std::string{"P1-2"}, std::string{second.attribute("id").value()}); +} diff --git a/src/private/mxtest/core/IdSchemaDriftTest.cpp b/src/private/mxtest/core/IdSchemaDriftTest.cpp new file mode 100644 index 000000000..363fc3e9e --- /dev/null +++ b/src/private/mxtest/core/IdSchemaDriftTest.cpp @@ -0,0 +1,204 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +// Drift guard for the hand-written identity-attribute table in +// IdIntegrity.cpp. This re-derives the table from the schema and fails while +// the two disagree, so an xs:ID or xs:IDREF added by a later MusicXML version +// gets noticed here. A reference's target element is documentation only, so +// the schema cannot be asked about it and this does not check it. + +#include "cpul/cpulTestHarness.h" + +#include "mx/core/IdIntegrity.h" + +#include "mxtest/file/PathRoot.h" + +#include "pugixml.hpp" + +#include +#include +#include +#include +#include + +using namespace mx::core; + +// (attribute name, "ID" or "IDREF"), keyed by element name. +using IdSchemaAttributes = std::set>; +using IdSchemaByElement = std::map; + +struct IdSchema +{ + std::map attributeGroups; + std::map complexTypes; +}; + +inline std::string_view idSchemaLocalName(const char *qualified) +{ + const std::string_view name{qualified}; + const auto colon = name.find(':'); + return colon == std::string_view::npos ? name : name.substr(colon + 1); +} + +/// The identity attributes a complexType or attributeGroup owns, following +/// attributeGroup references and extension bases. A child element +/// declaration owns whatever it declares, so the walk stops there. +inline void idSchemaCollectAttributes(const IdSchema &schema, pugi::xml_node node, std::set &visited, + IdSchemaAttributes &out) +{ + for (pugi::xml_node child = node.first_child(); child; child = child.next_sibling()) + { + const std::string_view tag = idSchemaLocalName(child.name()); + if (tag == "element" || tag == "annotation") + { + continue; + } + if (tag == "attribute") + { + const std::string_view type = idSchemaLocalName(child.attribute("type").value()); + if (type == "ID" || type == "IDREF") + { + out.emplace(child.attribute("name").value(), std::string{type}); + } + continue; + } + if (tag == "attributeGroup") + { + const auto found = schema.attributeGroups.find(idSchemaLocalName(child.attribute("ref").value())); + if (found != schema.attributeGroups.end() && visited.insert(found->second).second) + { + idSchemaCollectAttributes(schema, found->second, visited, out); + } + continue; + } + if (tag == "extension" || tag == "restriction") + { + const auto found = schema.complexTypes.find(idSchemaLocalName(child.attribute("base").value())); + if (found != schema.complexTypes.end() && visited.insert(found->second).second) + { + idSchemaCollectAttributes(schema, found->second, visited, out); + } + } + idSchemaCollectAttributes(schema, child, visited, out); + } +} + +/// The complexType an element declaration uses, named or inline. +inline pugi::xml_node idSchemaTypeOf(const IdSchema &schema, pugi::xml_node element) +{ + const auto named = schema.complexTypes.find(idSchemaLocalName(element.attribute("type").value())); + if (named != schema.complexTypes.end()) + { + return named->second; + } + for (pugi::xml_node child = element.first_child(); child; child = child.next_sibling()) + { + if (idSchemaLocalName(child.name()) == "complexType") + { + return child; + } + } + return pugi::xml_node{}; +} + +inline void idSchemaCollectElements(const IdSchema &schema, pugi::xml_node node, IdSchemaByElement &out) +{ + for (pugi::xml_node child = node.first_child(); child; child = child.next_sibling()) + { + if (idSchemaLocalName(child.name()) == "element" && child.attribute("name")) + { + if (const pugi::xml_node type = idSchemaTypeOf(schema, child)) + { + IdSchemaAttributes attributes; + std::set visited; + idSchemaCollectAttributes(schema, type, visited, attributes); + if (!attributes.empty()) + { + IdSchemaAttributes &entry = out[child.attribute("name").value()]; + entry.insert(attributes.begin(), attributes.end()); + } + } + } + idSchemaCollectElements(schema, child, out); + } +} + +TEST(TableMatchesSchema, IdSchemaDrift) +{ + const std::string schemaPath = std::string{MX_REPO_ROOT_PATH} + "/docs/musicxml-4.0-ed15c23.xsd"; + pugi::xml_document schemaDoc; + CHECK(schemaDoc.load_file(schemaPath.c_str())); + + IdSchema schema; + for (pugi::xml_node child = schemaDoc.document_element().first_child(); child; child = child.next_sibling()) + { + const std::string_view tag = idSchemaLocalName(child.name()); + if (!child.attribute("name")) + { + continue; + } + if (tag == "attributeGroup") + { + schema.attributeGroups.emplace(child.attribute("name").value(), child); + } + else if (tag == "complexType") + { + schema.complexTypes.emplace(child.attribute("name").value(), child); + } + } + + IdSchemaByElement derived; + idSchemaCollectElements(schema, schemaDoc, derived); + CHECK(!derived.empty()); + + IdSchemaByElement fromTable; + for (const IdAttribute &entry : idAttributes()) + { + // One entry per element name: the lookup in IdIntegrity.cpp + // classifies by name alone, so a second entry would break it. + CHECK(fromTable[entry.element].empty()); + fromTable[entry.element].emplace(entry.attribute, entry.kind == IdAttributeKind::definition ? "ID" : "IDREF"); + } + + // The schema has to agree that one name means one thing: an element + // declared in two places must type its identity attribute the same way + // in both. + for (const auto &derivedEntry : derived) + { + CHECK_EQUAL(std::size_t{1}, derivedEntry.second.size()); + } + + std::string drift; + for (const auto &derivedEntry : derived) + { + const auto found = fromTable.find(derivedEntry.first); + if (found == fromTable.end()) + { + drift += "missing from the table: " + derivedEntry.first + "\n"; + } + else if (found->second != derivedEntry.second) + { + drift += "table disagrees with the schema: " + derivedEntry.first + "\n"; + } + } + for (const auto &tableEntry : fromTable) + { + if (derived.find(tableEntry.first) == derived.end()) + { + drift += "not in the schema: " + tableEntry.first + "\n"; + } + } + CHECK_EQUAL(std::string{}, drift); +} + +TEST(EveryReferenceNamesATarget, IdSchemaDrift) +{ + // The schema cannot express the target, so nothing above catches a new + // reference entry that was left without one. + for (const IdAttribute &entry : idAttributes()) + { + const bool hasTarget = !std::string_view{entry.target}.empty(); + CHECK_EQUAL(entry.kind == IdAttributeKind::reference, hasTarget); + } +}