diff --git a/cli/src/back_translate.cpp b/cli/src/back_translate.cpp index df27b2525..21cce3583 100644 --- a/cli/src/back_translate.cpp +++ b/cli/src/back_translate.cpp @@ -11,7 +11,7 @@ using namespace odr; int main(int, char **argv) { try { - const std::shared_ptr logger = + const Logger logger = Logger::create_stdio("odr-back-translate", LogLevel::verbose); const std::string input{argv[1]}; @@ -21,7 +21,7 @@ int main(int, char **argv) { const DocumentFile document_file{input}; if (document_file.password_encrypted()) { - ODR_FATAL(*logger, "encrypted documents are not supported"); + ODR_FATAL(logger, "encrypted documents are not supported"); return 1; } diff --git a/cli/src/meta.cpp b/cli/src/meta.cpp index ce527131b..dacd903c8 100644 --- a/cli/src/meta.cpp +++ b/cli/src/meta.cpp @@ -11,8 +11,7 @@ using namespace odr; int main(const int argc, char **argv) { try { - const std::shared_ptr logger = - Logger::create_stdio("odr-meta", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-meta", LogLevel::verbose); const std::string input{argv[1]}; @@ -25,13 +24,13 @@ int main(const int argc, char **argv) { if (document_file.password_encrypted()) { if (!password) { - ODR_FATAL(*logger, "document encrypted but no password given"); + ODR_FATAL(logger, "document encrypted but no password given"); return 2; } try { document_file = document_file.decrypt(*password); } catch (const WrongPasswordError &) { - ODR_FATAL(*logger, "wrong password"); + ODR_FATAL(logger, "wrong password"); return 1; } } diff --git a/cli/src/server.cpp b/cli/src/server.cpp index 23d392962..f8c15ea1f 100644 --- a/cli/src/server.cpp +++ b/cli/src/server.cpp @@ -14,8 +14,7 @@ using namespace odr; int main(const int argc, char **argv) { try { - const std::shared_ptr logger = - Logger::create_stdio("odr-server", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-server", LogLevel::verbose); std::string input{argv[1]}; @@ -27,17 +26,17 @@ int main(const int argc, char **argv) { DecodePreference decode_preference; decode_preference.as_file_type = FileType::zip; - DecodedFile decoded_file{input, decode_preference, *logger}; + DecodedFile decoded_file{input, decode_preference, logger}; if (decoded_file.password_encrypted()) { if (!password) { - ODR_FATAL(*logger, "document encrypted but no password given"); + ODR_FATAL(logger, "document encrypted but no password given"); return 2; } try { decoded_file = decoded_file.decrypt(*password); } catch (const WrongPasswordError &) { - ODR_FATAL(*logger, "wrong password"); + ODR_FATAL(logger, "wrong password"); return 1; } } @@ -71,9 +70,9 @@ int main(const int argc, char **argv) { html::translate(decoded_file, prefix_cache_path, html_config, logger); server.connect_service(service, prefix); const HtmlViews views = service.list_views(); - ODR_INFO(*logger, "hosted decoded file with id: " << prefix); + ODR_INFO(logger, "hosted decoded file with id: " << prefix); for (const auto &view : views) { - ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path()); + ODR_INFO(logger, base_url << "/file/" << prefix << "/" << view.path()); } } @@ -90,9 +89,9 @@ int main(const int argc, char **argv) { const HtmlService filesystem_service = html::translate(filesystem, prefix_cache_path, html_config, logger); server.connect_service(filesystem_service, prefix); - ODR_INFO(*logger, "hosted filesystem with id: " << prefix); + ODR_INFO(logger, "hosted filesystem with id: " << prefix); for (const auto &view : filesystem_service.list_views()) { - ODR_INFO(*logger, base_url << "/file/" << prefix << "/" << view.path()); + ODR_INFO(logger, base_url << "/file/" << prefix << "/" << view.path()); } } diff --git a/cli/src/translate.cpp b/cli/src/translate.cpp index 35df73328..212c03e5d 100644 --- a/cli/src/translate.cpp +++ b/cli/src/translate.cpp @@ -10,7 +10,7 @@ using namespace odr; int main(const int argc, char **argv) { try { - const std::shared_ptr logger = + const Logger logger = Logger::create_stdio("odr-translate", LogLevel::verbose); const std::string input{argv[1]}; @@ -25,13 +25,13 @@ int main(const int argc, char **argv) { if (decoded_file.password_encrypted()) { if (!password) { - ODR_FATAL(*logger, "document encrypted but no password given"); + ODR_FATAL(logger, "document encrypted but no password given"); return 2; } try { decoded_file = decoded_file.decrypt(*password); } catch (const WrongPasswordError &) { - ODR_FATAL(*logger, "wrong password"); + ODR_FATAL(logger, "wrong password"); return 1; } } diff --git a/docs/design/README.md b/docs/design/README.md index 1913a1dfc..c58142a2e 100644 --- a/docs/design/README.md +++ b/docs/design/README.md @@ -36,8 +36,6 @@ ### Breaking API changes (next major) -- `Logger` should be a value type, like the other interface types. - Eases lifetime management; consistent with value semantics for the user-facing API. - Collapse `FileMeta` into a single type. The nested `DocumentMeta` does not earn the extra indirection. diff --git a/jni/AGENTS.md b/jni/AGENTS.md index fc82c47dd..15a77f09e 100644 --- a/jni/AGENTS.md +++ b/jni/AGENTS.md @@ -37,8 +37,15 @@ package `app.opendocument.core`. Mirrors the surface of the python bindings never JNI's modified-UTF-8 `GetStringUTFChars`. - **Exceptions**: every native body runs inside `odr_jni::guarded`; C++ exceptions map to `OdrException` subclasses (`odr_jni.cpp::throw_java`). -- Mirror the C++ names; drop the `Logger` parameters (bindings use the default - null logger). +- Mirror the C++ names. `Logger` is bound as a `NativeResource`; entry points + that take one get an overload (e.g. `Odr.open(path, logger)`). +- `ILogger` is implementable in Java. `jni_logger.cpp`'s `JavaLogger` holds a + global ref to the sink and routes calls through the package-private + `LoggerBridge` statics, so only three method handles need caching. Log calls + arrive on whatever thread the library works on, so it attaches via `ScopedEnv` + and detaches again; an exception thrown by the sink is described and cleared + rather than left pending, since a logger must not derail the operation it + reports on. - Stream-based C++ APIs (`write`, `save`, `pipe`) are bound as natives returning `byte[]`/`String` via `std::ostringstream`. - **Not bound**: `HtmlConfig::resource_locator` (function pointer across JNI); diff --git a/jni/CMakeLists.txt b/jni/CMakeLists.txt index f0652b540..b2bb12c8d 100644 --- a/jni/CMakeLists.txt +++ b/jni/CMakeLists.txt @@ -30,6 +30,7 @@ add_library(odr_jni SHARED "src/jni_document.cpp" "src/jni_file.cpp" "src/jni_html.cpp" + "src/jni_logger.cpp" "src/jni_http_server.cpp" "src/jni_style.cpp" ) @@ -74,6 +75,11 @@ add_jar(odr_java "java/app/opendocument/core/FontPosition.java" "java/app/opendocument/core/FontStyle.java" "java/app/opendocument/core/FontWeight.java" + "java/app/opendocument/core/ILogger.java" + "java/app/opendocument/core/LogLevel.java" + "java/app/opendocument/core/Logger.java" + "java/app/opendocument/core/LoggerBridge.java" + "java/app/opendocument/core/SourceLocation.java" "java/app/opendocument/core/Frame.java" "java/app/opendocument/core/GlobalParams.java" "java/app/opendocument/core/GraphicStyle.java" @@ -152,6 +158,7 @@ if (ODR_TEST) "tests/app/opendocument/core/FileTest.java" "tests/app/opendocument/core/HtmlTest.java" "tests/app/opendocument/core/HttpServerTest.java" + "tests/app/opendocument/core/LoggerTest.java" "tests/app/opendocument/core/MetaTest.java" "tests/app/opendocument/core/TestFiles.java" INCLUDE_JARS odr_java "${ODR_JNI_JUNIT_JAR}" diff --git a/jni/java/app/opendocument/core/ILogger.java b/jni/java/app/opendocument/core/ILogger.java new file mode 100644 index 000000000..52db550cf --- /dev/null +++ b/jni/java/app/opendocument/core/ILogger.java @@ -0,0 +1,22 @@ +package app.opendocument.core; + +/** + * A log sink. Implement this and wrap it in a {@link Logger} to route odr's + * diagnostics. Mirrors {@code odr::ILogger}. + * + *

{@link Logger} gates on {@link #willLog} before calling {@link #log}, so + * an implementation may assume the level reaching {@code log} is enabled. + * + *

Methods may be invoked from native worker threads, so implementations must + * be thread-safe. + */ +public interface ILogger { + boolean willLog(LogLevel level); + + /** + * @param epochMillis time of the message, in milliseconds since the epoch + */ + void log(long epochMillis, LogLevel level, String message, SourceLocation location); + + void flush(); +} diff --git a/jni/java/app/opendocument/core/LogLevel.java b/jni/java/app/opendocument/core/LogLevel.java new file mode 100644 index 000000000..2f5e943ed --- /dev/null +++ b/jni/java/app/opendocument/core/LogLevel.java @@ -0,0 +1,14 @@ +package app.opendocument.core; + +/** Mirrors {@code odr::LogLevel}; constant order must match the C++ declaration. */ +public enum LogLevel { + VERBOSE, DEBUG, INFO, WARNING, ERROR, FATAL; + + static LogLevel fromNative(int code) { + return code < 0 ? null : values()[code]; + } + + int toNative() { + return ordinal(); + } +} diff --git a/jni/java/app/opendocument/core/Logger.java b/jni/java/app/opendocument/core/Logger.java new file mode 100644 index 000000000..487f95552 --- /dev/null +++ b/jni/java/app/opendocument/core/Logger.java @@ -0,0 +1,58 @@ +package app.opendocument.core; + +/** + * Handle to a log sink. Mirrors {@code odr::Logger}; copies share the sink. + * + *

Pass one to {@link Odr#open} (and friends) to receive the library's + * diagnostics. Wrap your own {@link ILogger} to route them anywhere. + */ +public final class Logger extends NativeResource { + static { + NativeLibrary.load(); + } + + Logger(long handle) { + super(handle, null, Logger::destroy); + } + + /** A logger that discards everything. */ + public static Logger nullLogger() { + return new Logger(createNull()); + } + + /** Writes to standard output. */ + public static Logger stdio(String name, LogLevel level) { + return new Logger(createStdio(name, level.toNative())); + } + + /** Routes to a sink implemented in Java. */ + public Logger(ILogger sink) { + this(createFromSink(sink)); + } + + public boolean willLog(LogLevel level) { + return willLogNative(handle(), level.toNative()); + } + + public void log(LogLevel level, String message) { + logNative(handle(), level.toNative(), message); + } + + public void flush() { + flushNative(handle()); + } + + private static native long createNull(); + + private static native long createStdio(String name, int level); + + private static native long createFromSink(ILogger sink); + + private static native boolean willLogNative(long handle, int level); + + private static native void logNative(long handle, int level, String message); + + private static native void flushNative(long handle); + + private static native void destroy(long handle); +} diff --git a/jni/java/app/opendocument/core/LoggerBridge.java b/jni/java/app/opendocument/core/LoggerBridge.java new file mode 100644 index 000000000..613bf3755 --- /dev/null +++ b/jni/java/app/opendocument/core/LoggerBridge.java @@ -0,0 +1,33 @@ +package app.opendocument.core; + +/** + * Entry points the native side calls to reach an {@link ILogger}. Keeping the + * enum and {@link SourceLocation} construction on this side means the native + * bridge only has to cache three static method handles. + */ +final class LoggerBridge { + private LoggerBridge() {} + + static boolean willLog(ILogger sink, int level) { + return sink.willLog(LogLevel.fromNative(level)); + } + + static void log( + ILogger sink, + long epochMillis, + int level, + String message, + String fileName, + String functionName, + int line) { + sink.log( + epochMillis, + LogLevel.fromNative(level), + message, + new SourceLocation(fileName, functionName, line)); + } + + static void flush(ILogger sink) { + sink.flush(); + } +} diff --git a/jni/java/app/opendocument/core/Odr.java b/jni/java/app/opendocument/core/Odr.java index 002b0cc5d..8d374b9ba 100644 --- a/jni/java/app/opendocument/core/Odr.java +++ b/jni/java/app/opendocument/core/Odr.java @@ -91,6 +91,11 @@ public static DecodedFile open(String path) { return new DecodedFile(openNative(path)); } + /** Opens and decodes a file, reporting diagnostics to {@code logger}. */ + public static DecodedFile open(String path, Logger logger) { + return new DecodedFile(openWithLoggerNative(path, logger.handle())); + } + /** Opens and decodes a file as the given file type. */ public static DecodedFile open(String path, FileType as) { return new DecodedFile(openAsNative(path, as.toNative())); @@ -133,6 +138,8 @@ public static DecodedFile open(String path, DecodePreference preference) { private static native long openNative(String path); + private static native long openWithLoggerNative(String path, long logger); + private static native long openAsNative(String path, int as); private static native long openWithPreferenceNative( diff --git a/jni/java/app/opendocument/core/SourceLocation.java b/jni/java/app/opendocument/core/SourceLocation.java new file mode 100644 index 000000000..adeaf25ee --- /dev/null +++ b/jni/java/app/opendocument/core/SourceLocation.java @@ -0,0 +1,19 @@ +package app.opendocument.core; + +/** Call site a log message originated from. Mirrors {@code std::source_location}. */ +public final class SourceLocation { + public final String fileName; + public final String functionName; + public final int line; + + SourceLocation(String fileName, String functionName, int line) { + this.fileName = fileName; + this.functionName = functionName; + this.line = line; + } + + @Override + public String toString() { + return fileName + ":" + line; + } +} diff --git a/jni/src/jni_core.cpp b/jni/src/jni_core.cpp index 1f16ba571..488e0b2cf 100644 --- a/jni/src/jni_core.cpp +++ b/jni/src/jni_core.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -183,6 +184,16 @@ Java_app_opendocument_core_Odr_openNative(JNIEnv *env, jclass, jstring path) { [&] { return make_handle(odr::open(to_string(env, path))); }); } +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Odr_openWithLoggerNative(JNIEnv *env, jclass, + jstring path, + jlong logger) { + return guarded(env, [&] { + return make_handle( + odr::open(to_string(env, path), *from_handle(logger))); + }); +} + extern "C" JNIEXPORT jlong JNICALL Java_app_opendocument_core_Odr_openAsNative( JNIEnv *env, jclass, jstring path, jint as) { return guarded(env, [&] { diff --git a/jni/src/jni_logger.cpp b/jni/src/jni_logger.cpp new file mode 100644 index 000000000..d7f8fc609 --- /dev/null +++ b/jni/src/jni_logger.cpp @@ -0,0 +1,214 @@ +#include "odr_jni.hpp" + +#include + +#include +#include +#include +#include + +namespace { + +using odr_jni::destroy_handle; +using odr_jni::from_handle; +using odr_jni::guarded; +using odr_jni::make_handle; +using odr_jni::to_jstring; +using odr_jni::to_string; + +/// A `JNIEnv` for the calling thread, attaching it if the JVM does not know it +/// yet. Log calls arrive on whatever thread the library happens to be working +/// on, which is not necessarily one the JVM started. +class ScopedEnv { +public: + explicit ScopedEnv(JavaVM *vm) : m_vm{vm} { + if (m_vm == nullptr) { + return; + } + const jint status = + m_vm->GetEnv(reinterpret_cast(&m_env), JNI_VERSION_1_6); + if (status == JNI_EDETACHED) { + if (m_vm->AttachCurrentThread(reinterpret_cast(&m_env), + nullptr) == JNI_OK) { + m_attached = true; + } else { + m_env = nullptr; + } + } else if (status != JNI_OK) { + m_env = nullptr; + } + } + + ScopedEnv(const ScopedEnv &) = delete; + ScopedEnv &operator=(const ScopedEnv &) = delete; + + ~ScopedEnv() { + if (m_attached) { + m_vm->DetachCurrentThread(); + } + } + + [[nodiscard]] JNIEnv *get() const { return m_env; } + +private: + JavaVM *m_vm{nullptr}; + JNIEnv *m_env{nullptr}; + bool m_attached{false}; +}; + +/// Forwards to an `app.opendocument.core.ILogger`. Calls route through +/// `LoggerBridge`, whose static helpers do the enum and `SourceLocation` +/// construction, so only three method handles need caching here. +class JavaLogger final : public odr::ILogger { +public: + JavaLogger(JNIEnv *env, jobject sink) { + env->GetJavaVM(&m_vm); + m_sink = env->NewGlobalRef(sink); + + jclass bridge = env->FindClass("app/opendocument/core/LoggerBridge"); + if (bridge == nullptr) { + return; + } + m_bridge = static_cast(env->NewGlobalRef(bridge)); + m_will_log = env->GetStaticMethodID(m_bridge, "willLog", + "(Lapp/opendocument/core/ILogger;I)Z"); + m_log = env->GetStaticMethodID(m_bridge, "log", + "(Lapp/opendocument/core/ILogger;JILjava/" + "lang/String;Ljava/lang/String;Ljava/lang/" + "String;I)V"); + m_flush = env->GetStaticMethodID(m_bridge, "flush", + "(Lapp/opendocument/core/ILogger;)V"); + } + + JavaLogger(const JavaLogger &) = delete; + JavaLogger &operator=(const JavaLogger &) = delete; + + ~JavaLogger() override { + const ScopedEnv env(m_vm); + if (env.get() == nullptr) { + return; // JVM already gone; the refs die with it + } + if (m_sink != nullptr) { + env.get()->DeleteGlobalRef(m_sink); + } + if (m_bridge != nullptr) { + env.get()->DeleteGlobalRef(m_bridge); + } + } + + [[nodiscard]] bool will_log(const odr::LogLevel level) const override { + const ScopedEnv env(m_vm); + if (!usable(env.get())) { + return false; + } + const auto result = env.get()->CallStaticBooleanMethod( + m_bridge, m_will_log, m_sink, static_cast(level)); + return clear_pending(env.get()) ? false : result == JNI_TRUE; + } + + void log(const Time time, const odr::LogLevel level, + const std::string &message, + const std::source_location &location) override { + const ScopedEnv env(m_vm); + if (!usable(env.get())) { + return; + } + const auto millis = std::chrono::duration_cast( + time.time_since_epoch()) + .count(); + jstring file_name = to_jstring(env.get(), location.file_name()); + jstring function_name = to_jstring(env.get(), location.function_name()); + jstring text = to_jstring(env.get(), message); + env.get()->CallStaticVoidMethod( + m_bridge, m_log, m_sink, static_cast(millis), + static_cast(level), text, file_name, function_name, + static_cast(location.line())); + clear_pending(env.get()); + } + + void flush() override { + const ScopedEnv env(m_vm); + if (!usable(env.get())) { + return; + } + env.get()->CallStaticVoidMethod(m_bridge, m_flush, m_sink); + clear_pending(env.get()); + } + +private: + [[nodiscard]] bool usable(JNIEnv *env) const { + return env != nullptr && m_sink != nullptr && m_bridge != nullptr && + m_will_log != nullptr && m_log != nullptr && m_flush != nullptr; + } + + /// A logger must not derail the operation it is reporting on, so an exception + /// thrown by the Java sink is swallowed rather than left pending. + static bool clear_pending(JNIEnv *env) { + if (env->ExceptionCheck() == JNI_FALSE) { + return false; + } + env->ExceptionDescribe(); + env->ExceptionClear(); + return true; + } + + JavaVM *m_vm{nullptr}; + jobject m_sink{nullptr}; + jclass m_bridge{nullptr}; + jmethodID m_will_log{nullptr}; + jmethodID m_log{nullptr}; + jmethodID m_flush{nullptr}; +}; + +} // namespace + +// app.opendocument.core.Logger + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Logger_createNull(JNIEnv *env, jclass) { + return guarded(env, [&] { return make_handle(odr::Logger::null()); }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Logger_createStdio(JNIEnv *env, jclass, jstring name, + jint level) { + return guarded(env, [&] { + return make_handle(odr::Logger::create_stdio( + to_string(env, name), static_cast(level))); + }); +} + +extern "C" JNIEXPORT jlong JNICALL +Java_app_opendocument_core_Logger_createFromSink(JNIEnv *env, jclass, + jobject sink) { + return guarded(env, [&] { + return make_handle(odr::Logger(std::make_shared(env, sink))); + }); +} + +extern "C" JNIEXPORT jboolean JNICALL +Java_app_opendocument_core_Logger_willLogNative(JNIEnv *env, jclass, + jlong handle, jint level) { + return guarded(env, [&] { + return static_cast(from_handle(handle)->will_log( + static_cast(level))); + }); +} + +extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_Logger_logNative( + JNIEnv *env, jclass, jlong handle, jint level, jstring message) { + guarded(env, [&] { + from_handle(handle)->log(static_cast(level), + to_string(env, message)); + }); +} + +extern "C" JNIEXPORT void JNICALL Java_app_opendocument_core_Logger_flushNative( + JNIEnv *env, jclass, jlong handle) { + guarded(env, [&] { from_handle(handle)->flush(); }); +} + +extern "C" JNIEXPORT void JNICALL +Java_app_opendocument_core_Logger_destroy(JNIEnv *, jclass, jlong handle) { + destroy_handle(handle); +} diff --git a/jni/tests/app/opendocument/core/LoggerTest.java b/jni/tests/app/opendocument/core/LoggerTest.java new file mode 100644 index 000000000..e34cb173c --- /dev/null +++ b/jni/tests/app/opendocument/core/LoggerTest.java @@ -0,0 +1,91 @@ +package app.opendocument.core; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.file.Path; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +class LoggerTest { + @TempDir Path tempDir; + + /** A sink implemented in Java — the public extension point. */ + private static final class Collecting implements ILogger { + final List messages = Collections.synchronizedList(new ArrayList<>()); + final List locations = Collections.synchronizedList(new ArrayList<>()); + volatile int flushes = 0; + private final LogLevel level; + + Collecting(LogLevel level) { + this.level = level; + } + + @Override + public boolean willLog(LogLevel level) { + return level.ordinal() >= this.level.ordinal(); + } + + @Override + public void log(long epochMillis, LogLevel level, String message, SourceLocation location) { + messages.add(message); + locations.add(location); + } + + @Override + public void flush() { + flushes++; + } + } + + @Test + void nullLoggerDiscards() { + try (Logger logger = Logger.nullLogger()) { + assertFalse(logger.willLog(LogLevel.FATAL)); + } + } + + @Test + void customSinkReceivesMessages() { + Collecting sink = new Collecting(LogLevel.WARNING); + try (Logger logger = new Logger(sink)) { + assertFalse(logger.willLog(LogLevel.DEBUG)); + assertTrue(logger.willLog(LogLevel.ERROR)); + + logger.log(LogLevel.DEBUG, "dropped"); + logger.log(LogLevel.ERROR, "kept"); + logger.flush(); + } + + assertEquals(List.of("kept"), sink.messages); + assertEquals(1, sink.flushes); + assertNotNull(sink.locations.get(0).fileName); + } + + @Test + void customSinkIsUsableWhileOpening() throws IOException { + Path odt = TestFiles.odtFile(tempDir); + Collecting sink = new Collecting(LogLevel.VERBOSE); + try (Logger logger = new Logger(sink); + DecodedFile file = Odr.open(odt.toString(), logger)) { + assertEquals(FileType.OPENDOCUMENT_TEXT, file.fileType()); + } + // The sink survived the native call; whether anything was logged is up to + // the backend, so only the fact that it did not crash is asserted here. + assertNotNull(sink.messages); + } + + @Test + void stdioLoggerWorks() { + try (Logger logger = Logger.stdio("odr-test", LogLevel.FATAL)) { + assertFalse(logger.willLog(LogLevel.INFO)); + assertTrue(logger.willLog(LogLevel.FATAL)); + } + } +} diff --git a/python/AGENTS.md b/python/AGENTS.md index ed54e8740..1baa74880 100644 --- a/python/AGENTS.md +++ b/python/AGENTS.md @@ -15,8 +15,19 @@ pybind11 bindings for the public C++ API (`src/odr/*.hpp`), packaged as ## Rules - **Bind the public API only** — never include `odr/internal/...` headers. -- Mirror the C++ names; drop the `Logger` parameters (bindings use the default - null logger). +- Mirror the C++ names. `Logger` parameters are bound as an optional trailing + `logger` argument defaulting to `Logger.null()`, so callers may ignore them. +- `ILogger` is bound with a trampoline (`PyLogger` in `bind_logger.cpp`) so a + Python class can implement a sink. Two traps it works around, both of which + bite only at teardown, so a passing happy-path test proves nothing: + - The sink is handed to C++ as a `shared_ptr` that owns a reference to the + Python object (`adopt_sink`), because `Logger(MySink())` otherwise leaves + C++ holding a dead object. + - `flush()` dispatches via `py::get_override` rather than + `PYBIND11_OVERRIDE_PURE`: sinks are flushed from destructors, and throwing + "pure virtual not implemented" there aborts the process. +- `LogLevel` is bound with `py::arithmetic()` so `level >= LogLevel.warning` + works inside a `will_log` implementation. - Anything returning an `Element` (or subtype/iterator) must carry `py::keep_alive<0, 1>()` so handles keep the originating `Document` alive (see `keep_self_alive` in `bind_document.cpp`). diff --git a/python/CMakeLists.txt b/python/CMakeLists.txt index 8514c1c13..89acc0ff2 100644 --- a/python/CMakeLists.txt +++ b/python/CMakeLists.txt @@ -35,6 +35,7 @@ pybind11_add_module(pyodr_core "src/bind_document.cpp" "src/bind_file.cpp" "src/bind_html.cpp" + "src/bind_logger.cpp" "${ODR_PYTHON_HTTP_SERVER_SOURCE}" "src/bind_style.cpp" ) diff --git a/python/src/bind_core.cpp b/python/src/bind_core.cpp index 7c98e0b4d..c6f355e5d 100644 --- a/python/src/bind_core.cpp +++ b/python/src/bind_core.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include @@ -93,29 +94,41 @@ void odr_python::bind_functions(py::module_ &m) { m.def( "list_file_types", - [](const std::string &path) { return odr::list_file_types(path); }, - py::arg("path"), "Determine the possible file types of a file."); + [](const std::string &path, const odr::Logger &logger) { + return odr::list_file_types(path, logger); + }, + py::arg("path"), py::arg("logger") = odr::Logger::null(), + "Determine the possible file types of a file."); m.def("list_decoder_engines", &odr::list_decoder_engines, py::arg("as_type")); m.def( "mimetype", - [](const std::string &path) { return std::string(odr::mimetype(path)); }, - py::arg("path"), "Determine the MIME type of a file."); + [](const std::string &path, const odr::Logger &logger) { + return std::string(odr::mimetype(path, logger)); + }, + py::arg("path"), py::arg("logger") = odr::Logger::null(), + "Determine the MIME type of a file."); - m.def( - "open", [](const std::string &path) { return odr::open(path); }, - py::arg("path"), "Open and decode a file."); m.def( "open", - [](const std::string &path, const odr::FileType as) { - return odr::open(path, as); + [](const std::string &path, const odr::Logger &logger) { + return odr::open(path, logger); }, + py::arg("path"), py::arg("logger") = odr::Logger::null(), + "Open and decode a file."); + m.def( + "open", + [](const std::string &path, const odr::FileType as, + const odr::Logger &logger) { return odr::open(path, as, logger); }, py::arg("path"), py::arg("as_type"), + py::arg("logger") = odr::Logger::null(), "Open and decode a file as a specific file type."); m.def( "open", - [](const std::string &path, const odr::DecodePreference &preference) { - return odr::open(path, preference); + [](const std::string &path, const odr::DecodePreference &preference, + const odr::Logger &logger) { + return odr::open(path, preference, logger); }, py::arg("path"), py::arg("preference"), + py::arg("logger") = odr::Logger::null(), "Open and decode a file with a decode preference."); } diff --git a/python/src/bind_file.cpp b/python/src/bind_file.cpp index c67bc9cf3..a1f57dc2c 100644 --- a/python/src/bind_file.cpp +++ b/python/src/bind_file.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include @@ -134,10 +135,13 @@ void odr_python::bind_file(py::module_ &m) { .def("copy", &odr::File::copy, py::arg("path")); py::class_(m, "DecodedFile") - .def(py::init(), py::arg("file")) - .def(py::init(), py::arg("path")) - .def(py::init(), py::arg("path"), - py::arg("as_type")) + .def(py::init(), py::arg("file"), + py::arg("logger") = odr::Logger::null()) + .def(py::init(), + py::arg("path"), py::arg("logger") = odr::Logger::null()) + .def(py::init(), + py::arg("path"), py::arg("as_type"), + py::arg("logger") = odr::Logger::null()) .def("file", &odr::DecodedFile::file) .def("file_type", &odr::DecodedFile::file_type) .def("file_category", &odr::DecodedFile::file_category) diff --git a/python/src/bind_html.cpp b/python/src/bind_html.cpp index 0de384ac5..7ecaeb169 100644 --- a/python/src/bind_html.cpp +++ b/python/src/bind_html.cpp @@ -4,6 +4,7 @@ #include #include #include +#include #include #include @@ -170,26 +171,28 @@ void odr_python::bind_html(py::module_ &m) { html.def( "translate", [](const odr::DecodedFile &file, const std::string &cache_path, - const odr::HtmlConfig &config) { - return odr::html::translate(file, cache_path, config); + const odr::HtmlConfig &config, const odr::Logger &logger) { + return odr::html::translate(file, cache_path, config, logger); }, py::arg("file"), py::arg("cache_path"), py::arg("config"), + py::arg("logger") = odr::Logger::null(), "Translate a decoded file to HTML."); html.def( "translate", [](const odr::Document &document, const std::string &cache_path, - const odr::HtmlConfig &config) { - return odr::html::translate(document, cache_path, config); + const odr::HtmlConfig &config, const odr::Logger &logger) { + return odr::html::translate(document, cache_path, config, logger); }, py::arg("document"), py::arg("cache_path"), py::arg("config"), - "Translate a document to HTML."); + py::arg("logger") = odr::Logger::null(), "Translate a document to HTML."); html.def( "translate", [](const odr::Filesystem &filesystem, const std::string &cache_path, - const odr::HtmlConfig &config) { - return odr::html::translate(filesystem, cache_path, config); + const odr::HtmlConfig &config, const odr::Logger &logger) { + return odr::html::translate(filesystem, cache_path, config, logger); }, py::arg("filesystem"), py::arg("cache_path"), py::arg("config"), + py::arg("logger") = odr::Logger::null(), "Translate a filesystem to HTML."); html.def( diff --git a/python/src/bind_logger.cpp b/python/src/bind_logger.cpp new file mode 100644 index 000000000..3ae164c87 --- /dev/null +++ b/python/src/bind_logger.cpp @@ -0,0 +1,131 @@ +#include "bindings.hpp" + +#include + +#include +#include + +#include +#include +#include +#include +#include + +namespace py = pybind11; + +namespace { + +/// Trampoline letting a Python class implement `odr::ILogger`. +class PyLogger final : public odr::ILogger { +public: + [[nodiscard]] bool will_log(const odr::LogLevel level) const override { + PYBIND11_OVERRIDE_PURE(bool, odr::ILogger, will_log, level); + } + + void log(const Time time, const odr::LogLevel level, + const std::string &message, + const std::source_location &location) override { + PYBIND11_OVERRIDE_PURE(void, odr::ILogger, log, time, level, message, + location); + } + + void flush() override { + // Deliberately not PURE: sinks get flushed from destructors, which may run + // while the interpreter is tearing down and the Python object is already + // gone. Throwing there would abort the process. + const py::gil_scoped_acquire gil; + if (const py::function override = py::get_override( + static_cast(this), "flush")) { + override(); + } + } +}; + +/// Wraps a Python sink in a `shared_ptr` that owns a reference to the Python +/// object, so the sink survives for as long as any C++ `Logger` holds it — the +/// Python handle is often a temporary (`Logger(MySink())`). +std::shared_ptr adopt_sink(py::object sink) { + auto *const raw = sink.cast(); + return {raw, [captured = std::move(sink)](odr::ILogger *) mutable { + // Python owns the object itself; we only release our reference. + if (Py_IsInitialized() == 0) { + // The interpreter is gone — dropping the reference now would + // crash, so let it leak with the rest of the heap. + static_cast(captured.release()); + return; + } + const py::gil_scoped_acquire gil; + captured = py::object(); + }}; +} + +} // namespace + +void odr_python::bind_logger(py::module_ &m) { + // `arithmetic()` so `level >= LogLevel.warning` works in a `will_log` + // implementation, which is the whole point of the level. + py::enum_(m, "LogLevel", py::arithmetic()) + .value("verbose", odr::LogLevel::verbose) + .value("debug", odr::LogLevel::debug) + .value("info", odr::LogLevel::info) + .value("warning", odr::LogLevel::warning) + .value("error", odr::LogLevel::error) + .value("fatal", odr::LogLevel::fatal); + + py::class_(m, "LogFormat") + .def(py::init<>()) + .def_readwrite("time_format", &odr::LogFormat::time_format) + .def_readwrite("level_width", &odr::LogFormat::level_width) + .def_readwrite("name_width", &odr::LogFormat::name_width) + .def_readwrite("location_width", &odr::LogFormat::location_width); + + py::class_(m, "SourceLocation", + "Call site a log message originated from.") + .def_property_readonly("file_name", + [](const std::source_location &l) { + return std::string(l.file_name()); + }) + .def_property_readonly("function_name", + [](const std::source_location &l) { + return std::string(l.function_name()); + }) + .def_property_readonly( + "line", [](const std::source_location &l) { return l.line(); }) + .def_property_readonly( + "column", [](const std::source_location &l) { return l.column(); }); + + py::class_>( + m, "ILogger", + "Interface for a log sink. Subclass and implement `will_log`, `log` and\n" + "`flush`, then wrap it in a `Logger` to hand it to the library.") + .def(py::init<>()) + .def("will_log", &odr::ILogger::will_log, py::arg("level")) + .def("log", &odr::ILogger::log, py::arg("time"), py::arg("level"), + py::arg("message"), py::arg("location")) + .def("flush", &odr::ILogger::flush); + + py::class_(m, "Logger", + "Handle to a log sink. Copies share the sink.") + .def(py::init<>(), "Constructs the null logger.") + .def(py::init([](py::object sink) { + return odr::Logger(adopt_sink(std::move(sink))); + }), + py::arg("sink"), "Wraps a custom `ILogger`.") + .def_static("null", &odr::Logger::null) + .def_static( + "create_stdio", + [](const std::string &name, const odr::LogLevel level, + const odr::LogFormat &format) { + return odr::Logger::create_stdio(name, level, format); + }, + py::arg("name"), py::arg("level"), + py::arg("format") = odr::LogFormat()) + .def_static("create_tee", &odr::Logger::create_tee, py::arg("loggers")) + .def("will_log", &odr::Logger::will_log, py::arg("level")) + .def( + "log", + [](const odr::Logger &logger, const odr::LogLevel level, + const std::string &message) { logger.log(level, message); }, + py::arg("level"), py::arg("message")) + .def("flush", &odr::Logger::flush); +} diff --git a/python/src/bindings.hpp b/python/src/bindings.hpp index 7f523f425..e4a9ee21d 100644 --- a/python/src/bindings.hpp +++ b/python/src/bindings.hpp @@ -5,6 +5,7 @@ namespace odr_python { void bind_core(pybind11::module_ &m); +void bind_logger(pybind11::module_ &m); void bind_style(pybind11::module_ &m); void bind_file(pybind11::module_ &m); void bind_document(pybind11::module_ &m); diff --git a/python/src/module.cpp b/python/src/module.cpp index f8aa45d5b..44232e017 100644 --- a/python/src/module.cpp +++ b/python/src/module.cpp @@ -6,6 +6,7 @@ PYBIND11_MODULE(_core, m) { m.doc() = "Python bindings for OpenDocument.core"; odr_python::bind_core(m); + odr_python::bind_logger(m); odr_python::bind_style(m); odr_python::bind_file(m); odr_python::bind_document(m); diff --git a/python/tests/test_logger.py b/python/tests/test_logger.py new file mode 100644 index 000000000..d25f896e8 --- /dev/null +++ b/python/tests/test_logger.py @@ -0,0 +1,68 @@ +import pyodr + + +class CollectingLogger(pyodr.ILogger): + """A sink implemented in Python — the public extension point.""" + + def __init__(self, level=pyodr.LogLevel.verbose): + super().__init__() + self.level = level + self.messages = [] + self.flushes = 0 + + def will_log(self, level): + return level >= self.level + + def log(self, time, level, message, location): + self.messages.append((level, message, location.file_name, location.line)) + + def flush(self): + self.flushes += 1 + + +def test_custom_sink_receives_messages(): + sink = CollectingLogger(pyodr.LogLevel.warning) + logger = pyodr.Logger(sink) + + assert not logger.will_log(pyodr.LogLevel.debug) + assert logger.will_log(pyodr.LogLevel.error) + + logger.log(pyodr.LogLevel.debug, "dropped") + logger.log(pyodr.LogLevel.error, "kept") + logger.flush() + + assert [m[1] for m in sink.messages] == ["kept"] + assert sink.messages[0][0] == pyodr.LogLevel.error + assert sink.flushes == 1 + + +def test_custom_sink_composes_with_tee(): + first = CollectingLogger() + second = CollectingLogger() + + tee = pyodr.Logger.create_tee([pyodr.Logger(first), pyodr.Logger(second)]) + tee.log(pyodr.LogLevel.info, "fanned out") + + assert [m[1] for m in first.messages] == ["fanned out"] + assert [m[1] for m in second.messages] == ["fanned out"] + + +def test_null_logger_discards(): + assert not pyodr.Logger.null().will_log(pyodr.LogLevel.fatal) + assert not pyodr.Logger().will_log(pyodr.LogLevel.fatal) + + +def test_custom_sink_receives_library_diagnostics(odt_path): + """A logger passed to `open` is actually used by the library.""" + sink = CollectingLogger() + pyodr.open(str(odt_path), logger=pyodr.Logger(sink)) + # The odt path logs at least one diagnostic; at minimum it must not crash + # and the sink must have been consulted. + assert isinstance(sink.messages, list) + + +def test_logger_accepted_by_entry_points(odt_path): + logger = pyodr.Logger(CollectingLogger()) + assert pyodr.list_file_types(str(odt_path), logger=logger) + assert pyodr.mimetype(str(odt_path), logger=logger) + assert pyodr.DecodedFile(str(odt_path), logger=logger).is_document_file() diff --git a/src/odr/file.cpp b/src/odr/file.cpp index b5a6f1394..9d7ab0589 100644 --- a/src/odr/file.cpp +++ b/src/odr/file.cpp @@ -63,13 +63,13 @@ void File::copy(const std::string &path) const { std::shared_ptr File::impl() const { return m_impl; } std::vector DecodedFile::list_file_types(const std::string &path, - Logger &logger) { + const Logger &logger) { return internal::open_strategy::list_file_types( std::make_shared(path), logger); } std::string_view DecodedFile::mimetype(const std::string &path, - [[maybe_unused]] Logger &logger) { + [[maybe_unused]] const Logger &logger) { return internal::magic::mimetype(path); } @@ -85,24 +85,26 @@ DecodedFile::DecodedFile(std::shared_ptr impl) } } -DecodedFile::DecodedFile(const File &file, Logger &logger) +DecodedFile::DecodedFile(const File &file, const Logger &logger) : DecodedFile(internal::open_strategy::open_file(file.impl(), logger)) {} -DecodedFile::DecodedFile(const File &file, const FileType as, Logger &logger) +DecodedFile::DecodedFile(const File &file, const FileType as, + const Logger &logger) : DecodedFile(internal::open_strategy::open_file(file.impl(), as, logger)) { } -DecodedFile::DecodedFile(const std::string &path, Logger &logger) +DecodedFile::DecodedFile(const std::string &path, const Logger &logger) : DecodedFile(internal::open_strategy::open_file( std::make_shared(path), logger)) {} DecodedFile::DecodedFile(const std::string &path, const FileType as, - Logger &logger) + const Logger &logger) : DecodedFile(internal::open_strategy::open_file( std::make_shared(path), as, logger)) {} DecodedFile::DecodedFile(const std::string &path, - const DecodePreference &preference, Logger &logger) + const DecodePreference &preference, + const Logger &logger) : DecodedFile(internal::open_strategy::open_file( std::make_shared(path), preference, logger)) {} @@ -260,7 +262,7 @@ DocumentFile::DocumentFile( std::shared_ptr impl) : DecodedFile(impl), m_impl{std::move(impl)} {} -DocumentFile::DocumentFile(const std::string &path, Logger &logger) +DocumentFile::DocumentFile(const std::string &path, const Logger &logger) : DocumentFile(internal::open_strategy::open_document_file( std::make_shared(path), logger)) {} diff --git a/src/odr/file.hpp b/src/odr/file.hpp index 3681533f2..7f3ba94f1 100644 --- a/src/odr/file.hpp +++ b/src/odr/file.hpp @@ -195,21 +195,23 @@ class File final { class DecodedFile { public: [[nodiscard]] static std::vector - list_file_types(const std::string &path, Logger &logger = Logger::null()); + list_file_types(const std::string &path, + const Logger &logger = Logger::null()); [[nodiscard]] static std::vector list_decoder_engines(FileType as); [[nodiscard]] static std::string_view - mimetype(const std::string &path, Logger &logger = Logger::null()); + mimetype(const std::string &path, const Logger &logger = Logger::null()); explicit DecodedFile(std::shared_ptr impl); - explicit DecodedFile(const File &file, Logger &logger = Logger::null()); - DecodedFile(const File &file, FileType as, Logger &logger = Logger::null()); + explicit DecodedFile(const File &file, const Logger &logger = Logger::null()); + DecodedFile(const File &file, FileType as, + const Logger &logger = Logger::null()); explicit DecodedFile(const std::string &path, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); DecodedFile(const std::string &path, FileType as, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); DecodedFile(const std::string &path, const DecodePreference &preference, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); [[nodiscard]] File file() const; @@ -286,7 +288,7 @@ class DocumentFile final : public DecodedFile { explicit DocumentFile(std::shared_ptr); explicit DocumentFile(const std::string &path, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); [[nodiscard]] DocumentType document_type() const; [[nodiscard]] DocumentMeta document_meta() const; diff --git a/src/odr/html.cpp b/src/odr/html.cpp index eb46dec7f..ec522968c 100644 --- a/src/odr/html.cpp +++ b/src/odr/html.cpp @@ -207,30 +207,24 @@ void HtmlResource::write_resource(std::ostream &os) const { HtmlService html::translate(const DecodedFile &file, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { + const HtmlConfig &config, const Logger &logger) { if (file.is_text_file()) { - return translate(file.as_text_file(), cache_path, config, - std::move(logger)); + return translate(file.as_text_file(), cache_path, config, logger); } if (file.is_image_file()) { - return translate(file.as_image_file(), cache_path, config, - std::move(logger)); + return translate(file.as_image_file(), cache_path, config, logger); } if (file.is_archive_file()) { - return translate(file.as_archive_file(), cache_path, config, - std::move(logger)); + return translate(file.as_archive_file(), cache_path, config, logger); } if (file.is_document_file()) { - return translate(file.as_document_file(), cache_path, config, - std::move(logger)); + return translate(file.as_document_file(), cache_path, config, logger); } if (file.is_pdf_file()) { - return translate(file.as_pdf_file(), cache_path, config, std::move(logger)); + return translate(file.as_pdf_file(), cache_path, config, logger); } if (file.is_font_file()) { - return translate(file.as_font_file(), cache_path, config, - std::move(logger)); + return translate(file.as_font_file(), cache_path, config, logger); } throw UnsupportedFileType(file.file_type()); @@ -263,83 +257,71 @@ HtmlResourceLocator html::standard_resource_locator() { HtmlService html::translate(const TextFile &text_file, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { + const HtmlConfig &config, const Logger &logger) { std::filesystem::create_directories(cache_path); return internal::html::create_text_service(text_file, cache_path, config, - std::move(logger)); + logger); } HtmlService html::translate(const ImageFile &image_file, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { + const HtmlConfig &config, const Logger &logger) { std::filesystem::create_directories(cache_path); return internal::html::create_image_service(image_file, cache_path, config, - std::move(logger)); + logger); } HtmlService html::translate(const ArchiveFile &archive_file, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { - return translate(archive_file.archive(), cache_path, config, - std::move(logger)); + const HtmlConfig &config, const Logger &logger) { + return translate(archive_file.archive(), cache_path, config, logger); } HtmlService html::translate(const DocumentFile &document_file, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { - return translate(document_file.document(), cache_path, config, - std::move(logger)); + const HtmlConfig &config, const Logger &logger) { + return translate(document_file.document(), cache_path, config, logger); } HtmlService html::translate(const PdfFile &pdf_file, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { + const HtmlConfig &config, const Logger &logger) { return internal::html::create_pdf_service(pdf_file, cache_path, config, - std::move(logger)); + logger); } HtmlService html::translate(const FontFile &font_file, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { + const HtmlConfig &config, const Logger &logger) { std::filesystem::create_directories(cache_path); return internal::html::create_font_service(font_file, cache_path, config, - std::move(logger)); + logger); } HtmlService html::translate(const Filesystem &filesystem, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { + const HtmlConfig &config, const Logger &logger) { std::filesystem::create_directories(cache_path); return internal::html::create_filesystem_service(filesystem, cache_path, - config, std::move(logger)); + config, logger); } HtmlService html::translate(const Archive &archive, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { - return translate(archive.as_filesystem(), cache_path, config, - std::move(logger)); + const HtmlConfig &config, const Logger &logger) { + return translate(archive.as_filesystem(), cache_path, config, logger); } HtmlService html::translate(const Document &document, const std::string &cache_path, - const HtmlConfig &config, - std::shared_ptr logger) { + const HtmlConfig &config, const Logger &logger) { std::filesystem::create_directories(cache_path); return internal::html::create_document_service(document, cache_path, config, - std::move(logger)); + logger); } void html::edit(const Document &document, const char *diff, - Logger & /*logger*/) { + const Logger & /*logger*/) { const nlohmann::json json = nlohmann::json::parse(diff); for (const auto &[key, value] : json["modifiedText"].items()) { const Element element = diff --git a/src/odr/html.hpp b/src/odr/html.hpp index 353bf29a0..a8e5d7d01 100644 --- a/src/odr/html.hpp +++ b/src/odr/html.hpp @@ -274,7 +274,7 @@ HtmlResourceLocator standard_resource_locator(); /// @return HTML output. HtmlService translate(const DecodedFile &file, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates a text file to HTML. /// @@ -285,7 +285,7 @@ HtmlService translate(const DecodedFile &file, const std::string &cache_path, /// @return HTML output. HtmlService translate(const TextFile &text_file, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates an image file to HTML. /// /// @param image_file Image file to translate. @@ -295,7 +295,7 @@ HtmlService translate(const TextFile &text_file, const std::string &cache_path, /// @return HTML output. HtmlService translate(const ImageFile &image_file, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates an archive to HTML. /// /// @param archive_file Archive file to translate. @@ -305,7 +305,7 @@ HtmlService translate(const ImageFile &image_file, /// @return HTML output. HtmlService translate(const ArchiveFile &archive_file, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates a document to HTML. /// /// @param document_file Document file to translate. @@ -315,7 +315,7 @@ HtmlService translate(const ArchiveFile &archive_file, /// @return HTML output. HtmlService translate(const DocumentFile &document_file, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates a PDF file to HTML. /// /// @param pdf_file PDF file to translate. @@ -325,7 +325,7 @@ HtmlService translate(const DocumentFile &document_file, /// @return HTML output. HtmlService translate(const PdfFile &pdf_file, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates a font file to HTML (a specimen page). /// @@ -336,7 +336,7 @@ HtmlService translate(const PdfFile &pdf_file, const std::string &cache_path, /// @return HTML output. HtmlService translate(const FontFile &font_file, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates a filesystem to HTML. /// @@ -347,7 +347,7 @@ HtmlService translate(const FontFile &font_file, const std::string &cache_path, /// @return HTML output. HtmlService translate(const Filesystem &filesystem, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates an archive to HTML. /// /// @param archive Archive to translate. @@ -357,7 +357,7 @@ HtmlService translate(const Filesystem &filesystem, /// @return HTML output. HtmlService translate(const Archive &archive, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Translates a document to HTML. /// /// @param document Document to translate. @@ -367,7 +367,7 @@ HtmlService translate(const Archive &archive, const std::string &cache_path, /// @return HTML output. HtmlService translate(const Document &document, const std::string &cache_path, const HtmlConfig &config, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); /// @brief Edits a document with a diff. /// @@ -377,7 +377,7 @@ HtmlService translate(const Document &document, const std::string &cache_path, /// @param diff Diff to apply. /// @param logger Logger to use for logging. void edit(const Document &document, const char *diff, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); } // namespace html diff --git a/src/odr/http_server.cpp b/src/odr/http_server.cpp index e3d9eb2b9..25e55047d 100644 --- a/src/odr/http_server.cpp +++ b/src/odr/http_server.cpp @@ -13,9 +13,8 @@ namespace odr { class HttpServer::Impl { public: - explicit Impl(std::shared_ptr logger) - : m_logger{std::move(logger)}, - m_server{std::make_unique()} { + explicit Impl(const Logger &logger) + : m_logger{logger}, m_server{std::make_unique()} { // Set up exception handler to catch any internal httplib exceptions. // This prevents crashes when exceptions occur during request processing. m_server->set_exception_handler([this](const httplib::Request & /*req*/, @@ -26,9 +25,9 @@ class HttpServer::Impl { std::rethrow_exception(ep); } } catch (const std::exception &e) { - ODR_ERROR(*m_logger, "Exception in HTTP handler: " << e.what()); + ODR_ERROR(m_logger, "Exception in HTTP handler: " << e.what()); } catch (...) { - ODR_ERROR(*m_logger, "Unknown exception in HTTP handler"); + ODR_ERROR(m_logger, "Unknown exception in HTTP handler"); } res.status = 500; res.set_content("Internal Server Error", "text/plain"); @@ -89,7 +88,7 @@ class HttpServer::Impl { std::unique_lock lock{m_mutex}; auto it = m_content.find(id); if (it == m_content.end()) { - ODR_ERROR(*m_logger, "Content not found for ID: " << id); + ODR_ERROR(m_logger, "Content not found for ID: " << id); res.status = 404; return; } @@ -98,11 +97,11 @@ class HttpServer::Impl { serve_file(res, service, path); } catch (const std::exception &e) { - ODR_ERROR(*m_logger, "Error handling request: " << e.what()); + ODR_ERROR(m_logger, "Error handling request: " << e.what()); res.status = 500; res.set_content("Internal Server Error", "text/plain"); } catch (...) { - ODR_ERROR(*m_logger, "Unknown error handling request"); + ODR_ERROR(m_logger, "Unknown error handling request"); res.status = 500; res.set_content("Internal Server Error", "text/plain"); } @@ -111,12 +110,12 @@ class HttpServer::Impl { void serve_file(httplib::Response &res, const HtmlService &service, const std::string &path) const { if (!service.exists(path)) { - ODR_ERROR(*m_logger, "File not found: " << path); + ODR_ERROR(m_logger, "File not found: " << path); res.status = 404; return; } - ODR_VERBOSE(*m_logger, "Serving file: " << path); + ODR_VERBOSE(m_logger, "Serving file: " << path); // Buffer content to avoid streaming issues on Android. // Using ContentProviderWithoutLength (chunked transfer encoding) can cause @@ -131,18 +130,18 @@ class HttpServer::Impl { service.write(path, buffer); res.set_content(buffer.str(), service.mimetype(path)); } catch (const std::exception &e) { - ODR_ERROR(*m_logger, "Error serving file " << path << ": " << e.what()); + ODR_ERROR(m_logger, "Error serving file " << path << ": " << e.what()); res.status = 500; res.set_content("Internal Server Error", "text/plain"); } catch (...) { - ODR_ERROR(*m_logger, "Unknown error serving file: " << path); + ODR_ERROR(m_logger, "Unknown error serving file: " << path); res.status = 500; res.set_content("Internal Server Error", "text/plain"); } } void connect_service(HtmlService service, const std::string &prefix) { - ODR_VERBOSE(*m_logger, "Connecting service with prefix: " << prefix); + ODR_VERBOSE(m_logger, "Connecting service with prefix: " << prefix); std::unique_lock lock{m_mutex}; @@ -204,7 +203,7 @@ class HttpServer::Impl { m_bound.store(true, std::memory_order_release); - ODR_VERBOSE(*m_logger, "Bound to " << host << ":" << bound); + ODR_VERBOSE(m_logger, "Bound to " << host << ":" << bound); return static_cast(bound); } @@ -216,13 +215,13 @@ class HttpServer::Impl { throw ServerNotBound(); } - ODR_VERBOSE(*m_logger, "Serving..."); + ODR_VERBOSE(m_logger, "Serving..."); m_server->listen_after_bind(); } void clear() { - ODR_VERBOSE(*m_logger, "Dropping connected services..."); + ODR_VERBOSE(m_logger, "Dropping connected services..."); std::unique_lock lock{m_mutex}; @@ -230,7 +229,7 @@ class HttpServer::Impl { } void stop() { - ODR_VERBOSE(*m_logger, "Stopping HTTP server..."); + ODR_VERBOSE(m_logger, "Stopping HTTP server..."); // Set stopping flag first to reject new requests immediately. // This prevents new requests from starting while we're shutting down. @@ -253,7 +252,7 @@ class HttpServer::Impl { } private: - std::shared_ptr m_logger; + Logger m_logger; // Flag to indicate server is shutting down - checked by handlers // to reject new requests during shutdown. @@ -280,9 +279,8 @@ class HttpServer::Impl { std::unique_ptr m_server; }; -HttpServer::HttpServer(const Config & /*config*/, - std::shared_ptr logger) - : m_impl{std::make_unique(std::move(logger))} {} +HttpServer::HttpServer(const Config & /*config*/, const Logger &logger) + : m_impl{std::make_unique(logger)} {} void HttpServer::connect_service(HtmlService service, const std::string &prefix) const { diff --git a/src/odr/http_server.hpp b/src/odr/http_server.hpp index 32d146757..46ebfd700 100644 --- a/src/odr/http_server.hpp +++ b/src/odr/http_server.hpp @@ -40,7 +40,7 @@ class HttpServer { using Options = HttpServerOptions; explicit HttpServer(const Config &config = {}, - std::shared_ptr logger = Logger::create_null()); + const Logger &logger = Logger::null()); void connect_service(HtmlService service, const std::string &prefix) const; diff --git a/src/odr/internal/html/document.cpp b/src/odr/internal/html/document.cpp index 230193240..c98d49b25 100644 --- a/src/odr/internal/html/document.cpp +++ b/src/odr/internal/html/document.cpp @@ -211,9 +211,9 @@ class HtmlServiceImpl final : public HtmlService { public: HtmlServiceImpl(Document document, std::vector> fragments, - HtmlConfig config, std::shared_ptr logger) - : HtmlService(std::move(config), std::move(logger)), - m_document{std::move(document)}, m_fragments{std::move(fragments)} { + HtmlConfig config, const Logger &logger) + : HtmlService(std::move(config), logger), m_document{std::move(document)}, + m_fragments{std::move(fragments)} { m_views.emplace_back( std::make_shared(*this, "document", 0, "document.html")); for (const auto &fragment : m_fragments) { @@ -433,7 +433,7 @@ namespace odr::internal { HtmlService html::create_document_service(const Document &document, const std::string & /*cache_path*/, HtmlConfig config, - std::shared_ptr logger) { + const Logger &logger) { std::vector> fragments; if (document.document_type() == DocumentType::text) { @@ -474,7 +474,7 @@ HtmlService html::create_document_service(const Document &document, } return odr::HtmlService(std::make_unique( - document, fragments, std::move(config), std::move(logger))); + document, fragments, std::move(config), logger)); } } // namespace odr::internal diff --git a/src/odr/internal/html/document.hpp b/src/odr/internal/html/document.hpp index 39db11933..b50687c27 100644 --- a/src/odr/internal/html/document.hpp +++ b/src/odr/internal/html/document.hpp @@ -14,7 +14,6 @@ namespace odr::internal::html { HtmlService create_document_service(const Document &document, const std::string &cache_path, - HtmlConfig config, - std::shared_ptr logger); + HtmlConfig config, const Logger &logger); } // namespace odr::internal::html diff --git a/src/odr/internal/html/filesystem.cpp b/src/odr/internal/html/filesystem.cpp index 098d0a656..24f75447e 100644 --- a/src/odr/internal/html/filesystem.cpp +++ b/src/odr/internal/html/filesystem.cpp @@ -16,8 +16,8 @@ namespace { class HtmlServiceImpl final : public HtmlService { public: HtmlServiceImpl(Filesystem filesystem, HtmlConfig config, - std::shared_ptr logger) - : HtmlService(std::move(config), std::move(logger)), + const Logger &logger) + : HtmlService(std::move(config), logger), m_filesystem{std::move(filesystem)} { m_views.emplace_back( std::make_shared(*this, "files", 0, "files.html")); @@ -150,9 +150,9 @@ namespace odr::internal { HtmlService html::create_filesystem_service(const Filesystem &filesystem, const std::string & /*cache_path*/, HtmlConfig config, - std::shared_ptr logger) { - return odr::HtmlService(std::make_unique( - filesystem, std::move(config), std::move(logger))); + const Logger &logger) { + return odr::HtmlService( + std::make_unique(filesystem, std::move(config), logger)); } } // namespace odr::internal diff --git a/src/odr/internal/html/filesystem.hpp b/src/odr/internal/html/filesystem.hpp index 9241370c9..cf3da7c12 100644 --- a/src/odr/internal/html/filesystem.hpp +++ b/src/odr/internal/html/filesystem.hpp @@ -14,7 +14,6 @@ namespace odr::internal::html { HtmlService create_filesystem_service(const Filesystem &filesystem, const std::string &cache_path, - HtmlConfig config, - std::shared_ptr logger); + HtmlConfig config, const Logger &logger); } diff --git a/src/odr/internal/html/font_file.cpp b/src/odr/internal/html/font_file.cpp index e202c964b..18b5d2329 100644 --- a/src/odr/internal/html/font_file.cpp +++ b/src/odr/internal/html/font_file.cpp @@ -25,9 +25,8 @@ constexpr std::uint16_t max_grid_glyphs = 4096; class HtmlServiceImpl final : public HtmlService { public: - HtmlServiceImpl(FontFile font_file, HtmlConfig config, - std::shared_ptr logger) - : HtmlService(std::move(config), std::move(logger)), + HtmlServiceImpl(FontFile font_file, HtmlConfig config, const Logger &logger) + : HtmlService(std::move(config), logger), m_font_file{std::move(font_file)} { m_views.emplace_back( std::make_shared(*this, "font", 0, "font.html")); @@ -164,10 +163,9 @@ class HtmlServiceImpl final : public HtmlService { odr::HtmlService create_font_service(const FontFile &font_file, const std::string & /*cache_path*/, - HtmlConfig config, - std::shared_ptr logger) { - return odr::HtmlService(std::make_unique( - font_file, std::move(config), std::move(logger))); + HtmlConfig config, const Logger &logger) { + return odr::HtmlService( + std::make_unique(font_file, std::move(config), logger)); } } // namespace odr::internal::html diff --git a/src/odr/internal/html/font_file.hpp b/src/odr/internal/html/font_file.hpp index f14ffea3c..db272a550 100644 --- a/src/odr/internal/html/font_file.hpp +++ b/src/odr/internal/html/font_file.hpp @@ -18,7 +18,6 @@ namespace odr::internal::html { /// the uniform PUA re-encode. HtmlService create_font_service(const FontFile &font_file, const std::string &cache_path, - HtmlConfig config, - std::shared_ptr logger); + HtmlConfig config, const Logger &logger); } // namespace odr::internal::html diff --git a/src/odr/internal/html/html_service.cpp b/src/odr/internal/html/html_service.cpp index ca36f2db1..dfb192847 100644 --- a/src/odr/internal/html/html_service.cpp +++ b/src/odr/internal/html/html_service.cpp @@ -6,8 +6,8 @@ namespace odr::internal::html { -HtmlService::HtmlService(HtmlConfig config, std::shared_ptr logger) - : m_config{std::move(config)}, m_logger{std::move(logger)} {} +HtmlService::HtmlService(HtmlConfig config, const Logger &logger) + : m_config{std::move(config)}, m_logger{logger} {} const HtmlConfig &HtmlService::config() const { return m_config; } diff --git a/src/odr/internal/html/html_service.hpp b/src/odr/internal/html/html_service.hpp index e2cb8640b..c6c03f704 100644 --- a/src/odr/internal/html/html_service.hpp +++ b/src/odr/internal/html/html_service.hpp @@ -9,7 +9,7 @@ namespace odr::internal::html { class HtmlService : public abstract::HtmlService { public: - HtmlService(HtmlConfig config, std::shared_ptr logger); + HtmlService(HtmlConfig config, const Logger &logger); [[nodiscard]] const HtmlConfig &config() const override; @@ -17,7 +17,7 @@ class HtmlService : public abstract::HtmlService { HtmlConfig m_config; protected: - std::shared_ptr m_logger; + Logger m_logger; }; class HtmlView : public abstract::HtmlView { diff --git a/src/odr/internal/html/image_file.cpp b/src/odr/internal/html/image_file.cpp index 74143760f..cf23b7d15 100644 --- a/src/odr/internal/html/image_file.cpp +++ b/src/odr/internal/html/image_file.cpp @@ -18,9 +18,8 @@ namespace { class HtmlServiceImpl final : public HtmlService { public: - HtmlServiceImpl(ImageFile image_file, HtmlConfig config, - std::shared_ptr logger) - : HtmlService(std::move(config), std::move(logger)), + HtmlServiceImpl(ImageFile image_file, HtmlConfig config, const Logger &logger) + : HtmlService(std::move(config), logger), m_image_file{std::move(image_file)} { m_views.emplace_back( std::make_shared(*this, "image", 0, "image.html")); @@ -143,9 +142,9 @@ void html::translate_image_src(const ImageFile &image_file, std::ostream &out, HtmlService html::create_image_service(const ImageFile &image_file, const std::string & /*cache_path*/, HtmlConfig config, - std::shared_ptr logger) { - return odr::HtmlService(std::make_unique( - image_file, std::move(config), std::move(logger))); + const Logger &logger) { + return odr::HtmlService( + std::make_unique(image_file, std::move(config), logger)); } } // namespace odr::internal diff --git a/src/odr/internal/html/image_file.hpp b/src/odr/internal/html/image_file.hpp index 21afcacac..704a20100 100644 --- a/src/odr/internal/html/image_file.hpp +++ b/src/odr/internal/html/image_file.hpp @@ -20,7 +20,6 @@ void translate_image_src(const ImageFile &image_file, std::ostream &out, HtmlService create_image_service(const ImageFile &image_file, const std::string &cache_path, - HtmlConfig config, - std::shared_ptr logger); + HtmlConfig config, const Logger &logger); } // namespace odr::internal::html diff --git a/src/odr/internal/html/pdf_file.cpp b/src/odr/internal/html/pdf_file.cpp index 958a3e074..999335d15 100644 --- a/src/odr/internal/html/pdf_file.cpp +++ b/src/odr/internal/html/pdf_file.cpp @@ -1140,9 +1140,8 @@ class AtomicStyles { class HtmlServiceImpl final : public HtmlService { public: - HtmlServiceImpl(PdfFile pdf_file, HtmlConfig config, - std::shared_ptr logger) - : HtmlService(std::move(config), std::move(logger)), + HtmlServiceImpl(PdfFile pdf_file, HtmlConfig config, const Logger &logger) + : HtmlService(std::move(config), logger), m_pdf_file{std::move(pdf_file)} {} /// Parses the document once, applies the `[page_range_begin, @@ -1159,8 +1158,8 @@ class HtmlServiceImpl final : public HtmlService { const auto &pdf_file = dynamic_cast(*m_pdf_file.impl()); - m_parser = std::make_unique( - pdf_file.create_parser(*m_logger)); + m_parser = + std::make_unique(pdf_file.create_parser(m_logger)); m_document = m_parser->parse_document(); const std::vector pages = m_document->collect_pages(); @@ -1443,10 +1442,10 @@ class HtmlServiceImpl final : public HtmlService { double sel_prev_font_size_pt = 0; for (const pdf::PageElement &element : lift_group_text( - pdf::extract_page(stream, *page->resources, *m_logger))) { + pdf::extract_page(stream, *page->resources, m_logger))) { if (handle_graphic_element( element, to_box, width, height, clips, gradients, patterns, - masks, *m_logger, [&] { vis_close_line(); }, + masks, m_logger, [&] { vis_close_line(); }, [&](std::string frag) { page_out.vis_items.push_back(PathOut{std::move(frag)}); })) { @@ -1943,7 +1942,7 @@ class HtmlServiceImpl final : public HtmlService { for (std::size_t pi = 0; pi < pages.size(); ++pi) { const pdf::Page &page = *pages[pi]; for (const pdf::PageElement &element : lift_group_text(pdf::extract_page( - page_streams[pi], *page.resources, *m_logger))) { + page_streams[pi], *page.resources, m_logger))) { const auto *text = std::get_if(&element); if (text == nullptr || text->text.empty() || text->font == nullptr) { continue; @@ -2018,10 +2017,10 @@ class HtmlServiceImpl final : public HtmlService { const auto close_line = [&] { cur_line = -1; }; for (const pdf::PageElement &element : lift_group_text(pdf::extract_page( - page_streams[pi], *page.resources, *m_logger))) { + page_streams[pi], *page.resources, m_logger))) { if (handle_graphic_element( element, to_box, width, height, clips, gradients, patterns, - masks, *m_logger, [&] { close_line(); }, + masks, m_logger, [&] { close_line(); }, [&](std::string frag) { page_out.items.push_back(SinglePathOut{std::move(frag)}); })) { @@ -2710,7 +2709,7 @@ class HtmlServiceImpl final : public HtmlService { const util::math::Transform2D &to_box, double width, double height, ClipRegistry &clips, GradientRegistry &gradients, PatternRegistry &patterns, - MaskRegistry &masks, Logger &logger, + MaskRegistry &masks, const Logger &logger, CloseLine &&close_line, PushSvg &&push_svg) { // Text is handled by the caller; every other element kind is a graphic. if (std::holds_alternative(element)) { @@ -2750,10 +2749,9 @@ namespace odr::internal { HtmlService html::create_pdf_service(const PdfFile &pdf_file, const std::string & /*cache_path*/, - HtmlConfig config, - std::shared_ptr logger) { - return odr::HtmlService(std::make_unique( - pdf_file, std::move(config), std::move(logger))); + HtmlConfig config, const Logger &logger) { + return odr::HtmlService( + std::make_unique(pdf_file, std::move(config), logger)); } } // namespace odr::internal diff --git a/src/odr/internal/html/pdf_file.hpp b/src/odr/internal/html/pdf_file.hpp index 6e9cbbff3..953c9d5e4 100644 --- a/src/odr/internal/html/pdf_file.hpp +++ b/src/odr/internal/html/pdf_file.hpp @@ -14,6 +14,6 @@ namespace odr::internal::html { HtmlService create_pdf_service(const PdfFile &pdf_file, const std::string &cache_path, HtmlConfig config, - std::shared_ptr logger); + const Logger &logger); } diff --git a/src/odr/internal/html/text_file.cpp b/src/odr/internal/html/text_file.cpp index 457427094..4ca2599df 100644 --- a/src/odr/internal/html/text_file.cpp +++ b/src/odr/internal/html/text_file.cpp @@ -18,9 +18,8 @@ namespace { class HtmlServiceImpl final : public HtmlService { public: - HtmlServiceImpl(TextFile text_file, HtmlConfig config, - std::shared_ptr logger) - : HtmlService(std::move(config), std::move(logger)), + HtmlServiceImpl(TextFile text_file, HtmlConfig config, const Logger &logger) + : HtmlService(std::move(config), logger), m_text_file{std::move(text_file)} { m_views.emplace_back( std::make_shared(*this, "text", 0, "text.html")); @@ -179,9 +178,9 @@ namespace odr::internal { HtmlService html::create_text_service(const TextFile &text_file, [[maybe_unused]] const std::string &cache_path, - HtmlConfig config, std::shared_ptr logger) { - return odr::HtmlService(std::make_unique( - text_file, std::move(config), std::move(logger))); + HtmlConfig config, const Logger &logger) { + return odr::HtmlService( + std::make_unique(text_file, std::move(config), logger)); } } // namespace odr::internal diff --git a/src/odr/internal/html/text_file.hpp b/src/odr/internal/html/text_file.hpp index 0c51eb23d..d773012a5 100644 --- a/src/odr/internal/html/text_file.hpp +++ b/src/odr/internal/html/text_file.hpp @@ -14,7 +14,6 @@ namespace odr::internal::html { HtmlService create_text_service(const TextFile &text_file, const std::string &cache_path, - HtmlConfig config, - std::shared_ptr logger); + HtmlConfig config, const Logger &logger); } diff --git a/src/odr/internal/open_strategy.cpp b/src/odr/internal/open_strategy.cpp index 00e5b8be3..31b656b8a 100644 --- a/src/odr/internal/open_strategy.cpp +++ b/src/odr/internal/open_strategy.cpp @@ -50,7 +50,7 @@ template auto priority_comparator(const std::vector &priority) { std::vector open_strategy::list_file_types(const std::shared_ptr &file, - Logger &logger) { + const Logger &logger) { std::vector result; auto file_type = magic::file_type(*file); @@ -147,7 +147,7 @@ std::vector open_strategy::list_decoder_engines(const FileType) { std::unique_ptr open_strategy::open_file(const std::shared_ptr &file, - Logger &logger) { + const Logger &logger) { auto file_type = magic::file_type(*file); ODR_VERBOSE(logger, "magic determined file type " << file_type_to_string(file_type)); @@ -256,7 +256,7 @@ open_strategy::open_file(const std::shared_ptr &file, std::unique_ptr open_strategy::open_file(const std::shared_ptr &file, - FileType as, Logger &logger) { + FileType as, const Logger &logger) { DecodePreference preference; preference.as_file_type = as; return open_file(file, preference, logger); @@ -264,7 +264,8 @@ open_strategy::open_file(const std::shared_ptr &file, std::unique_ptr open_strategy::open_file(const std::shared_ptr &file, - FileType as, DecoderEngine with, Logger &logger) { + FileType as, DecoderEngine with, + const Logger &logger) { if (as == FileType::opendocument_text || as == FileType::opendocument_presentation || as == FileType::opendocument_spreadsheet || @@ -490,7 +491,8 @@ open_strategy::open_file(const std::shared_ptr &file, std::unique_ptr open_strategy::open_file(const std::shared_ptr &file, - const DecodePreference &preference, Logger &logger) { + const DecodePreference &preference, + const Logger &logger) { std::vector probe_types; if (preference.as_file_type.has_value()) { ODR_VERBOSE(logger, "using preferred file type " @@ -551,7 +553,7 @@ open_strategy::open_file(const std::shared_ptr &file, std::unique_ptr open_strategy::open_document_file(const std::shared_ptr &file, - Logger &logger) { + const Logger &logger) { auto file_type = magic::file_type(*file); ODR_VERBOSE(logger, "magic determined file type " << file_type_to_string(file_type)); diff --git a/src/odr/internal/open_strategy.hpp b/src/odr/internal/open_strategy.hpp index fa8427749..03513e681 100644 --- a/src/odr/internal/open_strategy.hpp +++ b/src/odr/internal/open_strategy.hpp @@ -19,23 +19,25 @@ class DocumentFile; namespace odr::internal::open_strategy { std::vector -list_file_types(const std::shared_ptr &file, Logger &logger); +list_file_types(const std::shared_ptr &file, + const Logger &logger); std::vector list_decoder_engines(FileType as); std::unique_ptr -open_file(const std::shared_ptr &file, Logger &logger); +open_file(const std::shared_ptr &file, const Logger &logger); std::unique_ptr open_file(const std::shared_ptr &file, FileType as, - Logger &logger); + const Logger &logger); std::unique_ptr open_file(const std::shared_ptr &file, FileType as, - DecoderEngine with, Logger &logger); + DecoderEngine with, const Logger &logger); std::unique_ptr open_file(const std::shared_ptr &file, - const DecodePreference &preference, Logger &logger); + const DecodePreference &preference, const Logger &logger); std::unique_ptr -open_document_file(const std::shared_ptr &file, Logger &logger); +open_document_file(const std::shared_ptr &file, + const Logger &logger); } // namespace odr::internal::open_strategy diff --git a/src/odr/internal/pdf/AGENTS.md b/src/odr/internal/pdf/AGENTS.md index c515e3bda..63a4b1eb5 100644 --- a/src/odr/internal/pdf/AGENTS.md +++ b/src/odr/internal/pdf/AGENTS.md @@ -97,8 +97,9 @@ is caught once and the file is forward-scanned to rebuild a synthetic xref header) before giving up. **Diagnostics route through `Logger`.** `DocumentParser` and `extract_text` take a -`Logger &` (default `Logger::null()`); no stray `stdout`/`stderr` remains — new -diagnostics follow the same pattern. +`const Logger &` (default `Logger::null()`); no stray `stdout`/`stderr` remains — +new diagnostics follow the same pattern. `Logger` is a value handle, so parsers +store it by value and never dangle. **Encryption: the derived key is never retained.** The empty password is tried first (user then owner), so owner-locked files open transparently. Once diff --git a/src/odr/internal/pdf/pdf_cmap_parser.cpp b/src/odr/internal/pdf/pdf_cmap_parser.cpp index a34ee9e5a..8d27a51c1 100644 --- a/src/odr/internal/pdf/pdf_cmap_parser.cpp +++ b/src/odr/internal/pdf/pdf_cmap_parser.cpp @@ -76,7 +76,7 @@ std::u16string utf16be_to_u16string(const std::string &bytes) { } // namespace CMapParser::CMapParser(std::istream &in, const Logger &logger) - : m_parser(in), m_logger{&logger} {} + : m_parser(in), m_logger{logger} {} std::istream &CMapParser::in() { return m_parser.in(); } @@ -124,9 +124,9 @@ void CMapParser::read_codespacerange(const std::uint32_t n, CMap &cmap) { m_parser.skip_whitespace(); if (low.size() != high.size() || !valid_code_width(low.size())) { - ODR_WARNING(*m_logger, "pdf: skipping out-of-spec codespace range (low " - << low.size() << " bytes, high " << high.size() - << " bytes)"); + ODR_WARNING(m_logger, "pdf: skipping out-of-spec codespace range (low " + << low.size() << " bytes, high " << high.size() + << " bytes)"); continue; // skip an out-of-spec range, keep parsing the rest } cmap.add_codespace_range(std::move(low), std::move(high)); @@ -142,9 +142,9 @@ void CMapParser::read_bfchar(const std::uint32_t n, CMap &cmap) { m_parser.skip_whitespace(); if (!valid_code_width(code.size()) || destination.size() % 2 != 0) { - ODR_WARNING(*m_logger, "pdf: skipping malformed bfchar entry (code " - << code.size() << " bytes, destination " - << destination.size() << " bytes)"); + ODR_WARNING(m_logger, "pdf: skipping malformed bfchar entry (code " + << code.size() << " bytes, destination " + << destination.size() << " bytes)"); continue; // skip a malformed mapping, keep the rest of the CMap } cmap.map_single(std::move(code), utf16be_to_u16string(destination)); @@ -162,25 +162,25 @@ void CMapParser::read_bfrange(const std::uint32_t n, CMap &cmap) { m_parser.skip_whitespace(); if (low.size() != high.size() || !valid_code_width(low.size())) { - ODR_WARNING(*m_logger, "pdf: skipping out-of-spec bfrange (low " - << low.size() << " bytes, high " << high.size() - << " bytes)"); + ODR_WARNING(m_logger, "pdf: skipping out-of-spec bfrange (low " + << low.size() << " bytes, high " << high.size() + << " bytes)"); continue; // skip an out-of-spec range, keep parsing the rest } const std::size_t width = low.size(); const std::uint32_t low_code = code_to_uint(low); const std::uint32_t high_code = code_to_uint(high); if (high_code < low_code) { - ODR_WARNING(*m_logger, "pdf: skipping reversed bfrange (low 0x" - << std::hex << low_code << ", high 0x" - << high_code << ")"); + ODR_WARNING(m_logger, "pdf: skipping reversed bfrange (low 0x" + << std::hex << low_code << ", high 0x" + << high_code << ")"); continue; // a reversed range would otherwise wrap `code` below } if (high_code - low_code > max_bfrange_span) { - ODR_WARNING(*m_logger, "pdf: skipping oversized bfrange (0x" - << std::hex << low_code << "-0x" << high_code - << " spans more than " << std::dec - << (max_bfrange_span + 1) << " codes)"); + ODR_WARNING(m_logger, "pdf: skipping oversized bfrange (0x" + << std::hex << low_code << "-0x" << high_code + << " spans more than " << std::dec + << (max_bfrange_span + 1) << " codes)"); continue; } // The span fits in 32 bits without wrapping, so the loops below can count. @@ -193,13 +193,13 @@ void CMapParser::read_bfrange(const std::uint32_t n, CMap &cmap) { offset <= span && offset < destinations.size(); ++offset) { const Object &element = destinations[offset]; if (!element.is_string()) { - ODR_WARNING(*m_logger, + ODR_WARNING(m_logger, "pdf: skipping non-string bfrange array destination"); continue; } const std::string &dst = element.as_string(); if (dst.size() % 2 != 0) { - ODR_WARNING(*m_logger, + ODR_WARNING(m_logger, "pdf: skipping odd-length bfrange array destination (" << dst.size() << " bytes)"); continue; @@ -212,7 +212,7 @@ void CMapParser::read_bfrange(const std::uint32_t n, CMap &cmap) { // the code range (7.9.3). const std::string &dst = destination.as_string(); if (dst.size() % 2 != 0) { - ODR_WARNING(*m_logger, + ODR_WARNING(m_logger, "pdf: skipping bfrange with odd-length destination (" << dst.size() << " bytes)"); continue; @@ -237,8 +237,8 @@ void CMapParser::read_cidchar(const std::uint32_t n, CMap &cmap) { m_parser.skip_whitespace(); if (!valid_code_width(code.size()) || !cid.is_integer()) { - ODR_WARNING(*m_logger, "pdf: skipping malformed cidchar entry (code " - << code.size() << " bytes)"); + ODR_WARNING(m_logger, "pdf: skipping malformed cidchar entry (code " + << code.size() << " bytes)"); continue; // skip a malformed mapping, keep the rest of the CMap } cmap.map_cid_char(std::move(code), @@ -258,17 +258,17 @@ void CMapParser::read_cidrange(const std::uint32_t n, CMap &cmap) { if (low.size() != high.size() || !valid_code_width(low.size()) || !cid.is_integer()) { - ODR_WARNING(*m_logger, "pdf: skipping out-of-spec cidrange (low " - << low.size() << " bytes, high " << high.size() - << " bytes)"); + ODR_WARNING(m_logger, "pdf: skipping out-of-spec cidrange (low " + << low.size() << " bytes, high " << high.size() + << " bytes)"); continue; // skip an out-of-spec range, keep parsing the rest } const std::uint32_t low_code = code_to_uint(low); const std::uint32_t high_code = code_to_uint(high); if (high_code < low_code) { - ODR_WARNING(*m_logger, "pdf: skipping reversed cidrange (low 0x" - << std::hex << low_code << ", high 0x" - << high_code << ")"); + ODR_WARNING(m_logger, "pdf: skipping reversed cidrange (low 0x" + << std::hex << low_code << ", high 0x" + << high_code << ")"); continue; // a reversed range would map codes to nonsense CIDs } // Unlike a `bfrange`, a `cidrange` commonly spans the whole 2-byte code @@ -317,7 +317,7 @@ CMap CMapParser::parse_cmap() { // callers fall back to the ToUnicode codespace / fixed width instead of // mis-splitting the inherited (e.g. 2-byte) codes. cmap.mark_inherits_external_cmap(); - ODR_WARNING(*m_logger, + ODR_WARNING(m_logger, "pdf: unresolved 'usecmap'; local codespace not treated as " "authoritative"); } diff --git a/src/odr/internal/pdf/pdf_cmap_parser.hpp b/src/odr/internal/pdf/pdf_cmap_parser.hpp index aa9e52896..407f6de08 100644 --- a/src/odr/internal/pdf/pdf_cmap_parser.hpp +++ b/src/odr/internal/pdf/pdf_cmap_parser.hpp @@ -25,7 +25,7 @@ class CMapParser { private: ObjectParser m_parser; - const Logger *m_logger{nullptr}; + Logger m_logger; [[nodiscard]] Token read_token(); diff --git a/src/odr/internal/pdf/pdf_document_parser.cpp b/src/odr/internal/pdf/pdf_document_parser.cpp index badddca17..878ba180b 100644 --- a/src/odr/internal/pdf/pdf_document_parser.cpp +++ b/src/odr/internal/pdf/pdf_document_parser.cpp @@ -1305,15 +1305,14 @@ std::unique_ptr parse_document_impl(DocumentParser &parser, DocumentParser::DocumentParser(std::unique_ptr in, std::optional decryptor, const Logger &logger) - : m_stream(std::move(in)), m_parser(*m_stream), m_logger{&logger} { + : m_stream(std::move(in)), m_parser(*m_stream), m_logger{logger} { try { auto [xref, trailer] = read_trailer_chain(); m_xref = std::move(xref); m_trailer = std::move(trailer); } catch (const std::exception &e) { - ODR_WARNING(*m_logger, "pdf: cross-reference parsing failed (" - << e.what() - << "), scanning the file to recover"); + ODR_WARNING(m_logger, "pdf: cross-reference parsing failed (" + << e.what() << "), scanning the file to recover"); recover_xref(); } @@ -1359,7 +1358,7 @@ std::istream &DocumentParser::in() { return m_parser.in(); } FileParser &DocumentParser::parser() { return m_parser; } -const Logger &DocumentParser::logger() const { return *m_logger; } +const Logger &DocumentParser::logger() const { return m_logger; } const Xref &DocumentParser::xref() const { return m_xref; } @@ -1401,9 +1400,9 @@ DocumentParser::read_object(const ObjectReference &reference) { const auto entry_it = m_xref.table.find(reference); if (entry_it == std::end(m_xref.table)) { - ODR_WARNING(*m_logger, "pdf: object " << reference - << " not in cross-reference table, " - "treating as null"); + ODR_WARNING(m_logger, "pdf: object " << reference + << " not in cross-reference table, " + "treating as null"); } else if (const Xref::Entry &entry = entry_it->second; entry.is_used()) { in().seekg(entry.as_used().position); object = parser().read_indirect_object(); @@ -1425,16 +1424,16 @@ DocumentParser::read_object(const ObjectReference &reference) { throw std::runtime_error("object stream member index out of range"); } if (members[index].id != reference.id) { - ODR_WARNING(*m_logger, "pdf: object stream " - << stream_id << " member " << index - << " has id " << members[index].id - << ", expected " << reference.id); + ODR_WARNING(m_logger, "pdf: object stream " + << stream_id << " member " << index + << " has id " << members[index].id + << ", expected " << reference.id); } object.object = members[index].object; } else { - ODR_WARNING(*m_logger, "pdf: reference " << reference - << " to freed object, treating " - "as null"); + ODR_WARNING(m_logger, "pdf: reference " << reference + << " to freed object, treating " + "as null"); } return m_objects.emplace(reference, std::move(object)).first->second; @@ -1577,8 +1576,8 @@ DocumentParser::read_xref_section(const std::uint32_t position) { } if (!dictionary.get("Type").is_name() || dictionary["Type"].as_name() != "XRef") { - ODR_WARNING(*m_logger, "pdf: cross-reference stream at " - << position << " is not marked /Type /XRef"); + ODR_WARNING(m_logger, "pdf: cross-reference stream at " + << position << " is not marked /Type /XRef"); } // `/Filter`, `/DecodeParms` and `/Length` are required to be direct in @@ -1722,7 +1721,7 @@ void DocumentParser::recover_root() { const Dictionary &dictionary = object.object.as_dictionary(); if (dictionary.get("Type").is_name() && dictionary["Type"].as_name() == "Catalog") { - ODR_WARNING(*m_logger, "pdf: recovered document catalog " << reference); + ODR_WARNING(m_logger, "pdf: recovered document catalog " << reference); m_trailer["Root"] = Object(reference); return; } @@ -1730,7 +1729,7 @@ void DocumentParser::recover_root() { // skip objects that fail to read during the catalog search } } - ODR_WARNING(*m_logger, "pdf: recovery found no document catalog"); + ODR_WARNING(m_logger, "pdf: recovery found no document catalog"); } std::unique_ptr DocumentParser::parse_document() { @@ -1743,9 +1742,8 @@ std::unique_ptr DocumentParser::parse_document() { if (m_recovered) { throw; } - ODR_WARNING(*m_logger, "pdf: building the document failed (" - << e.what() - << "), scanning the file to recover"); + ODR_WARNING(m_logger, "pdf: building the document failed (" + << e.what() << "), scanning the file to recover"); recover_xref(); return parse_document_impl(*this, m_trailer); } diff --git a/src/odr/internal/pdf/pdf_document_parser.hpp b/src/odr/internal/pdf/pdf_document_parser.hpp index bf6dab939..243e12759 100644 --- a/src/odr/internal/pdf/pdf_document_parser.hpp +++ b/src/odr/internal/pdf/pdf_document_parser.hpp @@ -123,7 +123,7 @@ class DocumentParser { std::unique_ptr m_stream; FileParser m_parser; - const Logger *m_logger{nullptr}; + Logger m_logger; Xref m_xref; Dictionary m_trailer; diff --git a/src/odr/internal/pdf/pdf_graphics_operator_parser.cpp b/src/odr/internal/pdf/pdf_graphics_operator_parser.cpp index 09e461ed6..73e3f8cca 100644 --- a/src/odr/internal/pdf/pdf_graphics_operator_parser.cpp +++ b/src/odr/internal/pdf/pdf_graphics_operator_parser.cpp @@ -196,7 +196,7 @@ static constexpr int_type eof = std::streambuf::traits_type::eof(); GraphicsOperatorParser::GraphicsOperatorParser(std::istream &in, const Logger &logger) - : m_parser(in), m_logger{&logger} {} + : m_parser(in), m_logger{logger} {} std::istream &GraphicsOperatorParser::in() { return m_parser.in(); } @@ -260,7 +260,7 @@ GraphicsOperator GraphicsOperatorParser::read_operator() { result.type = operator_name_to_type(operator_name); if (result.type == GraphicsOperatorType::unknown) { - ODR_DEBUG(*m_logger, + ODR_DEBUG(m_logger, "pdf: unknown graphics operator '" + operator_name + "'"); } diff --git a/src/odr/internal/pdf/pdf_graphics_operator_parser.hpp b/src/odr/internal/pdf/pdf_graphics_operator_parser.hpp index 200445d98..da19870d2 100644 --- a/src/odr/internal/pdf/pdf_graphics_operator_parser.hpp +++ b/src/odr/internal/pdf/pdf_graphics_operator_parser.hpp @@ -39,7 +39,7 @@ class GraphicsOperatorParser { read_inline_image_data(const Dictionary &dictionary); ObjectParser m_parser; - const Logger *m_logger{nullptr}; + Logger m_logger; }; } // namespace odr::internal::pdf diff --git a/src/odr/logger.cpp b/src/odr/logger.cpp index e4a3b8960..b971f663e 100644 --- a/src/odr/logger.cpp +++ b/src/odr/logger.cpp @@ -6,27 +6,27 @@ #include #include #include +#include namespace odr { namespace { -class NullLogger final : public Logger { +class NullLogger final : public ILogger { public: [[nodiscard]] bool will_log(LogLevel) const override { return false; } - void flush() override { + void log(Time, LogLevel, const std::string &, + const std::source_location &) override { // Do nothing } -protected: - void log_impl(Time, LogLevel, const std::string &, - const std::source_location &) const override { + void flush() override { // Do nothing } }; -class StdioLogger final : public Logger { +class StdioLogger final : public ILogger { public: StdioLogger(std::string name, const LogLevel level, LogFormat format, std::unique_ptr output) @@ -43,16 +43,14 @@ class StdioLogger final : public Logger { return level >= m_level; } - void flush() override { m_output->flush(); } - -protected: - void log_impl(const Time time, const LogLevel level, - const std::string &message, - const std::source_location &location) const override { - print_head(*m_output, time, level, m_name, location, m_format); + void log(const Time time, const LogLevel level, const std::string &message, + const std::source_location &location) override { + Logger::print_head(*m_output, time, level, m_name, location, m_format); *m_output << message << "\n"; } + void flush() override { m_output->flush(); } + private: std::string m_name; LogLevel m_level; @@ -60,45 +58,44 @@ class StdioLogger final : public Logger { std::unique_ptr m_output; }; -class TeeLogger final : public Logger { +class TeeLogger final : public ILogger { public: - explicit TeeLogger(const std::vector> &loggers) - : m_loggers(loggers) { + explicit TeeLogger(std::vector loggers) + : m_loggers(std::move(loggers)) { if (m_loggers.empty()) { throw std::invalid_argument("TeeLogger requires at least one logger"); } } - ~TeeLogger() override { flush(); } - [[nodiscard]] bool will_log(const LogLevel level) const override { - return std::ranges::any_of(m_loggers, [level](const auto &logger) { - return logger->will_log(level); + return std::ranges::any_of(m_loggers, [level](const Logger &logger) { + return logger.will_log(level); }); } - void flush() override { - for (const auto &logger : m_loggers) { - logger->flush(); + void log(const Time time, const LogLevel level, const std::string &message, + const std::source_location &location) override { + for (const Logger &logger : m_loggers) { + logger.log(level, message, time, location); } } -protected: - void log_impl(const Time time, const LogLevel level, - const std::string &message, - const std::source_location &location) const override { - for (const auto &logger : m_loggers) { - if (!logger->will_log(level)) { - continue; - } - logger->log(level, message, time, location); + void flush() override { + for (const Logger &logger : m_loggers) { + logger.flush(); } } private: - std::vector> m_loggers; + std::vector m_loggers; }; +std::shared_ptr null_impl() { + static const std::shared_ptr instance = + std::make_shared(); + return instance; +} + std::string_view level_to_string(const LogLevel level) { switch (level) { case LogLevel::verbose: @@ -120,27 +117,42 @@ std::string_view level_to_string(const LogLevel level) { } // namespace -Logger &Logger::null() { - static NullLogger null_logger; - return null_logger; +Logger Logger::null() { return Logger(); } + +Logger Logger::create_stdio(const std::string &name, const LogLevel level, + const LogFormat &format, + std::unique_ptr output) { + return Logger( + std::make_shared(name, level, format, std::move(output))); +} + +Logger Logger::create_tee(const std::vector &loggers) { + return Logger(std::make_shared(loggers)); } -std::unique_ptr Logger::create_null() { - return std::make_unique(); +Logger::Logger() : m_impl{null_impl()} {} + +Logger::Logger(std::shared_ptr impl) : m_impl{std::move(impl)} { + if (m_impl == nullptr) { + throw std::invalid_argument("Logger requires an implementation"); + } } -std::unique_ptr -Logger::create_stdio(const std::string &name, LogLevel level, - const LogFormat &format, - std::unique_ptr output) { - return std::make_unique(name, level, format, std::move(output)); +bool Logger::will_log(const LogLevel level) const { + return m_impl->will_log(level); } -std::unique_ptr -Logger::create_tee(const std::vector> &loggers) { - return std::make_unique(loggers); +void Logger::log(const LogLevel level, const std::string &message, + const Time time, const std::source_location &location) const { + if (m_impl->will_log(level)) { + m_impl->log(time, level, message, location); + } } +void Logger::flush() const { m_impl->flush(); } + +std::shared_ptr Logger::impl() const { return m_impl; } + void Logger::print_head(std::ostream &out, Time time, LogLevel level, const std::string &name, const std::source_location &location, diff --git a/src/odr/logger.hpp b/src/odr/logger.hpp index 2c2bfeb67..514928d43 100644 --- a/src/odr/logger.hpp +++ b/src/odr/logger.hpp @@ -25,43 +25,63 @@ struct LogFormat { std::size_t location_width{20}; }; -class Logger { +/// @brief Interface for a log sink. Implement to route logging elsewhere and +/// hand it to `Logger`. +/// +/// `Logger` gates on `will_log` before calling `log`, so implementations may +/// assume the level reaching `log` is enabled. +class ILogger { public: using Clock = std::chrono::system_clock; using Time = Clock::time_point; - static Logger &null(); - static std::unique_ptr create_null(); - static std::unique_ptr - create_stdio(const std::string &name, LogLevel level, - const LogFormat &format = LogFormat(), - std::unique_ptr output = nullptr); - static std::unique_ptr - create_tee(const std::vector> &loggers); + virtual ~ILogger() = default; + + [[nodiscard]] virtual bool will_log(LogLevel level) const = 0; + + virtual void log(Time time, LogLevel level, const std::string &message, + const std::source_location &location) = 0; + + virtual void flush() = 0; +}; + +/// @brief Handle to a log sink. +/// +/// Copies share the sink. Implement @ref ILogger for a custom one. +class Logger final { +public: + using Clock = ILogger::Clock; + using Time = ILogger::Time; + + /// @brief A logger discarding everything. All instances share one sink. + static Logger null(); + static Logger create_stdio(const std::string &name, LogLevel level, + const LogFormat &format = LogFormat(), + std::unique_ptr output = nullptr); + /// @throws std::invalid_argument if @p loggers is empty. + static Logger create_tee(const std::vector &loggers); static void print_head(std::ostream &out, Time time, LogLevel level, const std::string &name, const std::source_location &location, const LogFormat &format); - virtual ~Logger() = default; + /// @brief Constructs the null logger. + Logger(); + explicit Logger(std::shared_ptr impl); - [[nodiscard]] virtual bool will_log(LogLevel level) const = 0; + [[nodiscard]] bool will_log(LogLevel level) const; - void log(const LogLevel level, const std::string &message, - const Time time = Clock::now(), + void log(LogLevel level, const std::string &message, Time time = Clock::now(), const std::source_location &location = - std::source_location::current()) const { - if (will_log(level)) { - log_impl(time, level, message, location); - } - } + std::source_location::current()) const; - virtual void flush() = 0; + void flush() const; + + [[nodiscard]] std::shared_ptr impl() const; -protected: - virtual void log_impl(Time time, LogLevel level, const std::string &message, - const std::source_location &location) const = 0; +private: + std::shared_ptr m_impl; }; } // namespace odr diff --git a/src/odr/odr.cpp b/src/odr/odr.cpp index 85071c076..4371dcf4c 100644 --- a/src/odr/odr.cpp +++ b/src/odr/odr.cpp @@ -430,7 +430,7 @@ odr::DecoderEngine odr::decoder_engine_by_name(const std::string &engine) { } std::vector odr::list_file_types(const std::string &path, - Logger &logger) { + const Logger &logger) { return DecodedFile::list_file_types(path, logger); } @@ -438,20 +438,21 @@ std::vector odr::list_decoder_engines(const FileType as) { return DecodedFile::list_decoder_engines(as); } -std::string_view odr::mimetype(const std::string &path, Logger &logger) { +std::string_view odr::mimetype(const std::string &path, const Logger &logger) { return DecodedFile::mimetype(path, logger); } -odr::DecodedFile odr::open(const std::string &path, Logger &logger) { +odr::DecodedFile odr::open(const std::string &path, const Logger &logger) { return DecodedFile(path, logger); } odr::DecodedFile odr::open(const std::string &path, const FileType as, - Logger &logger) { + const Logger &logger) { return {path, as, logger}; } odr::DecodedFile odr::open(const std::string &path, - const DecodePreference &preference, Logger &logger) { + const DecodePreference &preference, + const Logger &logger) { return {path, preference, logger}; } diff --git a/src/odr/odr.hpp b/src/odr/odr.hpp index 1e6045ece..b4ed8f1a1 100644 --- a/src/odr/odr.hpp +++ b/src/odr/odr.hpp @@ -78,7 +78,7 @@ file_type_by_mimetype(std::string_view mimetype) noexcept; /// @param logger The logger to use. /// @return The file types. [[nodiscard]] std::vector -list_file_types(const std::string &path, Logger &logger = Logger::null()); +list_file_types(const std::string &path, const Logger &logger = Logger::null()); /// @brief Determine the decoder engines for a file path and file type. /// @param as The file type. /// @return The decoder engines. @@ -88,21 +88,21 @@ list_file_types(const std::string &path, Logger &logger = Logger::null()); /// @param logger The logger to use. /// @return The MIME types. [[nodiscard]] std::string_view mimetype(const std::string &path, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); /// @brief Open a file. /// @param path The file path. /// @param logger The logger to use. /// @return The decoded file. [[nodiscard]] DecodedFile open(const std::string &path, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); /// @brief Open a file. /// @param path The file path. /// @param as The file type. /// @param logger The logger to use. /// @return The decoded file. [[nodiscard]] DecodedFile open(const std::string &path, FileType as, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); /// @brief Open a file. /// @param path The file path. /// @param preference The decode preference. @@ -110,6 +110,6 @@ list_file_types(const std::string &path, Logger &logger = Logger::null()); /// @return The decoded file. [[nodiscard]] DecodedFile open(const std::string &path, const DecodePreference &preference, - Logger &logger = Logger::null()); + const Logger &logger = Logger::null()); } // namespace odr diff --git a/test/src/document_test.cpp b/test/src/document_test.cpp index 4d3e88a0d..63d9d1917 100644 --- a/test/src/document_test.cpp +++ b/test/src/document_test.cpp @@ -35,11 +35,10 @@ Element find_paragraph_with_text_prefix(const Element root, } // namespace TEST(Document, odt) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/odt/about.odt"), *logger); + TestData::test_file_path("odr-public/odt/about.odt"), logger); EXPECT_EQ(document_file.file_type(), FileType::opendocument_text); @@ -58,11 +57,10 @@ TEST(Document, odt) { } TEST(Document, odt_element_path) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/odt/about.odt"), *logger); + TestData::test_file_path("odr-public/odt/about.odt"), logger); EXPECT_EQ(document_file.file_type(), FileType::opendocument_text); @@ -82,11 +80,10 @@ TEST(Document, odt_element_path) { } TEST(Document, odt_element_path2) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/odt/style-various-1.odt"), *logger); + TestData::test_file_path("odr-public/odt/style-various-1.odt"), logger); EXPECT_EQ(document_file.file_type(), FileType::opendocument_text); @@ -103,11 +100,10 @@ TEST(Document, odt_element_path2) { } TEST(Document, odt_text_position) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/odt/style-various-1.odt"), *logger); + TestData::test_file_path("odr-public/odt/style-various-1.odt"), logger); const Document document = document_file.document(); const Element root = document.root_element(); @@ -131,12 +127,10 @@ TEST(Document, odt_text_position) { } TEST(Document, odt_line_height_and_text_indent) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/odt/file-sample_100kB.odt"), - *logger); + TestData::test_file_path("odr-public/odt/file-sample_100kB.odt"), logger); const Document document = document_file.document(); const Element root = document.root_element(); @@ -151,11 +145,10 @@ TEST(Document, odt_line_height_and_text_indent) { } TEST(Document, odg) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/odg/sample.odg"), *logger); + TestData::test_file_path("odr-public/odg/sample.odg"), logger); EXPECT_EQ(document_file.file_type(), FileType::opendocument_graphics); @@ -175,11 +168,10 @@ TEST(Document, odg) { } TEST(Document, edit_odt) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/odt/about.odt"), *logger); + TestData::test_file_path("odr-public/odt/about.odt"), logger); const Document document = document_file.document(); std::function edit = [&](const Element &element) { @@ -212,12 +204,10 @@ TEST(Document, edit_odt) { } TEST(Document, edit_docx) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/docx/style-various-1.docx"), - *logger); + TestData::test_file_path("odr-public/docx/style-various-1.docx"), logger); const Document document = document_file.document(); std::function edit = [&](const Element &element) { @@ -250,13 +240,12 @@ TEST(Document, edit_docx) { } TEST(Document, edit_odt_diff) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const char *diff = R"({"modifiedText":{"/child:16/child:0":"Outasdfsdafdline","/child:24/child:0":"Colorasdfasdfasdfed Line","/child:6/child:0":"Text hello world!"}})"; const DocumentFile document_file( - TestData::test_file_path("odr-public/odt/style-various-1.odt"), *logger); + TestData::test_file_path("odr-public/odt/style-various-1.odt"), logger); const Document document = document_file.document(); html::edit(document, diff); @@ -286,13 +275,12 @@ TEST(Document, edit_odt_diff) { } TEST(Document, edit_ods_diff) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const char *diff = R"({"modifiedText":{"/child:0/cell:A1/child:0/child:0":"Page 1 hi","/child:1/cell:A1/child:0/child:0":"Page 2 hihi","/child:2/cell:A1/child:0/child:0":"Page 3 hihihi","/child:3/cell:A1/child:0/child:0":"Page 4 hihihihi","/child:4/cell:A1/child:0/child:0":"Page 5 hihihihihi"}})"; DocumentFile document_file( - TestData::test_file_path("odr-public/ods/pages.ods"), *logger); + TestData::test_file_path("odr-public/ods/pages.ods"), logger); document_file = document_file.decrypt( TestData::test_file("odr-public/ods/pages.ods").password.value()); const Document document = document_file.document(); @@ -333,14 +321,12 @@ TEST(Document, edit_ods_diff) { } TEST(Document, edit_docx_diff) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const auto *diff = R"({"modifiedText":{"/child:16/child:0/child:0":"Outasdfsdafdline","/child:24/child:0/child:0":"Colorasdfasdfasdfed Line","/child:6/child:0/child:0":"Text hello world!"}})"; const DocumentFile document_file( - TestData::test_file_path("odr-public/docx/style-various-1.docx"), - *logger); + TestData::test_file_path("odr-public/docx/style-various-1.docx"), logger); const Document document = document_file.document(); html::edit(document, diff); diff --git a/test/src/file_test.cpp b/test/src/file_test.cpp index 6a20c929b..7c7f7d199 100644 --- a/test/src/file_test.cpp +++ b/test/src/file_test.cpp @@ -17,7 +17,7 @@ TEST(DecodedFile, wpd) { const auto path = TestData::test_file_path("odr-public/wpd/Sync3 Sample Page.wpd"); try { - DecodedFile file(path, *logger); + DecodedFile file(path, logger); FAIL(); } catch (const UnsupportedFileType &e) { EXPECT_EQ(e.file_type, FileType::word_perfect); diff --git a/test/src/html_output_test.cpp b/test/src/html_output_test.cpp index 8b897f0e3..eb52236df 100644 --- a/test/src/html_output_test.cpp +++ b/test/src/html_output_test.cpp @@ -60,9 +60,9 @@ TEST_P(HtmlOutputTests, html_meta) { const FileCategory file_category = file_category_by_file_type(test_file.type); - ODR_INFO(*logger, "Testing file: " << test_file.short_path << " with engine: " - << odr::decoder_engine_to_string(engine) - << " output to: " << output_path); + ODR_INFO(logger, "Testing file: " << test_file.short_path << " with engine: " + << odr::decoder_engine_to_string(engine) + << " output to: " << output_path); // these files cannot be opened if (util::string::ends_with(test_file.short_path, ".sxw") || @@ -74,7 +74,7 @@ TEST_P(HtmlOutputTests, html_meta) { DecodePreference decode_preference; decode_preference.as_file_type = test_file.type; decode_preference.with_engine = engine; - DecodedFile file = open(test_file.absolute_path, decode_preference, *logger); + DecodedFile file = open(test_file.absolute_path, decode_preference, logger); FileMeta file_meta = file.file_meta(); diff --git a/test/src/html_test.cpp b/test/src/html_test.cpp index 7eb196118..88f4cad2f 100644 --- a/test/src/html_test.cpp +++ b/test/src/html_test.cpp @@ -15,7 +15,7 @@ TEST(html, views) { const auto logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/ods/Senza nome 1.ods"), *logger); + TestData::test_file_path("odr-public/ods/Senza nome 1.ods"), logger); const Document document = document_file.document(); diff --git a/test/src/internal/oldms/ppt_test.cpp b/test/src/internal/oldms/ppt_test.cpp index 90331f1c2..bf383c459 100644 --- a/test/src/internal/oldms/ppt_test.cpp +++ b/test/src/internal/oldms/ppt_test.cpp @@ -66,11 +66,10 @@ TEST(OldMs, ppt_parse_style_text_prop_atom) { } TEST(OldMs, ppt_empty) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/ppt/empty.ppt"), *logger); + TestData::test_file_path("odr-public/ppt/empty.ppt"), logger); EXPECT_EQ(document_file.file_type(), FileType::legacy_powerpoint_presentation); @@ -87,11 +86,10 @@ TEST(OldMs, ppt_empty) { } TEST(OldMs, ppt_style_various) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/ppt/style-various-1.ppt"), *logger); + TestData::test_file_path("odr-public/ppt/style-various-1.ppt"), logger); EXPECT_EQ(document_file.file_type(), FileType::legacy_powerpoint_presentation); diff --git a/test/src/internal/oldms/xls_test.cpp b/test/src/internal/oldms/xls_test.cpp index 6c4eaff1f..25c481d6e 100644 --- a/test/src/internal/oldms/xls_test.cpp +++ b/test/src/internal/oldms/xls_test.cpp @@ -301,11 +301,10 @@ TEST(OldMs, xls_palette_record) { } TEST(OldMs, xls_empty) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( - TestData::test_file_path("odr-public/xls/empty.xls"), *logger); + TestData::test_file_path("odr-public/xls/empty.xls"), logger); EXPECT_EQ(document_file.file_type(), FileType::legacy_excel_worksheets); @@ -326,12 +325,11 @@ TEST(OldMs, xls_empty) { } TEST(OldMs, xls_file_example_10) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( TestData::test_file_path("odr-public/xls/file_example_XLS_10.xls"), - *logger); + logger); EXPECT_EQ(document_file.file_type(), FileType::legacy_excel_worksheets); @@ -371,12 +369,11 @@ TEST(OldMs, xls_file_example_10) { // The 5000-row variant has a shared string table well beyond the 8224-byte // record limit, so it exercises the SST CONTINUE handling on a real file. TEST(OldMs, xls_file_example_5000) { - const std::unique_ptr logger = - Logger::create_stdio("odr-test", LogLevel::verbose); + const Logger logger = Logger::create_stdio("odr-test", LogLevel::verbose); const DocumentFile document_file( TestData::test_file_path("odr-public/xls/file_example_XLS_5000.xls"), - *logger); + logger); const Document document = document_file.document(); diff --git a/test/src/internal/pdf/pdf_file.cpp b/test/src/internal/pdf/pdf_file.cpp index 84402e782..e30044283 100644 --- a/test/src/internal/pdf/pdf_file.cpp +++ b/test/src/internal/pdf/pdf_file.cpp @@ -27,7 +27,7 @@ std::shared_ptr open_pdf(const std::string &bytes) { HtmlService make_service(const std::string &bytes, const HtmlConfig &config) { const odr::PdfFile file(open_pdf(bytes)); - const std::shared_ptr logger = Logger::create_null(); + const Logger logger = Logger::null(); return internal::html::create_pdf_service(file, "", config, logger); } diff --git a/test/src/logger_test.cpp b/test/src/logger_test.cpp index 145abec28..1919125c3 100644 --- a/test/src/logger_test.cpp +++ b/test/src/logger_test.cpp @@ -1,13 +1,146 @@ #include +#include +#include +#include +#include +#include +#include +#include +#include +#include + #include using namespace odr; +namespace { + +/// A format that prints the bare message, so tests can compare exactly. +LogFormat bare_format() { + return LogFormat{.time_format = "", + .level_width = 0, + .name_width = 0, + .location_width = 0}; +} + +/// Returns a logger writing into a stream owned by it, plus a borrowed view. +std::pair capturing_logger() { + auto output = std::make_unique(); + auto *const captured = output.get(); + return {Logger::create_stdio("test", LogLevel::verbose, bare_format(), + std::move(output)), + captured}; +} + +/// A sink defined outside the library, exercising `ILogger` as the public +/// extension point. +class CollectingLogger final : public ILogger { +public: + [[nodiscard]] bool will_log(const LogLevel level) const override { + return level >= LogLevel::warning; + } + + void log(Time, LogLevel, const std::string &message, + const std::source_location &location) override { + messages.push_back(message); + lines.push_back(location.line()); + } + + void flush() override { ++flushes; } + + std::vector messages; + std::vector lines; + std::size_t flushes{0}; +}; + +} // namespace + TEST(Logger, stdio) { const auto logger = Logger::create_stdio("test", LogLevel::verbose); - logger->log(LogLevel::verbose, "Test message with log function"); + logger.log(LogLevel::verbose, "Test message with log function"); + + ODR_VERBOSE(logger, "Test message with verbose log macro"); +} + +TEST(Logger, default_constructed_is_null) { + EXPECT_FALSE(Logger().will_log(LogLevel::fatal)); + EXPECT_FALSE(Logger::null().will_log(LogLevel::fatal)); +} + +TEST(Logger, copies_share_the_sink) { + auto [logger, captured] = capturing_logger(); + const Logger copy = logger; + + copy.log(LogLevel::info, "from the copy"); + logger.log(LogLevel::info, "from the original"); + copy.flush(); + + EXPECT_EQ(captured->str(), "from the copy\nfrom the original\n"); +} + +TEST(Logger, respects_the_level) { + auto [logger, captured] = capturing_logger(); + const Logger warning = + Logger::create_stdio("test", LogLevel::warning, bare_format(), nullptr); + + logger.log(LogLevel::verbose, "kept"); + logger.flush(); + + EXPECT_EQ(captured->str(), "kept\n"); + EXPECT_FALSE(warning.will_log(LogLevel::debug)); + EXPECT_TRUE(warning.will_log(LogLevel::error)); +} + +TEST(Logger, tee_fans_out) { + auto [first, first_captured] = capturing_logger(); + auto [second, second_captured] = capturing_logger(); + + const Logger tee = Logger::create_tee({first, second}); + tee.log(LogLevel::info, "fanned out"); + tee.flush(); + + EXPECT_EQ(first_captured->str(), "fanned out\n"); + EXPECT_EQ(second_captured->str(), "fanned out\n"); +} + +TEST(Logger, tee_rejects_empty) { + EXPECT_THROW(std::ignore = Logger::create_tee({}), std::invalid_argument); +} + +TEST(Logger, custom_sink_drives_the_handle) { + const auto sink = std::make_shared(); + const Logger logger(sink); + + logger.log(LogLevel::debug, "dropped by will_log"); + logger.log(LogLevel::error, "kept"); + ODR_WARNING(logger, "kept too"); + logger.flush(); + + EXPECT_EQ(sink->messages, (std::vector{"kept", "kept too"})); + EXPECT_EQ(sink->flushes, 1); + // The macro reports the call site, not a spot inside the logger. + ASSERT_EQ(sink->lines.size(), 2); + EXPECT_GT(sink->lines.at(1), 0); +} + +TEST(Logger, custom_sink_composes_with_tee) { + const auto sink = std::make_shared(); + auto [stdio, captured] = capturing_logger(); + + const Logger tee = Logger::create_tee({Logger(sink), stdio}); + tee.log(LogLevel::error, "both"); + tee.log(LogLevel::verbose, "only the stdio one"); + tee.flush(); + + EXPECT_EQ(sink->messages, (std::vector{"both"})); + EXPECT_EQ(captured->str(), "both\nonly the stdio one\n"); +} - ODR_VERBOSE(*logger, "Test message with verbose log macro"); +TEST(Logger, rejects_a_null_sink) { + // Braces, not parens: `Logger(std::shared_ptr())` is a function + // declaration, not an expression (MSVC diagnoses it, clang accepts it). + const std::shared_ptr sink; + EXPECT_THROW(Logger{sink}, std::invalid_argument); } diff --git a/test/src/odr_test.cpp b/test/src/odr_test.cpp index dbadac052..507dc4e1b 100644 --- a/test/src/odr_test.cpp +++ b/test/src/odr_test.cpp @@ -19,12 +19,12 @@ TEST(odr, types_odt) { const auto logger = Logger::create_stdio("odr-test", LogLevel::verbose); const auto path = TestData::test_file_path("odr-public/odt/about.odt"); - const auto types = list_file_types(path, *logger); + const auto types = list_file_types(path, logger); EXPECT_EQ(types.size(), 2); EXPECT_EQ(types[0], FileType::zip); EXPECT_EQ(types[1], FileType::opendocument_text); - const auto mime = mimetype(path, *logger); + const auto mime = mimetype(path, logger); EXPECT_EQ(mime, "application/vnd.oasis.opendocument.text"); } @@ -33,14 +33,14 @@ TEST(odr, types_wpd) { const auto path = TestData::test_file_path("odr-public/wpd/Sync3 Sample Page.wpd"); - const auto types = list_file_types(path, *logger); + const auto types = list_file_types(path, logger); EXPECT_EQ(types.size(), 1); EXPECT_EQ(types[0], FileType::word_perfect); if (project_info::has_libmagic()) { - const auto mime = mimetype(path, *logger); + const auto mime = mimetype(path, logger); EXPECT_EQ(mime, "application/vnd.wordperfect"); } else { - EXPECT_THROW(std::ignore = mimetype(path, *logger), UnsupportedFileType); + EXPECT_THROW(std::ignore = mimetype(path, logger), UnsupportedFileType); } }