From e930099e9cf62a69b231fd3d39a7de10b1182b8d Mon Sep 17 00:00:00 2001 From: Matthew James Briggs Date: Sat, 12 Sep 2026 20:53:45 +0200 Subject: [PATCH] feat: carry structured location and exception cause in api errors (#432) ApiError now says where and keeps what. Location holds the place in the XML (element path, byte offset) or in the score (part, measure, staff, voice, tick); the WriteRefusal sites stamp it from the cursor already in scope. std::bad_alloc reaching the boundary is reported as ResultCode::outOfMemory; every other caught exception is internalError with the exception itself kept in std::exception_ptr cause. formatError renders an error the same way every time. --- src/include/mx/api/Result.h | 47 ++++++- src/private/mx/api/MusicXml.cpp | 75 ++++++++--- src/private/mx/api/Result.cpp | 96 ++++++++++++++ src/private/mx/impl/NoteWriter.cpp | 25 +++- src/private/mxtest/api/MusicXmlTest.cpp | 162 ++++++++++++++++++++++++ src/private/mxtest/api/ResultTest.cpp | 79 ++++++++++++ 6 files changed, 454 insertions(+), 30 deletions(-) create mode 100644 src/private/mx/api/Result.cpp create mode 100644 src/private/mxtest/api/ResultTest.cpp diff --git a/src/include/mx/api/Result.h b/src/include/mx/api/Result.h index 89ef51abc..baf1a33aa 100644 --- a/src/include/mx/api/Result.h +++ b/src/include/mx/api/Result.h @@ -5,6 +5,7 @@ #pragma once #include +#include #include #include #include @@ -31,18 +32,56 @@ enum class ResultCode tooManyElements, invalidDocument, unsupportedVersion, // mirrored from the core parse boundary - // TODO: this badly swallows exception information. we need more variants - // or something like a site and message. - internalError, // caught exception; nothing escapes (api-level) + outOfMemory, // the machine ran out of memory (api-level) + internalError, // any other caught exception (api-level) +}; + +// Where an error happened, as far as mx knows. An error may know the place +// in the XML (xmlPath, byteOffset), the place in the music (part, measure, +// staff, voice, and time), or neither. Unknown values are -1 for the +// numbers and empty for the path. +struct Location +{ + // The path through the XML elements to where the error is, e.g. + // /score-partwise/part[1]/measure[3]/note[2] + std::string xmlPath; + + // Zero-based positions in the score. -1 means mx does not know. + int partIndex = -1; + int measureIndex = -1; + int staffIndex = -1; + int voiceIndex = -1; + + // When in the music the error is, counted in ticks the way + // ScoreData::ticksPerQuarter describes. -1 means mx does not know. + int tickTimePosition = -1; + + // How many bytes into the raw XML the error is. This is set when the XML + // itself could not be parsed. -1 means mx does not know. + long long byteOffset = -1; }; struct ApiError { ResultCode code = ResultCode::internalError; - std::string path; // e.g. /score-partwise/part[1]/measure[3]/note[2] + + // Where the error happened, where mx knows it. + Location location; + + // A human-readable description of what went wrong. std::string message; + + // When mx catches an exception, the exception itself is kept here so a + // caller can look at it or throw it again. Most errors are not + // exceptions, and this is null for those. + std::exception_ptr cause = nullptr; }; +// Writes an error on one line, the same way every time, so that logs and +// test output are consistent and easy to read. +// e.g. "mx: tooManyElements at part=0 measure=0: at most 8 beam occurrences" +std::string formatError(const ApiError &error); + // A small expected-like result, the same shape as mx::core's Result. template class Result { diff --git a/src/private/mx/api/MusicXml.cpp b/src/private/mx/api/MusicXml.cpp index 16607d5ce..90682bb6f 100644 --- a/src/private/mx/api/MusicXml.cpp +++ b/src/private/mx/api/MusicXml.cpp @@ -70,12 +70,18 @@ ResultCode mirrorToApiResultCode(core::ErrorCode code) ApiError mirrorToApiError(const core::Error &error) { - return ApiError{mirrorToApiResultCode(error.code), error.path, error.message}; + Location location; + location.xmlPath = error.path; + return ApiError{mirrorToApiResultCode(error.code), location, error.message}; } -ApiError musicXmlInternalError(const char *function, const std::string &message) +// Builds the error for a caught exception. Call it inside a catch block only: +// current_exception() keeps the exception alive in `cause`, so a caller can +// look at it or throw it again. Most entry points repeat the same three +// catches (out of memory, known exception, anything else). +ApiError caughtError(const char *function, ResultCode code, const std::string &message) { - return ApiError{ResultCode::internalError, "", std::string{function} + ": " + message}; + return ApiError{code, Location{}, std::string{function} + ": " + message, std::current_exception()}; } std::string musicXmlFileExtension(const std::string &filePath) @@ -150,16 +156,19 @@ Result MusicXml::fromFile(const std::string &filePath) if (loaded.status == pugi::status_file_not_found || loaded.status == pugi::status_io_error || loaded.status == pugi::status_out_of_memory) { - return ApiError{ResultCode::ioError, filePath, loaded.description()}; + return ApiError{ResultCode::ioError, Location{}, + "could not read '" + filePath + "' (" + loaded.description() + ")"}; } + Location location; + location.byteOffset = loaded.offset; if (musicXmlFileExtension(filePath) == "mxl") { std::stringstream ss; ss << "it looks like you are trying to parse a compressed musicxml file, which is currently " << "unsupported. https://github.com/webern/mx/issues/66 (" << loaded.description() << ")"; - return ApiError{ResultCode::xmlSyntaxError, filePath, ss.str()}; + return ApiError{ResultCode::xmlSyntaxError, location, ss.str()}; } - return ApiError{ResultCode::xmlSyntaxError, filePath, loaded.description()}; + return ApiError{ResultCode::xmlSyntaxError, location, loaded.description()}; } auto parsed = core::parse(xdoc); @@ -170,13 +179,17 @@ Result MusicXml::fromFile(const std::string &filePath) return MusicXml{core::Document{std::move(parsed).value()}, true}; } + catch (const std::bad_alloc &) + { + return caughtError("MusicXml::fromFile", ResultCode::outOfMemory, "out of memory"); + } catch (const std::exception &e) { - return musicXmlInternalError("MusicXml::fromFile", e.what()); + return caughtError("MusicXml::fromFile", ResultCode::internalError, e.what()); } catch (...) { - return musicXmlInternalError("MusicXml::fromFile", "unknown exception"); + return caughtError("MusicXml::fromFile", ResultCode::internalError, "unknown exception"); } } @@ -188,7 +201,9 @@ Result MusicXml::fromStream(std::istream &stream) const pugi::xml_parse_result loaded = xdoc.load(stream, pugi::parse_default | pugi::parse_doctype); if (!loaded) { - return ApiError{ResultCode::xmlSyntaxError, "", loaded.description()}; + Location location; + location.byteOffset = loaded.offset; + return ApiError{ResultCode::xmlSyntaxError, location, loaded.description()}; } auto parsed = core::parse(xdoc); @@ -199,13 +214,17 @@ Result MusicXml::fromStream(std::istream &stream) return MusicXml{core::Document{std::move(parsed).value()}, true}; } + catch (const std::bad_alloc &) + { + return caughtError("MusicXml::fromStream", ResultCode::outOfMemory, "out of memory"); + } catch (const std::exception &e) { - return musicXmlInternalError("MusicXml::fromStream", e.what()); + return caughtError("MusicXml::fromStream", ResultCode::internalError, e.what()); } catch (...) { - return musicXmlInternalError("MusicXml::fromStream", "unknown exception"); + return caughtError("MusicXml::fromStream", ResultCode::internalError, "unknown exception"); } } @@ -225,17 +244,21 @@ Result MusicXml::writeToFile(const std::string &filePath) const } if (!xdoc.save_file(filePath.c_str(), " ")) { - return ApiError{ResultCode::ioError, filePath, "writeToFile: could not write the file"}; + return ApiError{ResultCode::ioError, Location{}, "writeToFile: could not write '" + filePath + "'"}; } return Result{}; } + catch (const std::bad_alloc &) + { + return caughtError("MusicXml::writeToFile", ResultCode::outOfMemory, "out of memory"); + } catch (const std::exception &e) { - return musicXmlInternalError("MusicXml::writeToFile", e.what()); + return caughtError("MusicXml::writeToFile", ResultCode::internalError, e.what()); } catch (...) { - return musicXmlInternalError("MusicXml::writeToFile", "unknown exception"); + return caughtError("MusicXml::writeToFile", ResultCode::internalError, "unknown exception"); } } @@ -256,13 +279,17 @@ Result MusicXml::writeToStream(std::ostream &stream) const xdoc.save(stream, " "); return Result{}; } + catch (const std::bad_alloc &) + { + return caughtError("MusicXml::writeToStream", ResultCode::outOfMemory, "out of memory"); + } catch (const std::exception &e) { - return musicXmlInternalError("MusicXml::writeToStream", e.what()); + return caughtError("MusicXml::writeToStream", ResultCode::internalError, e.what()); } catch (...) { - return musicXmlInternalError("MusicXml::writeToStream", "unknown exception"); + return caughtError("MusicXml::writeToStream", ResultCode::internalError, "unknown exception"); } } @@ -304,13 +331,17 @@ Result fromScore(const ScoreData &score) // model will not represent. return refusal.error(); } + catch (const std::bad_alloc &) + { + return caughtError("fromScore", ResultCode::outOfMemory, "out of memory"); + } catch (const std::exception &e) { - return musicXmlInternalError("fromScore", e.what()); + return caughtError("fromScore", ResultCode::internalError, e.what()); } catch (...) { - return musicXmlInternalError("fromScore", "unknown exception"); + return caughtError("fromScore", ResultCode::internalError, "unknown exception"); } } @@ -334,13 +365,17 @@ Result getScore(const MusicXml &document) impl::ScoreReader reader{coreDocument.asScorePartwise()}; return reader.getScoreData(); } + catch (const std::bad_alloc &) + { + return caughtError("getScore", ResultCode::outOfMemory, "out of memory"); + } catch (const std::exception &e) { - return musicXmlInternalError("getScore", e.what()); + return caughtError("getScore", ResultCode::internalError, e.what()); } catch (...) { - return musicXmlInternalError("getScore", "unknown exception"); + return caughtError("getScore", ResultCode::internalError, "unknown exception"); } } diff --git a/src/private/mx/api/Result.cpp b/src/private/mx/api/Result.cpp new file mode 100644 index 000000000..e6a4b8864 --- /dev/null +++ b/src/private/mx/api/Result.cpp @@ -0,0 +1,96 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mx/api/Result.h" + +namespace mx +{ +namespace api +{ +std::string formatError(const ApiError &error) +{ + const char *codeName = "internalError"; + switch (error.code) + { + case ResultCode::ioError: + codeName = "ioError"; + break; + case ResultCode::xmlSyntaxError: + codeName = "xmlSyntaxError"; + break; + case ResultCode::unknownElement: + codeName = "unknownElement"; + break; + case ResultCode::unknownAttribute: + codeName = "unknownAttribute"; + break; + case ResultCode::missingRequiredElement: + codeName = "missingRequiredElement"; + break; + case ResultCode::missingRequiredAttribute: + codeName = "missingRequiredAttribute"; + break; + case ResultCode::wrongElementOrder: + codeName = "wrongElementOrder"; + break; + case ResultCode::tooManyElements: + codeName = "tooManyElements"; + break; + case ResultCode::invalidDocument: + codeName = "invalidDocument"; + break; + case ResultCode::unsupportedVersion: + codeName = "unsupportedVersion"; + break; + case ResultCode::outOfMemory: + codeName = "outOfMemory"; + break; + case ResultCode::internalError: + codeName = "internalError"; + break; + } + + std::string text{"mx: "}; + text += codeName; + + // the place in the document or the score, if known + std::string where; + const auto appendWhere = [&where](const char *name, long long value) { + if (value < 0) + { + return; + } + if (!where.empty()) + { + where += ' '; + } + where += name; + where += '='; + where += std::to_string(value); + }; + + where += error.location.xmlPath; + appendWhere("part", error.location.partIndex); + appendWhere("measure", error.location.measureIndex); + appendWhere("staff", error.location.staffIndex); + appendWhere("voice", error.location.voiceIndex); + appendWhere("tick", error.location.tickTimePosition); + appendWhere("offset", error.location.byteOffset); + + if (!where.empty()) + { + text += " at "; + text += where; + } + + if (!error.message.empty()) + { + text += ": "; + text += error.message; + } + + return text; +} +} // namespace api +} // namespace mx diff --git a/src/private/mx/impl/NoteWriter.cpp b/src/private/mx/impl/NoteWriter.cpp index f8debbb7b..5f140fac8 100644 --- a/src/private/mx/impl/NoteWriter.cpp +++ b/src/private/mx/impl/NoteWriter.cpp @@ -65,6 +65,22 @@ core::Syllabic convertLyricSyllabicForNoteWriter(api::LyricSyllabic value) return core::Syllabic::single(); } +// The refusal sites below know where they are: the core error says why the +// note was refused, and the cursor says where in the score the note sits. +// Together they make an error a caller can act on -- the core error alone +// does not say which note was the problem. +WriteRefusal writeRefusalAt(const MeasureCursor &cursor, const core::Error &error) +{ + api::Location location; + location.xmlPath = error.path; + location.partIndex = cursor.partIndex; + location.measureIndex = cursor.measureIndex; + location.staffIndex = cursor.staffIndex; + location.voiceIndex = cursor.voiceIndex; + location.tickTimePosition = cursor.tickTimePosition; + return WriteRefusal{api::ApiError{api::ResultCode::tooManyElements, location, "NoteWriter: " + error.message}}; +} + NoteWriter::NoteWriter(const api::NoteData &inNoteData, const MeasureCursor &inCursor, const ScoreWriter &inScoreWriter, bool isPreviousNoteAChordMember, const std::vector &inSiblingNotes, int inNumVoices, const std::string &inVoiceLabel) @@ -160,8 +176,7 @@ core::Note NoteWriter::getNote(bool isStartOfChord) const { // Refuse, don't drop: the core caps // beams at 8; silently discarding the ninth would lose data. - throw WriteRefusal{api::ApiError{api::ResultCode::tooManyElements, added.error().path, - "NoteWriter: " + added.error().message}}; + throw writeRefusalAt(myCursor, added.error()); } ++beamIndex; } @@ -294,8 +309,7 @@ void NoteWriter::assembleNoteChoice() const const auto added = inner.addTie(tie); if (!added) { - throw WriteRefusal{api::ApiError{api::ResultCode::tooManyElements, added.error().path, - "NoteWriter: " + added.error().message}}; + throw writeRefusalAt(myCursor, added.error()); } } choiceObj.setGraceNoteChoice(core::GraceNoteChoice::graceNormalNoteGroup(std::move(inner))); @@ -319,8 +333,7 @@ void NoteWriter::assembleNoteChoice() const const auto added = choiceObj.addTie(tie); if (!added) { - throw WriteRefusal{api::ApiError{api::ResultCode::tooManyElements, added.error().path, - "NoteWriter: " + added.error().message}}; + throw writeRefusalAt(myCursor, added.error()); } } myOutNote.setChoice(core::NoteChoice::normalNoteGroup(std::move(choiceObj))); diff --git a/src/private/mxtest/api/MusicXmlTest.cpp b/src/private/mxtest/api/MusicXmlTest.cpp index ca8ef5e41..fbf82992a 100644 --- a/src/private/mxtest/api/MusicXmlTest.cpp +++ b/src/private/mxtest/api/MusicXmlTest.cpp @@ -14,6 +14,9 @@ #include "mx/core/generated/PageLayout.h" #include "mxtest/file/Path.h" +#include +#include + using namespace std; using namespace mx::api; @@ -578,4 +581,163 @@ TEST(writeMxVersion_offSuppressesStamp, MusicXml) T_END +// --- Error reporting ------------------------------------------------------- +// The api never lets an exception through; it returns errors. These pin what +// an error carries: the reason, the place in the score, and, for caught +// exceptions, the exception itself. + +// A ninth beam is more than the core model holds, so fromScore refuses it. +// The refusal was discovered while walking the score, so it says which note +// was the problem, not just that a limit exists somewhere. +TEST(tooManyElementsCarriesThePlaceInTheScore, MusicXml) +{ + ScoreData score; + score.ticksPerQuarter = 4; + score.parts.emplace_back(); + auto &part = score.parts.back(); + part.measures.emplace_back(); + auto &measure = part.measures.back(); + measure.staves.emplace_back(); + auto ¬e = measure.staves.back().voices[0].notes.emplace_back(); + note.durationData.durationName = DurationName::quarter; + note.durationData.durationTimeTicks = 4; + for (int i = 0; i < 9; ++i) + { + note.beams.push_back(Beam::extend); + } + + const auto result = fromScore(score); + REQUIRE(!result.ok()); + const auto &error = result.error(); + CHECK(ResultCode::tooManyElements == error.code); + CHECK_EQUAL(0, error.location.partIndex); + CHECK_EQUAL(0, error.location.measureIndex); + CHECK_EQUAL(0, error.location.staffIndex); + CHECK_EQUAL(0, error.location.voiceIndex); + CHECK_EQUAL(0, error.location.tickTimePosition); + CHECK(error.message.find("at most 8 beam occurrences") != std::string::npos); + CHECK(formatError(error).find("at part=0 measure=0 staff=0 voice=0 tick=0") != std::string::npos); + + // a refusal is a choice mx made, not a caught exception + CHECK(!error.cause); +} + +T_END + +// A stream that fails mid-read the way a broken file or connection might. +// Pugixml asks a stream where it is and how long it is before reading, so +// the stream must look seekable; every read then throws. +class ThrowingStreambuf : public std::streambuf +{ + public: + explicit ThrowingStreambuf(std::exception_ptr inToThrow) : myToThrow{inToThrow}, myPosition{0} + { + } + + protected: + pos_type seekoff(off_type offset, std::ios_base::seekdir direction, std::ios_base::openmode) override + { + if (direction == std::ios_base::beg) + { + myPosition = offset; + } + else if (direction == std::ios_base::end) + { + myPosition = size() + offset; + } + else + { + myPosition += offset; + } + return pos_type{myPosition}; + } + + pos_type seekpos(pos_type position, std::ios_base::openmode) override + { + myPosition = position; + return pos_type{myPosition}; + } + + std::streamsize xsgetn(char_type *, std::streamsize) override + { + std::rethrow_exception(myToThrow); + } + + int_type underflow() override + { + std::rethrow_exception(myToThrow); + } + + private: + std::streamsize size() const + { + return 16; + } + + std::exception_ptr myToThrow; + std::streampos myPosition; +}; + +// A std::bad_alloc that reaches the boundary is reported as outOfMemory. +// Real exhaustion is not practical to cause in a test; what matters is the +// mapping, and a bad_alloc thrown mid-parse exercises it exactly. +TEST(outOfMemoryIsReportedNotThrown, MusicXml) +{ + ThrowingStreambuf buf{std::make_exception_ptr(std::bad_alloc{})}; + std::istream stream{&buf}; + // std::istream catches exceptions thrown by its streambuf, sets badbit, + // and only rethrows if badbit is in the exception mask + stream.exceptions(std::ios_base::badbit); + const auto result = MusicXml::fromStream(stream); + + REQUIRE(!result.ok()); + CHECK_EQUAL(ResultCode::outOfMemory, result.error().code); + + // the exception itself came through, not just a message about it + REQUIRE(result.error().cause); + bool caughtBadAlloc = false; + try + { + std::rethrow_exception(result.error().cause); + } + catch (const std::bad_alloc &) + { + caughtBadAlloc = true; + } + CHECK(caughtBadAlloc); +} + +T_END + +// A stream that throws while being read is an unexpected exception: the +// boundary keeps it out of the caller's face, reports it as internalError, +// and keeps the exception in `cause` for whoever wants to look closer. +TEST(internalErrorKeepsTheException, MusicXml) +{ + ThrowingStreambuf buf{std::make_exception_ptr(std::runtime_error{"the stream failed mid-read"})}; + std::istream stream{&buf}; + stream.exceptions(std::ios_base::badbit); + const auto result = MusicXml::fromStream(stream); + + REQUIRE(!result.ok()); + CHECK_EQUAL(ResultCode::internalError, result.error().code); + + REQUIRE(result.error().cause); + bool caughtTheStreamsException = false; + std::string what; + try + { + std::rethrow_exception(result.error().cause); + } + catch (const std::exception &e) + { + caughtTheStreamsException = true; + what = e.what(); + } + CHECK(caughtTheStreamsException); + CHECK(!what.empty()); +} + +T_END + #endif diff --git a/src/private/mxtest/api/ResultTest.cpp b/src/private/mxtest/api/ResultTest.cpp new file mode 100644 index 000000000..7467bb373 --- /dev/null +++ b/src/private/mxtest/api/ResultTest.cpp @@ -0,0 +1,79 @@ +// MusicXML Class Library +// Copyright (c) by Matthew James Briggs +// Distributed under the MIT License + +#include "mxtest/control/CompileControl.h" +#ifdef MX_COMPILE_API_TESTS + +#include "cpul/cpulTestHarness.h" +#include "mx/api/Result.h" + +using namespace mx::api; + +// A fresh Location knows nothing: every position is -1. Errors built without +// a position in mind read as "unknown", never as a plausible wrong place. +TEST(locationDefaultsToUnknown, Result) +{ + const Location location{}; + CHECK_EQUAL(-1, location.partIndex); + CHECK_EQUAL(-1, location.measureIndex); + CHECK_EQUAL(-1, location.staffIndex); + CHECK_EQUAL(-1, location.voiceIndex); + CHECK_EQUAL(-1, location.tickTimePosition); + CHECK_EQUAL(-1, location.byteOffset); + CHECK(location.xmlPath.empty()); +} + +T_END + +// formatError is the one way errors are written to logs, so its output is +// pinned: an error rendered today renders the same next year. Unknown +// positions are left out rather than printed as part=-1. +TEST(formatErrorRendersCodeMessageAndPosition, Result) +{ + ApiError error{}; + error.code = ResultCode::tooManyElements; + error.location.partIndex = 0; + error.location.measureIndex = 2; + error.location.tickTimePosition = 480; + error.message = "at most 8 beam occurrences"; + CHECK_EQUAL(std::string{"mx: tooManyElements at part=0 measure=2 tick=480: " + "at most 8 beam occurrences"}, + formatError(error)); +} + +T_END + +TEST(formatErrorRendersXmlPath, Result) +{ + ApiError error{}; + error.code = ResultCode::unknownElement; + error.location.xmlPath = "/score-partwise/part[1]/measure[3]"; + error.message = "nope"; + CHECK_EQUAL(std::string{"mx: unknownElement at /score-partwise/part[1]/measure[3]: nope"}, formatError(error)); +} + +T_END + +TEST(formatErrorRendersByteOffset, Result) +{ + ApiError error{}; + error.code = ResultCode::xmlSyntaxError; + error.location.byteOffset = 12; + error.message = "Error parsing document declaration"; + CHECK_EQUAL(std::string{"mx: xmlSyntaxError at offset=12: Error parsing document declaration"}, formatError(error)); +} + +T_END + +// Nothing is known and nothing was said: no empty "at", no dangling colon. +TEST(formatErrorRendersBareCode, Result) +{ + ApiError error{}; + error.code = ResultCode::ioError; + CHECK_EQUAL(std::string{"mx: ioError"}, formatError(error)); +} + +T_END + +#endif