diff --git a/src/include/mx/api/EncodingData.h b/src/include/mx/api/EncodingData.h index 4fd7c3229..55ce2213c 100644 --- a/src/include/mx/api/EncodingData.h +++ b/src/include/mx/api/EncodingData.h @@ -60,6 +60,19 @@ class EncodingData std::vector software; std::vector supportedItems; std::vector miscelaneousFields; + + // mx stamps its own provenance node (the mx URL plus the build's git + // SHA) into on every api write. The intent is to aid in diagnosing and + // fixing mx defects: when a file mx wrote turns out to be wrong, the stamp ties + // the bad output back to the exact mx build that produced it. That stamp is + // deliberately intrusive -- it injects a node the source never had into every + // file mx writes -- so this flag exists to turn it off when byte-faithful output + // is needed (e.g. reproducing a defect without mx's own marker in the diff). + // + // Defaults true, including for files parsed without the stamp. When false, + // writeToFile/writeToStream omit mx's node; the user's own entries are + // always written regardless. + bool writeMxVersion = true; }; MXAPI_EQUALS_BEGIN(SupportedItem) @@ -90,6 +103,7 @@ MXAPI_EQUALS_MEMBER(encodingDescription) MXAPI_EQUALS_MEMBER(software) MXAPI_EQUALS_MEMBER(supportedItems) MXAPI_EQUALS_MEMBER(miscelaneousFields) +MXAPI_EQUALS_MEMBER(writeMxVersion) MXAPI_EQUALS_END; MXAPI_NOT_EQUALS_AND_VECTORS(EncodingData); } // namespace api diff --git a/src/private/mx/api/DocumentManager.cpp b/src/private/mx/api/DocumentManager.cpp index 7488fda75..70f515eb1 100644 --- a/src/private/mx/api/DocumentManager.cpp +++ b/src/private/mx/api/DocumentManager.cpp @@ -23,7 +23,17 @@ namespace mx { namespace api { -using DocumentMap = std::map; +// A stored document plus the api write directives that the core model cannot +// carry. writeMxVersion governs whether writeTo*() stamps mx's provenance +// (see EncodingData::writeMxVersion); it defaults true, including for +// parsed documents (whose source never had the stamp). +struct StoredDocument +{ + mx::core::DocumentPtr document; + bool writeMxVersion = true; +}; + +using DocumentMap = std::map; namespace { @@ -160,7 +170,7 @@ Result DocumentManager::createFromFile(const std::string &filePath) auto mxdoc = std::make_shared(std::move(parsed).value()); LOCK_DOCUMENT_MANAGER - myImpl->myMap[myImpl->myCurrentId] = std::move(mxdoc); + myImpl->myMap[myImpl->myCurrentId] = StoredDocument{std::move(mxdoc), true}; return myImpl->myCurrentId++; } catch (const std::exception &e) @@ -193,7 +203,7 @@ Result DocumentManager::createFromStream(std::istream &stream) auto mxdoc = std::make_shared(std::move(parsed).value()); LOCK_DOCUMENT_MANAGER - myImpl->myMap[myImpl->myCurrentId] = std::move(mxdoc); + myImpl->myMap[myImpl->myCurrentId] = StoredDocument{std::move(mxdoc), true}; return myImpl->myCurrentId++; } catch (const std::exception &e) @@ -224,7 +234,7 @@ Result DocumentManager::createFromScore(const ScoreData &score) } LOCK_DOCUMENT_MANAGER - myImpl->myMap[myImpl->myCurrentId] = std::move(mxdoc); + myImpl->myMap[myImpl->myCurrentId] = StoredDocument{std::move(mxdoc), score.encoding.writeMxVersion}; return myImpl->myCurrentId++; } catch (const impl::WriteRefusal &refusal) @@ -256,7 +266,15 @@ Result DocumentManager::writeToFile(int documentId, const std::string &fil } pugi::xml_document xdoc; - core::serializeWithAttribution(withWriteVersion(*it->second), xdoc); + const core::Document toWrite = withWriteVersion(*it->second.document); + if (it->second.writeMxVersion) + { + core::serializeWithAttribution(toWrite, xdoc); + } + else + { + core::serialize(toWrite, xdoc); + } if (!xdoc.save_file(filePath.c_str(), " ")) { return ApiError{ResultCode::ioError, filePath, "writeToFile: could not write the file"}; @@ -286,7 +304,15 @@ Result DocumentManager::writeToStream(int documentId, std::ostream &stream } pugi::xml_document xdoc; - core::serializeWithAttribution(withWriteVersion(*it->second), xdoc); + const core::Document toWrite = withWriteVersion(*it->second.document); + if (it->second.writeMxVersion) + { + core::serializeWithAttribution(toWrite, xdoc); + } + else + { + core::serialize(toWrite, xdoc); + } xdoc.save(stream, " "); return Result{}; } @@ -315,16 +341,16 @@ Result DocumentManager::getData(int documentId) const // Convert into a local partwise copy and read that; the stored // document is untouched. The old mutate-and-restore dance (convert // the stored document to partwise, read, convert back) is gone. - if (it->second->isScoreTimewise()) + if (it->second.document->isScoreTimewise()) { - const core::ScorePartwise scorePartwise = impl::timewisePartwise(it->second->asScoreTimewise()); + const core::ScorePartwise scorePartwise = impl::timewisePartwise(it->second.document->asScoreTimewise()); impl::ScoreReader reader{scorePartwise}; auto score = reader.getScoreData(); score.musicXmlType = "timewise"; return score; } - impl::ScoreReader reader{it->second->asScorePartwise()}; + impl::ScoreReader reader{it->second.document->asScorePartwise()}; return reader.getScoreData(); } catch (const std::exception &e) @@ -360,7 +386,7 @@ mx::core::DocumentPtr DocumentManager::getDocument(int documentId) const return mx::core::DocumentPtr{}; } - return it->second; + return it->second.document; } int DocumentManager::getUniqueId() diff --git a/src/private/mx/impl/EncodingFunctions.cpp b/src/private/mx/impl/EncodingFunctions.cpp index bccc63c5a..ec75f257b 100644 --- a/src/private/mx/impl/EncodingFunctions.cpp +++ b/src/private/mx/impl/EncodingFunctions.cpp @@ -35,6 +35,17 @@ void createEncoding(const api::EncodingData &inEncoding, core::ScoreHeaderGroup encoding.addChoice(core::EncodingChoice::encoder(std::move(encoder))); } + // Emit before : this is the order the common + // authoring tools (MuseScore, Finale, Sibelius) write, so it minimizes + // child reordering on round-trip. MusicXML's encoding choice has + // no required order, so any order is schema-valid. + for (const auto &s : inEncoding.software) + { + hasIdentification = true; + hasEncoding = true; + encoding.addChoice(core::EncodingChoice::software(s)); + } + core::YyyyMmDd tryDate{inEncoding.encodingDate.year, inEncoding.encodingDate.month, inEncoding.encodingDate.day}; const bool isYearValid = inEncoding.encodingDate.year == tryDate.year(); const bool isMonthValid = inEncoding.encodingDate.month == tryDate.month(); @@ -53,13 +64,6 @@ void createEncoding(const api::EncodingData &inEncoding, core::ScoreHeaderGroup encoding.addChoice(core::EncodingChoice::encodingDescription(inEncoding.encodingDescription)); } - for (const auto &s : inEncoding.software) - { - hasIdentification = true; - hasEncoding = true; - encoding.addChoice(core::EncodingChoice::software(s)); - } - for (const auto &s : inEncoding.supportedItems) { hasIdentification = true; diff --git a/src/private/mxtest/api/CorpusRoundtripMain.cpp b/src/private/mxtest/api/CorpusRoundtripMain.cpp index 3b0a12eb1..321f9f22a 100644 --- a/src/private/mxtest/api/CorpusRoundtripMain.cpp +++ b/src/private/mxtest/api/CorpusRoundtripMain.cpp @@ -113,6 +113,56 @@ void addMxAttribution(pugi::xml_node scoreRoot) .set_value(mx::core::mxSoftwareAttribution().c_str()); } +// mx::api flattens into typed buckets (api::EncodingData) and re-emits +// its children in a fixed canonical order on write -- encoder, software, +// encoding-date, encoding-description, supports (see createEncoding in +// EncodingFunctions.cpp). A source whose lists those children in any +// other order therefore round-trips with an identical child multiset but a +// different order (issue #220). That ordering is a deliberate property of the +// simplified api, not a fidelity loss, so the comparison treats child +// order as insignificant: stable-sort both documents' children into the +// api's canonical order before comparing. Genuine drops/adds/value changes still +// surface; only pure reordering is normalized away. The stable sort keeps the +// relative order within a child kind -- multiple /, and mx's +// own trailing stamp -- which the api does preserve, so the user +// software and the stamp stay aligned on both sides (otherwise the reorder +// misaligns them into a spurious value mismatch). +// +// Must run after normalizeForComparison(): that strips inter-element whitespace, so +// 's direct children are pure elements here. +void canonicalizeEncodingChildOrder(pugi::xml_node scoreRoot) +{ + pugi::xml_node encoding = scoreRoot.child("identification").child("encoding"); + if (!encoding) + return; + + const auto rank = [](std::string_view name) -> int { + if (name == "encoder") + return 0; + if (name == "software") + return 1; + if (name == "encoding-date") + return 2; + if (name == "encoding-description") + return 3; + if (name == "supports") + return 4; + return 5; // unknown children sort last, preserving their relative order + }; + + std::vector children; + for (pugi::xml_node c = encoding.first_child(); c; c = c.next_sibling()) + children.push_back(c); + + std::stable_sort(children.begin(), children.end(), + [&](const pugi::xml_node &a, const pugi::xml_node &b) { return rank(a.name()) < rank(b.name()); }); + + // append_move pulls each node to the end; visiting the sorted vector in order + // leaves encoding's children in exactly that order. + for (const auto &c : children) + encoding.append_move(c); +} + bool hasSuffix(const std::string &name, std::string_view suffix) { return name.size() >= suffix.size() && name.compare(name.size() - suffix.size(), suffix.size(), suffix) == 0; @@ -276,6 +326,11 @@ RoundtripResult runRoundtrip(const std::string &absolutePath) mxtest::corert::Fixer fixer(absolutePath); fixer.applyToExpected(expectedDoc); + // The api re-emits children in canonical order; treat that order as + // insignificant by canonicalizing both sides before comparing (#220). + canonicalizeEncodingChildOrder(expectedDoc.document_element()); + canonicalizeEncodingChildOrder(actualDoc.document_element()); + // Compare const auto fail = mxtest::corert::compareElements(expectedDoc.document_element(), actualDoc.document_element()); if (fail.isFailure) @@ -358,6 +413,7 @@ void dumpDocuments(const std::string &absolutePath, const std::string &relPath, mxtest::corert::normalizeForComparison(expectedDoc); mxtest::corert::Fixer fixer(absolutePath); fixer.applyToExpected(expectedDoc); + canonicalizeEncodingChildOrder(expectedDoc.document_element()); if (!expectedDoc.save_file(expectedPath.c_str())) std::cerr << "dump: failed to write " << expectedPath << "\n"; @@ -414,6 +470,7 @@ void dumpDocuments(const std::string &absolutePath, const std::string &relPath, return; } mxtest::corert::normalizeForComparison(actualDoc); + canonicalizeEncodingChildOrder(actualDoc.document_element()); if (!actualDoc.save_file(actualPath.c_str())) std::cerr << "dump: failed to write " << actualPath << "\n"; } diff --git a/src/private/mxtest/api/DocumentManagerTest.cpp b/src/private/mxtest/api/DocumentManagerTest.cpp index ed74544dd..9797927fb 100644 --- a/src/private/mxtest/api/DocumentManagerTest.cpp +++ b/src/private/mxtest/api/DocumentManagerTest.cpp @@ -8,6 +8,7 @@ #include "cpul/cpulTestHarness.h" #include "mx/api/DefaultsData.h" #include "mx/api/DocumentManager.h" +#include "mx/core/Attribution.h" #include "mx/core/generated/Document.h" #include "mx/core/generated/MarginType.h" #include "mx/core/generated/PageLayout.h" @@ -398,4 +399,47 @@ TEST(RoundTrip_SupportedItems_isSupported, DocumentManager) T_END +// --- writeMxVersion: suppressing mx's provenance stamp ---------------------- +// mx stamps its own (text begins with kMxSoftwareMarker) into +// on every api write. EncodingData::writeMxVersion defaults true -- +// even for files parsed without the stamp -- but the caller may set it false to +// suppress mx's node while still emitting the user's own . + +// Serialize a ScoreData to a string via the api write path (no reload). +inline std::string writeScoreToString(const ScoreData &input) +{ + auto &docMngr = DocumentManager::getInstance(); + const auto createResult = docMngr.createFromScore(input); + REQUIRE(createResult.ok()); + const int writeId = createResult.value(); + std::ostringstream oss; + const auto writeResult = docMngr.writeToStream(writeId, oss); + REQUIRE(writeResult.ok()); + docMngr.destroyDocument(writeId); + return oss.str(); +} + +TEST(writeMxVersion_defaultsTrueAndStamps, DocumentManager) +{ + // Parsed from a real file that never carried mx's stamp; the flag still + // defaults true, so the written output gains the stamp. + ScoreData score = getScore(); + CHECK(score.encoding.writeMxVersion); + const std::string xml = writeScoreToString(score); + CHECK(xml.find(std::string{mx::core::kMxSoftwareMarker}) != std::string::npos); +} + +T_END + +TEST(writeMxVersion_offSuppressesStamp, DocumentManager) +{ + // Turn the stamp off after parsing: the written output must not contain it. + ScoreData score = getScore(); + score.encoding.writeMxVersion = false; + const std::string xml = writeScoreToString(score); + CHECK(xml.find(std::string{mx::core::kMxSoftwareMarker}) == std::string::npos); +} + +T_END + #endif diff --git a/src/private/mxtest/api/roundtrip-baseline.txt b/src/private/mxtest/api/roundtrip-baseline.txt index e873f5649..bfc9a3a02 100644 --- a/src/private/mxtest/api/roundtrip-baseline.txt +++ b/src/private/mxtest/api/roundtrip-baseline.txt @@ -93,3 +93,59 @@ lysuite/ly12a_Clefs.xml mjbsuite/ChordDirectionPlacement.xml mjbsuite/hello_timewise.xml synthetic/key-accidental.3.0.xml + +# Unblocked by #220: the api flattens into typed buckets (EncodingData) +# and re-emits its children in a fixed canonical order (encoder, encoding-date, +# encoding-description, software, supports). Real-world files list them in other +# orders, so the round-trip diverged only by child order -- a deliberate +# property of the simplified api, not a fidelity loss. The harness now canonicalizes +# child order on both sides before comparing (which also realigns the +# user against mx's trailing stamp, fixing the secondary value mismatch). +ksuite/k009b_Slur_Attributes.xml +musuite/testAccidentals1.xml +musuite/testBarStyles.xml +musuite/testChordNoVoice_ref.xml +musuite/testClefs1.xml +musuite/testCompleteMeasureRests.xml +musuite/testDCalCoda.xml +musuite/testDCalFine.xml +musuite/testDalSegno.xml +musuite/testDynamics1.xml +musuite/testDynamics2.xml +musuite/testDynamics3.xml +musuite/testDynamics3_ref.xml +musuite/testEmptyMeasure.xml +musuite/testFiguredBass1.xml +musuite/testHello.xml +musuite/testImplicitMeasure1.xml +musuite/testKeysig1.xml +musuite/testLineBreaks.xml +musuite/testManualBreaks.xml +musuite/testMeasureLength.xml +musuite/testMultiMeasureRest1_ref.xml +musuite/testMultiMeasureRest2_ref.xml +musuite/testMultiMeasureRest3_ref.xml +musuite/testMultipleNotations_ref.xml +musuite/testNonUniqueThings.xml +musuite/testNonUniqueThings_ref.xml +musuite/testNotesRests1.xml +musuite/testNotesRests2.xml +musuite/testPartsSpecialCases_ref.xml +musuite/testRestsNoType.xml +musuite/testRestsNoType_ref.xml +musuite/testSlurs.xml +musuite/testStaffTwoKeySigs.xml +musuite/testStringVoiceName_ref.xml +musuite/testSystemBrackets1.xml +musuite/testSystemBrackets2.xml +musuite/testTimesig1.xml +musuite/testTimesig3.xml +musuite/testUnusualDurations.xml +musuite/testVoiceMapper1_ref.xml +musuite/testVoiceMapper2.xml +musuite/testVoiceMapper2_ref.xml +musuite/testVoiceMapper3.xml +musuite/testVoiceMapper3_ref.xml +musuite/testVoicePiano1.xml +musuite/testVolta1.xml +musuite/testWords1.xml