From ee92c584b7eb54910635c86fc50a7310f281b6dc Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Fri, 18 Sep 2026 20:54:40 +0200 Subject: [PATCH] feat: check ID uniqueness and ID references MusicXML types an element id as xs:ID, which has to be unique in the document, or xs:IDREF, which has to match one. mx::core models both as Token, which keeps a value a legal name but cannot see the rest of the document, so neither rule was checked. Adds one pass over the pugixml tree in mx::core, run when a document is read and when it is written. Reading reports and changes nothing, so the parse/serialize round trip still holds. Writing renames a duplicate, leaving the id on the first element to claim it, so we do not emit an invalid document. Both report at warning through the diagnostics channel, with two new codes for a duplicate and for a reference that does not resolve. writeToFile and writeToStream gain a Diagnostics overload, which the other public entry points already had. This also resolves the standing TODO about the ID loophole that getCoreDocument opens. The element and attribute table is hand written. IdSchemaDriftTest re-derives it from the schema and fails if the two disagree, so gen/ is untouched. Closes #397 --- CMakeLists.txt | 2 + src/include/mx/api/Diagnostics.h | 4 +- src/include/mx/api/MusicXml.h | 16 +- src/private/mx/api/MusicXml.cpp | 40 ++- src/private/mx/core/IdIntegrity.cpp | 282 ++++++++++++++++++ src/private/mx/core/IdIntegrity.h | 61 ++++ src/private/mx/core/ParseContext.h | 7 +- src/private/mxtest/api/DiagnosticsTest.cpp | 70 +++++ src/private/mxtest/core/IdIntegrityTest.cpp | 198 ++++++++++++ src/private/mxtest/core/IdSchemaDriftTest.cpp | 204 +++++++++++++ 10 files changed, 875 insertions(+), 9 deletions(-) create mode 100644 src/private/mx/core/IdIntegrity.cpp create mode 100644 src/private/mx/core/IdIntegrity.h create mode 100644 src/private/mxtest/core/IdIntegrityTest.cpp create mode 100644 src/private/mxtest/core/IdSchemaDriftTest.cpp 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); + } +}