From 42cda7461baf35c5469c4da61c7f573e139817fd Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 12:08:26 +0200 Subject: [PATCH 01/10] fix: retain plugin library lifetime --- docs/dialog-sdk-reference.md | 26 +++++++-- docs/toolbox-porting-gap-analysis.md | 47 +++++++++------- .../include/pj_base/data_source_protocol.h | 4 +- .../include/pj_base/message_parser_protocol.h | 8 +-- pj_base/include/pj_base/plugin_data_api.h | 2 +- .../include/pj_base/sdk/service_registry.hpp | 2 +- pj_base/include/pj_base/toolbox_protocol.h | 4 +- .../include/pj_plugins/host/dialog_handle.hpp | 23 +++++--- .../pj_plugins/host/dialog_library.hpp | 15 ++--- .../dialog_protocol/src/dialog_library.cpp | 32 +++++------ .../tests/dialog_library_test.cpp | 16 ++++++ pj_plugins/docs/ARCHITECTURE.md | 56 +++++++++++-------- pj_plugins/docs/REQUIREMENTS.md | 19 ++++--- pj_plugins/docs/data-source-guide.md | 19 ++++--- pj_plugins/docs/dialog-plugin-guide.md | 31 +++++----- pj_plugins/docs/message-parser-guide.md | 26 ++++----- pj_plugins/docs/toolbox-guide.md | 20 +++---- .../pj_plugins/host/data_source_handle.hpp | 11 +++- .../pj_plugins/host/data_source_library.hpp | 20 ++++--- .../pj_plugins/host/message_parser_handle.hpp | 9 ++- .../host/message_parser_library.hpp | 20 ++++--- .../host/service_registry_builder.hpp | 4 +- .../pj_plugins/host/toolbox_handle.hpp | 9 ++- .../pj_plugins/host/toolbox_library.hpp | 20 ++++--- pj_plugins/src/data_source_library.cpp | 38 ++++++------- pj_plugins/src/detail/library_loader.hpp | 5 ++ pj_plugins/src/message_parser_library.cpp | 36 +++++------- pj_plugins/src/toolbox_library.cpp | 35 +++++------- pj_plugins/tests/data_source_library_test.cpp | 15 +++++ .../tests/message_parser_library_test.cpp | 16 ++++++ pj_plugins/tests/toolbox_plugin_test.cpp | 15 +++++ 31 files changed, 360 insertions(+), 243 deletions(-) diff --git a/docs/dialog-sdk-reference.md b/docs/dialog-sdk-reference.md index b3e1d2e..1271063 100644 --- a/docs/dialog-sdk-reference.md +++ b/docs/dialog-sdk-reference.md @@ -53,9 +53,10 @@ For the full tutorial, see [dialog-plugin-guide.md](../pj_plugins/docs/dialog-pl | Method | Description | |--------|-------------| | `setButtonText(name, text)` | Set button label | -| `setShortcut(name, key_sequence)` | Assign keyboard shortcut (e.g. `"Ctrl+A"`) **NEW** | +| `setButtonIcon(name, svg_data)` | Set an inline SVG icon | +| `setShortcut(name, key_sequence)` | Assign keyboard shortcut (e.g. `"Ctrl+A"`) | | `setFilePicker(name, text, filter, title)` | Turn into file picker | -| `setFolderPicker(name, text, title)` | Turn into folder picker **NEW** | +| `setFolderPicker(name, text, title)` | Turn into folder picker | ### QListWidget @@ -71,13 +72,23 @@ For the full tutorial, see [dialog-plugin-guide.md](../pj_plugins/docs/dialog-pl | `setTableHeaders(name, vector)` | Set column headers | | `setTableRows(name, vector>)` | Set row data | | `setSelectedRows(name, vector)` | Set selected row indices | -| `setDisabledRows(name, vector)` | Grey out rows (non-selectable) **NEW** | +| `setDisabledRows(name, vector)` | Grey out rows (non-selectable) | + +### QFrame Chart Container + +| Method | Description | +|--------|-------------| +| `setChartSeries(name, vector)` | Create/update chart series inside a QFrame | +| `clearChart(name)` | Remove chart series | +| `setChartZoomEnabled(name, bool)` | Enable chart zoom/pan events | ### QPlainTextEdit | Method | Description | |--------|-------------| -| `setPlainText(name, text)` | Set plain text content **NEW** | +| `setPlainText(name, text)` | Set plain text content | +| `setCodeContent(name, code)` | Set editable code content | +| `setCodeLanguage(name, lang)` | Set syntax highlighting language such as `"lua"` or `"python"` | ### QTabWidget @@ -98,6 +109,7 @@ For the full tutorial, see [dialog-plugin-guide.md](../pj_plugins/docs/dialog-pl |--------|-------------| | `setEnabled(name, bool)` | Enable/disable widget | | `setVisible(name, bool)` | Show/hide widget | +| `setDropTarget(name, bool)` | Accept dropped item labels and emit `onItemsDropped` | ### Dialog-level Commands @@ -121,9 +133,12 @@ Override these in your `DialogPluginTyped` subclass. Return `true` when state ch | `onValueChanged(name, double)` | QDoubleSpinBox | New double value | | `onClicked(name)` | QPushButton | (no payload) | | `onFileSelected(name, path)` | QPushButton (file picker) | Selected file path | -| `onFolderSelected(name, path)` | QPushButton (folder picker) | Selected folder path **NEW** | +| `onFolderSelected(name, path)` | QPushButton (folder picker) | Selected folder path | | `onSelectionChanged(name, items)` | QListWidget, QTableWidget | Vector of selected item texts | | `onItemDoubleClicked(name, index)` | QListWidget, QTableWidget | Row index of double-clicked item | +| `onCodeChanged(name, code)` | QPlainTextEdit code editor | Edited code | +| `onItemsDropped(name, items)` | Any widget with `setDropTarget` | Dropped item labels | +| `onChartViewChanged(name, x_min, x_max, y_min, y_max)` | QFrame chart container | Visible chart range | | `onTabChanged(name, index)` | QTabWidget | New tab index | --- @@ -137,7 +152,6 @@ Override these in your `DialogPluginTyped` subclass. Return `true` when state ch | `onRejected()` | User clicked Cancel | void | | `saveConfig()` | Host persisting state | JSON string | | `loadConfig(json)` | Host restoring state | `true` if state changed | -| `lastError()` | Host checking for errors | Error string or empty | --- diff --git a/docs/toolbox-porting-gap-analysis.md b/docs/toolbox-porting-gap-analysis.md index b0d324a..9c3e1c4 100644 --- a/docs/toolbox-porting-gap-analysis.md +++ b/docs/toolbox-porting-gap-analysis.md @@ -1,17 +1,18 @@ # Toolbox Porting — SDK Gap Analysis -This document provides an exhaustive comparison of PlotJuggler 3.x toolbox plugin -features against the current PJ 4.x SDK capabilities. It identifies what Dialog SDK -extensions are required to port the existing toolboxes with full feature parity. +This historical document compares PlotJuggler 3.x toolbox plugin features +against the PJ 4.x SDK capabilities available when the porting work began. +Some gaps listed below have since been closed in the Dialog SDK. **Scope:** `plotjuggler_core` (Dialog SDK, `ToolboxPluginBase`) + `pj-official-plugins` (Quaternion to RPY port as the reference implementation). -**Summary:** The SDK is sufficient for simple, headless processing toolboxes (Quaternion -reaches ~80% feature parity). FFT drops to ~60%. The Lua Reactive Script Editor cannot -be ported without major SDK extensions. Six gaps are blocking and require new -infrastructure: embedded chart widget, zoom event, drag-drop on chart, editable code -editor, reactive time-tick, and ScatterXY output type. +**Current status:** Dialog SDK support now exists for chart containers, +chart zoom/pan events, drop targets, editable `QPlainTextEdit` code editors, +and periodic ticks. Treat the original gap analysis below as historical +context for porting priorities, not as a current reference. Remaining +porting risks should be revalidated against the current SDK and datastore +surface; ScatterXY-style output remains a separate datastore concern. --- @@ -51,7 +52,10 @@ In the current port: - No preview — the transform runs directly on OK - Modal dialog that opens and closes -This is not a failure of the port — it is a limitation of the current SDK: `DialogPluginTyped` does not support preview widgets (charts) or drag-and-drop. Those features require Dialog SDK extensions. +At the time of the original port, `DialogPluginTyped` did not support preview +widgets or drag-and-drop. Current Dialog SDKs provide chart containers, +chart range events, and generic drop targets; any toolbox port should be +revalidated against those APIs before treating this section as blocking. The end-to-end wiring is not yet confirmed at 100%. The plugin loads, the dialog opens with combo boxes, but we have not verified that the transform produces data visible in the plots. There may be a mismatch in catalog field names. @@ -74,11 +78,11 @@ Three toolbox plugins exist in `plotjuggler/plotjuggler_plugins/`: | Aspect | PJ 3.x | New SDK | |--------|--------|---------| | Plugin owns its UI | Yes — full `QWidget` with arbitrary children | No — `.ui` file + host-rendered dialog runtime | -| Data access | Direct reference to `PlotDataMapRef` | Via handles: `catalogSnapshot`, `readSeries`, `appendRecord` | +| Data access | Direct reference to `PlotDataMapRef` | Via handles: `catalogSnapshot`, `readSeriesArrow`, `appendRecord` | | Communication | Qt signals/slots | C ABI vtables + JSON config | | Qt dependency | Required | None in core/plugin SDK; GUI hosts supply their toolkit runtime | -| Embedded chart preview | Integrated (`PlotWidgetBase`) | Not available in the SDK | -| Drag-and-drop | Via `eventFilter` in the plugin | Not supported by the dialog protocol | +| Embedded chart preview | Integrated (`PlotWidgetBase`) | Available through chart data on QFrame containers | +| Drag-and-drop | Via `eventFilter` in the plugin | Available through dialog drop targets and `onItemsDropped` | | Output type | `PlotData` (time series) **or** `PlotDataXY` (scatter) | Time-indexed series only | | Transform registry | Yes — re-applied on layout reload | No — outputs are static data | | Reactive execution | `ReactiveLuaFunction` re-runs on every slider tick | `onTick()` exists but cannot write to datastore or access current timestamp | @@ -250,7 +254,10 @@ The Lua editor uses `QCodeEditor` (external library) with: | Instances | 3: global code, function body, library | | Font size | Ctrl+wheel: 8–14 pt range, persisted to `QSettings` | -The current SDK has `setPlainText()` for **read-only** text display only. The dialog docs explicitly state that `QTextEdit` and `QPlainTextEdit` are **not supported** by the widget binding system. Without an editable code widget, the Lua editor cannot exist. +The current Dialog SDK has `setPlainText()`, editable `setCodeContent()`, +`setCodeLanguage()`, and `onCodeChanged()` for `QPlainTextEdit`-based code +editing. The Lua editor port still needs product-level validation, but an +editable code widget is no longer a missing SDK primitive. **SDK equivalent needed:** ```cpp @@ -299,7 +306,7 @@ The sol2 Lua API exposed to scripts: | `CreatedSeriesTime` | `at(i)`, `clear()`, `push_back(x,y)`, `size()` | | `CreatedSeriesXY` | `at(i)`, `clear()`, `push_back(x,y)`, `size()` | -The current SDK `onTick()` in `DialogPluginTyped` fires periodically while the dialog is open, but: +Dialog-level `onTick()` in `DialogPluginTyped` fires periodically while the dialog is open, but: - Has no access to the current time slider value - Cannot call `toolboxHost().appendRecord()` — the toolbox host is separate from the dialog plugin - Cannot be used to implement time-reactive series generation @@ -388,13 +395,13 @@ The Lua editor persists several settings directly via `QSettings` outside of `sa | 4.2 | Transform registry (re-apply on reload) | MEDIUM | MEDIUM | HIGH | MEDIUM | | 4.3 | `QSettings` fine-grained persistence | LOW | LOW | MEDIUM | LOW | -**Feature parity estimate without SDK changes:** +**Original feature parity estimate from the first gap analysis:** | Toolbox | Parity | Blocking gaps | |---------|--------|---------------| -| Quaternion | ~80% | No chart preview; no drag-drop auto-fill | -| FFT | ~60% | No chart preview; no drag-drop; no zoom-aware range; no ScatterXY output | -| Lua Editor | ~10% | No editable code widget; no reactive execution | +| Quaternion | ~80% | Originally blocked by chart preview and drag-drop auto-fill; revalidate against current chart/drop APIs | +| FFT | ~60% | Revalidate chart, drag-drop, and zoom support; ScatterXY output remains a datastore question | +| Lua Editor | ~10% | Editable code widget now exists; reactive execution still needs product-level validation | --- @@ -478,4 +485,6 @@ void appendArrowScatterIpc(TopicHandle topic, --- -*Gaps 1.1, 1.4, 2.1, 3.1, 3.3, and 4.1 are blocking for their respective toolboxes and require new SDK infrastructure. The remaining gaps represent UX degradation that can be partially mitigated with workarounds within the current SDK.* +*Historical note: several gaps called blocking in this document have since +received SDK support. Before using this as a porting checklist, re-run the +gap analysis against the current Dialog SDK and datastore APIs.* diff --git a/pj_base/include/pj_base/data_source_protocol.h b/pj_base/include/pj_base/data_source_protocol.h index 584ede1..f5890e7 100644 --- a/pj_base/include/pj_base/data_source_protocol.h +++ b/pj_base/include/pj_base/data_source_protocol.h @@ -2,7 +2,7 @@ * @file data_source_protocol.h * @brief C ABI protocol for DataSource plugins (version 4). * - * v4 summary of changes vs v3: + * v4 summary: * - Arrow C Data Interface at the write boundary: bulk loaders use * SourceWriteHost::append_arrow_stream instead of per-row appends. * See pj_base/plugin_data_api.h. append_arrow_ipc is removed. @@ -46,7 +46,7 @@ extern "C" { * Reads of any slot added after v4.0 must be gated with PJ_HAS_TAIL_SLOT. * * Computed as `offsetof(last v4.0 slot) + sizeof(its function pointer)`. - * Last v4.0 slot is `get_plugin_extension` (promoted from v3.1 tail). + * Last v4.0 slot is `get_plugin_extension`. */ #define PJ_DATA_SOURCE_MIN_VTABLE_SIZE \ (offsetof(PJ_data_source_vtable_t, get_plugin_extension) + sizeof(const void* (*)(void*, PJ_string_view_t))) diff --git a/pj_base/include/pj_base/message_parser_protocol.h b/pj_base/include/pj_base/message_parser_protocol.h index e3c06ae..1da565e 100644 --- a/pj_base/include/pj_base/message_parser_protocol.h +++ b/pj_base/include/pj_base/message_parser_protocol.h @@ -2,7 +2,7 @@ * @file message_parser_protocol.h * @brief C ABI protocol for MessageParser plugins (version 4). * - * v4 summary of changes vs v3: + * v4 summary: * - Every vtable slot is PJ_NOEXCEPT and carries a thread-class tag. * - Parser write host (pj.parser_write.v1) no longer has * append_arrow_ipc — see plugin_data_api.h. Parsers stay per-record; @@ -35,7 +35,7 @@ extern "C" { * MUST NOT GROW when new tail slots are appended. See PJ_ABI_VERSION comment * in plugin_data_api.h for the rationale. * - * Last v4.0 slot is `get_plugin_extension` (promoted from v3 tail). + * Last v4.0 slot is `get_plugin_extension`. */ #define PJ_MESSAGE_PARSER_MIN_VTABLE_SIZE \ (offsetof(PJ_message_parser_vtable_t, get_plugin_extension) + sizeof(const void* (*)(void*, PJ_string_view_t))) @@ -71,8 +71,8 @@ typedef struct PJ_message_parser_vtable_t { * and the marketplace; must be unique per plugin. * "name" — human-readable plugin name (string). * "version" — semver version string (string). - * "encoding" — encoding this parser handles (string). The host uses - * this to match binding requests to parsers. + * "encoding" — encodings this parser handles (array of strings). The host + * uses this to match binding requests to parsers. */ const char* manifest_json; diff --git a/pj_base/include/pj_base/plugin_data_api.h b/pj_base/include/pj_base/plugin_data_api.h index 58dc4cc..1994a1f 100644 --- a/pj_base/include/pj_base/plugin_data_api.h +++ b/pj_base/include/pj_base/plugin_data_api.h @@ -43,7 +43,7 @@ extern "C" { * (e.g. DataSource + Dialog in one .so) work without any extra ceremony. * Do not redefine it manually. * - * v4 plugins advertise version 4. Breaking v3→v4 changes: + * v4 plugins advertise version 4. Data-plane changes from the pre-v4 design: * - Arrow C Data Interface replaces Arrow IPC bytes at the boundary * (append_arrow_stream + read_series_arrow). * - append_arrow_ipc removed from all write hosts. diff --git a/pj_base/include/pj_base/sdk/service_registry.hpp b/pj_base/include/pj_base/sdk/service_registry.hpp index 7821cce..c8a6e4b 100644 --- a/pj_base/include/pj_base/sdk/service_registry.hpp +++ b/pj_base/include/pj_base/sdk/service_registry.hpp @@ -14,7 +14,7 @@ namespace PJ::sdk { /// Typed C++ wrapper around `PJ_service_registry_t`. /// -/// Plugins receive a registry via their v3 `bind()` virtual. Two lookup +/// Plugins receive a registry via their v4 `bind()` virtual. Two lookup /// styles: /// - `get()` — `std::optional`; miss yields `nullopt`. /// - `require()` — `Expected`; miss yields an error string. diff --git a/pj_base/include/pj_base/toolbox_protocol.h b/pj_base/include/pj_base/toolbox_protocol.h index f039d1d..f471d07 100644 --- a/pj_base/include/pj_base/toolbox_protocol.h +++ b/pj_base/include/pj_base/toolbox_protocol.h @@ -2,7 +2,7 @@ * @file toolbox_protocol.h * @brief C ABI protocol for Toolbox plugins (version 4). * - * v4 summary of changes vs v3: + * v4 summary: * - Toolbox host (pj.toolbox_write.v1) now uses Arrow C Data Interface * for bulk write (append_arrow_stream) and read (read_series_arrow). * The materialised-vector read_series and byte-based append_arrow_ipc @@ -32,7 +32,7 @@ extern "C" { * MUST NOT GROW when new tail slots are appended. See PJ_ABI_VERSION comment * in plugin_data_api.h for the rationale. * - * Last v4.0 slot is `get_plugin_extension` (promoted from v3 tail). + * Last v4.0 slot is `get_plugin_extension`. */ #define PJ_TOOLBOX_MIN_VTABLE_SIZE \ (offsetof(PJ_toolbox_vtable_t, get_plugin_extension) + sizeof(const void* (*)(void*, PJ_string_view_t))) diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp index c9d9fe8..6f5c11a 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include #include @@ -12,7 +13,8 @@ namespace PJ { /// RAII wrapper around a plugin vtable + context (protocol v4). class DialogHandle { public: - explicit DialogHandle(const PJ_dialog_vtable_t* vt) : vt_(vt) { + explicit DialogHandle(const PJ_dialog_vtable_t* vt, std::shared_ptr library_owner = {}) + : vt_(vt), library_owner_(std::move(library_owner)) { if (vt_) { assert(vt_->protocol_version == PJ_DIALOG_PROTOCOL_VERSION); ctx_ = vt_->create(); @@ -20,13 +22,13 @@ class DialogHandle { } /// Non-owning handle from an externally managed context (e.g. a plugin's embedded dialog). - static DialogHandle borrowed(const PJ_dialog_vtable_t* vt, void* ctx) { - return DialogHandle(vt, ctx, false); + static DialogHandle borrowed(const PJ_dialog_vtable_t* vt, void* ctx, std::shared_ptr library_owner = {}) { + return DialogHandle(vt, ctx, false, std::move(library_owner)); } /// Non-owning handle built from a PJ_borrowed_dialog_t fat pointer. - static DialogHandle fromBorrowed(PJ_borrowed_dialog_t borrowed_ref) { - return DialogHandle(borrowed_ref.vtable, borrowed_ref.ctx, false); + static DialogHandle fromBorrowed(PJ_borrowed_dialog_t borrowed_ref, std::shared_ptr library_owner = {}) { + return DialogHandle(borrowed_ref.vtable, borrowed_ref.ctx, false, std::move(library_owner)); } ~DialogHandle() { @@ -35,7 +37,11 @@ class DialogHandle { } } - DialogHandle(DialogHandle&& other) noexcept : vt_(other.vt_), ctx_(other.ctx_), owned_(other.owned_) { + DialogHandle(DialogHandle&& other) noexcept + : vt_(other.vt_), + ctx_(other.ctx_), + owned_(other.owned_), + library_owner_(std::move(other.library_owner_)) { other.vt_ = nullptr; other.ctx_ = nullptr; other.owned_ = false; @@ -46,6 +52,7 @@ class DialogHandle { std::swap(vt_, other.vt_); std::swap(ctx_, other.ctx_); std::swap(owned_, other.owned_); + std::swap(library_owner_, other.library_owner_); } return *this; } @@ -104,11 +111,13 @@ class DialogHandle { } private: - DialogHandle(const PJ_dialog_vtable_t* vt, void* ctx, bool owned) : vt_(vt), ctx_(ctx), owned_(owned) {} + DialogHandle(const PJ_dialog_vtable_t* vt, void* ctx, bool owned, std::shared_ptr library_owner = {}) + : vt_(vt), ctx_(ctx), owned_(owned), library_owner_(std::move(library_owner)) {} const PJ_dialog_vtable_t* vt_ = nullptr; void* ctx_ = nullptr; bool owned_ = true; + std::shared_ptr library_owner_; static std::string safeString(const char* s) { return s ? std::string(s) : std::string(); diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp index a915154..fb53fb6 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp @@ -3,6 +3,7 @@ #include #include +#include #include #include @@ -12,9 +13,9 @@ namespace PJ { /// Loads a standalone dialog plugin shared library and provides factory access. /// -/// The library is dlopen'd with RTLD_LOCAL on load() and dlclose'd on -/// destruction. The vtable pointer remains valid for the library's lifetime. -/// Move-only; not copyable. +/// The library is dlopen'd with RTLD_LOCAL on load(). Dialog handles created +/// from this loader keep the DSO loaded even if the DialogLibrary object is +/// destroyed. Move-only; not copyable. class DialogLibrary { public: DialogLibrary() = default; @@ -34,14 +35,14 @@ class DialogLibrary { return handle_ != nullptr && vtable_ != nullptr; } - /// Raw vtable pointer. Valid for the lifetime of this DialogLibrary. + /// Raw vtable pointer. Valid while this library or any handle created from it is alive. [[nodiscard]] const PJ_dialog_vtable_t* vtable() const { return vtable_; } /// Create a new owning plugin instance. [[nodiscard]] DialogHandle createHandle() const { - return DialogHandle(vtable_); + return DialogHandle(vtable_, handle_); } /// Filesystem path the library was loaded from. @@ -50,11 +51,11 @@ class DialogLibrary { } private: - DialogLibrary(void* handle, const PJ_dialog_vtable_t* vtable, std::string path); + DialogLibrary(std::shared_ptr handle, const PJ_dialog_vtable_t* vtable, std::string path); void reset(); - void* handle_ = nullptr; + std::shared_ptr handle_; const PJ_dialog_vtable_t* vtable_ = nullptr; std::string path_; }; diff --git a/pj_plugins/dialog_protocol/src/dialog_library.cpp b/pj_plugins/dialog_protocol/src/dialog_library.cpp index 85b7c93..967570e 100644 --- a/pj_plugins/dialog_protocol/src/dialog_library.cpp +++ b/pj_plugins/dialog_protocol/src/dialog_library.cpp @@ -38,6 +38,10 @@ void closeLibraryHandle(void* handle) { #endif } +std::shared_ptr adoptLibraryHandle(void* handle) { + return std::shared_ptr(handle, [](void* loaded_handle) { closeLibraryHandle(loaded_handle); }); +} + Expected loadEntryPoint(void* handle) { #if defined(_WIN32) auto symbol = GetProcAddress(reinterpret_cast(handle), "PJ_get_dialog_vtable"); @@ -58,64 +62,58 @@ Expected loadEntryPoint(void* handle) { } // namespace -DialogLibrary::DialogLibrary(void* handle, const PJ_dialog_vtable_t* vtable, std::string path) - : handle_(handle), vtable_(vtable), path_(std::move(path)) {} +DialogLibrary::DialogLibrary(std::shared_ptr handle, const PJ_dialog_vtable_t* vtable, std::string path) + : handle_(std::move(handle)), vtable_(vtable), path_(std::move(path)) {} DialogLibrary::~DialogLibrary() { reset(); } DialogLibrary::DialogLibrary(DialogLibrary&& other) noexcept - : handle_(other.handle_), vtable_(other.vtable_), path_(std::move(other.path_)) { - other.handle_ = nullptr; + : handle_(std::move(other.handle_)), vtable_(other.vtable_), path_(std::move(other.path_)) { other.vtable_ = nullptr; } DialogLibrary& DialogLibrary::operator=(DialogLibrary&& other) noexcept { if (this != &other) { reset(); - handle_ = other.handle_; + handle_ = std::move(other.handle_); vtable_ = other.vtable_; path_ = std::move(other.path_); - other.handle_ = nullptr; other.vtable_ = nullptr; } return *this; } Expected DialogLibrary::load(std::string_view path) { - auto handle = loadLibraryHandle(path); - if (!handle) { - return unexpected(handle.error()); + auto raw_handle = loadLibraryHandle(path); + if (!raw_handle) { + return unexpected(raw_handle.error()); } + auto handle = adoptLibraryHandle(*raw_handle); - auto entry = loadEntryPoint(*handle); + auto entry = loadEntryPoint(handle.get()); if (!entry) { - closeLibraryHandle(*handle); return unexpected(entry.error()); } const PJ_dialog_vtable_t* vtable = (*entry)(); if (vtable == nullptr) { - closeLibraryHandle(*handle); return unexpected(std::string("PJ_get_dialog_vtable returned null")); } if (vtable->protocol_version != PJ_DIALOG_PROTOCOL_VERSION) { - closeLibraryHandle(*handle); return unexpected(std::string("Dialog protocol version mismatch")); } if (vtable->struct_size < sizeof(PJ_dialog_vtable_t)) { - closeLibraryHandle(*handle); return unexpected(std::string("Dialog vtable is smaller than expected")); } - return DialogLibrary(*handle, vtable, std::string(path)); + return DialogLibrary(std::move(handle), vtable, std::string(path)); } void DialogLibrary::reset() { if (handle_ != nullptr) { - closeLibraryHandle(handle_); - handle_ = nullptr; + handle_.reset(); vtable_ = nullptr; path_.clear(); } diff --git a/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp b/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp index afc7bcd..890c8ca 100644 --- a/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp +++ b/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #ifndef PJ_MOCK_DIALOG_PLUGIN_PATH @@ -52,6 +53,21 @@ TEST(DialogLibraryTest, LoadInvalidPath) { EXPECT_FALSE(lib); } +TEST(DialogLibraryTest, HandleKeepsSharedLibraryLoadedAfterLibraryObjectDies) { + std::unique_ptr handle; + { + auto lib = PJ::DialogLibrary::load(PJ_MOCK_DIALOG_PLUGIN_PATH); + ASSERT_TRUE(lib) << lib.error(); + handle = std::make_unique(lib->createHandle()); + ASSERT_NE(handle->context(), nullptr); + } + + auto j = nlohmann::json::parse(handle->manifest(), nullptr, false); + ASSERT_FALSE(j.is_discarded()); + EXPECT_EQ(j["name"], "Mock Dialog"); + handle.reset(); +} + TEST(DialogLibraryTest, MoveSemantics) { auto lib = PJ::DialogLibrary::load(PJ_MOCK_DIALOG_PLUGIN_PATH); ASSERT_TRUE(lib) << lib.error(); diff --git a/pj_plugins/docs/ARCHITECTURE.md b/pj_plugins/docs/ARCHITECTURE.md index d1be28a..6d71896 100644 --- a/pj_plugins/docs/ARCHITECTURE.md +++ b/pj_plugins/docs/ARCHITECTURE.md @@ -6,8 +6,8 @@ Seven rules the loader and every plugin author rely on. Breaking any of these is an ABI break and requires a v5 bump. 1. **Boot-level ABI symbol.** Every plugin .so exports - `pj_plugin_abi_version` as a `const uint32_t` symbol independent of - any vtable. The host `dlsym`s it BEFORE fetching the family vtable; + `pj_plugin_abi_version` as a `uint32_t` symbol independent of any + vtable. The host `dlsym`s it BEFORE fetching the family vtable; missing or mismatched symbol is a fail-fast rejection with a specific error. The symbol is emitted at file scope by `pj_base/include/pj_base/plugin_abi_export.h`, which is transitively @@ -105,8 +105,7 @@ extensions; graduate to stable (`pj..v1`) once locked in. All four plugin families (DataSource, MessageParser, Toolbox, Dialog) track protocol v4. Key v4 distinguishing features (a superset of everything the -previously-circulated v3 design included — v3 was never an official -release, and its changes roll into v4): +previously-circulated pre-v4 design included): - **Arrow C Data Interface at the data boundary.** The write-host vtables expose `append_arrow_stream(ArrowArrayStream*)` as the @@ -135,8 +134,7 @@ release, and its changes roll into v4): Plugin-local symbol isolation is left to `-fvisibility=hidden`. Structural shape inherited from the pre-v4 design work (carries the -service registry, error out-params, and typed borrowed-dialog patterns -that had been developed in the unreleased v3 iteration): +service registry, error out-params, and typed borrowed-dialog patterns): - **Service registry as the sole binding mechanism.** Plugin vtables expose a single `bind(ctx, registry, err)` slot. The host registers all services @@ -147,20 +145,23 @@ that had been developed in the unreleased v3 iteration): - **Structured errors everywhere.** All fallible ABI calls take a `PJ_error_t* out_error` out-parameter. The old per-plugin `get_last_error` slot is gone. -- **Unified write surface.** The three previous write-host vtables - (`PJ_source_write_host_vtable_t`, `PJ_parser_write_host_vtable_t`, - `PJ_toolbox_host_vtable_t`) collapse into one `PJ_write_surface_vtable_t`. - Service name selects semantics; host implementations enforce scope. - Three SDK facade views (`SourceWriteHostView`, `ParserWriteHostView`, - `ToolboxHostView`) still present family-appropriate APIs at the C++ level. +- **Shared write contract, typed ABI services.** DataSource, MessageParser, + and Toolbox all write through the same datastore backend and follow the same + scalar/Arrow ownership rules, but the ABI keeps three distinct service + vtables: `PJ_source_write_host_vtable_t`, + `PJ_parser_write_host_vtable_t`, and `PJ_toolbox_host_vtable_t`. + The service name selects the family-specific type (`"pj.source_write.v1"`, + `"pj.parser_write.v1"`, `"pj.toolbox_write.v1"`), so the compiler prevents + a parser from calling source/toolbox-only operations. - **Typed borrowed dialog.** `get_dialog_context()` returning `void*` is replaced by `get_dialog()` returning a `PJ_borrowed_dialog_t` fat pointer `{ctx, const PJ_dialog_vtable_t* vtable}`. -- **Uniform core plugin-vtable prefix.** Data source, message parser, and - toolbox vtables start with `protocol_version, struct_size, create, - destroy, manifest_json, capabilities, bind, save_config, load_config` in - that order. Dialogs expose a GUI-oriented protocol with - `get_manifest()`/`get_ui_content()` instead. +- **Family-specific plugin vtables after the common prefix.** DataSource, + MessageParser, and Toolbox vtables share + `protocol_version, struct_size, create, destroy, manifest_json`; subsequent + slots are family-specific. For example, DataSource and Toolbox have + `capabilities`, while MessageParser has `bind_schema`. Dialogs expose a + GUI-oriented protocol with `get_manifest()`/`get_ui_content()` instead. Service traits (`pj_base/sdk/service_traits.hpp`, `sdk/toolbox_plugin_base.hpp`) map canonical names to their ABI type and @@ -186,7 +187,9 @@ C ABI protocol → C++ SDK base class → Host loader + RAII handle 3. **Host loader + RAII handle** — host-side code that dlopen's the `.so`, resolves the vtable symbol, validates version/size, and wraps instances - in move-only RAII handles. + in move-only RAII handles. Handles retain shared ownership of the loaded + DSO, so plugin code remains mapped until every instance created from that + DSO has been destroyed. ## 2. Module Structure @@ -281,8 +284,8 @@ at load time. Mismatches produce a clear error. All SDK base classes: - Generate the C vtable via `vtableWithCreate()` at static init. - Validate compile-time manifest JSON string literals (required keys) via `PJ_ASSERT`; dialog manifests are runtime strings and are validated by the host catalog when inspected. -- Catch all C++ exceptions in trampolines, store via `setLastError()`, and - return `false`/`null` across the ABI boundary. +- Catch all C++ exceptions in trampolines, populate `PJ_error_t` out-params + when available, and return `false`/`null` across the ABI boundary. **Trampoline pattern:** Each base class has a private set of `static` trampoline functions (e.g. `trampoline_start`) that cast the `void* ctx` to @@ -333,6 +336,9 @@ Each family has a move-only RAII handle: - Constructor calls `vt->create()` to allocate the plugin instance. - Destructor calls `vt->destroy(ctx)`. +- Handles created by a loader retain a shared DSO owner; destroying or + hot-reloading the loader/catalog entry cannot `dlclose` the plugin while + live handles still call its vtable. - No copy, move-only semantics. - Methods delegate to vtable functions with the stored context pointer. @@ -341,6 +347,8 @@ dialogs that are members of another plugin (e.g. a DataSource's dialog). A borrowed handle does NOT call `create()` or `destroy()` — it wraps a pre-existing context pointer obtained via `getDialog()` (which plugin authors implement with the SDK helper `PJ::borrowDialog(dialog_member_)`). +The owning plugin handle must outlive the borrowed dialog because it owns both +the dialog object storage and the shared DSO lifetime token. ## 7. Dialog Host Runtime @@ -430,10 +438,10 @@ timestamp_column, err)`: `.release()` on the holder after a successful append so its destructor becomes a no-op. - On **failure** (returns `false`): ownership is NOT transferred. The - host guarantees it has already called `stream->release` on any - partially-consumed stream before surfacing the error via - `PJ_error_t` — but the stream struct itself stays on the plugin - side. `ArrowStreamHolder`'s destructor handles this automatically. + plugin remains responsible for releasing the stream. This includes cases + where the host inspected or partially consumed the stream before returning + an error. `ArrowStreamHolder`'s destructor handles this automatically when + the plugin uses the recommended rvalue-ref SDK overload. - `timestamp_column` names the int64 column whose values are nanoseconds since Unix epoch. Passing an empty view means "synthesise a monotonic timestamp per row"; useful for streams with no natural diff --git a/pj_plugins/docs/REQUIREMENTS.md b/pj_plugins/docs/REQUIREMENTS.md index 23ff170..64c5bec 100644 --- a/pj_plugins/docs/REQUIREMENTS.md +++ b/pj_plugins/docs/REQUIREMENTS.md @@ -85,9 +85,11 @@ Stateful interactive tools with full data access. ## 4. Shared Host Services -### 4.1 Write Surface +### 4.1 Write Services -Shared by DataSource, MessageParser, and Toolbox. Supports: +DataSource, MessageParser, and Toolbox share the same datastore write +contract and backend, but bind family-specific ABI services so each plugin +only sees operations it is allowed to call. These services support: - **Incremental writes** — `appendRecord()` / `appendBoundRecord()` with named or pre-resolved field values. Used by parsers and streaming @@ -98,11 +100,14 @@ Shared by DataSource, MessageParser, and Toolbox. Supports: path for file-based sources and toolbox bulk imports. The parser write surface is per-record only — the host coalesces parser output into Arrow batches internally before committing to storage. -- **Topic and field management** — `ensureTopic()`, `ensureField()`. - -Family-specific permissions differ (Toolbox can create data sources; DataSource -and MessageParser are bound to one), but the underlying write contract is -the same. +- **Topic and field management** — `ensureTopic()`, `ensureField()`, where + available for the family. Parsers are bound to one topic and only create + fields within that topic. + +The service names are `"pj.source_write.v1"`, `"pj.parser_write.v1"`, and +`"pj.toolbox_write.v1"`. Family-specific permissions differ: Toolbox can +create data sources, DataSource writes under its own source, and MessageParser +is bound to one topic. ### 4.2 Read Surface diff --git a/pj_plugins/docs/data-source-guide.md b/pj_plugins/docs/data-source-guide.md index 791ba94..6e13c60 100644 --- a/pj_plugins/docs/data-source-guide.md +++ b/pj_plugins/docs/data-source-guide.md @@ -398,7 +398,7 @@ engine. | `ensureField(topic, name, type)` | Optional: pre-register a field. Enables `appendBoundRecord`. | | `appendRecord(topic, timestamp, fields)` | Write a row of named field values. Auto-creates new fields. | | `appendBoundRecord(topic, timestamp, fields)` | Write using pre-resolved field handles (faster). | -| `appendArrowStream(topic, stream, ts_col)` | Hand an `ArrowArrayStream*` (Arrow C Data Interface) to the host for bulk ingest. Host drains and releases on success. | +| `appendArrowStream(topic, stream, ts_col)` | Hand an `ArrowArrayStream*` (Arrow C Data Interface) to the host for bulk ingest. Success transfers ownership to the host; failure leaves ownership with the plugin. | ### Runtime host — control plane @@ -651,7 +651,8 @@ from the callback thread. The host guarantees the following call ordering: 1. `create()` — always first. -2. `bind_write_host()` and `bind_runtime_host()` — before `start()`. +2. `bind(registry)` — before `start()`. The SDK default bind resolves + `"pj.source_write.v1"` and `"pj.runtime.v1"` from the service registry. 3. `load_config()` — before `start()`, may be called multiple times. 4. `start()` — transitions from idle/configuring to starting. 5. `poll()` — only while running (after `start()` returns success). @@ -700,12 +701,11 @@ for (const auto& row : rows) { } ``` -**setLastError()** — available for `void` methods (e.g. `stop()`) that cannot -return a status. For all other methods, prefer returning `unexpected()`. - **Exception safety** — the SDK base class catches all C++ exceptions in virtual -method trampolines and converts them to `setLastError()` + false return. You -never need to worry about exceptions crossing the C ABI boundary. +method trampolines and converts them to `PJ_error_t` out-params plus a +`false`/safe return value. In plugin code, prefer returning `unexpected()` +from fallible virtuals; `stop()` should be idempotent and swallow cleanup +failures internally. ## Dialog Integration @@ -808,6 +808,11 @@ PJ_DIALOG_PLUGIN(MyDialog) 8. source.start() ← already has the config ``` +The `DataSourceHandle` retains the plugin DSO while the source instance is +alive, so the dialog vtable remains callable even if the catalog reloads the +plugin entry. The borrowed dialog still must not outlive the source handle, +because the source owns the dialog object itself. + ### Config persistence `source.saveConfig()` serializes everything (dialog + source state). diff --git a/pj_plugins/docs/dialog-plugin-guide.md b/pj_plugins/docs/dialog-plugin-guide.md index 31629fc..6849286 100644 --- a/pj_plugins/docs/dialog-plugin-guide.md +++ b/pj_plugins/docs/dialog-plugin-guide.md @@ -290,18 +290,22 @@ work like polling a server for available topics. | QDoubleSpinBox | `setValue(double)` | `onValueChanged(name, double)` | | QPushButton | `setButtonText` | `onClicked(name)` | | QPushButton (file picker) | `setFilePicker` | `onFileSelected(name, path)` | +| QPushButton (folder picker) | `setFolderPicker` | `onFolderSelected(name, path)` | | QLabel | `setLabel` | (none — display only) | | QListWidget | `setListItems`, `setSelectedItems` | `onSelectionChanged(name, items)`, `onItemDoubleClicked(name, index)` | | QTableWidget | `setTableHeaders`, `setTableRows`, `setSelectedRows` | `onSelectionChanged(name, items)` | +| QPlainTextEdit | `setPlainText`, `setCodeContent`, `setCodeLanguage` | `onCodeChanged(name, code)` for code editors | +| QFrame (chart container) | `setChartSeries`, `clearChart`, `setChartZoomEnabled` | `onChartViewChanged(name, x_min, x_max, y_min, y_max)` | | QTabWidget | `setTabIndex` | `onTabChanged(name, index)` | | QDialogButtonBox | `setOkEnabled` | (none — host handles OK/Cancel) | -All widgets also support `setEnabled(name, bool)` and `setVisible(name, bool)`. +All widgets also support `setEnabled(name, bool)`, `setVisible(name, bool)`, +and `setDropTarget(name, bool)`. Drop targets receive +`onItemsDropped(name, items)`. -> **Note:** `QTextEdit`, `QPlainTextEdit`, and `QTableView` (model-based) are -> **not supported** by the widget binding system. Use `QTableWidget` for tabular -> data (e.g. topic lists, preview tables) and `QLabel` or `QListWidget` for text -> display. +> **Note:** `QTextEdit` and `QTableView` (model-based) are not supported by the +> widget binding system. Use `QPlainTextEdit` for plain text or code editing, +> and `QTableWidget` for tabular data such as topic lists and preview tables. ## Optional Features @@ -442,16 +446,10 @@ void onRejected() override { ### Error reporting -Override `lastError()` to surface error messages to the host. Return an empty -string when there is no error: - -```cpp -std::string lastError() const override { - std::string err = std::move(error_); - error_.clear(); - return err; -} -``` +Fallible dialog callbacks return `bool` through the C ABI and receive a +`PJ_error_t*` out-param. In the C++ SDK, throw only for exceptional failures or +return `false` from event handlers that do not handle an event. The SDK +trampolines catch exceptions and populate `PJ_error_t` for the host. ### Dialog geometry persistence @@ -576,6 +574,9 @@ PJ_DIALOG_PLUGIN(MyDialog) // also specialises PJ::dialogVtableFor() The host resolves both vtables, creates a borrowed `DialogHandle` from the source's dialog context, and drives the dialog through its dialog runtime. After the dialog completes, the source reads its dialog member's state directly. +The source handle owns the dialog object storage and keeps the plugin DSO +loaded, so any borrowed dialog handle must be scoped inside the source handle's +lifetime. See `pj_plugins/docs/data-source-guide.md` for the full DataSource-side documentation of this pattern. diff --git a/pj_plugins/docs/message-parser-guide.md b/pj_plugins/docs/message-parser-guide.md index 1ede495..34ed10e 100644 --- a/pj_plugins/docs/message-parser-guide.md +++ b/pj_plugins/docs/message-parser-guide.md @@ -44,8 +44,9 @@ class MyJsonParser : public PJ::MessageParserPluginBase { ### 2. Implement parse() -When `parse()` is called, the write host is already bound via -`bindWriteHost()`. Use `writeHost()` (protected) to write decoded fields. +When `parse()` is called, the SDK default `bind()` has already resolved the +parser write service from the host's service registry. Use `writeHost()` +(protected) to write decoded fields. Return `okStatus()` on success, or `unexpected("reason")` on failure. ```cpp @@ -105,12 +106,12 @@ target_link_libraries(my_parser_plugin PRIVATE pj_base pj_dialog_sdk) The host drives the parser through these phases: ``` -create() → bind_write_host() → [bind_schema()] → parse()* → destroy() +create() -> bind(registry) -> [bind_schema()] -> parse()* -> destroy() ``` 1. **create** — the host calls `create()` to allocate a new parser instance. -2. **bind_write_host** — the host provides the data-plane write host. Must be - called before `parse()`. +2. **bind** — the host provides a service registry. The SDK default bind + resolves `"pj.parser_write.v1"`. Must complete before `parse()`. 3. **bind_schema** (optional) — for parsers that need schema (Protobuf, ROS, IDL), the host provides the schema bytes and type name. Parsers that don't need schema (JSON, Influx) can ignore this call. @@ -340,7 +341,7 @@ Example: ## Threading Model -All parser callbacks — `parse()`, `bindSchema()`, `bindWriteHost()`, +All parser callbacks — `bind()`, `bindSchema()`, `parse()`, `loadConfig()`, `saveConfig()` — are called **on the host's thread**. The host guarantees single-threaded access per parser instance: no two callbacks will overlap for the same instance. @@ -354,13 +355,13 @@ and call it from a background thread. The host guarantees the following call ordering: 1. `create()` — always first. -2. `bind_write_host()` — before `parse()`. +2. `bind(registry)` — before `parse()`. 3. `bind_schema()` (optional) — before `parse()`, called at most once. 4. `load_config()` — before `parse()`, may be called multiple times. 5. `parse()` — called once per message, may be called many times. 6. `destroy()` — always last. -The host will never call `parse()` before `bind_write_host()`, and `destroy()` +The host will never call `parse()` before `bind(registry)`, and `destroy()` is always the last call. ## Error Handling @@ -386,12 +387,9 @@ if (!field) { - Do **not** leave the parser in an inconsistent state — ensure field handles and internal buffers remain valid for the next `parse()` call. -**setLastError()** — available for fine-grained error reporting. For most -cases, returning `unexpected()` from `parse()` is sufficient. - **Exception safety** — the SDK base class catches all C++ exceptions in -virtual method trampolines and converts them to `setLastError()` + false -return. You never need to worry about exceptions crossing the C ABI boundary. +virtual method trampolines and converts them to `PJ_error_t` out-params plus +`false`. You never need to worry about exceptions crossing the C ABI boundary. ## How Parsers Are Used (Host Perspective) @@ -406,7 +404,7 @@ DataSource Host MessageParser │ topic="sensor/imu", │ │ │ encoding="protobuf", │──→ load parser .so │ │ type_name="ImuSample", │──→ create() │ - │ schema=descriptor_bytes) │──→ bind_write_host() │ + │ schema=descriptor_bytes) │──→ bind(registry) │ │ │──→ bind_schema("ImuSample", │ │ │ descriptor_bytes) │ │ ←── binding handle │ │ diff --git a/pj_plugins/docs/toolbox-guide.md b/pj_plugins/docs/toolbox-guide.md index 6825582..7560cfd 100644 --- a/pj_plugins/docs/toolbox-guide.md +++ b/pj_plugins/docs/toolbox-guide.md @@ -117,16 +117,17 @@ Toolbox plugins have no state machine — they are either alive or destroyed. Activation and deactivation are dialog visibility concerns handled by the host. ``` -create → bind_toolbox_host → bind_runtime_host → load_config - → [show dialog] → user interacts → plugin reads/writes via toolbox host - → plugin calls notifyDataChanged() - → save_config → destroy +create -> bind(registry) -> load_config + -> [show dialog] -> user interacts -> plugin reads/writes via toolbox host + -> plugin calls notifyDataChanged() + -> save_config -> destroy ``` The host guarantees the following call ordering: 1. `create()` — always first. -2. `bind_toolbox_host()` and `bind_runtime_host()` — before any interaction. +2. `bind(registry)` — before any interaction. The SDK default bind resolves + `"pj.toolbox_write.v1"` and `"pj.toolbox_runtime.v1"`. 3. `load_config()` — before showing the dialog, may be called multiple times. 4. User interaction phase — plugin reads/writes data on demand. 5. `save_config()` — before destroy, when the host persists layout. @@ -134,7 +135,7 @@ The host guarantees the following call ordering: ## Host Services Available to Plugins -Two host bindings are provided before the plugin becomes interactive: +Two host services are provided before the plugin becomes interactive: ### Toolbox host — data plane @@ -160,7 +161,6 @@ Access via `runtimeHost()`. Use this for diagnostics and UI refresh. |---|---| | `reportMessage(level, text)` | Send info/warning/error to the host UI log. | | `notifyDataChanged()` | Tell the host that data was modified; refresh UI. | -| `lastError()` | Read the last host-side error message. | ### Reading a series via Arrow @@ -262,12 +262,12 @@ if (!source) { ``` **Exception safety** — the SDK base class catches all C++ exceptions in virtual -method trampolines and converts them to `setLastError()` + false return. No -exceptions cross the C ABI boundary. +method trampolines and converts them to `PJ_error_t` out-params plus `false`. +No exceptions cross the C ABI boundary. ## Threading Model -All plugin callbacks — `bindToolboxHost()`, `bindRuntimeHost()`, +All plugin callbacks — `bind()`, `loadConfig()`, `saveConfig()`, `getDialog()` — are called **on the host's thread**. The host guarantees single-threaded access per plugin instance. diff --git a/pj_plugins/include/pj_plugins/host/data_source_handle.hpp b/pj_plugins/include/pj_plugins/host/data_source_handle.hpp index 1afd00f..aee47cf 100644 --- a/pj_plugins/include/pj_plugins/host/data_source_handle.hpp +++ b/pj_plugins/include/pj_plugins/host/data_source_handle.hpp @@ -20,6 +20,7 @@ #pragma once #include +#include #include #include #include @@ -33,7 +34,8 @@ namespace PJ { /// RAII handle owning a DataSource plugin instance. class DataSourceHandle { public: - explicit DataSourceHandle(const PJ_data_source_vtable_t* vt) : vt_(vt) { + explicit DataSourceHandle(const PJ_data_source_vtable_t* vt, std::shared_ptr library_owner = {}) + : vt_(vt), library_owner_(std::move(library_owner)) { if (vt_ != nullptr) { assert(vt_->protocol_version == PJ_DATA_SOURCE_PROTOCOL_VERSION); ctx_ = vt_->create(); @@ -46,7 +48,8 @@ class DataSourceHandle { } } - DataSourceHandle(DataSourceHandle&& other) noexcept : vt_(other.vt_), ctx_(other.ctx_) { + DataSourceHandle(DataSourceHandle&& other) noexcept + : vt_(other.vt_), ctx_(other.ctx_), library_owner_(std::move(other.library_owner_)) { other.vt_ = nullptr; other.ctx_ = nullptr; } @@ -55,6 +58,7 @@ class DataSourceHandle { if (this != &other) { std::swap(vt_, other.vt_); std::swap(ctx_, other.ctx_); + std::swap(library_owner_, other.library_owner_); } return *this; } @@ -153,7 +157,7 @@ class DataSourceHandle { } /// Query a plugin-exposed extension by reverse-DNS id. Tail-slot gated — - /// returns nullptr if the plugin was compiled against a v3.0 header that + /// returns nullptr if the plugin was compiled against an older v4 header that /// didn't have this slot, or if the plugin doesn't know the id. [[nodiscard]] const void* getPluginExtension(std::string_view id) const { if (!PJ_HAS_TAIL_SLOT(PJ_data_source_vtable_t, vt_, get_plugin_extension)) { @@ -174,6 +178,7 @@ class DataSourceHandle { private: const PJ_data_source_vtable_t* vt_ = nullptr; void* ctx_ = nullptr; + std::shared_ptr library_owner_; }; } // namespace PJ diff --git a/pj_plugins/include/pj_plugins/host/data_source_library.hpp b/pj_plugins/include/pj_plugins/host/data_source_library.hpp index d07d808..255f1b9 100644 --- a/pj_plugins/include/pj_plugins/host/data_source_library.hpp +++ b/pj_plugins/include/pj_plugins/host/data_source_library.hpp @@ -2,8 +2,9 @@ * @file data_source_library.hpp * @brief Host-side loader for DataSource plugin shared libraries. * - * DataSourceLibrary wraps dlopen/dlclose and resolves the plugin's vtable - * entry point. Create instances of the plugin via createHandle(). + * DataSourceLibrary wraps dlopen and resolves the plugin's vtable entry point. + * Create instances of the plugin via createHandle(); those handles keep the + * shared library loaded until their plugin instances are destroyed. * * Typical usage: * @code @@ -18,6 +19,7 @@ #include #include +#include #include #include @@ -28,9 +30,9 @@ namespace PJ { /** * Loads a DataSource plugin shared library and provides factory access. * - * The library is dlopen'd with RTLD_LOCAL on load() and dlclose'd on - * destruction. The vtable pointer remains valid for the library's lifetime. - * Move-only; not copyable. + * The library is dlopen'd with RTLD_LOCAL on load(). Plugin handles created + * from this loader keep the DSO loaded even if the DataSourceLibrary object is + * destroyed or moved out of a runtime catalog. Move-only; not copyable. */ class DataSourceLibrary { public: @@ -51,14 +53,14 @@ class DataSourceLibrary { return handle_ != nullptr && vtable_ != nullptr; } - /// Raw vtable pointer. Valid for the lifetime of this DataSourceLibrary. + /// Raw vtable pointer. Valid while this library or any handle created from it is alive. [[nodiscard]] const PJ_data_source_vtable_t* vtable() const { return vtable_; } /// Create a new plugin instance. Each handle is independent. [[nodiscard]] DataSourceHandle createHandle() const { - return DataSourceHandle(vtable_); + return DataSourceHandle(vtable_, handle_); } /// Resolve the dialog vtable from this .so. Returns error if not exported. @@ -70,11 +72,11 @@ class DataSourceLibrary { } private: - DataSourceLibrary(void* handle, const PJ_data_source_vtable_t* vtable, std::string path); + DataSourceLibrary(std::shared_ptr handle, const PJ_data_source_vtable_t* vtable, std::string path); void reset(); - void* handle_ = nullptr; + std::shared_ptr handle_; const PJ_data_source_vtable_t* vtable_ = nullptr; std::string path_; }; diff --git a/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp b/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp index 4d7e50d..bd5ee9b 100644 --- a/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp +++ b/pj_plugins/include/pj_plugins/host/message_parser_handle.hpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -20,7 +21,8 @@ namespace PJ { /// RAII handle owning a MessageParser plugin instance. class MessageParserHandle { public: - explicit MessageParserHandle(const PJ_message_parser_vtable_t* vt) : vt_(vt) { + explicit MessageParserHandle(const PJ_message_parser_vtable_t* vt, std::shared_ptr library_owner = {}) + : vt_(vt), library_owner_(std::move(library_owner)) { if (vt_ != nullptr) { assert(vt_->protocol_version == PJ_MESSAGE_PARSER_PROTOCOL_VERSION); ctx_ = vt_->create(); @@ -33,7 +35,8 @@ class MessageParserHandle { } } - MessageParserHandle(MessageParserHandle&& other) noexcept : vt_(other.vt_), ctx_(other.ctx_) { + MessageParserHandle(MessageParserHandle&& other) noexcept + : vt_(other.vt_), ctx_(other.ctx_), library_owner_(std::move(other.library_owner_)) { other.vt_ = nullptr; other.ctx_ = nullptr; } @@ -42,6 +45,7 @@ class MessageParserHandle { if (this != &other) { std::swap(vt_, other.vt_); std::swap(ctx_, other.ctx_); + std::swap(library_owner_, other.library_owner_); } return *this; } @@ -123,6 +127,7 @@ class MessageParserHandle { private: const PJ_message_parser_vtable_t* vt_ = nullptr; void* ctx_ = nullptr; + std::shared_ptr library_owner_; }; } // namespace PJ diff --git a/pj_plugins/include/pj_plugins/host/message_parser_library.hpp b/pj_plugins/include/pj_plugins/host/message_parser_library.hpp index 33578bf..c53faf4 100644 --- a/pj_plugins/include/pj_plugins/host/message_parser_library.hpp +++ b/pj_plugins/include/pj_plugins/host/message_parser_library.hpp @@ -2,8 +2,9 @@ * @file message_parser_library.hpp * @brief Host-side loader for MessageParser plugin shared libraries. * - * MessageParserLibrary wraps dlopen/dlclose and resolves the plugin's vtable - * entry point. Create instances of the plugin via createHandle(). + * MessageParserLibrary wraps dlopen and resolves the plugin's vtable entry + * point. Create instances of the plugin via createHandle(); those handles keep + * the shared library loaded until their plugin instances are destroyed. * * Typical usage: * @code @@ -18,6 +19,7 @@ #include #include +#include #include #include @@ -28,9 +30,9 @@ namespace PJ { /** * Loads a MessageParser plugin shared library and provides factory access. * - * The library is dlopen'd with RTLD_LOCAL on load() and dlclose'd on - * destruction. The vtable pointer remains valid for the library's lifetime. - * Move-only; not copyable. + * The library is dlopen'd with RTLD_LOCAL on load(). Plugin handles created + * from this loader keep the DSO loaded even if the MessageParserLibrary object + * is destroyed or moved out of a runtime catalog. Move-only; not copyable. */ class MessageParserLibrary { public: @@ -51,14 +53,14 @@ class MessageParserLibrary { return handle_ != nullptr && vtable_ != nullptr; } - /// Raw vtable pointer. Valid for the lifetime of this MessageParserLibrary. + /// Raw vtable pointer. Valid while this library or any handle created from it is alive. [[nodiscard]] const PJ_message_parser_vtable_t* vtable() const { return vtable_; } /// Create a new plugin instance. Each handle is independent. [[nodiscard]] MessageParserHandle createHandle() const { - return MessageParserHandle(vtable_); + return MessageParserHandle(vtable_, handle_); } /// Resolve the dialog vtable from this .so. Returns error if not exported. @@ -70,11 +72,11 @@ class MessageParserLibrary { } private: - MessageParserLibrary(void* handle, const PJ_message_parser_vtable_t* vtable, std::string path); + MessageParserLibrary(std::shared_ptr handle, const PJ_message_parser_vtable_t* vtable, std::string path); void reset(); - void* handle_ = nullptr; + std::shared_ptr handle_; const PJ_message_parser_vtable_t* vtable_ = nullptr; std::string path_; }; diff --git a/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp b/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp index 1f0adcb..595dda0 100644 --- a/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp +++ b/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp @@ -15,7 +15,7 @@ namespace PJ { /// Host-side assembler for `PJ_service_registry_t`. /// /// The host creates a builder, registers named services, and hands the -/// resulting `PJ_service_registry_t` view to each plugin via the v3 +/// resulting `PJ_service_registry_t` view to each plugin via the v4 /// `bind()` call. The builder owns an internal lookup table; the emitted /// registry is a thin fat pointer whose lifetime is tied to the builder. /// @@ -76,7 +76,7 @@ class ServiceRegistryBuilder { entries_.erase(std::string(name)); } - /// Return a fat pointer that plugins can pass through the v3 `bind()`. + /// Return a fat pointer that plugins can pass through the v4 `bind()`. /// The returned pointer is valid as long as the builder instance lives. [[nodiscard]] PJ_service_registry_t view() noexcept { return PJ_service_registry_t{this, &kVtable}; diff --git a/pj_plugins/include/pj_plugins/host/toolbox_handle.hpp b/pj_plugins/include/pj_plugins/host/toolbox_handle.hpp index 018b934..c538930 100644 --- a/pj_plugins/include/pj_plugins/host/toolbox_handle.hpp +++ b/pj_plugins/include/pj_plugins/host/toolbox_handle.hpp @@ -8,6 +8,7 @@ #include #include +#include #include #include #include @@ -19,7 +20,8 @@ namespace PJ { /// RAII handle owning a Toolbox plugin instance. class ToolboxHandle { public: - explicit ToolboxHandle(const PJ_toolbox_vtable_t* vt) : vt_(vt) { + explicit ToolboxHandle(const PJ_toolbox_vtable_t* vt, std::shared_ptr library_owner = {}) + : vt_(vt), library_owner_(std::move(library_owner)) { if (vt_ != nullptr) { assert(vt_->protocol_version == PJ_TOOLBOX_PLUGIN_PROTOCOL_VERSION); ctx_ = vt_->create(); @@ -32,7 +34,8 @@ class ToolboxHandle { } } - ToolboxHandle(ToolboxHandle&& other) noexcept : vt_(other.vt_), ctx_(other.ctx_) { + ToolboxHandle(ToolboxHandle&& other) noexcept + : vt_(other.vt_), ctx_(other.ctx_), library_owner_(std::move(other.library_owner_)) { other.vt_ = nullptr; other.ctx_ = nullptr; } @@ -41,6 +44,7 @@ class ToolboxHandle { if (this != &other) { std::swap(vt_, other.vt_); std::swap(ctx_, other.ctx_); + std::swap(library_owner_, other.library_owner_); } return *this; } @@ -118,6 +122,7 @@ class ToolboxHandle { private: const PJ_toolbox_vtable_t* vt_ = nullptr; void* ctx_ = nullptr; + std::shared_ptr library_owner_; }; } // namespace PJ diff --git a/pj_plugins/include/pj_plugins/host/toolbox_library.hpp b/pj_plugins/include/pj_plugins/host/toolbox_library.hpp index 8cecb7f..d47984d 100644 --- a/pj_plugins/include/pj_plugins/host/toolbox_library.hpp +++ b/pj_plugins/include/pj_plugins/host/toolbox_library.hpp @@ -2,8 +2,9 @@ * @file toolbox_library.hpp * @brief Host-side loader for Toolbox plugin shared libraries. * - * ToolboxLibrary wraps dlopen/dlclose and resolves the plugin's vtable - * entry point. Create instances of the plugin via createHandle(). + * ToolboxLibrary wraps dlopen and resolves the plugin's vtable entry point. + * Create instances of the plugin via createHandle(); those handles keep the + * shared library loaded until their plugin instances are destroyed. * * Typical usage: * @code @@ -18,6 +19,7 @@ #include #include +#include #include #include @@ -28,9 +30,9 @@ namespace PJ { /** * Loads a Toolbox plugin shared library and provides factory access. * - * The library is dlopen'd with RTLD_LOCAL on load() and dlclose'd on - * destruction. The vtable pointer remains valid for the library's lifetime. - * Move-only; not copyable. + * The library is dlopen'd with RTLD_LOCAL on load(). Plugin handles created + * from this loader keep the DSO loaded even if the ToolboxLibrary object is + * destroyed or moved out of a runtime catalog. Move-only; not copyable. */ class ToolboxLibrary { public: @@ -51,14 +53,14 @@ class ToolboxLibrary { return handle_ != nullptr && vtable_ != nullptr; } - /// Raw vtable pointer. Valid for the lifetime of this ToolboxLibrary. + /// Raw vtable pointer. Valid while this library or any handle created from it is alive. [[nodiscard]] const PJ_toolbox_vtable_t* vtable() const { return vtable_; } /// Create a new plugin instance. Each handle is independent. [[nodiscard]] ToolboxHandle createHandle() const { - return ToolboxHandle(vtable_); + return ToolboxHandle(vtable_, handle_); } /// Resolve the dialog vtable from this .so. Returns error if not exported. @@ -70,11 +72,11 @@ class ToolboxLibrary { } private: - ToolboxLibrary(void* handle, const PJ_toolbox_vtable_t* vtable, std::string path); + ToolboxLibrary(std::shared_ptr handle, const PJ_toolbox_vtable_t* vtable, std::string path); void reset(); - void* handle_ = nullptr; + std::shared_ptr handle_; const PJ_toolbox_vtable_t* vtable_ = nullptr; std::string path_; }; diff --git a/pj_plugins/src/data_source_library.cpp b/pj_plugins/src/data_source_library.cpp index 2bc1e6d..efa7b0d 100644 --- a/pj_plugins/src/data_source_library.cpp +++ b/pj_plugins/src/data_source_library.cpp @@ -6,70 +6,65 @@ namespace PJ { -DataSourceLibrary::DataSourceLibrary(void* handle, const PJ_data_source_vtable_t* vtable, std::string path) - : handle_(handle), vtable_(vtable), path_(std::move(path)) {} +DataSourceLibrary::DataSourceLibrary( + std::shared_ptr handle, const PJ_data_source_vtable_t* vtable, std::string path) + : handle_(std::move(handle)), vtable_(vtable), path_(std::move(path)) {} DataSourceLibrary::~DataSourceLibrary() { reset(); } DataSourceLibrary::DataSourceLibrary(DataSourceLibrary&& other) noexcept - : handle_(other.handle_), vtable_(other.vtable_), path_(std::move(other.path_)) { - other.handle_ = nullptr; + : handle_(std::move(other.handle_)), vtable_(other.vtable_), path_(std::move(other.path_)) { other.vtable_ = nullptr; } DataSourceLibrary& DataSourceLibrary::operator=(DataSourceLibrary&& other) noexcept { if (this != &other) { reset(); - handle_ = other.handle_; + handle_ = std::move(other.handle_); vtable_ = other.vtable_; path_ = std::move(other.path_); - other.handle_ = nullptr; other.vtable_ = nullptr; } return *this; } Expected DataSourceLibrary::load(std::string_view path) { - auto handle = detail::loadLibraryHandle(path); - if (!handle) { - return unexpected(handle.error()); + auto raw_handle = detail::loadLibraryHandle(path); + if (!raw_handle) { + return unexpected(raw_handle.error()); } + auto handle = detail::adoptLibraryHandle(*raw_handle); - if (auto abi = detail::checkPluginAbiVersion(*handle); !abi) { - detail::closeLibraryHandle(*handle); + if (auto abi = detail::checkPluginAbiVersion(handle.get()); !abi) { return unexpected(abi.error()); } - auto sym = detail::resolveSymbol(*handle, "PJ_get_data_source_vtable"); + auto sym = detail::resolveSymbol(handle.get(), "PJ_get_data_source_vtable"); if (!sym) { - detail::closeLibraryHandle(*handle); return unexpected(sym.error()); } auto entry = reinterpret_cast(*sym); const PJ_data_source_vtable_t* vtable = entry(); if (vtable == nullptr) { - detail::closeLibraryHandle(*handle); return unexpected(std::string("PJ_get_data_source_vtable returned null")); } if (vtable->protocol_version != PJ_DATA_SOURCE_PROTOCOL_VERSION) { - detail::closeLibraryHandle(*handle); return unexpected(std::string("DataSource protocol version mismatch")); } - // Use MIN_VTABLE_SIZE (pinned at v3.0), NOT sizeof() which grows per host + // Use MIN_VTABLE_SIZE (pinned at v4.0), NOT sizeof() which grows per host // release and would falsely reject plugins compiled against older headers. if (vtable->struct_size < PJ_DATA_SOURCE_MIN_VTABLE_SIZE) { - detail::closeLibraryHandle(*handle); - return unexpected(std::string("DataSource vtable smaller than v3.0 baseline")); + return unexpected(std::string("DataSource vtable smaller than v4.0 baseline")); } - return DataSourceLibrary(*handle, vtable, std::string(path)); + return DataSourceLibrary(std::move(handle), vtable, std::string(path)); } Expected DataSourceLibrary::resolveDialogVtable() const { - auto sym = detail::resolveSymbol(handle_, "PJ_get_dialog_vtable"); + auto sym = detail::resolveSymbol(handle_.get(), "PJ_get_dialog_vtable"); if (!sym) { return unexpected(sym.error()); } @@ -89,8 +84,7 @@ Expected DataSourceLibrary::resolveDialogVtable() con void DataSourceLibrary::reset() { if (handle_ != nullptr) { - detail::closeLibraryHandle(handle_); - handle_ = nullptr; + handle_.reset(); vtable_ = nullptr; path_.clear(); } diff --git a/pj_plugins/src/detail/library_loader.hpp b/pj_plugins/src/detail/library_loader.hpp index 53bc5a8..0ac38af 100644 --- a/pj_plugins/src/detail/library_loader.hpp +++ b/pj_plugins/src/detail/library_loader.hpp @@ -1,6 +1,7 @@ #pragma once #include +#include #include #include @@ -107,4 +108,8 @@ inline void closeLibraryHandle(void* handle) { #endif } +inline std::shared_ptr adoptLibraryHandle(void* handle) { + return std::shared_ptr(handle, [](void* loaded_handle) { closeLibraryHandle(loaded_handle); }); +} + } // namespace PJ::detail diff --git a/pj_plugins/src/message_parser_library.cpp b/pj_plugins/src/message_parser_library.cpp index 2c5e091..5b33c5f 100644 --- a/pj_plugins/src/message_parser_library.cpp +++ b/pj_plugins/src/message_parser_library.cpp @@ -6,68 +6,63 @@ namespace PJ { -MessageParserLibrary::MessageParserLibrary(void* handle, const PJ_message_parser_vtable_t* vtable, std::string path) - : handle_(handle), vtable_(vtable), path_(std::move(path)) {} +MessageParserLibrary::MessageParserLibrary( + std::shared_ptr handle, const PJ_message_parser_vtable_t* vtable, std::string path) + : handle_(std::move(handle)), vtable_(vtable), path_(std::move(path)) {} MessageParserLibrary::~MessageParserLibrary() { reset(); } MessageParserLibrary::MessageParserLibrary(MessageParserLibrary&& other) noexcept - : handle_(other.handle_), vtable_(other.vtable_), path_(std::move(other.path_)) { - other.handle_ = nullptr; + : handle_(std::move(other.handle_)), vtable_(other.vtable_), path_(std::move(other.path_)) { other.vtable_ = nullptr; } MessageParserLibrary& MessageParserLibrary::operator=(MessageParserLibrary&& other) noexcept { if (this != &other) { reset(); - handle_ = other.handle_; + handle_ = std::move(other.handle_); vtable_ = other.vtable_; path_ = std::move(other.path_); - other.handle_ = nullptr; other.vtable_ = nullptr; } return *this; } Expected MessageParserLibrary::load(std::string_view path) { - auto handle = detail::loadLibraryHandle(path); - if (!handle) { - return unexpected(handle.error()); + auto raw_handle = detail::loadLibraryHandle(path); + if (!raw_handle) { + return unexpected(raw_handle.error()); } + auto handle = detail::adoptLibraryHandle(*raw_handle); - if (auto abi = detail::checkPluginAbiVersion(*handle); !abi) { - detail::closeLibraryHandle(*handle); + if (auto abi = detail::checkPluginAbiVersion(handle.get()); !abi) { return unexpected(abi.error()); } - auto sym = detail::resolveSymbol(*handle, "PJ_get_message_parser_vtable"); + auto sym = detail::resolveSymbol(handle.get(), "PJ_get_message_parser_vtable"); if (!sym) { - detail::closeLibraryHandle(*handle); return unexpected(sym.error()); } auto entry = reinterpret_cast(*sym); const PJ_message_parser_vtable_t* vtable = entry(); if (vtable == nullptr) { - detail::closeLibraryHandle(*handle); return unexpected(std::string("PJ_get_message_parser_vtable returned null")); } if (vtable->protocol_version != PJ_MESSAGE_PARSER_PROTOCOL_VERSION) { - detail::closeLibraryHandle(*handle); return unexpected(std::string("MessageParser protocol version mismatch")); } if (vtable->struct_size < PJ_MESSAGE_PARSER_MIN_VTABLE_SIZE) { - detail::closeLibraryHandle(*handle); - return unexpected(std::string("MessageParser vtable smaller than v3.0 baseline")); + return unexpected(std::string("MessageParser vtable smaller than v4.0 baseline")); } - return MessageParserLibrary(*handle, vtable, std::string(path)); + return MessageParserLibrary(std::move(handle), vtable, std::string(path)); } Expected MessageParserLibrary::resolveDialogVtable() const { - auto sym = detail::resolveSymbol(handle_, "PJ_get_dialog_vtable"); + auto sym = detail::resolveSymbol(handle_.get(), "PJ_get_dialog_vtable"); if (!sym) { return unexpected(sym.error()); } @@ -87,8 +82,7 @@ Expected MessageParserLibrary::resolveDialogVtable() void MessageParserLibrary::reset() { if (handle_ != nullptr) { - detail::closeLibraryHandle(handle_); - handle_ = nullptr; + handle_.reset(); vtable_ = nullptr; path_.clear(); } diff --git a/pj_plugins/src/toolbox_library.cpp b/pj_plugins/src/toolbox_library.cpp index 2519c65..a796178 100644 --- a/pj_plugins/src/toolbox_library.cpp +++ b/pj_plugins/src/toolbox_library.cpp @@ -6,68 +6,62 @@ namespace PJ { -ToolboxLibrary::ToolboxLibrary(void* handle, const PJ_toolbox_vtable_t* vtable, std::string path) - : handle_(handle), vtable_(vtable), path_(std::move(path)) {} +ToolboxLibrary::ToolboxLibrary(std::shared_ptr handle, const PJ_toolbox_vtable_t* vtable, std::string path) + : handle_(std::move(handle)), vtable_(vtable), path_(std::move(path)) {} ToolboxLibrary::~ToolboxLibrary() { reset(); } ToolboxLibrary::ToolboxLibrary(ToolboxLibrary&& other) noexcept - : handle_(other.handle_), vtable_(other.vtable_), path_(std::move(other.path_)) { - other.handle_ = nullptr; + : handle_(std::move(other.handle_)), vtable_(other.vtable_), path_(std::move(other.path_)) { other.vtable_ = nullptr; } ToolboxLibrary& ToolboxLibrary::operator=(ToolboxLibrary&& other) noexcept { if (this != &other) { reset(); - handle_ = other.handle_; + handle_ = std::move(other.handle_); vtable_ = other.vtable_; path_ = std::move(other.path_); - other.handle_ = nullptr; other.vtable_ = nullptr; } return *this; } Expected ToolboxLibrary::load(std::string_view path) { - auto handle = detail::loadLibraryHandle(path); - if (!handle) { - return unexpected(handle.error()); + auto raw_handle = detail::loadLibraryHandle(path); + if (!raw_handle) { + return unexpected(raw_handle.error()); } + auto handle = detail::adoptLibraryHandle(*raw_handle); - if (auto abi = detail::checkPluginAbiVersion(*handle); !abi) { - detail::closeLibraryHandle(*handle); + if (auto abi = detail::checkPluginAbiVersion(handle.get()); !abi) { return unexpected(abi.error()); } - auto sym = detail::resolveSymbol(*handle, "PJ_get_toolbox_vtable"); + auto sym = detail::resolveSymbol(handle.get(), "PJ_get_toolbox_vtable"); if (!sym) { - detail::closeLibraryHandle(*handle); return unexpected(sym.error()); } auto entry = reinterpret_cast(*sym); const PJ_toolbox_vtable_t* vtable = entry(); if (vtable == nullptr) { - detail::closeLibraryHandle(*handle); return unexpected(std::string("PJ_get_toolbox_vtable returned null")); } if (vtable->protocol_version != PJ_TOOLBOX_PLUGIN_PROTOCOL_VERSION) { - detail::closeLibraryHandle(*handle); return unexpected(std::string("Toolbox protocol version mismatch")); } if (vtable->struct_size < PJ_TOOLBOX_MIN_VTABLE_SIZE) { - detail::closeLibraryHandle(*handle); - return unexpected(std::string("Toolbox vtable smaller than v3.0 baseline")); + return unexpected(std::string("Toolbox vtable smaller than v4.0 baseline")); } - return ToolboxLibrary(*handle, vtable, std::string(path)); + return ToolboxLibrary(std::move(handle), vtable, std::string(path)); } Expected ToolboxLibrary::resolveDialogVtable() const { - auto sym = detail::resolveSymbol(handle_, "PJ_get_dialog_vtable"); + auto sym = detail::resolveSymbol(handle_.get(), "PJ_get_dialog_vtable"); if (!sym) { return unexpected(sym.error()); } @@ -87,8 +81,7 @@ Expected ToolboxLibrary::resolveDialogVtable() const void ToolboxLibrary::reset() { if (handle_ != nullptr) { - detail::closeLibraryHandle(handle_); - handle_ = nullptr; + handle_.reset(); vtable_ = nullptr; path_.clear(); } diff --git a/pj_plugins/tests/data_source_library_test.cpp b/pj_plugins/tests/data_source_library_test.cpp index 2f6854d..ccf87fe 100644 --- a/pj_plugins/tests/data_source_library_test.cpp +++ b/pj_plugins/tests/data_source_library_test.cpp @@ -4,6 +4,7 @@ #include #include +#include #include #include "pj_base/plugin_data_api.h" @@ -155,6 +156,20 @@ TEST(DataSourceLibraryTest, BindFailsWithEmptyRegistry) { EXPECT_NE(status.error().find("pj.source_write.v1"), std::string::npos); } +TEST(DataSourceLibraryTest, HandleKeepsSharedLibraryLoadedAfterLibraryObjectDies) { + std::unique_ptr handle; + { + auto library = PJ::DataSourceLibrary::load(PJ_MOCK_DATA_SOURCE_PLUGIN_PATH); + ASSERT_TRUE(library) << library.error(); + handle = std::make_unique(library->createHandle()); + ASSERT_TRUE(handle->valid()); + } + + EXPECT_NE(handle->manifest().find("Mock DataSource"), std::string::npos); + EXPECT_NE(handle->capabilities(), 0u); + handle.reset(); +} + TEST(RuntimeHostViewTest, ListAvailableEncodingsReturnsEmptyWhenNullptr) { PJ::DataSourceRuntimeHostView view(makeRuntimeHost(false)); EXPECT_TRUE(view.listAvailableEncodings().empty()); diff --git a/pj_plugins/tests/message_parser_library_test.cpp b/pj_plugins/tests/message_parser_library_test.cpp index 67a160f..b16a7b8 100644 --- a/pj_plugins/tests/message_parser_library_test.cpp +++ b/pj_plugins/tests/message_parser_library_test.cpp @@ -3,6 +3,7 @@ #include #include +#include #include #include "pj_base/sdk/service_traits.hpp" @@ -65,6 +66,21 @@ TEST(MessageParserLibraryTest, SaveLoadConfig) { ASSERT_TRUE(handle.loadConfig(R"({"format":"compact"})")); } +TEST(MessageParserLibraryTest, HandleKeepsSharedLibraryLoadedAfterLibraryObjectDies) { + std::unique_ptr handle; + { + auto library = PJ::MessageParserLibrary::load(PJ_MOCK_JSON_PARSER_PLUGIN_PATH); + ASSERT_TRUE(library) << library.error(); + handle = std::make_unique(library->createHandle()); + ASSERT_TRUE(handle->valid()); + } + + EXPECT_NE(handle->manifest().find("Mock JSON Parser"), std::string::npos); + std::string cfg; + EXPECT_TRUE(handle->saveConfig(cfg)); + handle.reset(); +} + TEST(MessageParserLibraryTest, LoadNonexistentFails) { auto result = PJ::MessageParserLibrary::load("/nonexistent_path/fake_plugin.so"); EXPECT_FALSE(result); diff --git a/pj_plugins/tests/toolbox_plugin_test.cpp b/pj_plugins/tests/toolbox_plugin_test.cpp index a6594fa..e5ce94c 100644 --- a/pj_plugins/tests/toolbox_plugin_test.cpp +++ b/pj_plugins/tests/toolbox_plugin_test.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include "pj_base/plugin_data_api.h" @@ -131,6 +132,20 @@ TEST(ToolboxPluginTest, BindFailsWithoutMandatoryServices) { EXPECT_FALSE(status); } +TEST(ToolboxPluginTest, HandleKeepsSharedLibraryLoadedAfterLibraryObjectDies) { + std::unique_ptr handle; + { + auto library = PJ::ToolboxLibrary::load(PJ_MOCK_TOOLBOX_PLUGIN_PATH); + ASSERT_TRUE(library) << library.error(); + handle = std::make_unique(library->createHandle()); + ASSERT_TRUE(handle->valid()); + } + + EXPECT_NE(handle->manifest().find("Mock Toolbox"), std::string::npos); + EXPECT_EQ(handle->capabilities(), 0u); + handle.reset(); +} + TEST(ToolboxPluginTest, ReadTransformWriteFlowAndNotifyDataChanged) { auto library = PJ::ToolboxLibrary::load(PJ_MOCK_TOOLBOX_PLUGIN_PATH); ASSERT_TRUE(library) << library.error(); From 364e1d3ba88f21d32abf4f802035a2ccb0891122 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 12:20:44 +0200 Subject: [PATCH 02/10] fix: harden plugin host boundaries --- pj_datastore/src/plugin_data_host.cpp | 253 +++++++++++------- pj_plugins/CMakeLists.txt | 13 +- pj_plugins/src/data_source_library.cpp | 4 + pj_plugins/src/detail/vtable_validation.hpp | 83 ++++++ pj_plugins/src/message_parser_library.cpp | 4 + pj_plugins/src/plugin_catalog.cpp | 4 + pj_plugins/src/plugin_runtime_catalog.cpp | 14 +- pj_plugins/src/toolbox_library.cpp | 4 + pj_plugins/tests/data_source_library_test.cpp | 9 + .../tests/message_parser_library_test.cpp | 9 + .../tests/missing_required_slots_plugin.cpp | 116 ++++++++ pj_plugins/tests/plugin_catalog_test.cpp | 11 + pj_plugins/tests/toolbox_plugin_test.cpp | 9 + 13 files changed, 417 insertions(+), 116 deletions(-) create mode 100644 pj_plugins/src/detail/vtable_validation.hpp create mode 100644 pj_plugins/tests/missing_required_slots_plugin.cpp diff --git a/pj_datastore/src/plugin_data_host.cpp b/pj_datastore/src/plugin_data_host.cpp index 1899b9d..c1ca26b 100644 --- a/pj_datastore/src/plugin_data_host.cpp +++ b/pj_datastore/src/plugin_data_host.cpp @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -957,183 +958,227 @@ void propagateError(PJ_error_t* out_error, const char* msg) { sdk::fillError(out_error, 1, "datastore", msg != nullptr ? std::string_view(msg) : std::string_view{}); } -bool sourceEnsureTopic(void* ctx, PJ_string_view_t topic_name, TopicHandle* out_topic, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.ensureTopic(impl->source, toStringView(topic_name), out_topic)) { - propagateError(out_error, impl->core.lastError()); - return false; +template +bool guardHostCallback(PJ_error_t* out_error, Fn&& fn) noexcept { + try { + return fn(); + } catch (const std::exception& e) { + propagateError(out_error, e.what()); + } catch (...) { + propagateError(out_error, "unknown datastore host exception"); } - return true; + return false; +} + +bool sourceEnsureTopic(void* ctx, PJ_string_view_t topic_name, TopicHandle* out_topic, PJ_error_t* out_error) noexcept { + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.ensureTopic(impl->source, toStringView(topic_name), out_topic)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + return true; + }); } bool sourceEnsureField( void* ctx, TopicHandle topic, PJ_string_view_t field_name, PJ_primitive_type_t type, FieldHandle* out_field, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.ensureField(topic, toStringView(field_name), type, out_field)) { - propagateError(out_error, impl->core.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.ensureField(topic, toStringView(field_name), type, out_field)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + return true; + }); } bool sourceAppendRecord( void* ctx, TopicHandle topic, int64_t timestamp, const PJ_named_field_value_t* fields, std::size_t field_count, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.appendRecord(topic, timestamp, fields, field_count)) { - propagateError(out_error, impl->core.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.appendRecord(topic, timestamp, fields, field_count)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + return true; + }); } bool sourceAppendBoundRecord( void* ctx, TopicHandle topic, int64_t timestamp, const PJ_bound_field_value_t* fields, std::size_t field_count, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.appendBoundRecord(topic, timestamp, fields, field_count)) { - propagateError(out_error, impl->core.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.appendBoundRecord(topic, timestamp, fields, field_count)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + return true; + }); } bool sourceAppendArrowStream( void* ctx, TopicHandle topic, struct ArrowArrayStream* stream, PJ_string_view_t timestamp_column, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.appendArrowStream(topic, stream, timestamp_column)) { - // Failure: plugin retains ownership of the stream; we do NOT release. - propagateError(out_error, impl->core.lastError()); - return false; - } - // Success: host now owns the stream — release it. - if (stream != nullptr && stream->release != nullptr) { - stream->release(stream); - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.appendArrowStream(topic, stream, timestamp_column)) { + // Failure: plugin retains ownership of the stream; we do NOT release. + propagateError(out_error, impl->core.lastError()); + return false; + } + // Success: host now owns the stream — release it. + if (stream != nullptr && stream->release != nullptr) { + stream->release(stream); + } + return true; + }); } bool parserEnsureField( void* ctx, PJ_string_view_t field_name, PJ_primitive_type_t type, FieldHandle* out_field, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.ensureField(impl->topic, toStringView(field_name), type, out_field)) { - propagateError(out_error, impl->core.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.ensureField(impl->topic, toStringView(field_name), type, out_field)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + return true; + }); } bool parserAppendRecord( void* ctx, int64_t timestamp, const PJ_named_field_value_t* fields, std::size_t field_count, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.appendRecord(impl->topic, timestamp, fields, field_count)) { - propagateError(out_error, impl->core.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.appendRecord(impl->topic, timestamp, fields, field_count)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + return true; + }); } bool parserAppendBoundRecord( void* ctx, int64_t timestamp, const PJ_bound_field_value_t* fields, std::size_t field_count, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.appendBoundRecord(impl->topic, timestamp, fields, field_count)) { - propagateError(out_error, impl->core.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.appendBoundRecord(impl->topic, timestamp, fields, field_count)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + return true; + }); } bool toolboxCreateDataSource( void* ctx, PJ_string_view_t name, DataSourceHandle* out_source, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.write.createDataSource(toStringView(name), out_source)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.write.createDataSource(toStringView(name), out_source)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + return true; + }); } bool toolboxEnsureTopic( void* ctx, DataSourceHandle source, PJ_string_view_t topic_name, TopicHandle* out_topic, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.write.ensureTopic(source, toStringView(topic_name), out_topic)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.write.ensureTopic(source, toStringView(topic_name), out_topic)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + return true; + }); } bool toolboxEnsureField( void* ctx, TopicHandle topic, PJ_string_view_t field_name, PJ_primitive_type_t type, FieldHandle* out_field, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.write.ensureField(topic, toStringView(field_name), type, out_field)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.write.ensureField(topic, toStringView(field_name), type, out_field)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + return true; + }); } bool toolboxAppendRecord( void* ctx, TopicHandle topic, int64_t timestamp, const PJ_named_field_value_t* fields, std::size_t field_count, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.write.appendRecord(topic, timestamp, fields, field_count)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.write.appendRecord(topic, timestamp, fields, field_count)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + return true; + }); } bool toolboxAppendBoundRecord( void* ctx, TopicHandle topic, int64_t timestamp, const PJ_bound_field_value_t* fields, std::size_t field_count, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.write.appendBoundRecord(topic, timestamp, fields, field_count)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.write.appendBoundRecord(topic, timestamp, fields, field_count)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + return true; + }); } bool toolboxAppendArrowStream( void* ctx, TopicHandle topic, struct ArrowArrayStream* stream, PJ_string_view_t timestamp_column, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.write.appendArrowStream(topic, stream, timestamp_column)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - if (stream != nullptr && stream->release != nullptr) { - stream->release(stream); - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.write.appendArrowStream(topic, stream, timestamp_column)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + if (stream != nullptr && stream->release != nullptr) { + stream->release(stream); + } + return true; + }); } bool toolboxAcquireCatalogSnapshot(void* ctx, PJ_catalog_snapshot_t* out_snapshot, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.acquireCatalogSnapshot(out_snapshot)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.acquireCatalogSnapshot(out_snapshot)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + return true; + }); } bool toolboxReadSeriesArrow( void* ctx, FieldHandle field, struct ArrowSchema* out_schema, struct ArrowArray* out_array, PJ_error_t* out_error) noexcept { - auto* impl = static_cast(ctx); - if (!impl->core.readSeriesArrow(field, out_schema, out_array)) { - propagateError(out_error, impl->core.write.lastError()); - return false; - } - return true; + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.readSeriesArrow(field, out_schema, out_array)) { + propagateError(out_error, impl->core.write.lastError()); + return false; + } + return true; + }); } /// RAII holder for the plugin-owned `fetch_ctx` passed to push_lazy. Stores diff --git a/pj_plugins/CMakeLists.txt b/pj_plugins/CMakeLists.txt index e8f2d68..a24b8e6 100644 --- a/pj_plugins/CMakeLists.txt +++ b/pj_plugins/CMakeLists.txt @@ -84,6 +84,11 @@ target_compile_features(invalid_optional_manifest_data_source_plugin PRIVATE cxx target_compile_options(invalid_optional_manifest_data_source_plugin PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(invalid_optional_manifest_data_source_plugin PRIVATE pj_base) +add_library(missing_required_slots_plugin SHARED tests/missing_required_slots_plugin.cpp) +target_compile_features(missing_required_slots_plugin PRIVATE cxx_std_20) +target_compile_options(missing_required_slots_plugin PRIVATE ${PJ_WARNING_FLAGS}) +target_link_libraries(missing_required_slots_plugin PRIVATE pj_base) + endif() # PJ_BUILD_TESTS # --------------------------------------------------------------------------- @@ -161,6 +166,8 @@ target_link_libraries(pj_plugin_runtime_catalog pj_toolbox_host pj_plugin_catalog pj_base + PRIVATE + nlohmann_json::nlohmann_json ) if(PJ_BUILD_TESTS) @@ -181,6 +188,7 @@ target_link_libraries(mock_toolbox_plugin PRIVATE pj_base) add_executable(data_source_library_test tests/data_source_library_test.cpp) target_compile_definitions(data_source_library_test PRIVATE PJ_MOCK_DATA_SOURCE_PLUGIN_PATH="$" + PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH="$" ) target_compile_options(data_source_library_test PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(data_source_library_test PRIVATE @@ -215,6 +223,7 @@ add_test(NAME file_source_integration_test COMMAND file_source_integration_test) add_executable(message_parser_library_test tests/message_parser_library_test.cpp) target_compile_definitions(message_parser_library_test PRIVATE PJ_MOCK_JSON_PARSER_PLUGIN_PATH="$" + PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH="$" ) target_compile_options(message_parser_library_test PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(message_parser_library_test PRIVATE @@ -243,6 +252,7 @@ add_test(NAME message_parser_library_test COMMAND message_parser_library_test) add_executable(toolbox_plugin_test tests/toolbox_plugin_test.cpp) target_compile_definitions(toolbox_plugin_test PRIVATE PJ_MOCK_TOOLBOX_PLUGIN_PATH="$" + PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH="$" ) target_compile_options(toolbox_plugin_test PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(toolbox_plugin_test PRIVATE @@ -261,6 +271,7 @@ target_compile_definitions(plugin_catalog_test PRIVATE PJ_MOCK_DIALOG_PLUGIN_PATH="$" PJ_MISSING_ID_PLUGIN_PATH="$" PJ_INVALID_OPTIONAL_PLUGIN_PATH="$" + PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH="$" ) target_compile_options(plugin_catalog_test PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(plugin_catalog_test PRIVATE @@ -268,7 +279,7 @@ target_link_libraries(plugin_catalog_test PRIVATE ) add_dependencies(plugin_catalog_test mock_data_source_plugin mock_json_parser_plugin mock_toolbox_plugin mock_dialog_plugin missing_id_data_source_plugin - invalid_optional_manifest_data_source_plugin) + invalid_optional_manifest_data_source_plugin missing_required_slots_plugin) add_test(NAME plugin_catalog_test COMMAND plugin_catalog_test) endif() # PJ_BUILD_TESTS diff --git a/pj_plugins/src/data_source_library.cpp b/pj_plugins/src/data_source_library.cpp index efa7b0d..e030319 100644 --- a/pj_plugins/src/data_source_library.cpp +++ b/pj_plugins/src/data_source_library.cpp @@ -3,6 +3,7 @@ #include #include "detail/library_loader.hpp" +#include "detail/vtable_validation.hpp" namespace PJ { @@ -59,6 +60,9 @@ Expected DataSourceLibrary::load(std::string_view path) { if (vtable->struct_size < PJ_DATA_SOURCE_MIN_VTABLE_SIZE) { return unexpected(std::string("DataSource vtable smaller than v4.0 baseline")); } + if (auto status = detail::validateRequiredSlots(vtable); !status) { + return unexpected(status.error()); + } return DataSourceLibrary(std::move(handle), vtable, std::string(path)); } diff --git a/pj_plugins/src/detail/vtable_validation.hpp b/pj_plugins/src/detail/vtable_validation.hpp new file mode 100644 index 0000000..b5a4aec --- /dev/null +++ b/pj_plugins/src/detail/vtable_validation.hpp @@ -0,0 +1,83 @@ +#pragma once + +#include +#include +#include + +#include "pj_base/data_source_protocol.h" +#include "pj_base/expected.hpp" +#include "pj_base/message_parser_protocol.h" +#include "pj_base/toolbox_protocol.h" + +namespace PJ::detail { + +struct RequiredSlot { + std::string_view name; + bool present; +}; + +inline Status validateRequiredSlots(std::string_view family, std::initializer_list slots) { + for (const auto& slot : slots) { + if (!slot.present) { + return unexpected(std::string(family) + " vtable missing required slot: " + std::string(slot.name)); + } + } + return okStatus(); +} + +inline Status validateRequiredSlots(const PJ_data_source_vtable_t* vtable) { + return validateRequiredSlots( + "DataSource", + { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"capabilities", vtable->capabilities != nullptr}, + {"bind", vtable->bind != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"start", vtable->start != nullptr}, + {"stop", vtable->stop != nullptr}, + {"pause", vtable->pause != nullptr}, + {"resume", vtable->resume != nullptr}, + {"poll", vtable->poll != nullptr}, + {"current_state", vtable->current_state != nullptr}, + {"get_dialog", vtable->get_dialog != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); +} + +inline Status validateRequiredSlots(const PJ_message_parser_vtable_t* vtable) { + return validateRequiredSlots( + "MessageParser", + { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"bind", vtable->bind != nullptr}, + {"bind_schema", vtable->bind_schema != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"parse", vtable->parse != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); +} + +inline Status validateRequiredSlots(const PJ_toolbox_vtable_t* vtable) { + return validateRequiredSlots( + "Toolbox", + { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"capabilities", vtable->capabilities != nullptr}, + {"bind", vtable->bind != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"get_dialog", vtable->get_dialog != nullptr}, + {"on_data_changed", vtable->on_data_changed != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); +} + +} // namespace PJ::detail diff --git a/pj_plugins/src/message_parser_library.cpp b/pj_plugins/src/message_parser_library.cpp index 5b33c5f..ebe91d4 100644 --- a/pj_plugins/src/message_parser_library.cpp +++ b/pj_plugins/src/message_parser_library.cpp @@ -3,6 +3,7 @@ #include #include "detail/library_loader.hpp" +#include "detail/vtable_validation.hpp" namespace PJ { @@ -57,6 +58,9 @@ Expected MessageParserLibrary::load(std::string_view path) if (vtable->struct_size < PJ_MESSAGE_PARSER_MIN_VTABLE_SIZE) { return unexpected(std::string("MessageParser vtable smaller than v4.0 baseline")); } + if (auto status = detail::validateRequiredSlots(vtable); !status) { + return unexpected(status.error()); + } return MessageParserLibrary(std::move(handle), vtable, std::string(path)); } diff --git a/pj_plugins/src/plugin_catalog.cpp b/pj_plugins/src/plugin_catalog.cpp index f822291..1cf47d6 100644 --- a/pj_plugins/src/plugin_catalog.cpp +++ b/pj_plugins/src/plugin_catalog.cpp @@ -10,6 +10,7 @@ #include #include "detail/library_loader.hpp" +#include "detail/vtable_validation.hpp" #include "pj_base/data_source_protocol.h" #include "pj_base/message_parser_protocol.h" #include "pj_base/toolbox_protocol.h" @@ -63,6 +64,9 @@ Expected probeDirectVtable( if (vt->struct_size < min_vtable_size) { return unexpected(std::string(family_name) + " vtable smaller than v4.0 baseline"); } + if (auto status = detail::validateRequiredSlots(vt); !status) { + return unexpected(status.error()); + } return ManifestCandidate{family, vt->manifest_json == nullptr ? "" : vt->manifest_json}; } diff --git a/pj_plugins/src/plugin_runtime_catalog.cpp b/pj_plugins/src/plugin_runtime_catalog.cpp index 07d6a50..32975f9 100644 --- a/pj_plugins/src/plugin_runtime_catalog.cpp +++ b/pj_plugins/src/plugin_runtime_catalog.cpp @@ -1,7 +1,8 @@ #include "pj_plugins/host/plugin_runtime_catalog.hpp" +#include + #include -#include #include #include @@ -400,16 +401,7 @@ std::string PluginRuntimeCatalog::listAvailableEncodings() const { } } - std::ostringstream out; - out << '['; - for (size_t i = 0; i < unique_encodings.size(); ++i) { - if (i > 0) { - out << ','; - } - out << '"' << unique_encodings[i] << '"'; - } - out << ']'; - return out.str(); + return nlohmann::json(unique_encodings).dump(); } void PluginRuntimeCatalog::report(DiagnosticLevel level, const std::string& id, std::string message) const { diff --git a/pj_plugins/src/toolbox_library.cpp b/pj_plugins/src/toolbox_library.cpp index a796178..e9afc44 100644 --- a/pj_plugins/src/toolbox_library.cpp +++ b/pj_plugins/src/toolbox_library.cpp @@ -3,6 +3,7 @@ #include #include "detail/library_loader.hpp" +#include "detail/vtable_validation.hpp" namespace PJ { @@ -56,6 +57,9 @@ Expected ToolboxLibrary::load(std::string_view path) { if (vtable->struct_size < PJ_TOOLBOX_MIN_VTABLE_SIZE) { return unexpected(std::string("Toolbox vtable smaller than v4.0 baseline")); } + if (auto status = detail::validateRequiredSlots(vtable); !status) { + return unexpected(status.error()); + } return ToolboxLibrary(std::move(handle), vtable, std::string(path)); } diff --git a/pj_plugins/tests/data_source_library_test.cpp b/pj_plugins/tests/data_source_library_test.cpp index ccf87fe..3c1129b 100644 --- a/pj_plugins/tests/data_source_library_test.cpp +++ b/pj_plugins/tests/data_source_library_test.cpp @@ -14,6 +14,9 @@ #ifndef PJ_MOCK_DATA_SOURCE_PLUGIN_PATH #error "PJ_MOCK_DATA_SOURCE_PLUGIN_PATH must be defined" #endif +#ifndef PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH +#error "PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH must be defined" +#endif namespace { @@ -145,6 +148,12 @@ TEST(DataSourceLibraryTest, LoadsSharedPluginAndDrivesInstance) { EXPECT_EQ(handle.currentState(), PJ::DataSourceState::kStopped); } +TEST(DataSourceLibraryTest, RejectsMissingRequiredVtableSlot) { + auto library = PJ::DataSourceLibrary::load(PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH); + ASSERT_FALSE(library); + EXPECT_NE(library.error().find("DataSource vtable missing required slot: start"), std::string::npos); +} + TEST(DataSourceLibraryTest, BindFailsWithEmptyRegistry) { auto library = PJ::DataSourceLibrary::load(PJ_MOCK_DATA_SOURCE_PLUGIN_PATH); ASSERT_TRUE(library); diff --git a/pj_plugins/tests/message_parser_library_test.cpp b/pj_plugins/tests/message_parser_library_test.cpp index b16a7b8..ac6c60d 100644 --- a/pj_plugins/tests/message_parser_library_test.cpp +++ b/pj_plugins/tests/message_parser_library_test.cpp @@ -13,6 +13,9 @@ #ifndef PJ_MOCK_JSON_PARSER_PLUGIN_PATH #error "PJ_MOCK_JSON_PARSER_PLUGIN_PATH must be defined" #endif +#ifndef PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH +#error "PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH must be defined" +#endif namespace { @@ -23,6 +26,12 @@ TEST(MessageParserLibraryTest, LoadMockPlugin) { EXPECT_EQ(library->vtable()->protocol_version, PJ_MESSAGE_PARSER_PROTOCOL_VERSION); } +TEST(MessageParserLibraryTest, RejectsMissingRequiredVtableSlot) { + auto library = PJ::MessageParserLibrary::load(PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH); + ASSERT_FALSE(library); + EXPECT_NE(library.error().find("MessageParser vtable missing required slot: parse"), std::string::npos); +} + TEST(MessageParserLibraryTest, ManifestRoundTrip) { auto library = PJ::MessageParserLibrary::load(PJ_MOCK_JSON_PARSER_PLUGIN_PATH); ASSERT_TRUE(library) << library.error(); diff --git a/pj_plugins/tests/missing_required_slots_plugin.cpp b/pj_plugins/tests/missing_required_slots_plugin.cpp new file mode 100644 index 0000000..58a3790 --- /dev/null +++ b/pj_plugins/tests/missing_required_slots_plugin.cpp @@ -0,0 +1,116 @@ +#include "pj_base/data_source_protocol.h" +#include "pj_base/message_parser_protocol.h" +#include "pj_base/toolbox_protocol.h" + +extern "C" PJ_DATA_SOURCE_EXPORT const uint32_t pj_plugin_abi_version = PJ_ABI_VERSION; + +namespace { + +void* create() noexcept { + return reinterpret_cast(0x1); +} + +void destroy(void*) noexcept {} + +uint64_t capabilities(void*) noexcept { + return 0; +} + +bool bind(void*, PJ_service_registry_t, PJ_error_t*) noexcept { + return true; +} + +bool bindSchema(void*, PJ_string_view_t, PJ_bytes_view_t, PJ_error_t*) noexcept { + return true; +} + +bool saveConfig(void*, PJ_string_view_t* out_json, PJ_error_t*) noexcept { + static constexpr const char* kJson = "{}"; + if (out_json != nullptr) { + out_json->data = kJson; + out_json->size = 2; + } + return true; +} + +bool loadConfig(void*, PJ_string_view_t, PJ_error_t*) noexcept { + return true; +} + +bool ok(void*, PJ_error_t*) noexcept { + return true; +} + +void stop(void*) noexcept {} + +PJ_data_source_state_t state(void*) noexcept { + return PJ_DATA_SOURCE_STATE_IDLE; +} + +PJ_borrowed_dialog_t dialog(void*) noexcept { + return PJ_borrowed_dialog_t{nullptr, nullptr}; +} + +const void* extension(void*, PJ_string_view_t) noexcept { + return nullptr; +} + +} // namespace + +extern "C" PJ_DATA_SOURCE_EXPORT const PJ_data_source_vtable_t* PJ_get_data_source_vtable() noexcept { + static const PJ_data_source_vtable_t vt = { + .protocol_version = PJ_DATA_SOURCE_PROTOCOL_VERSION, + .struct_size = sizeof(PJ_data_source_vtable_t), + .create = create, + .destroy = destroy, + .manifest_json = R"({"id":"missing-source-slot","name":"Missing Source Slot","version":"1.0.0"})", + .capabilities = capabilities, + .bind = bind, + .save_config = saveConfig, + .load_config = loadConfig, + .start = nullptr, + .stop = stop, + .pause = ok, + .resume = ok, + .poll = ok, + .current_state = state, + .get_dialog = dialog, + .get_plugin_extension = extension, + }; + return &vt; +} + +extern "C" PJ_MESSAGE_PARSER_EXPORT const PJ_message_parser_vtable_t* PJ_get_message_parser_vtable() noexcept { + static const PJ_message_parser_vtable_t vt = { + .protocol_version = PJ_MESSAGE_PARSER_PROTOCOL_VERSION, + .struct_size = sizeof(PJ_message_parser_vtable_t), + .create = create, + .destroy = destroy, + .manifest_json = R"({"id":"missing-parser-slot","name":"Missing Parser Slot","version":"1.0.0","encoding":["x"]})", + .bind = bind, + .bind_schema = bindSchema, + .save_config = saveConfig, + .load_config = loadConfig, + .parse = nullptr, + .get_plugin_extension = extension, + }; + return &vt; +} + +extern "C" PJ_TOOLBOX_EXPORT const PJ_toolbox_vtable_t* PJ_get_toolbox_vtable() noexcept { + static const PJ_toolbox_vtable_t vt = { + .protocol_version = PJ_TOOLBOX_PLUGIN_PROTOCOL_VERSION, + .struct_size = sizeof(PJ_toolbox_vtable_t), + .create = create, + .destroy = destroy, + .manifest_json = R"({"id":"missing-toolbox-slot","name":"Missing Toolbox Slot","version":"1.0.0"})", + .capabilities = capabilities, + .bind = bind, + .save_config = saveConfig, + .load_config = loadConfig, + .get_dialog = dialog, + .on_data_changed = nullptr, + .get_plugin_extension = extension, + }; + return &vt; +} diff --git a/pj_plugins/tests/plugin_catalog_test.cpp b/pj_plugins/tests/plugin_catalog_test.cpp index 1f0511b..ad747a9 100644 --- a/pj_plugins/tests/plugin_catalog_test.cpp +++ b/pj_plugins/tests/plugin_catalog_test.cpp @@ -104,6 +104,17 @@ TEST_F(PluginCatalogTest, InvalidOptionalManifestFieldIsReportedAsDiagnostic) { EXPECT_NE(result->diagnostics[0].message.find("invalid_optional"), std::string::npos); } +TEST_F(PluginCatalogTest, MissingRequiredVtableSlotIsReportedAsDiagnostic) { + copyPlugin(PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH, pluginFileName("missing_required_slots")); + + auto result = scanPluginDsos(dir_); + ASSERT_TRUE(result.has_value()) << result.error(); + EXPECT_TRUE(result->plugins.empty()); + ASSERT_EQ(result->diagnostics.size(), 1U); + EXPECT_NE(result->diagnostics[0].message.find("missing required slot"), std::string::npos); + EXPECT_NE(result->diagnostics[0].message.find("missing_required_slots"), std::string::npos); +} + TEST_F(PluginCatalogTest, ScanContinuesAfterBrokenDso) { copyPlugin(PJ_MOCK_DATA_SOURCE_PLUGIN_PATH, pluginFileName("valid")); std::ofstream(dir_ / pluginFileName("broken")) << "not a shared library"; diff --git a/pj_plugins/tests/toolbox_plugin_test.cpp b/pj_plugins/tests/toolbox_plugin_test.cpp index e5ce94c..7ade763 100644 --- a/pj_plugins/tests/toolbox_plugin_test.cpp +++ b/pj_plugins/tests/toolbox_plugin_test.cpp @@ -13,6 +13,9 @@ #ifndef PJ_MOCK_TOOLBOX_PLUGIN_PATH #error "PJ_MOCK_TOOLBOX_PLUGIN_PATH must be defined" #endif +#ifndef PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH +#error "PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH must be defined" +#endif namespace { @@ -103,6 +106,12 @@ TEST(ToolboxPluginTest, LoadsSharedLibraryAndValidatesVtable) { EXPECT_EQ(library->vtable()->protocol_version, static_cast(PJ_TOOLBOX_PLUGIN_PROTOCOL_VERSION)); } +TEST(ToolboxPluginTest, RejectsMissingRequiredVtableSlot) { + auto library = PJ::ToolboxLibrary::load(PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH); + ASSERT_FALSE(library); + EXPECT_NE(library.error().find("Toolbox vtable missing required slot: on_data_changed"), std::string::npos); +} + TEST(ToolboxPluginTest, BindHostsAndConfigRoundTrip) { auto library = PJ::ToolboxLibrary::load(PJ_MOCK_TOOLBOX_PLUGIN_PATH); ASSERT_TRUE(library) << library.error(); From d2228ddc5e29f3c4194fd5edbfece6e0d07cadb4 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 12:24:46 +0200 Subject: [PATCH 03/10] docs: clarify plugin author guidance --- pj_plugins/docs/data-source-guide.md | 67 ++++++++++++++++++++++++- pj_plugins/docs/dialog-plugin-guide.md | 61 ++++++++++++++++++++++ pj_plugins/docs/message-parser-guide.md | 62 +++++++++++++++++++++++ pj_plugins/docs/toolbox-guide.md | 60 +++++++++++++++++++++- 4 files changed, 246 insertions(+), 4 deletions(-) diff --git a/pj_plugins/docs/data-source-guide.md b/pj_plugins/docs/data-source-guide.md index 6e13c60..6cab725 100644 --- a/pj_plugins/docs/data-source-guide.md +++ b/pj_plugins/docs/data-source-guide.md @@ -7,6 +7,15 @@ > through the author-facing workflow; `ARCHITECTURE.md` is the binding > reference when the two disagree. +> **Vocabulary used throughout this guide** (defined in `pj_base/expected.hpp`, +> `pj_base/span.hpp`): +> - `PJ::Status` — alias for `PJ::Expected`. Return `PJ::okStatus()` on +> success or `PJ::unexpected("reason")` on failure. +> - `PJ::Expected` — like `std::expected`. Test with +> `if (!result)`; access via `*result`; access the error via `result.error()`. +> - `PJ::Span` — non-owning view over contiguous `T` (like `std::span`). +> - `PJ::Timestamp` — `int64_t` nanoseconds since the Unix epoch. + ## What is a DataSource? A DataSource plugin is a shared library (`.so` / `.dylib` / `.dll`) that @@ -14,16 +23,58 @@ acquires data — from files, network streams, hardware, etc. — and feeds it into PlotJuggler. Plugins link only against `pj_base` (no Qt, no host internals) and communicate through a stable C ABI. +## Choosing a Base Class + +Pick the first row that matches your input shape: + +| Your input | Base class | Capability flag | Override | +|---|---|---|---| +| One-shot file import (CSV, Parquet, MCAP) | `PJ::FileSourceBase` | `kCapabilityDirectIngest` | `importData()` | +| Live stream, you decode the payload | `PJ::StreamSourceBase` | `kCapabilityDirectIngest` | `onStart()`, `onPoll()`, `onStop()` | +| Live transport (MQTT/ZMQ/UDP) where payload encoding varies | `PJ::StreamSourceBase` | `kCapabilityDelegatedIngest` | `onStart()`, `onPoll()`, `onStop()` + bind a parser | +| None of the above (full manual lifecycle) | `PJ::DataSourcePluginBase` | declare your own | `start()`, `stop()`, `currentState()`, … | + +Most plugins want one of the first three. Reach for `DataSourcePluginBase` +directly only when the supplied state machines genuinely don't fit. + ## Quick Start -1. Subclass `PJ::FileSourceBase` (file importer) or `PJ::StreamSourceBase` - (live stream), or `PJ::DataSourcePluginBase` for full control. +1. Pick a base class from the table above. 2. Implement the required virtuals (see Common Patterns below). 3. Export with `PJ_DATA_SOURCE_PLUGIN(YourClass, R"({"id":"...","name":"...","version":"..."})")` 4. Build as a shared library linking `pj_base` A complete example lives at `pj_plugins/examples/mock_data_source.cpp`. +## Plugin Contract + +Follow these rules. Some are enforced by the loader or SDK trampolines; others +prevent runtime failures and confusing host behaviour. + +**MUST** +- Return `PJ::okStatus()` on success and `PJ::unexpected("reason")` on failure + from every fallible method. +- Call `runtimeHost().notifyState(...)` on every state transition you trigger. +- Call host methods (`writeHost()`, `runtimeHost()`) only from the host's + callback thread — the same thread that invoked `onStart`/`onPoll`/`onStop`/ + `importData`/`pause`/`resume`. +- Make `onStop()` idempotent. +- Persist all state needed for headless restart in `saveConfig()`. The host + must be able to `loadConfig(saved) → start()` without showing a dialog. + +**MUST NOT** +- Throw exceptions across virtual overrides — the SDK trampolines catch them, + but the host receives a generic error and the plugin loses the chance to + report a useful reason. +- Call host methods from a background thread you spawned. Buffer in plugin + memory and flush from `onPoll()`. +- Call `runtimeHost().progressFinish()` from a `FileSourceBase` subclass — + the base class calls it for you after `importData()` returns. +- Re-release an `ArrowArrayStream` after `appendArrowStream()` returns + success; the host already consumed and released it. Use + `PJ::sdk::ArrowStreamHolder` to get this right automatically. +- Resume from a terminal state (`stopped`, `failed`). Create a new instance. + ## Step by Step ### 1. Declare your class @@ -842,3 +893,15 @@ See `pj_plugins/docs/dialog-plugin-guide.md` for the dialog protocol itself. - `pj_plugins/examples/mock_source_with_dialog.cpp` demonstrates the DataSource-owned dialog pattern: a combined `.so` with two vtables, shared state via member ownership, and dialog read-only accessors. + +## Common Mistakes + +| Symptom | Cause | Fix | +|---|---|---| +| Plugin loads but the host UI never shows progress | `progressStart()` not called before the loop, or progress bar finished early | Call `progressStart(label, total, cancellable)` once before the work loop; let `FileSourceBase` call `progressFinish()` for you | +| Host crashes or terminates partway through ingest | A plugin method threw an exception across the C ABI | Catch within the override and convert to `PJ::unexpected("…")`; the SDK trampoline only catches as a last resort | +| Streaming source drops messages under load | `recv()` is called inline in `onPoll()` at the host's polling rate | Spawn a background recv thread, buffer under a mutex, drain in `onPoll()` | +| `appendArrowStream()` succeeds but the program crashes at scope exit | Stream released twice — once by the host, once by the plugin's RAII | Use `PJ::sdk::ArrowStreamHolder` and pass via `std::move(stream)`; the holder disarms on success | +| Plugin restarts headlessly but loses configuration | State stored only in the dialog or only in `QSettings`, not in `saveConfig()` JSON | Round-trip every field through `saveConfig()` / `loadConfig()`; the host does not provide ambient persistence | +| Host emits "DataSource protocol version mismatch" at load | Plugin built against an older or newer `pj_base` than the host | Rebuild against the host's `pj_base`; check `PJ_DATA_SOURCE_PROTOCOL_VERSION` | +| `kCapabilitySupportsPause` declared but pause does nothing | `pause()`/`resume()` not overridden — `StreamSourceBase` does not wire them | Subclass `DataSourcePluginBase` directly, or override `pause()`/`resume()` and add the capability flag | diff --git a/pj_plugins/docs/dialog-plugin-guide.md b/pj_plugins/docs/dialog-plugin-guide.md index 6849286..9d83676 100644 --- a/pj_plugins/docs/dialog-plugin-guide.md +++ b/pj_plugins/docs/dialog-plugin-guide.md @@ -7,6 +7,24 @@ > calls happen on the main (GUI) thread; see `ARCHITECTURE.md` for the > full thread-class contract. +> **Vocabulary used throughout this guide**: +> - `PJ::WidgetData` — JSON builder for outbound widget state (host reads). +> - `PJ::WidgetDataView` — read-only parsed view the host applies to widgets. +> - Object name — the `objectName` attribute on a Qt widget. This is the key +> that links code to UI and is required on every interactive widget. + +## Required UI Conventions + +The host renders your `.ui` XML through `QUiLoader`. Three conventions are +non-negotiable; misnaming will silently break the dialog at runtime with no +compile-time error. + +| Requirement | Why it matters | +|---|---| +| The `QDialogButtonBox` MUST be named exactly `buttonBox` (camelCase) | The host calls `findChild("buttonBox")` to wire OK/Cancel. Other names yield a dialog with no buttons. | +| The `QDialogButtonBox` MUST set the `standardButtons` property in the XML | Without it, the box instantiates with no buttons even when found by name. | +| Every interactive widget MUST have a unique `objectName` | All `WidgetData` setters and event handlers address widgets by name. | + ## What is a Dialog Plugin? A dialog plugin is a shared library (`.so` / `.dylib` / `.dll`) that drives a @@ -29,6 +47,36 @@ renders the widgets, and relays events to the plugin over the C vtable. A complete example lives at `pj_plugins/dialog_protocol/examples/mock_dialog.cpp`. +## Plugin Contract + +Follow these rules. Some are enforced by manifest validation or host UI wiring; +others prevent runtime failures that are silent at compile time. + +**MUST** +- Honour the [Required UI Conventions](#required-ui-conventions) above + (`buttonBox` naming, `standardButtons`, every interactive widget has an + `objectName`). +- Return `true` from an event handler iff plugin-internal state changed; the + host re-reads `widget_data()` only on `true`. Returning `true` always wastes + re-renders; returning `false` after a real change leaves the UI stale. +- Validate every `manifest()` JSON string at build time — the host rejects + manifests missing `id`, `name`, or `version`. +- When overriding either `onValueChanged` overload (int or double), add + `using PJ::DialogPluginTyped::onValueChanged;` in the class body. C++ + name hiding will otherwise drop the un-overridden overload silently. + +**MUST NOT** +- Throw exceptions across virtual overrides. The SDK trampolines catch them, + but the event is reported to the host as a generic failure. +- Block the GUI thread inside `widget_data()` or event handlers (no I/O, + no sleeps). Long work belongs in `onTick()` or a host-thread-friendly + background pattern. +- Use `QTextEdit` or model-based `QTableView` — the widget binding system + does not support them. Use `QPlainTextEdit` for plain text/code editing, + or `QLabel`, `QListWidget`, and `QTableWidget` for display/table cases. +- Retain the JSON string returned by `widget_data()` on the host side past + the next `widget_data()` call on the same dialog. + ## Step by Step ### 1. Declare your class @@ -580,3 +628,16 @@ lifetime. See `pj_plugins/docs/data-source-guide.md` for the full DataSource-side documentation of this pattern. + +## Common Mistakes + +| Symptom | Cause | Fix | +|---|---|---| +| Dialog window has no OK / Cancel buttons | `QDialogButtonBox` not named `buttonBox`, or `standardButtons` property missing in XML | Rename to `buttonBox`, add `QDialogButtonBox::Cancel\|QDialogButtonBox::Ok` | +| Overriding `onValueChanged(int)` silently disables the `double` version (or vice versa) | C++ name hiding | Add `using PJ::DialogPluginTyped::onValueChanged;` in the class body | +| UI does not update after an event | Event handler returned `false`, so the host did not re-read `widget_data()` | Return `true` whenever internal state changed | +| `setText`/`setValue`/etc. has no visible effect | Wrong `objectName`, or widget type not in the [Widget Reference Table](#widget-reference-table) | Match XML `objectName` exactly; replace `QTextEdit`/`QTableView` with supported widgets | +| File picker button does nothing | `setFilePicker(...)` not called in `widget_data()` for this `objectName` | Call it once per `widget_data()` so the host wires the click | +| Sub-dialog opens repeatedly on every refresh | `requestSubDialog()` left set in `widget_data()` after the request fires | Set a one-shot flag; clear it before calling `requestSubDialog()` | +| Dialog state lost on layout reload | `loadConfig()` never restored fields, or `saveConfig()` returned `"{}"` | Round-trip every field through `saveConfig()` / `loadConfig()` | +| Manifest missing `id`/`name`/`version` causes load failure | Host rejects manifests missing required string keys | Validate the manifest in unit tests; the SDK does not assert this for dialogs at build time | diff --git a/pj_plugins/docs/message-parser-guide.md b/pj_plugins/docs/message-parser-guide.md index 34ed10e..072535d 100644 --- a/pj_plugins/docs/message-parser-guide.md +++ b/pj_plugins/docs/message-parser-guide.md @@ -6,6 +6,29 @@ > committing to storage). For ABI evolution rules, error semantics, and > noexcept discipline see `ARCHITECTURE.md`. +> **Vocabulary used throughout this guide**: +> - `PJ::Status` — alias for `PJ::Expected`. Return `PJ::okStatus()` / +> `PJ::unexpected("reason")`. +> - `PJ::Span` — non-owning view over the payload bytes. +> - `PJ::Timestamp` — `int64_t` nanoseconds since the Unix epoch. The host +> provides one; the parser may override it from the payload (see +> "Embedded timestamp extraction"). + +## When MessageParser is the Wrong Choice + +The parser write surface is **per-record only** in v4. If your input is +naturally batch-shaped (Parquet, MCAP indices, Arrow IPC byte streams), prefer +a **DataSource** that calls `writeHost().appendArrowStream(...)` unless the +encoding is strongly parser-shaped. Writing a batch producer as a parser leaves +throughput on the table and forces awkward single-record loops. + +A MessageParser is the right choice when: +- Each message is independently decodable (JSON line, single Protobuf message, + ROS message, Influx line). +- You produce up to a few dozen named numeric fields per call. +- The encoding may be paired with multiple transports (a Protobuf parser used + by both an MQTT and a ZMQ source). + ## What is a MessageParser? A MessageParser plugin is a shared library (`.so` / `.dylib` / `.dll`) that @@ -28,6 +51,33 @@ the host, which routes them to the appropriate parser based on encoding name. A complete example lives at `pj_plugins/examples/mock_json_parser.cpp`. +## Plugin Contract + +Follow these rules. Some are enforced by manifest validation or SDK +trampolines; others prevent runtime parse failures. + +**MUST** +- Return `PJ::okStatus()` / `PJ::unexpected("reason")` from `parse()` and other + fallible methods. +- Honour the encoding(s) declared in the manifest's `"encoding"` array — the + host routes binding requests by encoding name. +- Leave the parser in a consistent state after a failed `parse()` so the next + `parse()` call still works (do not corrupt cached field handles or schemas). +- Keep `parse()` topic-agnostic — the write host is already topic-scoped at + bind time. `writeHost().ensureField("x")` becomes `/x` in the + datastore automatically. + +**MUST NOT** +- Throw exceptions across `parse()`/`bindSchema()`/`bind(registry)`/ + `loadConfig()`/`saveConfig()`. The SDK trampolines catch as a fallback, + but the host loses the chance to report a useful reason. +- Call any host method outside the host's callback thread. +- Cache the write host view across instances or threads — it is bound to one + parser instance for one topic. +- Assume `bindSchema()` is called for every encoding. JSON / text parsers may + never receive it. Only require schema for encodings that need it + (Protobuf, ROS, IDL). + ## Step by Step ### 1. Declare your class @@ -462,3 +512,15 @@ dispatch code. - `pj_base/tests/message_parser_plugin_base_test.cpp` — comprehensive test fixture exercising the full SDK surface: vtable generation, bind/parse round-trip, schema binding, config persistence, and exception safety. + +## Common Mistakes + +| Symptom | Cause | Fix | +|---|---|---| +| Plugin loads but `parse()` is never called | `encoding` array in the manifest does not match the encoding name the source requests | Match the binding request's encoding string exactly (case-sensitive) | +| Host cannot bind the parser at runtime | Manifest missing the `encoding` array entirely | Add `"encoding": ["…"]` (required for parsers) | +| First `parse()` after restart fails silently | Schema-derived state lost; `bindSchema()` was not called for this encoding because none is needed | Initialize lazily in `parse()` for schema-less encodings | +| `appendBoundRecord()` returns failure | Field handles obtained before `bind(registry)` completed (or reused after `destroy`) | Acquire field handles inside `parse()` or after `bind(registry)`; never cache them across instances | +| Throughput is one quarter of the source's input rate | Wrong family — the source emits batches but the parser decodes one record at a time | Move bulk decoding to a DataSource that calls `appendArrowStream()` | +| Embedded timestamp ignored even when configured | Parser uses the host-supplied `timestamp_ns` and never extracts from payload | Read the config flag in `loadConfig`; choose between extracted and host timestamps inside `parse()` | +| Memory grows unboundedly across `parse()` calls | Per-message state leaked (e.g. descriptor pools rebuilt per call) | Build schema-derived caches in `bindSchema()` or `loadConfig()`, not in `parse()` | diff --git a/pj_plugins/docs/toolbox-guide.md b/pj_plugins/docs/toolbox-guide.md index 7560cfd..dab2ee2 100644 --- a/pj_plugins/docs/toolbox-guide.md +++ b/pj_plugins/docs/toolbox-guide.md @@ -7,13 +7,27 @@ > `PJ::sdk::ArrowSchemaHolder` / `ArrowArrayHolder` for scope-bound > release. See `ARCHITECTURE.md` for the full ABI rules. +> **Vocabulary used throughout this guide**: +> - `PJ::Status` — alias for `PJ::Expected`. Return `PJ::okStatus()` or +> `PJ::unexpected("reason")`. +> - `PJ::sdk::ArrowSchemaHolder` / `ArrowArrayHolder` / `ArrowStreamHolder` — +> RAII wrappers from `pj_base/sdk/arrow.hpp` that release Arrow C Data +> Interface structs at scope exit. +> - "Catalog snapshot" — a read-only view of every data source, topic, and +> field present in the host at the moment of acquisition. + +> **Toolbox is the most powerful family.** It alone can read existing data, +> create new data sources, and write derived outputs. Treat that power with +> care — see [Plugin Contract](#plugin-contract) below for the rules and +> conventions the host expects you to follow. + ## What is a Toolbox? A Toolbox plugin is a shared library (`.so` / `.dylib` / `.dll`) that provides **stateful interactive tools** with full read+write access to host data. Unlike DataSource (write-only, streaming lifecycle) and MessageParser (headless, request/response), Toolbox plugins are long-lived, UI-driven, and may create -new data sources, transform existing data, or perform destructive updates. +new data sources or transform existing data into new outputs. Plugins link only against `pj_base` (no Qt, no host internals) and communicate through a stable C ABI. @@ -33,6 +47,36 @@ editor, custom data transforms. A complete example lives at `pj_plugins/examples/mock_toolbox.cpp`. +## Plugin Contract + +Follow these rules. The toolbox family has read+write+create permissions, so +the contract is broader than the other families. + +**MUST** +- Return `PJ::okStatus()` / `PJ::unexpected("reason")` from every fallible + method. +- Call `runtimeHost().notifyDataChanged()` after any successful write that the + user should see in the UI. Coalesce per logical operation, not per record — + one call per "I just produced a new series" is the right granularity. +- Wrap all `read_series_arrow` returns in `PJ::sdk::ArrowSchemaHolder` / + `ArrowArrayHolder` so the release callbacks fire on scope exit. +- Persist tool state in `saveConfig()` so a layout reload restores the same + view. The host has no ambient persistence. +- Only call host methods from the host's callback thread. Background work + must marshal back through the host thread to write data. + +**MUST NOT** +- Throw exceptions across virtual overrides. +- Hold an `ArrowSchema*` / `ArrowArray*` past the scope of the holders that + own them — the host may reuse the underlying buffers. +- Treat `catalogSnapshot()` as live data. Snapshots are immutable views at + acquisition time; reacquire after writes if you need to see your own + changes. +- Create ambiguous output sources whose names collide with existing user data + unless the user explicitly chose that name. Duplicate names create distinct + datasets, but they are confusing in the UI; check the catalog and pick a + unique derived-data name by default. + ## Step by Step ### 1. Declare your class @@ -160,7 +204,7 @@ Access via `runtimeHost()`. Use this for diagnostics and UI refresh. | Method | Purpose | |---|---| | `reportMessage(level, text)` | Send info/warning/error to the host UI log. | -| `notifyDataChanged()` | Tell the host that data was modified; refresh UI. | +| `notifyDataChanged()` | Tell the host that data was modified; refresh UI. Idempotent and cheap; coalesce per logical operation, not per record. | ### Reading a series via Arrow @@ -318,3 +362,15 @@ row-of-fields shape. See - `pj_plugins/tests/toolbox_plugin_test.cpp` — end-to-end host-side test using `PJ::ToolboxTestStore` (in `pj_plugins/include/pj_plugins/testing/`) to drive a toolbox plugin through ingest, transform, and config scenarios. + +## Common Mistakes + +| Symptom | Cause | Fix | +|---|---|---| +| New series do not appear in the host UI after a write | `notifyDataChanged()` was never called | Call it once after each logical write batch | +| `read_series_arrow` succeeds but later code crashes accessing the data | `ArrowSchema` / `ArrowArray` released early, or held by raw pointer past holder scope | Use `PJ::sdk::ArrowSchemaHolder` / `ArrowArrayHolder` and keep them alive while the data is read | +| Catalog reads stale data immediately after writing | `catalogSnapshot()` was acquired before the write | Reacquire the snapshot after `notifyDataChanged()` | +| Duplicate source names appear in the UI | `createDataSource(name)` always creates a new dataset, even when the display name already exists | Check the catalog first; pick a unique derived-data name or surface a confirmation in the dialog | +| Plugin works in tests but crashes in the host | Host method called from a thread the toolbox spawned | Marshal back to the host thread (use the dialog's `onTick` or a host-thread queue) | +| Bulk transform output is one row at a time | Output written record-by-record instead of via Arrow | Build an `ArrowArrayStream` and use `appendArrowStream()` for the output | +| Plugin restarts but the tool's view is empty | Config not persisted | Round-trip every UI-relevant field through `saveConfig()` / `loadConfig()` | From 22f024a901dc9de40a7ee5ef4ef31554c9158e4e Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 12:38:56 +0200 Subject: [PATCH 04/10] fix: avoid dialog instantiation during catalog scans Add a static manifest tail slot to dialog vtables and teach catalog discovery to use it when present, while preserving the legacy create/get_manifest fallback for existing plugins. Update SDK examples and docs for the two-argument dialog export, cover the static-manifest path in tests, and simplify redundant unexpected(std::string(...)) error construction. --- .../tests/data_source_plugin_base_test.cpp | 10 +-- pj_base/tests/expected_test.cpp | 6 +- .../tests/message_parser_plugin_base_test.cpp | 2 +- pj_datastore/src/arrow_import.cpp | 24 +++--- pj_datastore/src/derived_engine.cpp | 8 +- pj_datastore/src/writer.cpp | 6 +- pj_plugins/CMakeLists.txt | 11 ++- .../dialog_protocol/examples/mock_dialog.cpp | 16 ++-- .../include/pj_plugins/dialog_protocol.h | 17 +++++ .../pj_plugins/sdk/dialog_plugin_base.hpp | 30 +++++--- .../dialog_protocol/src/dialog_library.cpp | 17 +++-- .../tests/plugin_lifecycle_test.cpp | 15 ++++ pj_plugins/docs/ARCHITECTURE.md | 10 ++- pj_plugins/docs/data-source-guide.md | 9 ++- pj_plugins/docs/dialog-plugin-guide.md | 36 +++++---- pj_plugins/docs/message-parser-guide.md | 5 +- pj_plugins/docs/toolbox-guide.md | 2 +- pj_plugins/examples/mock_data_source.cpp | 10 +-- pj_plugins/examples/mock_file_source.cpp | 6 +- pj_plugins/examples/mock_json_parser.cpp | 2 +- pj_plugins/examples/mock_schema_parser.cpp | 6 +- .../examples/mock_source_with_dialog.cpp | 17 +++-- .../pj_plugins/host/config_envelope.hpp | 8 +- .../pj_plugins/host/plugin_catalog.hpp | 8 +- .../host/service_registry_builder.hpp | 7 +- pj_plugins/src/data_source_library.cpp | 14 ++-- pj_plugins/src/detail/library_loader.hpp | 15 ++-- pj_plugins/src/detail/vtable_validation.hpp | 6 +- pj_plugins/src/message_parser_library.cpp | 14 ++-- pj_plugins/src/plugin_catalog.cpp | 61 +++++++++------- pj_plugins/src/toolbox_library.cpp | 14 ++-- pj_plugins/tests/plugin_catalog_test.cpp | 8 ++ .../tests/static_manifest_dialog_plugin.cpp | 73 +++++++++++++++++++ .../src/scene_decoder_protobuf.cpp | 18 ++--- 34 files changed, 337 insertions(+), 174 deletions(-) create mode 100644 pj_plugins/tests/static_manifest_dialog_plugin.cpp diff --git a/pj_base/tests/data_source_plugin_base_test.cpp b/pj_base/tests/data_source_plugin_base_test.cpp index 75422d2..1d40f6d 100644 --- a/pj_base/tests/data_source_plugin_base_test.cpp +++ b/pj_base/tests/data_source_plugin_base_test.cpp @@ -34,11 +34,11 @@ class MockDataSource : public PJ::DataSourcePluginBase { PJ::Status start() override { if (!writeHostBound()) { state_ = PJ::DataSourceState::kFailed; - return PJ::unexpected(std::string("write host not bound")); + return PJ::unexpected("write host not bound"); } if (!runtimeHostBound()) { state_ = PJ::DataSourceState::kFailed; - return PJ::unexpected(std::string("runtime host not bound")); + return PJ::unexpected("runtime host not bound"); } state_ = PJ::DataSourceState::kStarting; @@ -52,7 +52,7 @@ class MockDataSource : public PJ::DataSourcePluginBase { runtimeHost().progressFinish(); state_ = PJ::DataSourceState::kFailed; runtimeHost().notifyState(state_); - return PJ::unexpected(std::string("progress canceled")); + return PJ::unexpected("progress canceled"); } } runtimeHost().progressFinish(); @@ -111,7 +111,7 @@ class MockDataSource : public PJ::DataSourcePluginBase { PJ::Status pause() override { if (state_ != PJ::DataSourceState::kRunning) { - return PJ::unexpected(std::string("pause requires running state")); + return PJ::unexpected("pause requires running state"); } state_ = PJ::DataSourceState::kPaused; runtimeHost().notifyState(state_); @@ -120,7 +120,7 @@ class MockDataSource : public PJ::DataSourcePluginBase { PJ::Status resume() override { if (state_ != PJ::DataSourceState::kPaused) { - return PJ::unexpected(std::string("resume requires paused state")); + return PJ::unexpected("resume requires paused state"); } state_ = PJ::DataSourceState::kRunning; runtimeHost().notifyState(state_); diff --git a/pj_base/tests/expected_test.cpp b/pj_base/tests/expected_test.cpp index 0c6ea96..c9b80c3 100644 --- a/pj_base/tests/expected_test.cpp +++ b/pj_base/tests/expected_test.cpp @@ -16,7 +16,7 @@ TEST(ExpectedTest, HoldsValue) { } TEST(ExpectedTest, HoldsError) { - Expected result = unexpected(std::string("boom")); + Expected result = unexpected("boom"); ASSERT_FALSE(result.has_value()); EXPECT_FALSE(static_cast(result)); @@ -32,7 +32,7 @@ TEST(ExpectedTest, MutableAccessToValue) { } TEST(ExpectedTest, MutableAccessToError) { - Expected result = unexpected(std::string("err")); + Expected result = unexpected("err"); ASSERT_FALSE(result.has_value()); result.error().append("or"); @@ -44,7 +44,7 @@ TEST(ExpectedTest, AllowsValueAndErrorToUseSameType) { ASSERT_TRUE(value_result.has_value()); EXPECT_EQ(value_result.value(), "value"); - Expected error_result = unexpected(std::string("error")); + Expected error_result = unexpected("error"); ASSERT_FALSE(error_result.has_value()); EXPECT_EQ(error_result.error(), "error"); } diff --git a/pj_base/tests/message_parser_plugin_base_test.cpp b/pj_base/tests/message_parser_plugin_base_test.cpp index 2e5f43d..49dc4cc 100644 --- a/pj_base/tests/message_parser_plugin_base_test.cpp +++ b/pj_base/tests/message_parser_plugin_base_test.cpp @@ -35,7 +35,7 @@ class MockParser : public PJ::MessageParserPluginBase { PJ::Status parse(PJ::Timestamp timestamp_ns, PJ::Span payload) override { if (!writeHostBound()) { - return PJ::unexpected(std::string("write host not bound")); + return PJ::unexpected("write host not bound"); } // Parse payload as a text-encoded double std::string text(reinterpret_cast(payload.data()), payload.size()); diff --git a/pj_datastore/src/arrow_import.cpp b/pj_datastore/src/arrow_import.cpp index 163f6fb..137a946 100644 --- a/pj_datastore/src/arrow_import.cpp +++ b/pj_datastore/src/arrow_import.cpp @@ -293,7 +293,7 @@ PJ::Expected, std::vector(schema), nullptr); if (rc != NANOARROW_OK) { - return PJ::unexpected(std::string("Failed to initialize ArrowArrayView from schema")); + return PJ::unexpected("Failed to initialize ArrowArrayView from schema"); } nanoarrow::UniqueArray batch; @@ -319,7 +319,7 @@ PJ::Status ingestBatchesFromStream( rc = stream->get_next(stream, batch.get()); if (rc != NANOARROW_OK) { const char* err = stream->get_last_error != nullptr ? stream->get_last_error(stream) : nullptr; - return PJ::unexpected(std::string("Failed to read next batch: ") + (err != nullptr ? err : "unknown")); + return PJ::unexpected(fmt::format("Failed to read next batch: {}", err != nullptr ? err : "unknown")); } if (batch->release == nullptr) { break; // end of stream @@ -332,7 +332,7 @@ PJ::Status ingestBatchesFromStream( rc = ArrowArrayViewSetArray(array_view.get(), batch.get(), nullptr); if (rc != NANOARROW_OK) { - return PJ::unexpected(std::string("Failed to set array on ArrowArrayView")); + return PJ::unexpected("Failed to set array on ArrowArrayView"); } std::vector timestamps; @@ -385,13 +385,13 @@ PJ::Expected, std::vectorget_schema(stream.get(), schema.get()); if (rc != NANOARROW_OK) { - return PJ::unexpected(std::string("Failed to read schema from IPC stream")); + return PJ::unexpected("Failed to read schema from IPC stream"); } return mappingsFromSchema(schema.get()); @@ -404,14 +404,14 @@ PJ::Expected, std::vector, std::vector>> schemaFromArrowStream( ArrowArrayStream* stream) { if (stream == nullptr || stream->get_schema == nullptr) { - return PJ::unexpected(std::string("null ArrowArrayStream or missing get_schema")); + return PJ::unexpected("null ArrowArrayStream or missing get_schema"); } nanoarrow::UniqueSchema schema; const int rc = stream->get_schema(stream, schema.get()); if (rc != NANOARROW_OK) { const char* err = stream->get_last_error != nullptr ? stream->get_last_error(stream) : nullptr; - return PJ::unexpected(std::string("Failed to read schema from ArrowArrayStream: ") + (err != nullptr ? err : "")); + return PJ::unexpected(fmt::format("Failed to read schema from ArrowArrayStream: {}", err != nullptr ? err : "")); } return mappingsFromSchema(schema.get()); @@ -430,13 +430,13 @@ PJ::Status importIpcStream( nanoarrow::UniqueArrayStream stream; int rc = ArrowIpcArrayStreamReaderInit(stream.get(), &input, nullptr); if (rc != NANOARROW_OK) { - return PJ::unexpected(std::string("Failed to initialize IPC stream reader")); + return PJ::unexpected("Failed to initialize IPC stream reader"); } nanoarrow::UniqueSchema schema; rc = stream->get_schema(stream.get(), schema.get()); if (rc != NANOARROW_OK) { - return PJ::unexpected(std::string("Failed to read schema from IPC stream")); + return PJ::unexpected("Failed to read schema from IPC stream"); } return ingestBatchesFromStream(writer, topic_id, stream.get(), schema.get(), mappings, timestamp_column); @@ -450,14 +450,14 @@ PJ::Status importArrowStream( DataWriter& writer, TopicId topic_id, ArrowArrayStream* stream, const std::vector& mappings, int timestamp_column) { if (stream == nullptr || stream->get_schema == nullptr || stream->get_next == nullptr) { - return PJ::unexpected(std::string("null ArrowArrayStream or missing callbacks")); + return PJ::unexpected("null ArrowArrayStream or missing callbacks"); } nanoarrow::UniqueSchema schema; int rc = stream->get_schema(stream, schema.get()); if (rc != NANOARROW_OK) { const char* err = stream->get_last_error != nullptr ? stream->get_last_error(stream) : nullptr; - return PJ::unexpected(std::string("Failed to read schema from ArrowArrayStream: ") + (err != nullptr ? err : "")); + return PJ::unexpected(fmt::format("Failed to read schema from ArrowArrayStream: {}", err != nullptr ? err : "")); } return ingestBatchesFromStream(writer, topic_id, stream, schema.get(), mappings, timestamp_column); diff --git a/pj_datastore/src/derived_engine.cpp b/pj_datastore/src/derived_engine.cpp index 89eaa0d..96d5b07 100644 --- a/pj_datastore/src/derived_engine.cpp +++ b/pj_datastore/src/derived_engine.cpp @@ -336,7 +336,7 @@ PJ::Expected DerivedEngine::addSisoTransform( fmt::format("add_siso_transform: SISO requires single-column input, got {} columns", num_cols)); } if (!leaf_primitive) { - return PJ::unexpected(std::string("add_siso_transform: could not determine leaf primitive type")); + return PJ::unexpected("add_siso_transform: could not determine leaf primitive type"); } StorageKind in_kind = storageKindOf(*leaf_primitive); @@ -411,13 +411,13 @@ PJ::Expected DerivedEngine::addMimoTransform( std::vector input_topic_ids, std::vector output_topic_names, PJ::DatasetId output_dataset_id, std::unique_ptr op) { if (input_topic_ids.empty()) { - return PJ::unexpected(std::string("add_mimo_transform: requires at least one input topic")); + return PJ::unexpected("add_mimo_transform: requires at least one input topic"); } if (output_topic_names.empty()) { - return PJ::unexpected(std::string("add_mimo_transform: requires at least one output topic name")); + return PJ::unexpected("add_mimo_transform: requires at least one output topic name"); } if (!op) { - return PJ::unexpected(std::string("add_mimo_transform: null transform op")); + return PJ::unexpected("add_mimo_transform: null transform op"); } // 1. Validate all inputs and determine their StorageKinds. diff --git a/pj_datastore/src/writer.cpp b/pj_datastore/src/writer.cpp index 7440600..52ae1bb 100644 --- a/pj_datastore/src/writer.cpp +++ b/pj_datastore/src/writer.cpp @@ -214,7 +214,7 @@ Expected DataWriter::registerTopic(DatasetId dataset_id, TopicDescripto Expected DataWriter::bindTopicWriter(TopicId topic_id) { const auto* storage = impl_->engine.getTopicStorage(topic_id); if (storage == nullptr) { - return PJ::unexpected("Topic " + std::to_string(topic_id) + " not found"); + return PJ::unexpected(fmt::format("Topic {} not found", topic_id)); } // Ensure column descriptors are cached @@ -242,7 +242,7 @@ Expected DataWriter::resolveField(TopicId topic_id, std::string_view fi auto col_it = impl_->topic_columns.find(topic_id); if (col_it == impl_->topic_columns.end()) { - return PJ::unexpected("Topic " + std::to_string(topic_id) + " not found"); + return PJ::unexpected(fmt::format("Topic {} not found", topic_id)); } for (const auto& col : col_it->second) { @@ -250,7 +250,7 @@ Expected DataWriter::resolveField(TopicId topic_id, std::string_view fi return col.field_id; } } - return PJ::unexpected("Field '" + std::string(field_path) + "' not found in topic " + std::to_string(topic_id)); + return PJ::unexpected(fmt::format("Field '{}' not found in topic {}", field_path, topic_id)); } // --------------------------------------------------------------------------- diff --git a/pj_plugins/CMakeLists.txt b/pj_plugins/CMakeLists.txt index a24b8e6..7469240 100644 --- a/pj_plugins/CMakeLists.txt +++ b/pj_plugins/CMakeLists.txt @@ -4,6 +4,7 @@ add_subdirectory(dialog_protocol) # pj_plugin_catalog — host-side embedded-manifest discovery # --------------------------------------------------------------------------- +find_package(fmt REQUIRED) find_package(nlohmann_json REQUIRED) add_library(pj_plugin_catalog STATIC @@ -17,6 +18,7 @@ target_link_libraries(pj_plugin_catalog pj_base PRIVATE pj_dialog_protocol + fmt::fmt ${CMAKE_DL_LIBS} nlohmann_json::nlohmann_json ) @@ -181,6 +183,11 @@ target_compile_features(mock_toolbox_plugin PRIVATE cxx_std_20) target_compile_options(mock_toolbox_plugin PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(mock_toolbox_plugin PRIVATE pj_base) +add_library(static_manifest_dialog_plugin SHARED tests/static_manifest_dialog_plugin.cpp) +target_compile_features(static_manifest_dialog_plugin PRIVATE cxx_std_20) +target_compile_options(static_manifest_dialog_plugin PRIVATE ${PJ_WARNING_FLAGS}) +target_link_libraries(static_manifest_dialog_plugin PRIVATE pj_dialog_protocol) + # --------------------------------------------------------------------------- # Tests # --------------------------------------------------------------------------- @@ -269,6 +276,7 @@ target_compile_definitions(plugin_catalog_test PRIVATE PJ_MOCK_JSON_PARSER_PLUGIN_PATH="$" PJ_MOCK_TOOLBOX_PLUGIN_PATH="$" PJ_MOCK_DIALOG_PLUGIN_PATH="$" + PJ_STATIC_MANIFEST_DIALOG_PLUGIN_PATH="$" PJ_MISSING_ID_PLUGIN_PATH="$" PJ_INVALID_OPTIONAL_PLUGIN_PATH="$" PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH="$" @@ -279,7 +287,8 @@ target_link_libraries(plugin_catalog_test PRIVATE ) add_dependencies(plugin_catalog_test mock_data_source_plugin mock_json_parser_plugin mock_toolbox_plugin mock_dialog_plugin missing_id_data_source_plugin - invalid_optional_manifest_data_source_plugin missing_required_slots_plugin) + invalid_optional_manifest_data_source_plugin missing_required_slots_plugin + static_manifest_dialog_plugin) add_test(NAME plugin_catalog_test COMMAND plugin_catalog_test) endif() # PJ_BUILD_TESTS diff --git a/pj_plugins/dialog_protocol/examples/mock_dialog.cpp b/pj_plugins/dialog_protocol/examples/mock_dialog.cpp index d4de98f..0c52dbf 100644 --- a/pj_plugins/dialog_protocol/examples/mock_dialog.cpp +++ b/pj_plugins/dialog_protocol/examples/mock_dialog.cpp @@ -4,6 +4,13 @@ namespace { +constexpr const char* kManifestJson = R"({ + "id": "mock-dialog", + "name": "Mock Dialog", + "version": "1.0.0", + "description": "A minimal dialog plugin for testing" +})"; + constexpr const char* kUiContent = R"( MockDialog @@ -57,12 +64,7 @@ class MockDialog : public PJ::DialogPluginTyped { public: std::string manifest() const override { - return R"({ - "id": "mock-dialog", - "name": "Mock Dialog", - "version": "1.0.0", - "description": "A minimal dialog plugin for testing" - })"; + return kManifestJson; } std::string ui_content() const override { @@ -135,4 +137,4 @@ class MockDialog : public PJ::DialogPluginTyped { bool verbose_ = false; }; -PJ_DIALOG_PLUGIN(MockDialog) +PJ_DIALOG_PLUGIN(MockDialog, kManifestJson) diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h b/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h index ad3e670..d2bc089 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h +++ b/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h @@ -3,6 +3,7 @@ #include #include +#include #include "pj_base/plugin_data_api.h" @@ -12,6 +13,15 @@ extern "C" { #define PJ_DIALOG_PROTOCOL_VERSION 4 +/* + * Minimum vtable size for v4.0 compatibility, pinned at v4.0 release. + * Loaders reject plugins whose `struct_size < PJ_DIALOG_MIN_VTABLE_SIZE`. + * New slots may be appended at the tail without increasing this floor; + * host reads of appended slots must be gated with PJ_HAS_TAIL_SLOT. + */ +#define PJ_DIALOG_MIN_VTABLE_SIZE \ + (offsetof(PJ_dialog_vtable_t, load_config) + sizeof(bool (*)(void*, PJ_string_view_t, PJ_error_t*))) + /* Export macro for plugin shared libraries */ #if defined(_WIN32) #define PJ_DIALOG_EXPORT __declspec(dllexport) @@ -63,7 +73,14 @@ typedef struct PJ_dialog_vtable_t { /* [main-thread] Configuration round-trip. */ bool (*save_config)(void* ctx, PJ_string_view_t* out_json, PJ_error_t* out_error) PJ_NOEXCEPT; bool (*load_config)(void* ctx, PJ_string_view_t config_json, PJ_error_t* out_error) PJ_NOEXCEPT; + + /* [metadata] Optional static JSON manifest for metadata-only catalog + * discovery. When present, the host reads this instead of instantiating + * the dialog during scans. */ + const char* manifest_json; } PJ_dialog_vtable_t; +/* The vtable above is ABI-APPENDABLE: new slots may be added at the tail; + * host reads guard with PJ_HAS_TAIL_SLOT. See PJ_DIALOG_MIN_VTABLE_SIZE. */ /* * Every dialog plugin exports this symbol. diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp index 7ba1c05..d9832a6 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp @@ -49,13 +49,13 @@ class DialogPluginBase { } template - static const PJ_dialog_vtable_t* vtableWithCreate(CreateFn create_fn) { + static const PJ_dialog_vtable_t* vtableWithCreate(CreateFn create_fn, const char* manifest_json = nullptr) { static const PJ_dialog_vtable_t vt = { PJ_DIALOG_PROTOCOL_VERSION, sizeof(PJ_dialog_vtable_t), create_fn, trampoline_destroy, trampoline_get_manifest, trampoline_get_ui_content, trampoline_get_widget_data, trampoline_on_widget_event, trampoline_on_tick, trampoline_on_accepted, trampoline_on_rejected, trampoline_save_config, - trampoline_load_config, + trampoline_load_config, manifest_json, }; return &vt; } @@ -243,6 +243,10 @@ PJ_borrowed_dialog_t borrowDialog(DialogT& dialog) noexcept { /// another family (DataSource, MessageParser, Toolbox). The shared /// `pj_plugin_abi_version` export uses weak linkage, so duplicate definitions /// across family macros in the same DSO collapse into one COMDAT entry. +/// Prefer `PJ_DIALOG_PLUGIN(ClassName, manifest_json_literal)` for new +/// plugins so catalog scans can read metadata without instantiating the +/// dialog. The one-argument form is kept for source compatibility and leaves +/// the optional static manifest tail slot null. /// /// Emits three things: /// 1. The boot-level `pj_plugin_abi_version` symbol the host loader checks @@ -252,16 +256,22 @@ PJ_borrowed_dialog_t borrowDialog(DialogT& dialog) noexcept { /// other plugin code (notably a host's `getDialog()` override) obtain /// the vtable pointer type-safely via `PJ::borrowDialog(member)` — /// no `extern "C"` forward declaration required in the plugin source. -#define PJ_DIALOG_PLUGIN(ClassName) \ +#define PJ_DIALOG_PLUGIN_SELECT(_1, _2, NAME, ...) NAME +#define PJ_DIALOG_PLUGIN(...) \ + PJ_DIALOG_PLUGIN_SELECT(__VA_ARGS__, PJ_DIALOG_PLUGIN_WITH_MANIFEST, PJ_DIALOG_PLUGIN_LEGACY)(__VA_ARGS__) +#define PJ_DIALOG_PLUGIN_LEGACY(ClassName) PJ_DIALOG_PLUGIN_WITH_MANIFEST(ClassName, nullptr) +#define PJ_DIALOG_PLUGIN_WITH_MANIFEST(ClassName, ManifestJson) \ PJ_EXPORT_PLUGIN_ABI_VERSION(PJ_DIALOG_EXPORT) \ extern "C" PJ_DIALOG_EXPORT const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept { \ - static const PJ_dialog_vtable_t* vt = PJ::DialogPluginBase::vtableWithCreate([]() noexcept -> void* { \ - try { \ - return new ClassName(); \ - } catch (...) { \ - return nullptr; \ - } \ - }); \ + static const PJ_dialog_vtable_t* vt = PJ::DialogPluginBase::vtableWithCreate( \ + []() noexcept -> void* { \ + try { \ + return new ClassName(); \ + } catch (...) { \ + return nullptr; \ + } \ + }, \ + ManifestJson); \ return vt; \ } \ namespace PJ { \ diff --git a/pj_plugins/dialog_protocol/src/dialog_library.cpp b/pj_plugins/dialog_protocol/src/dialog_library.cpp index 967570e..7fc512e 100644 --- a/pj_plugins/dialog_protocol/src/dialog_library.cpp +++ b/pj_plugins/dialog_protocol/src/dialog_library.cpp @@ -15,13 +15,14 @@ Expected loadLibraryHandle(std::string_view path) { #if defined(_WIN32) HMODULE module = LoadLibraryA(std::string(path).c_str()); if (module == nullptr) { - return unexpected(std::string("LoadLibraryA failed")); + return unexpected("LoadLibraryA failed"); } return reinterpret_cast(module); #else void* handle = dlopen(std::string(path).c_str(), RTLD_NOW | RTLD_LOCAL); if (handle == nullptr) { - return unexpected(std::string(dlerror())); + const char* error = dlerror(); + return unexpected(error == nullptr ? "" : error); } return handle; #endif @@ -46,7 +47,7 @@ Expected loadEntryPoint(void* handle) { #if defined(_WIN32) auto symbol = GetProcAddress(reinterpret_cast(handle), "PJ_get_dialog_vtable"); if (symbol == nullptr) { - return unexpected(std::string("PJ_get_dialog_vtable not found")); + return unexpected("PJ_get_dialog_vtable not found"); } return reinterpret_cast(symbol); #else @@ -54,7 +55,7 @@ Expected loadEntryPoint(void* handle) { void* symbol = dlsym(handle, "PJ_get_dialog_vtable"); const char* err = dlerror(); if (err != nullptr) { - return unexpected(std::string(err)); + return unexpected(err); } return reinterpret_cast(symbol); #endif @@ -99,13 +100,13 @@ Expected DialogLibrary::load(std::string_view path) { const PJ_dialog_vtable_t* vtable = (*entry)(); if (vtable == nullptr) { - return unexpected(std::string("PJ_get_dialog_vtable returned null")); + return unexpected("PJ_get_dialog_vtable returned null"); } if (vtable->protocol_version != PJ_DIALOG_PROTOCOL_VERSION) { - return unexpected(std::string("Dialog protocol version mismatch")); + return unexpected("Dialog protocol version mismatch"); } - if (vtable->struct_size < sizeof(PJ_dialog_vtable_t)) { - return unexpected(std::string("Dialog vtable is smaller than expected")); + if (vtable->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { + return unexpected("Dialog vtable smaller than v4.0 baseline"); } return DialogLibrary(std::move(handle), vtable, std::string(path)); diff --git a/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp b/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp index 6c638d4..408b717 100644 --- a/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp +++ b/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -9,6 +10,11 @@ // Defined in mock_dialog.cpp, linked statically extern "C" const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept; +static_assert(offsetof(PJ_dialog_vtable_t, load_config) == 88, "v4 dialog baseline slot pinned"); +static_assert(PJ_DIALOG_MIN_VTABLE_SIZE == 96, "Dialog MIN vtable size is pinned at v4.0"); +static_assert(offsetof(PJ_dialog_vtable_t, manifest_json) == 96, "Dialog static manifest tail slot appended"); +static_assert(sizeof(PJ_dialog_vtable_t) == 104, "Dialog vtable size updated deliberately on append"); + class PluginLifecycleTest : public ::testing::Test { protected: void SetUp() override { @@ -37,6 +43,7 @@ TEST_F(PluginLifecycleTest, ProtocolVersion) { TEST_F(PluginLifecycleTest, StructSize) { EXPECT_EQ(vt_->struct_size, sizeof(PJ_dialog_vtable_t)); + EXPECT_EQ(PJ_DIALOG_MIN_VTABLE_SIZE, offsetof(PJ_dialog_vtable_t, manifest_json)); } TEST_F(PluginLifecycleTest, AllFunctionPointersNonNull) { @@ -51,6 +58,8 @@ TEST_F(PluginLifecycleTest, AllFunctionPointersNonNull) { EXPECT_NE(vt_->on_rejected, nullptr); EXPECT_NE(vt_->save_config, nullptr); EXPECT_NE(vt_->load_config, nullptr); + ASSERT_TRUE(PJ_HAS_TAIL_SLOT(PJ_dialog_vtable_t, vt_, manifest_json)); + EXPECT_NE(vt_->manifest_json, nullptr); } // --- Manifest --- @@ -64,6 +73,12 @@ TEST_F(PluginLifecycleTest, ManifestIsValidJson) { EXPECT_TRUE(j.contains("version")); } +TEST_F(PluginLifecycleTest, StaticManifestMatchesRuntimeManifest) { + ASSERT_TRUE(PJ_HAS_TAIL_SLOT(PJ_dialog_vtable_t, vt_, manifest_json)); + ASSERT_NE(vt_->manifest_json, nullptr); + EXPECT_EQ(nlohmann::json::parse(vt_->manifest_json), nlohmann::json::parse(vt_->get_manifest(ctx_))); +} + TEST_F(PluginLifecycleTest, ManifestPointerStable) { const char* p1 = vt_->get_manifest(ctx_); const char* p2 = vt_->get_manifest(ctx_); diff --git a/pj_plugins/docs/ARCHITECTURE.md b/pj_plugins/docs/ARCHITECTURE.md index 6d71896..223d009 100644 --- a/pj_plugins/docs/ARCHITECTURE.md +++ b/pj_plugins/docs/ARCHITECTURE.md @@ -122,7 +122,8 @@ previously-circulated pre-v4 design included): (reserved for a future `"pj.thread_check.v1"` service). - **Embedded-manifest plugin discovery.** Each DSO exports a family-specific protocol vtable with embedded metadata (`manifest_json` - for data sources, parsers, and toolboxes; `get_manifest()` for dialogs). + for data sources, parsers, toolboxes, and newly built dialogs; legacy v4.0 + dialogs fall back to `create()` + `get_manifest()` during inspection). Host-side `PJ::scanPluginDsos(dir)` (in `pj_plugins/host/plugin_catalog.hpp`) walks platform plugin libraries, loads each candidate, validates the ABI and protocol vtable, and parses @@ -161,7 +162,8 @@ service registry, error out-params, and typed borrowed-dialog patterns): `protocol_version, struct_size, create, destroy, manifest_json`; subsequent slots are family-specific. For example, DataSource and Toolbox have `capabilities`, while MessageParser has `bind_schema`. Dialogs expose a - GUI-oriented protocol with `get_manifest()`/`get_ui_content()` instead. + GUI-oriented protocol with `get_manifest()`/`get_ui_content()` and an + optional static `manifest_json` tail slot for metadata-only discovery. Service traits (`pj_base/sdk/service_traits.hpp`, `sdk/toolbox_plugin_base.hpp`) map canonical names to their ABI type and @@ -279,11 +281,11 @@ at load time. Mismatches produce a clear error. | DataSource (stream) | `StreamSourceBase` | `onStart()`, `onPoll()`, `onStop()`, `extraCapabilities()` | same macro | | MessageParser | `MessageParserPluginBase` | `parse()` | `PJ_MESSAGE_PARSER_PLUGIN(Class, manifest)` | | Toolbox | `ToolboxPluginBase` | `capabilities()` | `PJ_TOOLBOX_PLUGIN(Class, manifest)` | -| Dialog | `DialogPluginTyped` | `manifest()`, `ui_content()`, `widget_data()`, event handlers | `PJ_DIALOG_PLUGIN(Class)` (works standalone or co-resident with another family) | +| Dialog | `DialogPluginTyped` | `manifest()`, `ui_content()`, `widget_data()`, event handlers | `PJ_DIALOG_PLUGIN(Class, manifest)` (or legacy `PJ_DIALOG_PLUGIN(Class)`; works standalone or co-resident with another family) | All SDK base classes: - Generate the C vtable via `vtableWithCreate()` at static init. -- Validate compile-time manifest JSON string literals (required keys) via `PJ_ASSERT`; dialog manifests are runtime strings and are validated by the host catalog when inspected. +- Validate compile-time manifest JSON string literals (required keys) via `PJ_ASSERT`; dialog manifests supplied through the static macro path are parsed by the host catalog without instantiation, while legacy dialog manifests are validated through the fallback runtime path. - Catch all C++ exceptions in trampolines, populate `PJ_error_t` out-params when available, and return `false`/`null` across the ABI boundary. diff --git a/pj_plugins/docs/data-source-guide.md b/pj_plugins/docs/data-source-guide.md index 6cab725..f25082b 100644 --- a/pj_plugins/docs/data-source-guide.md +++ b/pj_plugins/docs/data-source-guide.md @@ -392,7 +392,7 @@ class UdpReceiver : public PJ::StreamSourceBase { std::string config_; std::string encoding_ = "json"; PJ::Expected binding_ = - PJ::unexpected(std::string("unset")); + PJ::unexpected("unset"); std::atomic running_{false}; std::thread recv_thread_; @@ -779,7 +779,8 @@ with no JSON serialization needed at runtime. │ getDialog() → borrowDialog(...) │ │ │ │ PJ_DATA_SOURCE_PLUGIN(MySource) │ → exports DataSource vtable -│ PJ_DIALOG_PLUGIN(MyDialog) │ → exports Dialog vtable +│ PJ_DIALOG_PLUGIN(MyDialog, │ → exports Dialog vtable +│ kManifestJson) │ └──────────────────────────────────────┘ ``` @@ -830,7 +831,7 @@ class MySource : public PJ::StreamSourceBase { std::string saveConfig() const override { return dialog_.saveConfig(); } PJ::Status loadConfig(std::string_view json) override { return dialog_.loadConfig(json) ? PJ::okStatus() - : PJ::unexpected(std::string("bad config")); + : PJ::unexpected("bad config"); } private: @@ -842,7 +843,7 @@ class MySource : public PJ::StreamSourceBase { ```cpp PJ_DATA_SOURCE_PLUGIN(MySource, R"({"id":"my-source","name":"My Source","version":"1.0.0"})") -PJ_DIALOG_PLUGIN(MyDialog) +PJ_DIALOG_PLUGIN(MyDialog, kManifestJson) ``` ### Host-side flow diff --git a/pj_plugins/docs/dialog-plugin-guide.md b/pj_plugins/docs/dialog-plugin-guide.md index 9d83676..a139c64 100644 --- a/pj_plugins/docs/dialog-plugin-guide.md +++ b/pj_plugins/docs/dialog-plugin-guide.md @@ -41,7 +41,7 @@ renders the widgets, and relays events to the plugin over the C vtable. 4. Implement `widget_data()` to push state to the UI. 5. Override the typed event handlers you need (`onTextChanged`, `onIndexChanged`, `onToggled`, etc.) — return `true` when state changes. -6. Export with `PJ_DIALOG_PLUGIN(YourClass)`. +6. Export with `PJ_DIALOG_PLUGIN(YourClass, kManifestJson)`. 7. Build as a shared library linking `pj_dialog_sdk`. A complete example lives at @@ -88,15 +88,17 @@ Subclass `DialogPluginTyped` and override `manifest()`, `ui_content()`, #include #include +constexpr const char* kManifestJson = R"({ + "id": "my-dialog", + "name": "My Dialog", + "version": "1.0.0", + "description": "Example dialog plugin" +})"; + class MyDialog : public PJ::DialogPluginTyped { public: std::string manifest() const override { - return R"({ - "id": "my-dialog", - "name": "My Dialog", - "version": "1.0.0", - "description": "Example dialog plugin" - })"; + return kManifestJson; } std::string ui_content() const override { @@ -259,11 +261,14 @@ All handlers default to returning `false`. Override only the ones you need. At file scope, after the class definition: ```cpp -PJ_DIALOG_PLUGIN(MyDialog) +PJ_DIALOG_PLUGIN(MyDialog, kManifestJson) ``` This generates the `extern "C"` entry point (`PJ_get_dialog_vtable`) that the -host resolves via dlsym/GetProcAddress. +host resolves via dlsym/GetProcAddress. Passing the manifest literal lets the +catalog read metadata without instantiating the dialog. The legacy +`PJ_DIALOG_PLUGIN(MyDialog)` form remains supported for existing source, but +catalog discovery must instantiate those dialogs to call `manifest()`. ### 6. Build @@ -276,9 +281,12 @@ No Qt dependency is needed in the plugin — only the host links Qt. ## Manifest Schema -`manifest()` returns a JSON string. Unlike the other plugin families, the -dialog manifest is built at runtime (not a string literal in the vtable), but -the same required keys apply. +`manifest()` returns the same JSON string supplied to `PJ_DIALOG_PLUGIN`. +New dialogs should pass a static manifest literal to the macro so catalog +discovery can inspect metadata without constructing the dialog. Legacy dialogs +that use `PJ_DIALOG_PLUGIN(MyDialog)` without a manifest still load, but the +catalog must instantiate them to call `manifest()`. The same required keys +apply in both forms. | Key | Type | Required | Description | |-----|------|----------|-------------| @@ -615,8 +623,8 @@ class MySource : public PJ::StreamSourceBase { }; PJ_DATA_SOURCE_PLUGIN(MySource, R"({"id":"my-source","name":"My Source","version":"1.0.0"})") -PJ_DIALOG_PLUGIN(MyDialog) // also specialises PJ::dialogVtableFor() - // so PJ::borrowDialog picks up the right vtable. +PJ_DIALOG_PLUGIN(MyDialog, kManifestJson) // also specialises PJ::dialogVtableFor() + // so PJ::borrowDialog picks up the right vtable. ``` The host resolves both vtables, creates a borrowed `DialogHandle` from the diff --git a/pj_plugins/docs/message-parser-guide.md b/pj_plugins/docs/message-parser-guide.md index 072535d..f64d7d7 100644 --- a/pj_plugins/docs/message-parser-guide.md +++ b/pj_plugins/docs/message-parser-guide.md @@ -103,7 +103,7 @@ Return `okStatus()` on success, or `unexpected("reason")` on failure. PJ::Status MyJsonParser::parse(PJ::Timestamp timestamp_ns, PJ::Span payload) { if (!writeHostBound()) { - return PJ::unexpected(std::string("write host not bound")); + return PJ::unexpected("write host not bound"); } // Decode payload bytes into field values. @@ -322,7 +322,8 @@ and none is needed. │ loadConfig(json) applies it │ │ │ │ PJ_MESSAGE_PARSER_PLUGIN(...) │ → exports parser vtable -│ PJ_DIALOG_PLUGIN(ProtoDialog) │ → exports dialog vtable +│ PJ_DIALOG_PLUGIN(ProtoDialog, │ → exports dialog vtable +│ kManifestJson) │ └──────────────────────────────────┘ ``` diff --git a/pj_plugins/docs/toolbox-guide.md b/pj_plugins/docs/toolbox-guide.md index dab2ee2..bacd4cd 100644 --- a/pj_plugins/docs/toolbox-guide.md +++ b/pj_plugins/docs/toolbox-guide.md @@ -41,7 +41,7 @@ editor, custom data transforms. acquiring services), `saveConfig()`, `loadConfig()`, `getDialog()` 3. Export with `PJ_TOOLBOX_PLUGIN(YourClass, R"({"id":"...","name":"...","version":"..."})")` 4. If you ship an embedded dialog, also declare it as a - `DialogPluginTyped` subclass and add `PJ_DIALOG_PLUGIN(YourDialog)` + `DialogPluginTyped` subclass and add `PJ_DIALOG_PLUGIN(YourDialog, kManifestJson)` 5. Build as a shared library linking `pj_base` (+ `pj_dialog_sdk` if you have a dialog) diff --git a/pj_plugins/examples/mock_data_source.cpp b/pj_plugins/examples/mock_data_source.cpp index bbc7bf0..b086f60 100644 --- a/pj_plugins/examples/mock_data_source.cpp +++ b/pj_plugins/examples/mock_data_source.cpp @@ -22,11 +22,11 @@ class MockDataSource : public PJ::DataSourcePluginBase { PJ::Status start() override { if (!writeHostBound()) { state_ = PJ::DataSourceState::kFailed; - return PJ::unexpected(std::string("write host not bound")); + return PJ::unexpected("write host not bound"); } if (!runtimeHostBound()) { state_ = PJ::DataSourceState::kFailed; - return PJ::unexpected(std::string("runtime host not bound")); + return PJ::unexpected("runtime host not bound"); } state_ = PJ::DataSourceState::kStarting; @@ -40,7 +40,7 @@ class MockDataSource : public PJ::DataSourcePluginBase { runtimeHost().progressFinish(); state_ = PJ::DataSourceState::kFailed; runtimeHost().notifyState(state_); - return PJ::unexpected(std::string("progress canceled")); + return PJ::unexpected("progress canceled"); } } runtimeHost().progressFinish(); @@ -99,7 +99,7 @@ class MockDataSource : public PJ::DataSourcePluginBase { PJ::Status pause() override { if (state_ != PJ::DataSourceState::kRunning) { - return PJ::unexpected(std::string("pause requires running state")); + return PJ::unexpected("pause requires running state"); } state_ = PJ::DataSourceState::kPaused; runtimeHost().notifyState(state_); @@ -108,7 +108,7 @@ class MockDataSource : public PJ::DataSourcePluginBase { PJ::Status resume() override { if (state_ != PJ::DataSourceState::kPaused) { - return PJ::unexpected(std::string("resume requires paused state")); + return PJ::unexpected("resume requires paused state"); } state_ = PJ::DataSourceState::kRunning; runtimeHost().notifyState(state_); diff --git a/pj_plugins/examples/mock_file_source.cpp b/pj_plugins/examples/mock_file_source.cpp index ebac0ab..ead7f5a 100644 --- a/pj_plugins/examples/mock_file_source.cpp +++ b/pj_plugins/examples/mock_file_source.cpp @@ -20,7 +20,7 @@ class MockFileSource : public PJ::FileSourceBase { PJ::Status importData() override { if (!runtimeHost().progressStart("Importing", 3, true)) { - return PJ::unexpected(std::string("progress unavailable")); + return PJ::unexpected("progress unavailable"); } auto topic = writeHost().ensureTopic("mock/file_data"); @@ -30,7 +30,7 @@ class MockFileSource : public PJ::FileSourceBase { for (uint64_t i = 1; i <= 3; ++i) { if (runtimeHost().isStopRequested()) { - return PJ::unexpected(std::string("import cancelled")); + return PJ::unexpected("import cancelled"); } auto status = writeHost().appendRecord( @@ -41,7 +41,7 @@ class MockFileSource : public PJ::FileSourceBase { } if (!runtimeHost().progressUpdate(i)) { - return PJ::unexpected(std::string("import cancelled via progress")); + return PJ::unexpected("import cancelled via progress"); } } diff --git a/pj_plugins/examples/mock_json_parser.cpp b/pj_plugins/examples/mock_json_parser.cpp index 84e0e60..d0ad299 100644 --- a/pj_plugins/examples/mock_json_parser.cpp +++ b/pj_plugins/examples/mock_json_parser.cpp @@ -11,7 +11,7 @@ class MockJsonParser : public PJ::MessageParserPluginBase { public: PJ::Status parse(PJ::Timestamp timestamp_ns, PJ::Span payload) override { if (!writeHostBound()) { - return PJ::unexpected(std::string("write host not bound")); + return PJ::unexpected("write host not bound"); } std::string text(reinterpret_cast(payload.data()), payload.size()); double value = std::strtod(text.c_str(), nullptr); diff --git a/pj_plugins/examples/mock_schema_parser.cpp b/pj_plugins/examples/mock_schema_parser.cpp index 1572f25..43ca041 100644 --- a/pj_plugins/examples/mock_schema_parser.cpp +++ b/pj_plugins/examples/mock_schema_parser.cpp @@ -43,14 +43,14 @@ class MockSchemaParser : public PJ::MessageParserPluginBase { } if (field_names_.size() != 2) { - return PJ::unexpected(std::string("schema must define exactly 2 fields")); + return PJ::unexpected("schema must define exactly 2 fields"); } return PJ::okStatus(); } PJ::Status parse(PJ::Timestamp timestamp_ns, PJ::Span payload) override { if (!writeHostBound()) { - return PJ::unexpected(std::string("write host not bound")); + return PJ::unexpected("write host not bound"); } // Lazily bind fields on first parse. @@ -65,7 +65,7 @@ class MockSchemaParser : public PJ::MessageParserPluginBase { std::string text(reinterpret_cast(payload.data()), payload.size()); auto comma = text.find(','); if (comma == std::string::npos) { - return PJ::unexpected(std::string("expected comma-separated pair")); + return PJ::unexpected("expected comma-separated pair"); } double a = std::strtod(text.c_str(), nullptr); diff --git a/pj_plugins/examples/mock_source_with_dialog.cpp b/pj_plugins/examples/mock_source_with_dialog.cpp index 23723d2..abe020f 100644 --- a/pj_plugins/examples/mock_source_with_dialog.cpp +++ b/pj_plugins/examples/mock_source_with_dialog.cpp @@ -6,6 +6,13 @@ namespace { +constexpr const char* kDialogManifestJson = R"({ + "id": "mock-streamer-dialog", + "name": "Mock Streamer", + "version": "1.0.0", + "description": "A mock streaming data source with dialog" +})"; + constexpr const char* kUiContent = R"( MockStreamerDialog @@ -135,11 +142,7 @@ class MockStreamerDialog : public PJ::DialogPluginTyped { // --- Dialog protocol implementation --- std::string manifest() const override { - return R"({ - "name": "Mock Streamer", - "version": "1.0.0", - "description": "A mock streaming data source with dialog" - })"; + return kDialogManifestJson; } std::string ui_content() const override { @@ -360,7 +363,7 @@ class MockStreamerSource : public PJ::StreamSourceBase { } PJ::Status loadConfig(std::string_view json) override { - return dialog_.loadConfig(json) ? PJ::okStatus() : PJ::unexpected(std::string("bad config")); + return dialog_.loadConfig(json) ? PJ::okStatus() : PJ::unexpected("bad config"); } private: @@ -373,4 +376,4 @@ PJ_DATA_SOURCE_PLUGIN( MockStreamerSource, R"({"id":"mock-streamer-source","name":"Mock Streamer Source","version":"1.0.0",)" R"("description":"Combined DataSource+Dialog mock for integration testing"})") -PJ_DIALOG_PLUGIN(MockStreamerDialog) +PJ_DIALOG_PLUGIN(MockStreamerDialog, kDialogManifestJson) diff --git a/pj_plugins/include/pj_plugins/host/config_envelope.hpp b/pj_plugins/include/pj_plugins/host/config_envelope.hpp index a2d66d5..1bd9477 100644 --- a/pj_plugins/include/pj_plugins/host/config_envelope.hpp +++ b/pj_plugins/include/pj_plugins/host/config_envelope.hpp @@ -27,16 +27,16 @@ struct ConfigEnvelope { static Expected unpack(std::string_view envelope_json) { auto j = nlohmann::json::parse(envelope_json, nullptr, false); if (j.is_discarded()) { - return unexpected(std::string("invalid envelope JSON")); + return unexpected("invalid envelope JSON"); } if (!j.contains("version") || !j["version"].is_number_integer()) { - return unexpected(std::string("missing or invalid envelope version")); + return unexpected("missing or invalid envelope version"); } if (j["version"].get() != 1) { - return unexpected(std::string("unsupported envelope version")); + return unexpected("unsupported envelope version"); } if (!j.contains("source_config") || !j["source_config"].is_string()) { - return unexpected(std::string("missing source_config")); + return unexpected("missing source_config"); } Unpacked result; diff --git a/pj_plugins/include/pj_plugins/host/plugin_catalog.hpp b/pj_plugins/include/pj_plugins/host/plugin_catalog.hpp index ae4a95a..a532c68 100644 --- a/pj_plugins/include/pj_plugins/host/plugin_catalog.hpp +++ b/pj_plugins/include/pj_plugins/host/plugin_catalog.hpp @@ -5,9 +5,11 @@ * @brief Plugin discovery from each DSO's embedded manifest. * * Plugin DSOs export a family-specific vtable whose `manifest_json` field is - * the source of truth for local plugin metadata. The scanner walks plugin files, - * inspects those exports, parses the embedded manifest, and reports both valid - * descriptors and diagnostics for candidates that could not be used. + * the source of truth for local plugin metadata. Dialogs compiled against the + * original v4.0 protocol may lack that tail slot; the scanner falls back to + * their legacy `create()` + `get_manifest()` path. The scanner walks plugin + * files, inspects those exports, parses the embedded manifest, and reports both + * valid descriptors and diagnostics for candidates that could not be used. */ #include diff --git a/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp b/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp index 595dda0..90519cd 100644 --- a/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp +++ b/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp @@ -43,12 +43,13 @@ class ServiceRegistryBuilder { /// null fat-pointer field. [[nodiscard]] ::PJ::Expected tryRegisterService( std::string_view name, uint32_t protocol_version, PJ_service_t service) { + const std::string service_name(name); if (service.ctx == nullptr || service.vtable == nullptr) { - return ::PJ::unexpected(std::string("registerService: null ctx or vtable for '") + std::string(name) + "'"); + return ::PJ::unexpected("registerService: null ctx or vtable for '" + service_name + "'"); } - std::string key(name); + std::string key(service_name); if (entries_.find(key) != entries_.end()) { - return ::PJ::unexpected(std::string("registerService: duplicate name '") + std::string(name) + "'"); + return ::PJ::unexpected("registerService: duplicate name '" + service_name + "'"); } entries_[std::move(key)] = Entry{protocol_version, service}; return {}; diff --git a/pj_plugins/src/data_source_library.cpp b/pj_plugins/src/data_source_library.cpp index e030319..904783e 100644 --- a/pj_plugins/src/data_source_library.cpp +++ b/pj_plugins/src/data_source_library.cpp @@ -50,15 +50,15 @@ Expected DataSourceLibrary::load(std::string_view path) { const PJ_data_source_vtable_t* vtable = entry(); if (vtable == nullptr) { - return unexpected(std::string("PJ_get_data_source_vtable returned null")); + return unexpected("PJ_get_data_source_vtable returned null"); } if (vtable->protocol_version != PJ_DATA_SOURCE_PROTOCOL_VERSION) { - return unexpected(std::string("DataSource protocol version mismatch")); + return unexpected("DataSource protocol version mismatch"); } // Use MIN_VTABLE_SIZE (pinned at v4.0), NOT sizeof() which grows per host // release and would falsely reject plugins compiled against older headers. if (vtable->struct_size < PJ_DATA_SOURCE_MIN_VTABLE_SIZE) { - return unexpected(std::string("DataSource vtable smaller than v4.0 baseline")); + return unexpected("DataSource vtable smaller than v4.0 baseline"); } if (auto status = detail::validateRequiredSlots(vtable); !status) { return unexpected(status.error()); @@ -75,13 +75,13 @@ Expected DataSourceLibrary::resolveDialogVtable() con auto fn = reinterpret_cast(*sym); const PJ_dialog_vtable_t* vt = fn(); if (vt == nullptr) { - return unexpected(std::string("PJ_get_dialog_vtable returned null")); + return unexpected("PJ_get_dialog_vtable returned null"); } if (vt->protocol_version != PJ_DIALOG_PROTOCOL_VERSION) { - return unexpected(std::string("Dialog protocol version mismatch")); + return unexpected("Dialog protocol version mismatch"); } - if (vt->struct_size < sizeof(PJ_dialog_vtable_t)) { - return unexpected(std::string("Dialog vtable is smaller than expected")); + if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { + return unexpected("Dialog vtable smaller than v4.0 baseline"); } return vt; } diff --git a/pj_plugins/src/detail/library_loader.hpp b/pj_plugins/src/detail/library_loader.hpp index 0ac38af..c8972b8 100644 --- a/pj_plugins/src/detail/library_loader.hpp +++ b/pj_plugins/src/detail/library_loader.hpp @@ -51,7 +51,8 @@ inline Expected loadLibraryHandle(std::string_view path) { int flags = RTLD_NOW | RTLD_LOCAL; void* handle = dlopen(std::string(path).c_str(), flags); if (handle == nullptr) { - return unexpected(std::string(dlerror())); + const char* error = dlerror(); + return unexpected(error == nullptr ? "" : error); } return handle; #endif @@ -60,12 +61,13 @@ inline Expected loadLibraryHandle(std::string_view path) { /// Resolve a named symbol from a loaded library handle. inline Expected resolveSymbol(void* handle, const char* symbol_name) { if (handle == nullptr) { - return unexpected(std::string("library not loaded")); + return unexpected("library not loaded"); } #if defined(_WIN32) auto symbol = GetProcAddress(reinterpret_cast(handle), symbol_name); if (symbol == nullptr) { - return unexpected(std::string(symbol_name) + " not found"); + std::string name(symbol_name); + return unexpected(name + " not found"); } return reinterpret_cast(symbol); #else @@ -73,7 +75,7 @@ inline Expected resolveSymbol(void* handle, const char* symbol_name) { void* symbol = dlsym(handle, symbol_name); const char* err = dlerror(); if (err != nullptr) { - return unexpected(std::string(err)); + return unexpected(err); } return symbol; #endif @@ -85,14 +87,13 @@ inline Expected resolveSymbol(void* handle, const char* symbol_name) { inline Expected checkPluginAbiVersion(void* handle) { auto sym = resolveSymbol(handle, "pj_plugin_abi_version"); if (!sym) { - return unexpected(std::string("plugin missing pj_plugin_abi_version symbol: ") + sym.error()); + return unexpected("plugin missing pj_plugin_abi_version symbol: " + sym.error()); } const auto* plugin_abi = static_cast(*sym); if (plugin_abi == nullptr || *plugin_abi != PJ_ABI_VERSION) { const std::string actual = plugin_abi == nullptr ? "null" : std::to_string(*plugin_abi); return unexpected( - std::string("plugin pj_plugin_abi_version mismatch (expected ") + std::to_string(PJ_ABI_VERSION) + ", got " + - actual + ")"); + "plugin pj_plugin_abi_version mismatch (expected " + std::to_string(PJ_ABI_VERSION) + ", got " + actual + ")"); } return {}; } diff --git a/pj_plugins/src/detail/vtable_validation.hpp b/pj_plugins/src/detail/vtable_validation.hpp index b5a4aec..34fc57a 100644 --- a/pj_plugins/src/detail/vtable_validation.hpp +++ b/pj_plugins/src/detail/vtable_validation.hpp @@ -3,6 +3,7 @@ #include #include #include +#include #include "pj_base/data_source_protocol.h" #include "pj_base/expected.hpp" @@ -19,7 +20,10 @@ struct RequiredSlot { inline Status validateRequiredSlots(std::string_view family, std::initializer_list slots) { for (const auto& slot : slots) { if (!slot.present) { - return unexpected(std::string(family) + " vtable missing required slot: " + std::string(slot.name)); + std::string message(family); + message += " vtable missing required slot: "; + message += slot.name; + return unexpected(std::move(message)); } } return okStatus(); diff --git a/pj_plugins/src/message_parser_library.cpp b/pj_plugins/src/message_parser_library.cpp index ebe91d4..d7f1d20 100644 --- a/pj_plugins/src/message_parser_library.cpp +++ b/pj_plugins/src/message_parser_library.cpp @@ -50,13 +50,13 @@ Expected MessageParserLibrary::load(std::string_view path) const PJ_message_parser_vtable_t* vtable = entry(); if (vtable == nullptr) { - return unexpected(std::string("PJ_get_message_parser_vtable returned null")); + return unexpected("PJ_get_message_parser_vtable returned null"); } if (vtable->protocol_version != PJ_MESSAGE_PARSER_PROTOCOL_VERSION) { - return unexpected(std::string("MessageParser protocol version mismatch")); + return unexpected("MessageParser protocol version mismatch"); } if (vtable->struct_size < PJ_MESSAGE_PARSER_MIN_VTABLE_SIZE) { - return unexpected(std::string("MessageParser vtable smaller than v4.0 baseline")); + return unexpected("MessageParser vtable smaller than v4.0 baseline"); } if (auto status = detail::validateRequiredSlots(vtable); !status) { return unexpected(status.error()); @@ -73,13 +73,13 @@ Expected MessageParserLibrary::resolveDialogVtable() auto fn = reinterpret_cast(*sym); const PJ_dialog_vtable_t* vt = fn(); if (vt == nullptr) { - return unexpected(std::string("PJ_get_dialog_vtable returned null")); + return unexpected("PJ_get_dialog_vtable returned null"); } if (vt->protocol_version != PJ_DIALOG_PROTOCOL_VERSION) { - return unexpected(std::string("Dialog protocol version mismatch")); + return unexpected("Dialog protocol version mismatch"); } - if (vt->struct_size < sizeof(PJ_dialog_vtable_t)) { - return unexpected(std::string("Dialog vtable is smaller than expected")); + if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { + return unexpected("Dialog vtable smaller than v4.0 baseline"); } return vt; } diff --git a/pj_plugins/src/plugin_catalog.cpp b/pj_plugins/src/plugin_catalog.cpp index 1cf47d6..3379add 100644 --- a/pj_plugins/src/plugin_catalog.cpp +++ b/pj_plugins/src/plugin_catalog.cpp @@ -1,5 +1,7 @@ #include "pj_plugins/host/plugin_catalog.hpp" +#include + #include #include #include @@ -56,13 +58,13 @@ Expected probeDirectVtable( } const Vtable* vt = reinterpret_cast(*sym)(); if (vt == nullptr) { - return unexpected(std::string(symbol) + " returned null"); + return unexpected(fmt::format("{} returned null", symbol)); } if (vt->protocol_version != expected_protocol) { - return unexpected(std::string(family_name) + " protocol version mismatch"); + return unexpected(fmt::format("{} protocol version mismatch", family_name)); } if (vt->struct_size < min_vtable_size) { - return unexpected(std::string(family_name) + " vtable smaller than v4.0 baseline"); + return unexpected(fmt::format("{} vtable smaller than v4.0 baseline", family_name)); } if (auto status = detail::validateRequiredSlots(vt); !status) { return unexpected(status.error()); @@ -96,21 +98,24 @@ Expected tryDialog(void* handle) { auto entry = reinterpret_cast(*sym); const PJ_dialog_vtable_t* vt = entry(); if (vt == nullptr) { - return unexpected(std::string("PJ_get_dialog_vtable returned null")); + return unexpected("PJ_get_dialog_vtable returned null"); } if (vt->protocol_version != PJ_DIALOG_PROTOCOL_VERSION) { - return unexpected(std::string("Dialog protocol version mismatch")); + return unexpected("Dialog protocol version mismatch"); } - if (vt->struct_size < sizeof(PJ_dialog_vtable_t)) { - return unexpected(std::string("Dialog vtable smaller than v4.0 baseline")); + if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { + return unexpected("Dialog vtable smaller than v4.0 baseline"); } if (vt->create == nullptr || vt->destroy == nullptr || vt->get_manifest == nullptr) { - return unexpected(std::string("Dialog vtable missing required lifecycle slots")); + return unexpected("Dialog vtable missing required lifecycle slots"); + } + if (PJ_HAS_TAIL_SLOT(PJ_dialog_vtable_t, vt, manifest_json)) { + return ManifestCandidate{PluginFamily::kDialog, vt->manifest_json}; } void* ctx = vt->create(); if (ctx == nullptr) { - return unexpected(std::string("PJ_dialog_vtable_t::create returned null")); + return unexpected("PJ_dialog_vtable_t::create returned null"); } const char* manifest = vt->get_manifest(ctx); std::string manifest_json = manifest == nullptr ? "" : manifest; @@ -124,25 +129,25 @@ Expected findEmbeddedManifest(void* handle) { if (auto candidate = tryDataSource(handle)) { return *candidate; } else { - errors.push_back("data_source: " + candidate.error()); + errors.push_back(fmt::format("data_source: {}", candidate.error())); } if (auto candidate = tryMessageParser(handle)) { return *candidate; } else { - errors.push_back("message_parser: " + candidate.error()); + errors.push_back(fmt::format("message_parser: {}", candidate.error())); } if (auto candidate = tryToolbox(handle)) { return *candidate; } else { - errors.push_back("toolbox: " + candidate.error()); + errors.push_back(fmt::format("toolbox: {}", candidate.error())); } if (auto candidate = tryDialog(handle)) { return *candidate; } else { - errors.push_back("dialog: " + candidate.error()); + errors.push_back(fmt::format("dialog: {}", candidate.error())); } std::ostringstream out; @@ -160,11 +165,11 @@ Expected> readStringArray(const nlohmann::json& j, std: return values; } if (!it->is_array()) { - return unexpected(std::string("plugin embedded manifest key must be an array of strings: ") + std::string(key)); + return unexpected(fmt::format("plugin embedded manifest key must be an array of strings: {}", key)); } for (const auto& value : *it) { if (!value.is_string()) { - return unexpected(std::string("plugin embedded manifest key contains a non-string value: ") + std::string(key)); + return unexpected(fmt::format("plugin embedded manifest key contains a non-string value: {}", key)); } values.push_back(value.get()); } @@ -174,24 +179,24 @@ Expected> readStringArray(const nlohmann::json& j, std: Expected decodeManifest( const std::filesystem::path& dso_path, PluginFamily family, std::string_view manifest_json) { if (manifest_json.empty()) { - return unexpected(std::string("plugin embedded manifest is empty")); + return unexpected("plugin embedded manifest is empty"); } nlohmann::json j; try { j = nlohmann::json::parse(manifest_json); } catch (const nlohmann::json::exception& e) { - return unexpected(std::string("plugin embedded manifest is invalid JSON: ") + e.what()); + return unexpected(fmt::format("plugin embedded manifest is invalid JSON: {}", e.what())); } if (!j.is_object()) { - return unexpected(std::string("plugin embedded manifest must be a JSON object")); + return unexpected("plugin embedded manifest must be a JSON object"); } auto requiredString = [&](std::string_view key) -> Expected { const auto it = j.find(std::string(key)); if (it == j.end() || !it->is_string() || it->get().empty()) { - return unexpected(std::string("plugin embedded manifest missing required string key: ") + std::string(key)); + return unexpected(fmt::format("plugin embedded manifest missing required string key: {}", key)); } return it->get(); }; @@ -201,7 +206,7 @@ Expected decodeManifest( return std::string{}; } if (!it->is_string()) { - return unexpected(std::string("plugin embedded manifest key must be a string: ") + std::string(key)); + return unexpected(fmt::format("plugin embedded manifest key must be a string: {}", key)); } return it->get(); }; @@ -255,7 +260,7 @@ Expected decodeManifest( return unexpected(encoding.error()); } if (family == PluginFamily::kMessageParser && encoding->empty()) { - return unexpected(std::string("plugin embedded manifest missing required encoding array")); + return unexpected("plugin embedded manifest missing required encoding array"); } d.encoding = std::move(*encoding); @@ -282,9 +287,9 @@ std::string_view toString(PluginFamily family) noexcept { Expected inspectPluginDso(const std::filesystem::path& dso_path) { if (!hasDsoSuffix(dso_path)) { - return unexpected(std::string("not a platform plugin DSO: ") + dso_path.string()); + return unexpected(fmt::format("not a platform plugin DSO: {}", dso_path.string())); } - auto withPath = [&](const std::string& error) { return dso_path.string() + ": " + error; }; + auto withPath = [&](const std::string& error) { return fmt::format("{}: {}", dso_path.string(), error); }; auto handle = detail::loadLibraryHandle(dso_path.string()); if (!handle) { @@ -311,10 +316,10 @@ Expected inspectPluginDso(const std::filesystem::path& dso_pat Expected scanPluginDsos(const std::filesystem::path& directory) { std::error_code ec; if (!std::filesystem::exists(directory, ec)) { - return unexpected(std::string("plugin directory not found: ") + directory.string()); + return unexpected(fmt::format("plugin directory not found: {}", directory.string())); } if (!std::filesystem::is_directory(directory, ec)) { - return unexpected(std::string("plugin path is not a directory: ") + directory.string()); + return unexpected(fmt::format("plugin path is not a directory: {}", directory.string())); } PluginScanResult result; @@ -323,7 +328,7 @@ Expected scanPluginDsos(const std::filesystem::path& directory // scan and silently hide every later extension from the marketplace. std::filesystem::recursive_directory_iterator it(directory, ec); if (ec) { - result.diagnostics.push_back({directory, "directory iteration failed: " + ec.message()}); + result.diagnostics.push_back({directory, fmt::format("directory iteration failed: {}", ec.message())}); return result; } const std::filesystem::recursive_directory_iterator end; @@ -339,13 +344,13 @@ Expected scanPluginDsos(const std::filesystem::path& directory result.diagnostics.push_back({entry.path(), descriptor.error()}); } } else if (entry_ec) { - result.diagnostics.push_back({entry.path(), "stat failed: " + entry_ec.message()}); + result.diagnostics.push_back({entry.path(), fmt::format("stat failed: {}", entry_ec.message())}); } std::error_code inc_ec; it.increment(inc_ec); if (inc_ec) { - result.diagnostics.push_back({entry.path(), "directory iteration failed: " + inc_ec.message()}); + result.diagnostics.push_back({entry.path(), fmt::format("directory iteration failed: {}", inc_ec.message())}); // Skip the unreadable subtree but continue with the rest of the scan. it.pop(); } diff --git a/pj_plugins/src/toolbox_library.cpp b/pj_plugins/src/toolbox_library.cpp index e9afc44..9092ef5 100644 --- a/pj_plugins/src/toolbox_library.cpp +++ b/pj_plugins/src/toolbox_library.cpp @@ -49,13 +49,13 @@ Expected ToolboxLibrary::load(std::string_view path) { const PJ_toolbox_vtable_t* vtable = entry(); if (vtable == nullptr) { - return unexpected(std::string("PJ_get_toolbox_vtable returned null")); + return unexpected("PJ_get_toolbox_vtable returned null"); } if (vtable->protocol_version != PJ_TOOLBOX_PLUGIN_PROTOCOL_VERSION) { - return unexpected(std::string("Toolbox protocol version mismatch")); + return unexpected("Toolbox protocol version mismatch"); } if (vtable->struct_size < PJ_TOOLBOX_MIN_VTABLE_SIZE) { - return unexpected(std::string("Toolbox vtable smaller than v4.0 baseline")); + return unexpected("Toolbox vtable smaller than v4.0 baseline"); } if (auto status = detail::validateRequiredSlots(vtable); !status) { return unexpected(status.error()); @@ -72,13 +72,13 @@ Expected ToolboxLibrary::resolveDialogVtable() const auto fn = reinterpret_cast(*sym); const PJ_dialog_vtable_t* vt = fn(); if (vt == nullptr) { - return unexpected(std::string("PJ_get_dialog_vtable returned null")); + return unexpected("PJ_get_dialog_vtable returned null"); } if (vt->protocol_version != PJ_DIALOG_PROTOCOL_VERSION) { - return unexpected(std::string("Dialog protocol version mismatch")); + return unexpected("Dialog protocol version mismatch"); } - if (vt->struct_size < sizeof(PJ_dialog_vtable_t)) { - return unexpected(std::string("Dialog vtable is smaller than expected")); + if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { + return unexpected("Dialog vtable smaller than v4.0 baseline"); } return vt; } diff --git a/pj_plugins/tests/plugin_catalog_test.cpp b/pj_plugins/tests/plugin_catalog_test.cpp index ad747a9..e54aa58 100644 --- a/pj_plugins/tests/plugin_catalog_test.cpp +++ b/pj_plugins/tests/plugin_catalog_test.cpp @@ -87,6 +87,14 @@ TEST_F(PluginCatalogTest, InspectDialogDsoUsesEmbeddedManifest) { EXPECT_EQ(descriptor->family, PluginFamily::kDialog); } +TEST_F(PluginCatalogTest, InspectDialogDsoUsesStaticManifestWithoutCreate) { + auto descriptor = inspectPluginDso(PJ_STATIC_MANIFEST_DIALOG_PLUGIN_PATH); + ASSERT_TRUE(descriptor.has_value()) << descriptor.error(); + EXPECT_EQ(descriptor->id, "static-manifest-dialog"); + EXPECT_EQ(descriptor->name, "Static Manifest Dialog"); + EXPECT_EQ(descriptor->family, PluginFamily::kDialog); +} + TEST_F(PluginCatalogTest, MissingIdManifestIsRejected) { auto descriptor = inspectPluginDso(PJ_MISSING_ID_PLUGIN_PATH); ASSERT_FALSE(descriptor.has_value()); diff --git a/pj_plugins/tests/static_manifest_dialog_plugin.cpp b/pj_plugins/tests/static_manifest_dialog_plugin.cpp new file mode 100644 index 0000000..aa6a271 --- /dev/null +++ b/pj_plugins/tests/static_manifest_dialog_plugin.cpp @@ -0,0 +1,73 @@ +#include "pj_plugins/dialog_protocol.h" + +extern "C" PJ_DIALOG_EXPORT const uint32_t pj_plugin_abi_version = PJ_ABI_VERSION; + +namespace { + +constexpr const char* kStaticManifest = + R"({"id":"static-manifest-dialog","name":"Static Manifest Dialog","version":"1.0.0"})"; + +void* create() noexcept { + return nullptr; +} + +void destroy(void*) noexcept {} + +const char* getManifest(void*) noexcept { + return R"({"id":"runtime-dialog","name":"Runtime Dialog","version":"1.0.0"})"; +} + +const char* getUiContent(void*) noexcept { + return ""; +} + +const char* getWidgetData(void*) noexcept { + return "{}"; +} + +bool onWidgetEvent(void*, const char*, const char*, PJ_error_t*) noexcept { + return false; +} + +bool onTick(void*, PJ_error_t*) noexcept { + return false; +} + +void onAccepted(void*, const char*) noexcept {} + +void onRejected(void*) noexcept {} + +bool saveConfig(void*, PJ_string_view_t* out_json, PJ_error_t*) noexcept { + static constexpr const char* kJson = "{}"; + if (out_json != nullptr) { + out_json->data = kJson; + out_json->size = 2; + } + return true; +} + +bool loadConfig(void*, PJ_string_view_t, PJ_error_t*) noexcept { + return true; +} + +} // namespace + +extern "C" PJ_DIALOG_EXPORT const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept { + static const PJ_dialog_vtable_t vt = { + .protocol_version = PJ_DIALOG_PROTOCOL_VERSION, + .struct_size = sizeof(PJ_dialog_vtable_t), + .create = create, + .destroy = destroy, + .get_manifest = getManifest, + .get_ui_content = getUiContent, + .get_widget_data = getWidgetData, + .on_widget_event = onWidgetEvent, + .on_tick = onTick, + .on_accepted = onAccepted, + .on_rejected = onRejected, + .save_config = saveConfig, + .load_config = loadConfig, + .manifest_json = kStaticManifest, + }; + return &vt; +} diff --git a/pj_scene_protocol/src/scene_decoder_protobuf.cpp b/pj_scene_protocol/src/scene_decoder_protobuf.cpp index d03f262..97ee847 100644 --- a/pj_scene_protocol/src/scene_decoder_protobuf.cpp +++ b/pj_scene_protocol/src/scene_decoder_protobuf.cpp @@ -416,7 +416,7 @@ class ProtobufImageAnnotationsDecoder final : public ISceneDecoder { public: Expected decode(const uint8_t* data, size_t size) override { if (data == nullptr || size == 0) { - return unexpected(std::string("Protobuf ImageAnnotations: empty buffer")); + return unexpected("Protobuf ImageAnnotations: empty buffer"); } Reader r{data, data + size}; @@ -424,7 +424,7 @@ class ProtobufImageAnnotationsDecoder final : public ISceneDecoder { while (!r.eof()) { uint64_t tag = 0; if (!r.readVarint(tag)) { - return unexpected(std::string("Protobuf ImageAnnotations: bad tag")); + return unexpected("Protobuf ImageAnnotations: bad tag"); } uint32_t field = static_cast(tag >> 3); uint32_t wire = static_cast(tag & 0x7u); @@ -432,38 +432,38 @@ class ProtobufImageAnnotationsDecoder final : public ISceneDecoder { if (field == 2 && wire == 2) { uint64_t pa_len = 0; if (!r.readVarint(pa_len)) { - return unexpected(std::string("Protobuf ImageAnnotations: bad PointsAnnotation length")); + return unexpected("Protobuf ImageAnnotations: bad PointsAnnotation length"); } PointsAnnotation pa; pa.color = {0, 255, 0, 255}; pa.thickness = 2.0; if (!decodePointsAnnotation(r, pa_len, pa)) { - return unexpected(std::string("Protobuf ImageAnnotations: PointsAnnotation decode failed")); + return unexpected("Protobuf ImageAnnotations: PointsAnnotation decode failed"); } ia.points.push_back(std::move(pa)); } else if (field == 1 && wire == 2) { uint64_t ca_len = 0; if (!r.readVarint(ca_len)) { - return unexpected(std::string("Protobuf ImageAnnotations: bad CircleAnnotation length")); + return unexpected("Protobuf ImageAnnotations: bad CircleAnnotation length"); } CircleAnnotation ca; if (!decodeCircleAnnotation(r, ca_len, ca)) { - return unexpected(std::string("Protobuf ImageAnnotations: CircleAnnotation decode failed")); + return unexpected("Protobuf ImageAnnotations: CircleAnnotation decode failed"); } ia.circles.push_back(std::move(ca)); } else if (field == 3 && wire == 2) { uint64_t ta_len = 0; if (!r.readVarint(ta_len)) { - return unexpected(std::string("Protobuf ImageAnnotations: bad TextAnnotation length")); + return unexpected("Protobuf ImageAnnotations: bad TextAnnotation length"); } TextAnnotation ta; if (!decodeTextAnnotation(r, ta_len, ta)) { - return unexpected(std::string("Protobuf ImageAnnotations: TextAnnotation decode failed")); + return unexpected("Protobuf ImageAnnotations: TextAnnotation decode failed"); } ia.texts.push_back(std::move(ta)); } else { if (!r.skipField(wire)) { - return unexpected(std::string("Protobuf ImageAnnotations: skip failed")); + return unexpected("Protobuf ImageAnnotations: skip failed"); } } } From 02db88deb70af478ea9518ea63e6f1b6feeae6b7 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 12:42:47 +0200 Subject: [PATCH 05/10] fix: fallback for null dialog static manifests Use the dialog static manifest tail slot only when it is present and non-null. New-header plugins using the legacy one-argument dialog macro now correctly fall back to runtime manifest discovery. --- pj_plugins/CMakeLists.txt | 8 +++++- pj_plugins/src/plugin_catalog.cpp | 2 +- .../tests/legacy_macro_dialog_plugin.cpp | 25 +++++++++++++++++++ pj_plugins/tests/plugin_catalog_test.cpp | 8 ++++++ 4 files changed, 41 insertions(+), 2 deletions(-) create mode 100644 pj_plugins/tests/legacy_macro_dialog_plugin.cpp diff --git a/pj_plugins/CMakeLists.txt b/pj_plugins/CMakeLists.txt index 7469240..99dfd99 100644 --- a/pj_plugins/CMakeLists.txt +++ b/pj_plugins/CMakeLists.txt @@ -188,6 +188,11 @@ target_compile_features(static_manifest_dialog_plugin PRIVATE cxx_std_20) target_compile_options(static_manifest_dialog_plugin PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(static_manifest_dialog_plugin PRIVATE pj_dialog_protocol) +add_library(legacy_macro_dialog_plugin SHARED tests/legacy_macro_dialog_plugin.cpp) +target_compile_features(legacy_macro_dialog_plugin PRIVATE cxx_std_20) +target_compile_options(legacy_macro_dialog_plugin PRIVATE ${PJ_WARNING_FLAGS}) +target_link_libraries(legacy_macro_dialog_plugin PRIVATE pj_dialog_protocol) + # --------------------------------------------------------------------------- # Tests # --------------------------------------------------------------------------- @@ -277,6 +282,7 @@ target_compile_definitions(plugin_catalog_test PRIVATE PJ_MOCK_TOOLBOX_PLUGIN_PATH="$" PJ_MOCK_DIALOG_PLUGIN_PATH="$" PJ_STATIC_MANIFEST_DIALOG_PLUGIN_PATH="$" + PJ_LEGACY_MACRO_DIALOG_PLUGIN_PATH="$" PJ_MISSING_ID_PLUGIN_PATH="$" PJ_INVALID_OPTIONAL_PLUGIN_PATH="$" PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH="$" @@ -288,7 +294,7 @@ target_link_libraries(plugin_catalog_test PRIVATE add_dependencies(plugin_catalog_test mock_data_source_plugin mock_json_parser_plugin mock_toolbox_plugin mock_dialog_plugin missing_id_data_source_plugin invalid_optional_manifest_data_source_plugin missing_required_slots_plugin - static_manifest_dialog_plugin) + static_manifest_dialog_plugin legacy_macro_dialog_plugin) add_test(NAME plugin_catalog_test COMMAND plugin_catalog_test) endif() # PJ_BUILD_TESTS diff --git a/pj_plugins/src/plugin_catalog.cpp b/pj_plugins/src/plugin_catalog.cpp index 3379add..8f5439a 100644 --- a/pj_plugins/src/plugin_catalog.cpp +++ b/pj_plugins/src/plugin_catalog.cpp @@ -109,7 +109,7 @@ Expected tryDialog(void* handle) { if (vt->create == nullptr || vt->destroy == nullptr || vt->get_manifest == nullptr) { return unexpected("Dialog vtable missing required lifecycle slots"); } - if (PJ_HAS_TAIL_SLOT(PJ_dialog_vtable_t, vt, manifest_json)) { + if (PJ_HAS_TAIL_SLOT(PJ_dialog_vtable_t, vt, manifest_json) && vt->manifest_json != nullptr) { return ManifestCandidate{PluginFamily::kDialog, vt->manifest_json}; } diff --git a/pj_plugins/tests/legacy_macro_dialog_plugin.cpp b/pj_plugins/tests/legacy_macro_dialog_plugin.cpp new file mode 100644 index 0000000..86c8ccb --- /dev/null +++ b/pj_plugins/tests/legacy_macro_dialog_plugin.cpp @@ -0,0 +1,25 @@ +#include + +#include +#include + +class LegacyMacroDialog : public PJ::DialogPluginBase { + public: + std::string manifest() const override { + return R"({"id":"legacy-macro-dialog","name":"Legacy Macro Dialog","version":"1.0.0"})"; + } + + std::string ui_content() const override { + return ""; + } + + std::string widget_data() override { + return "{}"; + } + + bool onWidgetEvent(std::string_view, std::string_view) override { + return false; + } +}; + +PJ_DIALOG_PLUGIN(LegacyMacroDialog) diff --git a/pj_plugins/tests/plugin_catalog_test.cpp b/pj_plugins/tests/plugin_catalog_test.cpp index e54aa58..937be9e 100644 --- a/pj_plugins/tests/plugin_catalog_test.cpp +++ b/pj_plugins/tests/plugin_catalog_test.cpp @@ -95,6 +95,14 @@ TEST_F(PluginCatalogTest, InspectDialogDsoUsesStaticManifestWithoutCreate) { EXPECT_EQ(descriptor->family, PluginFamily::kDialog); } +TEST_F(PluginCatalogTest, InspectDialogDsoFallsBackWhenStaticManifestSlotIsNull) { + auto descriptor = inspectPluginDso(PJ_LEGACY_MACRO_DIALOG_PLUGIN_PATH); + ASSERT_TRUE(descriptor.has_value()) << descriptor.error(); + EXPECT_EQ(descriptor->id, "legacy-macro-dialog"); + EXPECT_EQ(descriptor->name, "Legacy Macro Dialog"); + EXPECT_EQ(descriptor->family, PluginFamily::kDialog); +} + TEST_F(PluginCatalogTest, MissingIdManifestIsRejected) { auto descriptor = inspectPluginDso(PJ_MISSING_ID_PLUGIN_PATH); ASSERT_FALSE(descriptor.has_value()); From 479adf7d903f823907c8a8caf180f20e6e0b3226 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 12:46:16 +0200 Subject: [PATCH 06/10] refactor: move plugin vtable validation out of header Keep the loader required-slot checks in a private implementation unit and wire them through an internal detail library. Also remove the dialog protocol subproject's global C++ standard override so it no longer downgrades the root build setting. --- pj_plugins/CMakeLists.txt | 11 +++ pj_plugins/dialog_protocol/CMakeLists.txt | 6 +- pj_plugins/src/detail/vtable_validation.cpp | 85 +++++++++++++++++++++ pj_plugins/src/detail/vtable_validation.hpp | 79 +------------------ 4 files changed, 101 insertions(+), 80 deletions(-) create mode 100644 pj_plugins/src/detail/vtable_validation.cpp diff --git a/pj_plugins/CMakeLists.txt b/pj_plugins/CMakeLists.txt index 99dfd99..7d6498f 100644 --- a/pj_plugins/CMakeLists.txt +++ b/pj_plugins/CMakeLists.txt @@ -7,6 +7,13 @@ add_subdirectory(dialog_protocol) find_package(fmt REQUIRED) find_package(nlohmann_json REQUIRED) +add_library(pj_plugin_loader_detail STATIC + src/detail/vtable_validation.cpp +) +target_compile_features(pj_plugin_loader_detail PUBLIC cxx_std_20) +target_compile_options(pj_plugin_loader_detail PRIVATE ${PJ_WARNING_FLAGS}) +target_link_libraries(pj_plugin_loader_detail PUBLIC pj_base) + add_library(pj_plugin_catalog STATIC src/plugin_catalog.cpp ) @@ -17,6 +24,7 @@ target_link_libraries(pj_plugin_catalog PUBLIC pj_base PRIVATE + pj_plugin_loader_detail pj_dialog_protocol fmt::fmt ${CMAKE_DL_LIBS} @@ -39,6 +47,7 @@ target_link_libraries(pj_data_source_host pj_dialog_protocol pj_plugin_catalog PRIVATE + pj_plugin_loader_detail ${CMAKE_DL_LIBS} ) @@ -108,6 +117,7 @@ target_link_libraries(pj_message_parser_host pj_base pj_dialog_protocol PRIVATE + pj_plugin_loader_detail ${CMAKE_DL_LIBS} ) @@ -148,6 +158,7 @@ target_link_libraries(pj_toolbox_host pj_base pj_dialog_protocol PRIVATE + pj_plugin_loader_detail ${CMAKE_DL_LIBS} ) diff --git a/pj_plugins/dialog_protocol/CMakeLists.txt b/pj_plugins/dialog_protocol/CMakeLists.txt index 09390db..b7e4d69 100644 --- a/pj_plugins/dialog_protocol/CMakeLists.txt +++ b/pj_plugins/dialog_protocol/CMakeLists.txt @@ -1,9 +1,5 @@ cmake_minimum_required(VERSION 3.20) project(pj_dialog_protocol LANGUAGES CXX) -set(CMAKE_CXX_STANDARD 17) -set(CMAKE_CXX_STANDARD_REQUIRED ON) -set(CMAKE_CXX_EXTENSIONS OFF) - # --- Header-only interface libraries --- @@ -19,6 +15,7 @@ target_link_libraries(pj_dialog_protocol INTERFACE pj_base) find_package(nlohmann_json REQUIRED) add_library(pj_dialog_sdk INTERFACE) +target_compile_features(pj_dialog_sdk INTERFACE cxx_std_17) target_link_libraries(pj_dialog_sdk INTERFACE pj_dialog_protocol nlohmann_json::nlohmann_json) target_include_directories(pj_dialog_sdk INTERFACE $ @@ -28,6 +25,7 @@ set_target_properties(pj_dialog_sdk PROPERTIES EXPORT_NAME Dialog) # Host-side C++ API (depends on protocol header + nlohmann/json) add_library(pj_dialog_host INTERFACE) +target_compile_features(pj_dialog_host INTERFACE cxx_std_17) target_link_libraries(pj_dialog_host INTERFACE pj_dialog_protocol nlohmann_json::nlohmann_json) target_include_directories(pj_dialog_host INTERFACE include) diff --git a/pj_plugins/src/detail/vtable_validation.cpp b/pj_plugins/src/detail/vtable_validation.cpp new file mode 100644 index 0000000..673f446 --- /dev/null +++ b/pj_plugins/src/detail/vtable_validation.cpp @@ -0,0 +1,85 @@ +#include "vtable_validation.hpp" + +#include +#include +#include +#include + +namespace PJ::detail { +namespace { + +struct RequiredSlot { + std::string_view name; + bool present; +}; + +Status validateSlots(std::string_view family, std::initializer_list slots) { + for (const auto& slot : slots) { + if (!slot.present) { + std::string message(family); + message += " vtable missing required slot: "; + message += slot.name; + return unexpected(std::move(message)); + } + } + return okStatus(); +} + +} // namespace + +Status validateRequiredSlots(const PJ_data_source_vtable_t* vtable) { + return validateSlots( + "DataSource", + { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"capabilities", vtable->capabilities != nullptr}, + {"bind", vtable->bind != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"start", vtable->start != nullptr}, + {"stop", vtable->stop != nullptr}, + {"pause", vtable->pause != nullptr}, + {"resume", vtable->resume != nullptr}, + {"poll", vtable->poll != nullptr}, + {"current_state", vtable->current_state != nullptr}, + {"get_dialog", vtable->get_dialog != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); +} + +Status validateRequiredSlots(const PJ_message_parser_vtable_t* vtable) { + return validateSlots( + "MessageParser", + { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"bind", vtable->bind != nullptr}, + {"bind_schema", vtable->bind_schema != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"parse", vtable->parse != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); +} + +Status validateRequiredSlots(const PJ_toolbox_vtable_t* vtable) { + return validateSlots( + "Toolbox", + { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"capabilities", vtable->capabilities != nullptr}, + {"bind", vtable->bind != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"get_dialog", vtable->get_dialog != nullptr}, + {"on_data_changed", vtable->on_data_changed != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); +} + +} // namespace PJ::detail diff --git a/pj_plugins/src/detail/vtable_validation.hpp b/pj_plugins/src/detail/vtable_validation.hpp index 34fc57a..ae0d99e 100644 --- a/pj_plugins/src/detail/vtable_validation.hpp +++ b/pj_plugins/src/detail/vtable_validation.hpp @@ -1,10 +1,5 @@ #pragma once -#include -#include -#include -#include - #include "pj_base/data_source_protocol.h" #include "pj_base/expected.hpp" #include "pj_base/message_parser_protocol.h" @@ -12,76 +7,8 @@ namespace PJ::detail { -struct RequiredSlot { - std::string_view name; - bool present; -}; - -inline Status validateRequiredSlots(std::string_view family, std::initializer_list slots) { - for (const auto& slot : slots) { - if (!slot.present) { - std::string message(family); - message += " vtable missing required slot: "; - message += slot.name; - return unexpected(std::move(message)); - } - } - return okStatus(); -} - -inline Status validateRequiredSlots(const PJ_data_source_vtable_t* vtable) { - return validateRequiredSlots( - "DataSource", - { - {"create", vtable->create != nullptr}, - {"destroy", vtable->destroy != nullptr}, - {"manifest_json", vtable->manifest_json != nullptr}, - {"capabilities", vtable->capabilities != nullptr}, - {"bind", vtable->bind != nullptr}, - {"save_config", vtable->save_config != nullptr}, - {"load_config", vtable->load_config != nullptr}, - {"start", vtable->start != nullptr}, - {"stop", vtable->stop != nullptr}, - {"pause", vtable->pause != nullptr}, - {"resume", vtable->resume != nullptr}, - {"poll", vtable->poll != nullptr}, - {"current_state", vtable->current_state != nullptr}, - {"get_dialog", vtable->get_dialog != nullptr}, - {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, - }); -} - -inline Status validateRequiredSlots(const PJ_message_parser_vtable_t* vtable) { - return validateRequiredSlots( - "MessageParser", - { - {"create", vtable->create != nullptr}, - {"destroy", vtable->destroy != nullptr}, - {"manifest_json", vtable->manifest_json != nullptr}, - {"bind", vtable->bind != nullptr}, - {"bind_schema", vtable->bind_schema != nullptr}, - {"save_config", vtable->save_config != nullptr}, - {"load_config", vtable->load_config != nullptr}, - {"parse", vtable->parse != nullptr}, - {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, - }); -} - -inline Status validateRequiredSlots(const PJ_toolbox_vtable_t* vtable) { - return validateRequiredSlots( - "Toolbox", - { - {"create", vtable->create != nullptr}, - {"destroy", vtable->destroy != nullptr}, - {"manifest_json", vtable->manifest_json != nullptr}, - {"capabilities", vtable->capabilities != nullptr}, - {"bind", vtable->bind != nullptr}, - {"save_config", vtable->save_config != nullptr}, - {"load_config", vtable->load_config != nullptr}, - {"get_dialog", vtable->get_dialog != nullptr}, - {"on_data_changed", vtable->on_data_changed != nullptr}, - {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, - }); -} +Status validateRequiredSlots(const PJ_data_source_vtable_t* vtable); +Status validateRequiredSlots(const PJ_message_parser_vtable_t* vtable); +Status validateRequiredSlots(const PJ_toolbox_vtable_t* vtable); } // namespace PJ::detail From 0f280f22267413673fd78caf7efa85055d729837 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 12:47:18 +0200 Subject: [PATCH 07/10] formatting --- .../include/pj_plugins/dialog_protocol.h | 2 +- .../include/pj_plugins/host/dialog_handle.hpp | 5 +- .../pj_plugins/host/dialog_library.hpp | 2 +- .../pj_plugins/sdk/dialog_plugin_base.hpp | 59 +++++++------ .../tests/dialog_library_test.cpp | 2 +- .../pj_plugins/host/data_source_library.hpp | 2 +- .../host/message_parser_library.hpp | 2 +- .../pj_plugins/host/toolbox_library.hpp | 2 +- pj_plugins/src/detail/vtable_validation.cpp | 83 +++++++++---------- pj_plugins/src/plugin_runtime_catalog.cpp | 3 +- .../tests/legacy_macro_dialog_plugin.cpp | 1 - .../tests/missing_required_slots_plugin.cpp | 3 +- 12 files changed, 84 insertions(+), 82 deletions(-) diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h b/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h index d2bc089..5034cd5 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h +++ b/pj_plugins/dialog_protocol/include/pj_plugins/dialog_protocol.h @@ -2,8 +2,8 @@ #define PJ_DIALOG_PROTOCOL_H #include -#include #include +#include #include "pj_base/plugin_data_api.h" diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp index 6f5c11a..0c13d0d 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_handle.hpp @@ -38,10 +38,7 @@ class DialogHandle { } DialogHandle(DialogHandle&& other) noexcept - : vt_(other.vt_), - ctx_(other.ctx_), - owned_(other.owned_), - library_owner_(std::move(other.library_owner_)) { + : vt_(other.vt_), ctx_(other.ctx_), owned_(other.owned_), library_owner_(std::move(other.library_owner_)) { other.vt_ = nullptr; other.ctx_ = nullptr; other.owned_ = false; diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp index fb53fb6..8722d9c 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/host/dialog_library.hpp @@ -2,8 +2,8 @@ #include -#include #include +#include #include #include diff --git a/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp b/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp index d9832a6..fffa4c4 100644 --- a/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp +++ b/pj_plugins/dialog_protocol/include/pj_plugins/sdk/dialog_plugin_base.hpp @@ -51,11 +51,20 @@ class DialogPluginBase { template static const PJ_dialog_vtable_t* vtableWithCreate(CreateFn create_fn, const char* manifest_json = nullptr) { static const PJ_dialog_vtable_t vt = { - PJ_DIALOG_PROTOCOL_VERSION, sizeof(PJ_dialog_vtable_t), create_fn, - trampoline_destroy, trampoline_get_manifest, trampoline_get_ui_content, - trampoline_get_widget_data, trampoline_on_widget_event, trampoline_on_tick, - trampoline_on_accepted, trampoline_on_rejected, trampoline_save_config, - trampoline_load_config, manifest_json, + PJ_DIALOG_PROTOCOL_VERSION, + sizeof(PJ_dialog_vtable_t), + create_fn, + trampoline_destroy, + trampoline_get_manifest, + trampoline_get_ui_content, + trampoline_get_widget_data, + trampoline_on_widget_event, + trampoline_on_tick, + trampoline_on_accepted, + trampoline_on_rejected, + trampoline_save_config, + trampoline_load_config, + manifest_json, }; return &vt; } @@ -257,26 +266,26 @@ PJ_borrowed_dialog_t borrowDialog(DialogT& dialog) noexcept { /// the vtable pointer type-safely via `PJ::borrowDialog(member)` — /// no `extern "C"` forward declaration required in the plugin source. #define PJ_DIALOG_PLUGIN_SELECT(_1, _2, NAME, ...) NAME -#define PJ_DIALOG_PLUGIN(...) \ +#define PJ_DIALOG_PLUGIN(...) \ PJ_DIALOG_PLUGIN_SELECT(__VA_ARGS__, PJ_DIALOG_PLUGIN_WITH_MANIFEST, PJ_DIALOG_PLUGIN_LEGACY)(__VA_ARGS__) #define PJ_DIALOG_PLUGIN_LEGACY(ClassName) PJ_DIALOG_PLUGIN_WITH_MANIFEST(ClassName, nullptr) -#define PJ_DIALOG_PLUGIN_WITH_MANIFEST(ClassName, ManifestJson) \ - PJ_EXPORT_PLUGIN_ABI_VERSION(PJ_DIALOG_EXPORT) \ - extern "C" PJ_DIALOG_EXPORT const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept { \ - static const PJ_dialog_vtable_t* vt = PJ::DialogPluginBase::vtableWithCreate( \ - []() noexcept -> void* { \ - try { \ - return new ClassName(); \ - } catch (...) { \ - return nullptr; \ - } \ - }, \ - ManifestJson); \ - return vt; \ - } \ - namespace PJ { \ - template <> \ - [[maybe_unused]] inline const PJ_dialog_vtable_t* dialogVtableFor() noexcept { \ - return PJ_get_dialog_vtable(); \ - } \ +#define PJ_DIALOG_PLUGIN_WITH_MANIFEST(ClassName, ManifestJson) \ + PJ_EXPORT_PLUGIN_ABI_VERSION(PJ_DIALOG_EXPORT) \ + extern "C" PJ_DIALOG_EXPORT const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept { \ + static const PJ_dialog_vtable_t* vt = PJ::DialogPluginBase::vtableWithCreate( \ + []() noexcept -> void* { \ + try { \ + return new ClassName(); \ + } catch (...) { \ + return nullptr; \ + } \ + }, \ + ManifestJson); \ + return vt; \ + } \ + namespace PJ { \ + template <> \ + [[maybe_unused]] inline const PJ_dialog_vtable_t* dialogVtableFor() noexcept { \ + return PJ_get_dialog_vtable(); \ + } \ } diff --git a/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp b/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp index 890c8ca..92342ab 100644 --- a/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp +++ b/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp @@ -2,8 +2,8 @@ #include -#include #include +#include #include #ifndef PJ_MOCK_DIALOG_PLUGIN_PATH diff --git a/pj_plugins/include/pj_plugins/host/data_source_library.hpp b/pj_plugins/include/pj_plugins/host/data_source_library.hpp index 255f1b9..4916e70 100644 --- a/pj_plugins/include/pj_plugins/host/data_source_library.hpp +++ b/pj_plugins/include/pj_plugins/host/data_source_library.hpp @@ -18,8 +18,8 @@ #include #include -#include #include +#include #include #include diff --git a/pj_plugins/include/pj_plugins/host/message_parser_library.hpp b/pj_plugins/include/pj_plugins/host/message_parser_library.hpp index c53faf4..376ae5d 100644 --- a/pj_plugins/include/pj_plugins/host/message_parser_library.hpp +++ b/pj_plugins/include/pj_plugins/host/message_parser_library.hpp @@ -18,8 +18,8 @@ #include #include -#include #include +#include #include #include diff --git a/pj_plugins/include/pj_plugins/host/toolbox_library.hpp b/pj_plugins/include/pj_plugins/host/toolbox_library.hpp index d47984d..8205ff6 100644 --- a/pj_plugins/include/pj_plugins/host/toolbox_library.hpp +++ b/pj_plugins/include/pj_plugins/host/toolbox_library.hpp @@ -18,8 +18,8 @@ #include #include -#include #include +#include #include #include diff --git a/pj_plugins/src/detail/vtable_validation.cpp b/pj_plugins/src/detail/vtable_validation.cpp index 673f446..f418176 100644 --- a/pj_plugins/src/detail/vtable_validation.cpp +++ b/pj_plugins/src/detail/vtable_validation.cpp @@ -29,57 +29,54 @@ Status validateSlots(std::string_view family, std::initializer_listcreate != nullptr}, - {"destroy", vtable->destroy != nullptr}, - {"manifest_json", vtable->manifest_json != nullptr}, - {"capabilities", vtable->capabilities != nullptr}, - {"bind", vtable->bind != nullptr}, - {"save_config", vtable->save_config != nullptr}, - {"load_config", vtable->load_config != nullptr}, - {"start", vtable->start != nullptr}, - {"stop", vtable->stop != nullptr}, - {"pause", vtable->pause != nullptr}, - {"resume", vtable->resume != nullptr}, - {"poll", vtable->poll != nullptr}, - {"current_state", vtable->current_state != nullptr}, - {"get_dialog", vtable->get_dialog != nullptr}, - {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, - }); + "DataSource", { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"capabilities", vtable->capabilities != nullptr}, + {"bind", vtable->bind != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"start", vtable->start != nullptr}, + {"stop", vtable->stop != nullptr}, + {"pause", vtable->pause != nullptr}, + {"resume", vtable->resume != nullptr}, + {"poll", vtable->poll != nullptr}, + {"current_state", vtable->current_state != nullptr}, + {"get_dialog", vtable->get_dialog != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); } Status validateRequiredSlots(const PJ_message_parser_vtable_t* vtable) { return validateSlots( - "MessageParser", - { - {"create", vtable->create != nullptr}, - {"destroy", vtable->destroy != nullptr}, - {"manifest_json", vtable->manifest_json != nullptr}, - {"bind", vtable->bind != nullptr}, - {"bind_schema", vtable->bind_schema != nullptr}, - {"save_config", vtable->save_config != nullptr}, - {"load_config", vtable->load_config != nullptr}, - {"parse", vtable->parse != nullptr}, - {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, - }); + "MessageParser", { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"bind", vtable->bind != nullptr}, + {"bind_schema", vtable->bind_schema != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"parse", vtable->parse != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); } Status validateRequiredSlots(const PJ_toolbox_vtable_t* vtable) { return validateSlots( - "Toolbox", - { - {"create", vtable->create != nullptr}, - {"destroy", vtable->destroy != nullptr}, - {"manifest_json", vtable->manifest_json != nullptr}, - {"capabilities", vtable->capabilities != nullptr}, - {"bind", vtable->bind != nullptr}, - {"save_config", vtable->save_config != nullptr}, - {"load_config", vtable->load_config != nullptr}, - {"get_dialog", vtable->get_dialog != nullptr}, - {"on_data_changed", vtable->on_data_changed != nullptr}, - {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, - }); + "Toolbox", { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"manifest_json", vtable->manifest_json != nullptr}, + {"capabilities", vtable->capabilities != nullptr}, + {"bind", vtable->bind != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + {"get_dialog", vtable->get_dialog != nullptr}, + {"on_data_changed", vtable->on_data_changed != nullptr}, + {"get_plugin_extension", vtable->get_plugin_extension != nullptr}, + }); } } // namespace PJ::detail diff --git a/pj_plugins/src/plugin_runtime_catalog.cpp b/pj_plugins/src/plugin_runtime_catalog.cpp index 32975f9..e5625d7 100644 --- a/pj_plugins/src/plugin_runtime_catalog.cpp +++ b/pj_plugins/src/plugin_runtime_catalog.cpp @@ -1,8 +1,7 @@ #include "pj_plugins/host/plugin_runtime_catalog.hpp" -#include - #include +#include #include #include diff --git a/pj_plugins/tests/legacy_macro_dialog_plugin.cpp b/pj_plugins/tests/legacy_macro_dialog_plugin.cpp index 86c8ccb..834ad72 100644 --- a/pj_plugins/tests/legacy_macro_dialog_plugin.cpp +++ b/pj_plugins/tests/legacy_macro_dialog_plugin.cpp @@ -1,5 +1,4 @@ #include - #include #include diff --git a/pj_plugins/tests/missing_required_slots_plugin.cpp b/pj_plugins/tests/missing_required_slots_plugin.cpp index 58a3790..6811e76 100644 --- a/pj_plugins/tests/missing_required_slots_plugin.cpp +++ b/pj_plugins/tests/missing_required_slots_plugin.cpp @@ -86,7 +86,8 @@ extern "C" PJ_MESSAGE_PARSER_EXPORT const PJ_message_parser_vtable_t* PJ_get_mes .struct_size = sizeof(PJ_message_parser_vtable_t), .create = create, .destroy = destroy, - .manifest_json = R"({"id":"missing-parser-slot","name":"Missing Parser Slot","version":"1.0.0","encoding":["x"]})", + .manifest_json = + R"({"id":"missing-parser-slot","name":"Missing Parser Slot","version":"1.0.0","encoding":["x"]})", .bind = bind, .bind_schema = bindSchema, .save_config = saveConfig, From bf231343e3220e693c9aa60b19be9885e679792b Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 13:02:23 +0200 Subject: [PATCH 08/10] feat: add parser arrow stream writes --- .../include/pj_base/message_parser_protocol.h | 5 +- pj_base/include/pj_base/plugin_data_api.h | 14 +- pj_base/include/pj_base/sdk/arrow.hpp | 7 +- .../include/pj_base/sdk/plugin_data_api.hpp | 49 ++++- .../sdk/testing/parser_write_recorder.hpp | 17 +- pj_base/tests/abi_layout_sentinels_test.cpp | 21 ++ pj_base/tests/plugin_data_api_test.cpp | 188 ++++++++++++++++++ pj_datastore/docs/USER_GUIDE.md | 55 ++++- pj_datastore/src/plugin_data_host.cpp | 20 +- .../tests/arrow_stream_round_trip_test.cpp | 81 +++++++- pj_plugins/docs/ARCHITECTURE.md | 7 +- pj_plugins/docs/REQUIREMENTS.md | 6 +- pj_plugins/docs/message-parser-guide.md | 49 +++-- 13 files changed, 468 insertions(+), 51 deletions(-) diff --git a/pj_base/include/pj_base/message_parser_protocol.h b/pj_base/include/pj_base/message_parser_protocol.h index 1da565e..e1f7c17 100644 --- a/pj_base/include/pj_base/message_parser_protocol.h +++ b/pj_base/include/pj_base/message_parser_protocol.h @@ -5,8 +5,9 @@ * v4 summary: * - Every vtable slot is PJ_NOEXCEPT and carries a thread-class tag. * - Parser write host (pj.parser_write.v1) no longer has - * append_arrow_ipc — see plugin_data_api.h. Parsers stay per-record; - * the host coalesces into Arrow batches internally. + * append_arrow_ipc — see plugin_data_api.h. Parsers normally write + * per-record, with an optional append_arrow_stream tail slot for + * parser-shaped formats that naturally decode batches. * * The host obtains the plugin's vtable via `PJ_get_message_parser_vtable()` * and drives the plugin through: create -> bind(registry) -> diff --git a/pj_base/include/pj_base/plugin_data_api.h b/pj_base/include/pj_base/plugin_data_api.h index 1994a1f..8e5bbbf 100644 --- a/pj_base/include/pj_base/plugin_data_api.h +++ b/pj_base/include/pj_base/plugin_data_api.h @@ -386,9 +386,10 @@ typedef struct { * Parser write host: single-topic writes. The bound topic is set at * service-creation time; the parser plugin never names it. * - * No append_arrow_stream: parsers are inherently per-message. The host - * internally coalesces per-record appends into Arrow batches before - * committing to storage — plugin authors never see the batch grain. */ + * append_arrow_stream is an optional tail slot for parser-shaped formats + * that naturally decode a batch in one parse() call. Ownership matches + * PJ_source_write_host_vtable_t::append_arrow_stream. Plugins must gate + * this slot with PJ_HAS_TAIL_SLOT when calling through the C ABI directly. */ typedef struct PJ_parser_write_host_vtable_t { uint32_t abi_version; uint32_t struct_size; @@ -407,6 +408,13 @@ typedef struct PJ_parser_write_host_vtable_t { bool (*append_bound_record)( void* ctx, int64_t timestamp, const PJ_bound_field_value_t* fields, size_t field_count, PJ_error_t* out_error) PJ_NOEXCEPT; + + /* [stream-thread] Optional batch path. Plugin hands ownership of an Arrow + * C Data Interface stream for the bound topic. The timestamp column rule + * and success/failure ownership contract are identical to + * PJ_source_write_host_vtable_t::append_arrow_stream. */ + bool (*append_arrow_stream)( + void* ctx, struct ArrowArrayStream* stream, PJ_string_view_t timestamp_column, PJ_error_t* out_error) PJ_NOEXCEPT; } PJ_parser_write_host_vtable_t; typedef struct { diff --git a/pj_base/include/pj_base/sdk/arrow.hpp b/pj_base/include/pj_base/sdk/arrow.hpp index 9ec3eb7..a33827f 100644 --- a/pj_base/include/pj_base/sdk/arrow.hpp +++ b/pj_base/include/pj_base/sdk/arrow.hpp @@ -130,13 +130,16 @@ using ArrowArrayHolder = detail::ArrowHolder<::ArrowArray>; /// /// Recommended usage: hand the holder by rvalue reference to the /// `appendArrowStream(ArrowStreamHolder&&, ...)` overload on -/// `SourceWriteHostView` / `ToolboxHostView`, which disarms the holder on -/// success: +/// `SourceWriteHostView`, `ParserWriteHostView`, or `ToolboxHostView`, +/// which disarms the holder on success: /// /// ArrowStreamHolder stream(buildMyStream()); /// auto status = writeHost.appendArrowStream(topic, std::move(stream), "timestamp"); /// // on success, holder is inert; on failure, destructor releases the stream. /// +/// Parser write hosts omit the `topic` argument because the host binds the +/// parser to one topic before parsing begins. +/// /// The raw-pointer overload of `appendArrowStream` remains as an ABI escape /// hatch for callers that own the stream through some other mechanism. using ArrowStreamHolder = detail::ArrowHolder<::ArrowArrayStream>; diff --git a/pj_base/include/pj_base/sdk/plugin_data_api.hpp b/pj_base/include/pj_base/sdk/plugin_data_api.hpp index aa3541e..9b1d779 100644 --- a/pj_base/include/pj_base/sdk/plugin_data_api.hpp +++ b/pj_base/include/pj_base/sdk/plugin_data_api.hpp @@ -243,7 +243,6 @@ class CatalogSnapshot { // // Arrow C Data Interface is the canonical bulk path (appendArrowStream). // Per-record helpers remain for streaming producers and simple plugins. -// The parser write host is strictly per-record — host coalesces internally. // --------------------------------------------------------------------------- // --- PJ_error_t helpers ------------------------------------------------------ @@ -439,6 +438,9 @@ class SourceWriteHostView { if (!valid()) { return unexpected("source write host is not bound"); } + if (!PJ_HAS_TAIL_SLOT(PJ_source_write_host_vtable_t, host_.vtable, append_arrow_stream)) { + return unexpected("source write host does not support append_arrow_stream"); + } PJ_error_t err{}; if (!host_.vtable->append_arrow_stream(host_.ctx, topic, stream, toAbiString(timestamp_column), &err)) { return unexpected(errorToString(err)); @@ -509,6 +511,36 @@ class ParserWriteHostView { return appendBoundRecord(timestamp, Span(fields.begin(), fields.size())); } + /// Bulk-write via Arrow C Data Interface into the parser's bound topic. + /// Recommended overload — takes an `ArrowStreamHolder` by rvalue reference + /// and disarms it on success. Same ownership rule as + /// `SourceWriteHostView::appendArrowStream`. + [[nodiscard]] Status appendArrowStream( + ArrowStreamHolder&& stream, std::string_view timestamp_column = "timestamp") const { + auto status = appendArrowStream(stream.get(), timestamp_column); + if (status) { + (void)stream.release(); + } + return status; + } + + /// Raw-pointer overload — ABI escape hatch. Prefer the rvalue-ref version + /// above. Ownership contract matches SourceWriteHostView::appendArrowStream. + [[nodiscard]] Status appendArrowStream( + struct ArrowArrayStream* stream, std::string_view timestamp_column = "timestamp") const { + if (!valid()) { + return unexpected("parser write host is not bound"); + } + if (!PJ_HAS_TAIL_SLOT(PJ_parser_write_host_vtable_t, host_.vtable, append_arrow_stream)) { + return unexpected("parser write host does not support append_arrow_stream"); + } + PJ_error_t err{}; + if (!host_.vtable->append_arrow_stream(host_.ctx, stream, toAbiString(timestamp_column), &err)) { + return unexpected(errorToString(err)); + } + return okStatus(); + } + [[nodiscard]] const PJ_parser_write_host_t& raw() const noexcept { return host_; } @@ -1053,6 +1085,9 @@ class ToolboxHostView { if (!valid()) { return unexpected("toolbox host is not bound"); } + if (!PJ_HAS_TAIL_SLOT(PJ_toolbox_host_vtable_t, host_.vtable, append_arrow_stream)) { + return unexpected("toolbox host does not support append_arrow_stream"); + } PJ_error_t err{}; if (!host_.vtable->append_arrow_stream(host_.ctx, topic, stream, toAbiString(timestamp_column), &err)) { return unexpected(errorToString(err)); @@ -1064,6 +1099,9 @@ class ToolboxHostView { if (!valid()) { return unexpected("toolbox host is not bound"); } + if (!PJ_HAS_TAIL_SLOT(PJ_toolbox_host_vtable_t, host_.vtable, acquire_catalog_snapshot)) { + return unexpected("toolbox host does not support acquire_catalog_snapshot"); + } PJ_catalog_snapshot_t raw{}; PJ_error_t err{}; if (!host_.vtable->acquire_catalog_snapshot(host_.ctx, &raw, &err)) { @@ -1087,6 +1125,9 @@ class ToolboxHostView { if (out_schema == nullptr || out_array == nullptr) { return unexpected("readSeriesArrow: out_schema and out_array must not be null"); } + if (!PJ_HAS_TAIL_SLOT(PJ_toolbox_host_vtable_t, host_.vtable, read_series_arrow)) { + return unexpected("toolbox host does not support read_series_arrow"); + } PJ_error_t err{}; if (!host_.vtable->read_series_arrow(host_.ctx, field, out_schema, out_array, &err)) { return unexpected(errorToString(err)); @@ -1107,9 +1148,9 @@ class ToolboxHostView { } ArrowSchemaHolder schema; ArrowArrayHolder array; - PJ_error_t err{}; - if (!host_.vtable->read_series_arrow(host_.ctx, field, schema.out(), array.out(), &err)) { - return unexpected(errorToString(err)); + auto status = readSeriesArrow(field, schema.out(), array.out()); + if (!status) { + return unexpected(status.error()); } return MaterializedSeriesView(std::move(schema), std::move(array)); } diff --git a/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp b/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp index 86b1a53..1dbf09e 100644 --- a/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp +++ b/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp @@ -24,6 +24,7 @@ */ #pragma once +#include #include #include #include @@ -73,13 +74,15 @@ class ParserWriteRecorder { /// Build a PJ_parser_write_host_t whose context points at *this*. The /// recorder must outlive the host handle. [[nodiscard]] PJ_parser_write_host_t makeHost() noexcept { - static const PJ_parser_write_host_vtable_t vtable = { - .abi_version = PJ_PLUGIN_DATA_API_VERSION, - .struct_size = sizeof(PJ_parser_write_host_vtable_t), - .ensure_field = &ParserWriteRecorder::trampolineEnsureField, - .append_record = &ParserWriteRecorder::trampolineAppendRecord, - .append_bound_record = &ParserWriteRecorder::trampolineAppendBoundRecord, - }; + static const PJ_parser_write_host_vtable_t vtable = [] { + PJ_parser_write_host_vtable_t table{}; + table.abi_version = PJ_PLUGIN_DATA_API_VERSION; + table.struct_size = offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream); + table.ensure_field = &ParserWriteRecorder::trampolineEnsureField; + table.append_record = &ParserWriteRecorder::trampolineAppendRecord; + table.append_bound_record = &ParserWriteRecorder::trampolineAppendBoundRecord; + return table; + }(); return PJ_parser_write_host_t{.ctx = this, .vtable = &vtable}; } diff --git a/pj_base/tests/abi_layout_sentinels_test.cpp b/pj_base/tests/abi_layout_sentinels_test.cpp index ccbf2fe..1206f9f 100644 --- a/pj_base/tests/abi_layout_sentinels_test.cpp +++ b/pj_base/tests/abi_layout_sentinels_test.cpp @@ -93,6 +93,27 @@ static_assert(sizeof(PJ_toolbox_vtable_t) == 88, "Toolbox vtable size (update de static_assert(PJ_TOOLBOX_MIN_VTABLE_SIZE == 88, "MIN vtable size is pinned at v4.0 — NEVER INCREASE"); static_assert(PJ_TOOLBOX_MIN_VTABLE_SIZE <= sizeof(PJ_toolbox_vtable_t), "MIN must never exceed current"); +// --- Write-host vtables (ABI-APPENDABLE within v4) -------------------------- +static_assert(offsetof(PJ_source_write_host_vtable_t, abi_version) == 0, "source write host prefix pinned"); +static_assert(offsetof(PJ_source_write_host_vtable_t, struct_size) == 4, "source write host prefix pinned"); +static_assert(offsetof(PJ_source_write_host_vtable_t, append_bound_record) == 32, "source write host baseline pinned"); +static_assert(offsetof(PJ_source_write_host_vtable_t, append_arrow_stream) == 40, "source write host bulk slot pinned"); +static_assert(sizeof(PJ_source_write_host_vtable_t) == 48, "Source write host size"); + +static_assert(offsetof(PJ_parser_write_host_vtable_t, abi_version) == 0, "parser write host prefix pinned"); +static_assert(offsetof(PJ_parser_write_host_vtable_t, struct_size) == 4, "parser write host prefix pinned"); +static_assert(offsetof(PJ_parser_write_host_vtable_t, append_bound_record) == 24, "parser write host baseline pinned"); +static_assert( + offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream) == 32, "parser write host bulk tail slot pinned"); +static_assert(sizeof(PJ_parser_write_host_vtable_t) == 40, "Parser write host size updated deliberately on append"); + +static_assert(offsetof(PJ_toolbox_host_vtable_t, abi_version) == 0, "toolbox host prefix pinned"); +static_assert(offsetof(PJ_toolbox_host_vtable_t, struct_size) == 4, "toolbox host prefix pinned"); +static_assert(offsetof(PJ_toolbox_host_vtable_t, append_bound_record) == 40, "toolbox host baseline pinned"); +static_assert(offsetof(PJ_toolbox_host_vtable_t, append_arrow_stream) == 48, "toolbox host bulk slot pinned"); +static_assert(offsetof(PJ_toolbox_host_vtable_t, read_series_arrow) == 64, "toolbox host read slot pinned"); +static_assert(sizeof(PJ_toolbox_host_vtable_t) == 72, "Toolbox host size"); + // --- ABI version symbol ------------------------------------------------------ static_assert(PJ_ABI_VERSION == 4, "v4 ABI version"); diff --git a/pj_base/tests/plugin_data_api_test.cpp b/pj_base/tests/plugin_data_api_test.cpp index 521f155..2c5b6c8 100644 --- a/pj_base/tests/plugin_data_api_test.cpp +++ b/pj_base/tests/plugin_data_api_test.cpp @@ -2,6 +2,9 @@ #include +#include +#include +#include #include #include "pj_base/sdk/plugin_data_api.hpp" @@ -16,6 +19,64 @@ static_assert(std::is_standard_layout_v); static_assert(std::is_standard_layout_v); static_assert(std::is_standard_layout_v); static_assert(std::is_standard_layout_v); +static_assert( + offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream) == + offsetof(PJ_parser_write_host_vtable_t, append_bound_record) + + sizeof(static_cast(nullptr)->append_bound_record), + "Parser append_arrow_stream must stay appended after the v4.0 baseline"); +static_assert( + sizeof(PJ_parser_write_host_vtable_t) == + offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream) + + sizeof(static_cast(nullptr)->append_arrow_stream), + "Parser write host size should only grow by tail appends"); + +struct TailSlotRecorder { + bool called = false; +}; + +bool sourceAppendArrowStream( + void* ctx, PJ_topic_handle_t, struct ArrowArrayStream*, PJ_string_view_t, PJ_error_t*) noexcept { + static_cast(ctx)->called = true; + return true; +} + +bool parserEnsureField( + void*, PJ_string_view_t, PJ_primitive_type_t, PJ_field_handle_t* out_field, PJ_error_t*) noexcept { + *out_field = PJ_field_handle_t{{1}, 2}; + return true; +} + +bool parserAppendRecord(void*, int64_t, const PJ_named_field_value_t*, std::size_t, PJ_error_t*) noexcept { + return true; +} + +bool parserAppendBoundRecord(void* ctx, int64_t, const PJ_bound_field_value_t*, std::size_t, PJ_error_t*) noexcept { + static_cast(ctx)->called = true; + return true; +} + +bool parserAppendArrowStream(void* ctx, struct ArrowArrayStream*, PJ_string_view_t, PJ_error_t*) noexcept { + static_cast(ctx)->called = true; + return true; +} + +bool toolboxAppendArrowStream( + void* ctx, PJ_topic_handle_t, struct ArrowArrayStream*, PJ_string_view_t, PJ_error_t*) noexcept { + static_cast(ctx)->called = true; + return true; +} + +bool toolboxAcquireCatalogSnapshot(void* ctx, PJ_catalog_snapshot_t* out_snapshot, PJ_error_t*) noexcept { + static_cast(ctx)->called = true; + *out_snapshot = PJ_catalog_snapshot_t{}; + return true; +} + +bool toolboxReadSeriesArrow( + void* ctx, PJ_field_handle_t, struct ArrowSchema*, struct ArrowArray*, PJ_error_t*) noexcept { + static_cast(ctx)->called = true; + return true; +} TEST(PluginDataApiTest, PrimitiveTypeRoundTripsThroughAbiEnum) { EXPECT_EQ(sdk::fromAbiType(sdk::toAbiType(PrimitiveType::kFloat32)), PrimitiveType::kFloat32); @@ -65,5 +126,132 @@ TEST(PluginDataApiTest, ToStringViewHandlesNullData) { EXPECT_NE(result.data(), nullptr); } +TEST(PluginDataApiTest, SourceWriteHostViewRejectsMissingAppendArrowStreamTailSlot) { + TailSlotRecorder recorder; + const PJ_source_write_host_vtable_t vtable = { + .abi_version = PJ_PLUGIN_DATA_API_VERSION, + .struct_size = offsetof(PJ_source_write_host_vtable_t, append_arrow_stream), + .append_arrow_stream = sourceAppendArrowStream, + }; + sdk::SourceWriteHostView view(PJ_source_write_host_t{.ctx = &recorder, .vtable = &vtable}); + + ArrowArrayStream stream{}; + auto status = view.appendArrowStream(PJ_topic_handle_t{1}, &stream); + + EXPECT_FALSE(status); + EXPECT_FALSE(recorder.called); + EXPECT_NE(status.error().find("append_arrow_stream"), std::string::npos); +} + +TEST(PluginDataApiTest, ParserWriteHostViewCurrentBaselineStillWorks) { + TailSlotRecorder recorder; + const PJ_parser_write_host_vtable_t vtable = { + .abi_version = PJ_PLUGIN_DATA_API_VERSION, + .struct_size = sizeof(PJ_parser_write_host_vtable_t), + .ensure_field = parserEnsureField, + .append_record = parserAppendRecord, + .append_bound_record = parserAppendBoundRecord, + }; + sdk::ParserWriteHostView view(PJ_parser_write_host_t{.ctx = &recorder, .vtable = &vtable}); + + auto status = view.appendBoundRecord(123, {}); + + EXPECT_TRUE(status) << status.error(); + EXPECT_TRUE(recorder.called); +} + +TEST(PluginDataApiTest, ParserWriteHostViewRejectsMissingAppendArrowStreamTailSlot) { + TailSlotRecorder recorder; + const PJ_parser_write_host_vtable_t vtable = { + .abi_version = PJ_PLUGIN_DATA_API_VERSION, + .struct_size = offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream), + .ensure_field = parserEnsureField, + .append_record = parserAppendRecord, + .append_bound_record = parserAppendBoundRecord, + .append_arrow_stream = parserAppendArrowStream, + }; + sdk::ParserWriteHostView view(PJ_parser_write_host_t{.ctx = &recorder, .vtable = &vtable}); + + ArrowArrayStream stream{}; + auto status = view.appendArrowStream(&stream); + + EXPECT_FALSE(status); + EXPECT_FALSE(recorder.called); + EXPECT_NE(status.error().find("append_arrow_stream"), std::string::npos); +} + +TEST(PluginDataApiTest, ParserWriteHostViewRoutesAppendArrowStreamTailSlot) { + TailSlotRecorder recorder; + const PJ_parser_write_host_vtable_t vtable = { + .abi_version = PJ_PLUGIN_DATA_API_VERSION, + .struct_size = sizeof(PJ_parser_write_host_vtable_t), + .ensure_field = parserEnsureField, + .append_record = parserAppendRecord, + .append_bound_record = parserAppendBoundRecord, + .append_arrow_stream = parserAppendArrowStream, + }; + sdk::ParserWriteHostView view(PJ_parser_write_host_t{.ctx = &recorder, .vtable = &vtable}); + + ArrowArrayStream stream{}; + auto status = view.appendArrowStream(&stream); + + EXPECT_TRUE(status) << status.error(); + EXPECT_TRUE(recorder.called); +} + +TEST(PluginDataApiTest, ToolboxHostViewRejectsMissingAppendArrowStreamTailSlot) { + TailSlotRecorder recorder; + const PJ_toolbox_host_vtable_t vtable = { + .abi_version = PJ_PLUGIN_DATA_API_VERSION, + .struct_size = offsetof(PJ_toolbox_host_vtable_t, append_arrow_stream), + .append_arrow_stream = toolboxAppendArrowStream, + }; + sdk::ToolboxHostView view(PJ_toolbox_host_t{.ctx = &recorder, .vtable = &vtable}); + + ArrowArrayStream stream{}; + auto status = view.appendArrowStream(PJ_topic_handle_t{1}, &stream); + + EXPECT_FALSE(status); + EXPECT_FALSE(recorder.called); + EXPECT_NE(status.error().find("append_arrow_stream"), std::string::npos); +} + +TEST(PluginDataApiTest, ToolboxHostViewRejectsMissingCatalogSnapshotTailSlot) { + TailSlotRecorder recorder; + const PJ_toolbox_host_vtable_t vtable = { + .abi_version = PJ_PLUGIN_DATA_API_VERSION, + .struct_size = offsetof(PJ_toolbox_host_vtable_t, acquire_catalog_snapshot), + .append_arrow_stream = toolboxAppendArrowStream, + .acquire_catalog_snapshot = toolboxAcquireCatalogSnapshot, + }; + sdk::ToolboxHostView view(PJ_toolbox_host_t{.ctx = &recorder, .vtable = &vtable}); + + auto snapshot = view.catalogSnapshot(); + + EXPECT_FALSE(snapshot); + EXPECT_FALSE(recorder.called); + EXPECT_NE(snapshot.error().find("acquire_catalog_snapshot"), std::string::npos); +} + +TEST(PluginDataApiTest, ToolboxHostViewRejectsMissingReadSeriesTailSlot) { + TailSlotRecorder recorder; + const PJ_toolbox_host_vtable_t vtable = { + .abi_version = PJ_PLUGIN_DATA_API_VERSION, + .struct_size = offsetof(PJ_toolbox_host_vtable_t, read_series_arrow), + .append_arrow_stream = toolboxAppendArrowStream, + .acquire_catalog_snapshot = toolboxAcquireCatalogSnapshot, + .read_series_arrow = toolboxReadSeriesArrow, + }; + sdk::ToolboxHostView view(PJ_toolbox_host_t{.ctx = &recorder, .vtable = &vtable}); + + ArrowSchema schema{}; + ArrowArray array{}; + auto status = view.readSeriesArrow(PJ_field_handle_t{{1}, 2}, &schema, &array); + + EXPECT_FALSE(status); + EXPECT_FALSE(recorder.called); + EXPECT_NE(status.error().find("read_series_arrow"), std::string::npos); +} + } // namespace } // namespace PJ diff --git a/pj_datastore/docs/USER_GUIDE.md b/pj_datastore/docs/USER_GUIDE.md index fca6da5..b2ff291 100644 --- a/pj_datastore/docs/USER_GUIDE.md +++ b/pj_datastore/docs/USER_GUIDE.md @@ -43,15 +43,31 @@ fields.push_back({"counter", value}); // value is int64_t ## 2. Writing Data +DataSource and Toolbox write hosts name the target topic on each write. A +MessageParser write host is already bound to one topic by the host, so parser +calls omit the topic argument and write into that bound topic. + ### Step 1: Get a topic handle +DataSource and Toolbox plugins create or resolve topics explicitly: + ```cpp auto topic = writeHost().ensureTopic("sensor/imu"); if (!topic) { /* handle error */ } ``` +MessageParser plugins do not call `ensureTopic()` on the parser write host. +They only create fields inside the already-bound topic: + +```cpp +auto field = writeHost().ensureField("temperature", PJ::PrimitiveType::kFloat64); +if (!field) { /* handle error */ } +``` + ### Step 2 (optional): Pre-register fields for the bound-write fast path +For DataSource and Toolbox writers: + ```cpp writeHost().ensureField(*topic, "accel.x", PJ::PrimitiveType::kFloat64); writeHost().ensureField(*topic, "accel.y", PJ::PrimitiveType::kFloat64); @@ -66,7 +82,8 @@ path and avoids mid-stream chunk sealing. ### Step 3: Append records -**By name** (flexible, resolves names each call): +**By name** (flexible, resolves names each call). DataSource and Toolbox +writers pass the topic: ```cpp std::vector fields = { @@ -78,7 +95,18 @@ std::vector fields = { auto status = writeHost().appendRecord(*topic, timestamp_ns, PJ::Span(fields)); ``` -**By handle** (pre-resolved, faster for high-rate data): +MessageParser writers omit the topic: + +```cpp +std::vector fields = { + {"temperature", 23.5}, + {"humidity", 61.0}, +}; +auto status = writeHost().appendRecord(timestamp_ns, PJ::Span(fields)); +``` + +**By handle** (pre-resolved, faster for high-rate data). DataSource and +Toolbox writers pass the topic: ```cpp auto fx = writeHost().ensureField(*topic, "accel.x", PJ::PrimitiveType::kFloat64); @@ -92,6 +120,14 @@ std::vector bound = { writeHost().appendBoundRecord(*topic, timestamp_ns, PJ::Span(bound)); ``` +Parser writers use the field handles from their bound topic: + +```cpp +auto temp = writeHost().ensureField("temperature", PJ::PrimitiveType::kFloat64); +std::vector bound = {{*temp, 23.5}}; +writeHost().appendBoundRecord(timestamp_ns, PJ::Span(bound)); +``` + ### Sparse Records Not every field needs data on every row. Fields omitted from `appendRecord()` are automatically null-filled. This is the correct way to handle sparse data: @@ -117,15 +153,24 @@ fields.push_back({prefix + "/" + key, value}); // safe — name is owned ### Bulk Arrow Import -For high-throughput file importers that already have Arrow data, use the -Arrow C Data Interface (`ArrowArrayStream`). The byte-based `appendArrowIpc` -slot was removed in ABI v4. +For high-throughput imports or parser-shaped payloads that already have Arrow +data, use the Arrow C Data Interface (`ArrowArrayStream`). The byte-based +`appendArrowIpc` slot was removed in ABI v4. + +DataSource and Toolbox writers pass the destination topic: ```cpp PJ::sdk::ArrowStreamHolder stream(buildMyStream()); auto status = writeHost().appendArrowStream(*topic, std::move(stream), "timestamp"); ``` +Parser writers omit the topic because the parser host is bound to one topic: + +```cpp +PJ::sdk::ArrowStreamHolder stream(buildMyPayloadStream()); +auto status = writeHost().appendArrowStream(std::move(stream), "timestamp"); +``` + `ArrowStreamHolder` is an RAII wrapper that auto-releases the stream; the `std::move` overload disarms it on success. See `pj_base/sdk/arrow.hpp` for the holder + stream-builder helpers. diff --git a/pj_datastore/src/plugin_data_host.cpp b/pj_datastore/src/plugin_data_host.cpp index c1ca26b..37022bc 100644 --- a/pj_datastore/src/plugin_data_host.cpp +++ b/pj_datastore/src/plugin_data_host.cpp @@ -1077,6 +1077,21 @@ bool parserAppendBoundRecord( }); } +bool parserAppendArrowStream( + void* ctx, struct ArrowArrayStream* stream, PJ_string_view_t timestamp_column, PJ_error_t* out_error) noexcept { + return guardHostCallback(out_error, [&] { + auto* impl = static_cast(ctx); + if (!impl->core.appendArrowStream(impl->topic, stream, timestamp_column)) { + propagateError(out_error, impl->core.lastError()); + return false; + } + if (stream != nullptr && stream->release != nullptr) { + stream->release(stream); + } + return true; + }); +} + bool toolboxCreateDataSource( void* ctx, PJ_string_view_t name, DataSourceHandle* out_source, PJ_error_t* out_error) noexcept { return guardHostCallback(out_error, [&] { @@ -1563,8 +1578,9 @@ const PJ_source_write_host_vtable_t kSourceWriteVTable = { }; const PJ_parser_write_host_vtable_t kParserWriteVTable = { - PJ_PLUGIN_DATA_API_VERSION, sizeof(PJ_parser_write_host_vtable_t), parserEnsureField, parserAppendRecord, - parserAppendBoundRecord, + PJ_PLUGIN_DATA_API_VERSION, sizeof(PJ_parser_write_host_vtable_t), + parserEnsureField, parserAppendRecord, + parserAppendBoundRecord, parserAppendArrowStream, }; const PJ_toolbox_host_vtable_t kToolboxVTable = { diff --git a/pj_datastore/tests/arrow_stream_round_trip_test.cpp b/pj_datastore/tests/arrow_stream_round_trip_test.cpp index ca02d2f..ed3f46e 100644 --- a/pj_datastore/tests/arrow_stream_round_trip_test.cpp +++ b/pj_datastore/tests/arrow_stream_round_trip_test.cpp @@ -2,9 +2,10 @@ * @file arrow_stream_round_trip_test.cpp * @brief End-to-end round trip through the v4 Arrow C Data Interface path. * - * Writes a known small time series into the datastore via - * DatastoreSourceWriteHost::append_arrow_stream (the v4 ABI slot), then reads - * it back via DatastoreToolboxHost::read_series_arrow, and verifies values. + * Writes known small time series into the datastore via + * DatastoreSourceWriteHost::append_arrow_stream and + * DatastoreParserWriteHost::append_arrow_stream (the v4 ABI slots), then + * reads them back via DatastoreToolboxHost::read_series_arrow. * * This exercises the Phase 1b host-side implementation without going through * a dlopen'd plugin — all ABI calls are made directly on the C vtable. @@ -204,5 +205,79 @@ TEST(ArrowStreamRoundTripTest, WriteViaAppendArrowStreamReadViaReadSeriesArrow) EXPECT_EQ(out_array.release, nullptr); } +TEST(ArrowStreamRoundTripTest, ParserWriteHostAppendArrowStreamWritesBoundTopic) { + DataEngine engine; + auto td_id = engine.createTimeDomain("parser_td"); + ASSERT_TRUE(td_id.has_value()) << td_id.error(); + auto ds_id = engine.createDataset(DatasetDescriptor{.source_name = "parser", .time_domain_id = *td_id}); + ASSERT_TRUE(ds_id.has_value()) << ds_id.error(); + + DatastoreSourceWriteHost source_write_host(engine, PJ_data_source_handle_t{static_cast(*ds_id)}); + auto source_vtable = source_write_host.raw(); + + PJ_topic_handle_t topic{}; + PJ_error_t err{}; + PJ_string_view_t topic_name{"parser_metric", 13}; + ASSERT_TRUE(source_vtable.vtable->ensure_topic(source_vtable.ctx, topic_name, &topic, &err)) << err.message; + + DatastoreParserWriteHost parser_write_host(engine, topic); + auto parser_vtable = parser_write_host.raw(); + + const std::vector timestamps = {10, 20, 30}; + const std::vector values = {7.0, 8.5, 9.25}; + auto built = makeStream(timestamps, values); + + ArrowArrayStream stream{}; + initOneBatchStream(&stream, std::move(built)); + + PJ_string_view_t ts_col_name{"ts_col", 6}; + ASSERT_TRUE(parser_vtable.vtable->append_arrow_stream(parser_vtable.ctx, &stream, ts_col_name, &err)) << err.message; + EXPECT_EQ(stream.release, nullptr); + + parser_write_host.flushPending(); + + DatastoreToolboxHost tb_host(engine); + auto tb_vtable = tb_host.raw(); + + PJ_catalog_snapshot_t snapshot{}; + ASSERT_TRUE(tb_vtable.vtable->acquire_catalog_snapshot(tb_vtable.ctx, &snapshot, &err)) << err.message; + + PJ_field_handle_t value_field{}; + bool value_found = false; + for (std::size_t i = 0; i < snapshot.field_count; ++i) { + const auto& f = snapshot.fields[i]; + if (std::string(f.name.data, f.name.size).find("value") != std::string::npos) { + value_field = f.handle; + value_found = true; + break; + } + } + snapshot.release(snapshot.release_ctx); + ASSERT_TRUE(value_found) << "field 'value' missing from catalog"; + + ArrowSchema out_schema{}; + ArrowArray out_array{}; + ASSERT_TRUE(tb_vtable.vtable->read_series_arrow(tb_vtable.ctx, value_field, &out_schema, &out_array, &err)) + << err.message; + ASSERT_NE(out_schema.release, nullptr); + ASSERT_NE(out_array.release, nullptr); + + nanoarrow::UniqueArrayView view; + ArrowError vf_err; + ASSERT_EQ(ArrowArrayViewInitFromSchema(view.get(), &out_schema, &vf_err), NANOARROW_OK) << vf_err.message; + ASSERT_EQ(ArrowArrayViewSetArray(view.get(), &out_array, &vf_err), NANOARROW_OK) << vf_err.message; + + ASSERT_EQ(out_array.length, static_cast(timestamps.size())); + for (int64_t i = 0; i < out_array.length; ++i) { + EXPECT_EQ(ArrowArrayViewGetIntUnsafe(view->children[0], i), timestamps[static_cast(i)]); + EXPECT_DOUBLE_EQ(ArrowArrayViewGetDoubleUnsafe(view->children[1], i), values[static_cast(i)]); + } + + out_schema.release(&out_schema); + out_array.release(&out_array); + EXPECT_EQ(out_schema.release, nullptr); + EXPECT_EQ(out_array.release, nullptr); +} + } // namespace } // namespace PJ diff --git a/pj_plugins/docs/ARCHITECTURE.md b/pj_plugins/docs/ARCHITECTURE.md index 223d009..f492fdf 100644 --- a/pj_plugins/docs/ARCHITECTURE.md +++ b/pj_plugins/docs/ARCHITECTURE.md @@ -428,7 +428,9 @@ All three share a common internal `WriteCore` that handles: ### Arrow C Data Interface ownership rules The v4 write path, `append_arrow_stream(ctx, topic, stream, -timestamp_column, err)`: +timestamp_column, err)` for source/toolbox hosts and +`append_arrow_stream(ctx, stream, timestamp_column, err)` for the +parser host: - The plugin constructs the `ArrowArrayStream` (typically via nanoarrow's `ArrowIpcArrayStreamReaderInit`, Parquet's @@ -448,6 +450,9 @@ timestamp_column, err)`: nanoseconds since Unix epoch. Passing an empty view means "synthesise a monotonic timestamp per row"; useful for streams with no natural time axis. +- Parser writes are already bound to one topic by the host service, so + the parser variant does not take a topic handle. Ownership rules are + otherwise identical. The v4 read path, `read_series_arrow(ctx, field, out_schema, out_array, err)`: diff --git a/pj_plugins/docs/REQUIREMENTS.md b/pj_plugins/docs/REQUIREMENTS.md index 64c5bec..1dca15a 100644 --- a/pj_plugins/docs/REQUIREMENTS.md +++ b/pj_plugins/docs/REQUIREMENTS.md @@ -97,9 +97,9 @@ only sees operations it is allowed to call. These services support: - **Bulk Arrow writes** — `appendArrowStream()` hands an `ArrowArrayStream*` (Arrow C Data Interface) to the host, which pulls all batches and takes ownership on success. This is the canonical - path for file-based sources and toolbox bulk imports. The parser - write surface is per-record only — the host coalesces parser output - into Arrow batches internally before committing to storage. + path for file-based sources, toolbox bulk imports, and parser-shaped + payloads that naturally decode many rows in one `parse()` call. Parser + batch writes are topic-scoped; the parser never names the topic. - **Topic and field management** — `ensureTopic()`, `ensureField()`, where available for the family. Parsers are bound to one topic and only create fields within that topic. diff --git a/pj_plugins/docs/message-parser-guide.md b/pj_plugins/docs/message-parser-guide.md index f64d7d7..89510e8 100644 --- a/pj_plugins/docs/message-parser-guide.md +++ b/pj_plugins/docs/message-parser-guide.md @@ -1,10 +1,10 @@ # Writing a MessageParser Plugin > **Tracks the v4 plugin ABI** (`PJ_ABI_VERSION == 4`). The parser -> write-host stays per-record in v4 (parsers decode one message at a -> time; the host coalesces into Arrow batches internally before -> committing to storage). For ABI evolution rules, error semantics, and -> noexcept discipline see `ARCHITECTURE.md`. +> write-host supports per-record writes and an optional Arrow stream +> batch path for parser-shaped formats that naturally decode batches. +> For ABI evolution rules, error semantics, and noexcept discipline see +> `ARCHITECTURE.md`. > **Vocabulary used throughout this guide**: > - `PJ::Status` — alias for `PJ::Expected`. Return `PJ::okStatus()` / @@ -16,16 +16,19 @@ ## When MessageParser is the Wrong Choice -The parser write surface is **per-record only** in v4. If your input is -naturally batch-shaped (Parquet, MCAP indices, Arrow IPC byte streams), prefer -a **DataSource** that calls `writeHost().appendArrowStream(...)` unless the -encoding is strongly parser-shaped. Writing a batch producer as a parser leaves -throughput on the table and forces awkward single-record loops. +If your input is a complete file/container or source-owned stream +(Parquet file, MCAP bag, database export), prefer a **DataSource** that calls +`writeHost().appendArrowStream(...)`. Use a MessageParser when the host has +already selected a topic/encoding and is handing you payloads to decode. If +one `parse()` call naturally yields a batch, the parser write host can accept +that batch via `writeHost().appendArrowStream(...)`. A MessageParser is the right choice when: - Each message is independently decodable (JSON line, single Protobuf message, ROS message, Influx line). - You produce up to a few dozen named numeric fields per call. +- Or, one payload for a selected encoding naturally expands to a batch for the + parser's bound topic. - The encoding may be paired with multiple transports (a Protobuf parser used by both an MQTT and a ZMQ source). @@ -180,16 +183,13 @@ topic. | `ensureField(name, type)` | Optional: pre-register a field. Enables `appendBoundRecord`. Returns a `FieldHandle`. | | `appendRecord(timestamp, fields)` | Write a row of named field values. Auto-creates new fields. | | `appendBoundRecord(timestamp, fields)` | Write using pre-resolved field handles (faster). | +| `appendArrowStream(stream, ts_col)` | Optional batch path. Hand an `ArrowArrayStream*` to the host for the parser's bound topic. Success transfers ownership to the host; failure leaves ownership with the plugin. | -The parser write surface is **per-record only** in v4. There is no -`appendArrowStream` / `appendArrowIpc` slot on the parser write host: -one `parse()` call decodes one message, so batch boundaries are the -host's concern, not the parser's. The host coalesces per-record -writes into Arrow batches internally before committing them to -storage. If you are porting a plugin that used to emit whole IPC -streams directly (a Parquet-to-Arrow bulk loader, for example), it -belongs as a **DataSource** plugin instead — see -`data-source-guide.md` for the `appendArrowStream` contract. +Most parsers should use `appendRecord()` or `appendBoundRecord()` because one +`parse()` call normally decodes one logical message. Use `appendArrowStream()` +when a single payload naturally contains many rows for the parser's already +bound topic. This keeps parser-shaped batch encodings in the parser family +without forcing per-row loops. ### Named vs bound writes @@ -220,6 +220,17 @@ const PJ::sdk::BoundFieldValue fields[] = { writeHost().appendBoundRecord(timestamp_ns, PJ::Span(fields)); ``` +For payloads that decode directly into an Arrow stream, prefer the RAII +overload so ownership is handled correctly: + +```cpp +PJ::sdk::ArrowStreamHolder stream(build_payload_stream(payload)); +auto status = writeHost().appendArrowStream(std::move(stream), "timestamp"); +if (!status) { + return status; +} +``` + ## Optional Features ### Schema binding @@ -522,6 +533,6 @@ dispatch code. | Host cannot bind the parser at runtime | Manifest missing the `encoding` array entirely | Add `"encoding": ["…"]` (required for parsers) | | First `parse()` after restart fails silently | Schema-derived state lost; `bindSchema()` was not called for this encoding because none is needed | Initialize lazily in `parse()` for schema-less encodings | | `appendBoundRecord()` returns failure | Field handles obtained before `bind(registry)` completed (or reused after `destroy`) | Acquire field handles inside `parse()` or after `bind(registry)`; never cache them across instances | -| Throughput is one quarter of the source's input rate | Wrong family — the source emits batches but the parser decodes one record at a time | Move bulk decoding to a DataSource that calls `appendArrowStream()` | +| Throughput is one quarter of the source's input rate | Parser decodes a naturally batched payload one row at a time | If the payload is parser-shaped, use parser `appendArrowStream()`; if it is a whole source/container, move bulk decoding to a DataSource | | Embedded timestamp ignored even when configured | Parser uses the host-supplied `timestamp_ns` and never extracts from payload | Read the config flag in `loadConfig`; choose between extracted and host timestamps inside `parse()` | | Memory grows unboundedly across `parse()` calls | Per-message state leaked (e.g. descriptor pools rebuilt per call) | Build schema-derived caches in `bindSchema()` or `loadConfig()`, not in `parse()` | From 153b7e1802b10c1b5592ff228a1d817934eea458 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Wed, 6 May 2026 16:35:20 +0200 Subject: [PATCH 09/10] fix: harden dialog plugin loading --- pj_base/include/pj_base/plugin_data_api.h | 7 ++ .../sdk/testing/parser_write_recorder.hpp | 2 +- pj_base/tests/abi_layout_sentinels_test.cpp | 3 + pj_plugins/CMakeLists.txt | 20 +++-- pj_plugins/dialog_protocol/CMakeLists.txt | 16 ++++ .../dialog_protocol/src/dialog_library.cpp | 80 ++++--------------- .../tests/dialog_library_test.cpp | 20 +++++ .../tests/missing_dialog_abi_plugin.cpp | 68 ++++++++++++++++ .../missing_dialog_required_slots_plugin.cpp | 66 +++++++++++++++ .../tests/plugin_lifecycle_test.cpp | 13 +++ pj_plugins/src/data_source_library.cpp | 3 + pj_plugins/src/detail/vtable_validation.cpp | 17 ++++ pj_plugins/src/detail/vtable_validation.hpp | 2 + pj_plugins/src/message_parser_library.cpp | 3 + pj_plugins/src/plugin_catalog.cpp | 6 +- pj_plugins/src/toolbox_library.cpp | 3 + pj_plugins/tests/plugin_catalog_test.cpp | 12 +++ .../tests/source_dialog_integration_test.cpp | 27 +++++++ 18 files changed, 293 insertions(+), 75 deletions(-) create mode 100644 pj_plugins/dialog_protocol/tests/missing_dialog_abi_plugin.cpp create mode 100644 pj_plugins/dialog_protocol/tests/missing_dialog_required_slots_plugin.cpp diff --git a/pj_base/include/pj_base/plugin_data_api.h b/pj_base/include/pj_base/plugin_data_api.h index 8e5bbbf..37a6173 100644 --- a/pj_base/include/pj_base/plugin_data_api.h +++ b/pj_base/include/pj_base/plugin_data_api.h @@ -417,6 +417,13 @@ typedef struct PJ_parser_write_host_vtable_t { void* ctx, struct ArrowArrayStream* stream, PJ_string_view_t timestamp_column, PJ_error_t* out_error) PJ_NOEXCEPT; } PJ_parser_write_host_vtable_t; +/* + * Parser write-host v4.0 floor, before append_arrow_stream was added as an + * optional tail slot. Hosts/plugins that care about the batch path must use + * PJ_HAS_TAIL_SLOT(PJ_parser_write_host_vtable_t, vtable, append_arrow_stream). + */ +#define PJ_PARSER_WRITE_HOST_MIN_VTABLE_SIZE (offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream)) + typedef struct { void* ctx; const PJ_parser_write_host_vtable_t* vtable; diff --git a/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp b/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp index 1dbf09e..3405002 100644 --- a/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp +++ b/pj_base/include/pj_base/sdk/testing/parser_write_recorder.hpp @@ -77,7 +77,7 @@ class ParserWriteRecorder { static const PJ_parser_write_host_vtable_t vtable = [] { PJ_parser_write_host_vtable_t table{}; table.abi_version = PJ_PLUGIN_DATA_API_VERSION; - table.struct_size = offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream); + table.struct_size = PJ_PARSER_WRITE_HOST_MIN_VTABLE_SIZE; table.ensure_field = &ParserWriteRecorder::trampolineEnsureField; table.append_record = &ParserWriteRecorder::trampolineAppendRecord; table.append_bound_record = &ParserWriteRecorder::trampolineAppendBoundRecord; diff --git a/pj_base/tests/abi_layout_sentinels_test.cpp b/pj_base/tests/abi_layout_sentinels_test.cpp index 1206f9f..82872fe 100644 --- a/pj_base/tests/abi_layout_sentinels_test.cpp +++ b/pj_base/tests/abi_layout_sentinels_test.cpp @@ -106,6 +106,9 @@ static_assert(offsetof(PJ_parser_write_host_vtable_t, append_bound_record) == 24 static_assert( offsetof(PJ_parser_write_host_vtable_t, append_arrow_stream) == 32, "parser write host bulk tail slot pinned"); static_assert(sizeof(PJ_parser_write_host_vtable_t) == 40, "Parser write host size updated deliberately on append"); +static_assert(PJ_PARSER_WRITE_HOST_MIN_VTABLE_SIZE == 32, "Parser write host min size remains v4.0 baseline"); +static_assert( + PJ_PARSER_WRITE_HOST_MIN_VTABLE_SIZE <= sizeof(PJ_parser_write_host_vtable_t), "MIN must never exceed current"); static_assert(offsetof(PJ_toolbox_host_vtable_t, abi_version) == 0, "toolbox host prefix pinned"); static_assert(offsetof(PJ_toolbox_host_vtable_t, struct_size) == 4, "toolbox host prefix pinned"); diff --git a/pj_plugins/CMakeLists.txt b/pj_plugins/CMakeLists.txt index 7d6498f..bc18e7e 100644 --- a/pj_plugins/CMakeLists.txt +++ b/pj_plugins/CMakeLists.txt @@ -1,19 +1,23 @@ -add_subdirectory(dialog_protocol) - -# --------------------------------------------------------------------------- -# pj_plugin_catalog — host-side embedded-manifest discovery -# --------------------------------------------------------------------------- - find_package(fmt REQUIRED) find_package(nlohmann_json REQUIRED) add_library(pj_plugin_loader_detail STATIC src/detail/vtable_validation.cpp ) +target_include_directories(pj_plugin_loader_detail PUBLIC + ${CMAKE_CURRENT_SOURCE_DIR}/src + ${CMAKE_CURRENT_SOURCE_DIR}/dialog_protocol/include +) target_compile_features(pj_plugin_loader_detail PUBLIC cxx_std_20) target_compile_options(pj_plugin_loader_detail PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(pj_plugin_loader_detail PUBLIC pj_base) +add_subdirectory(dialog_protocol) + +# --------------------------------------------------------------------------- +# pj_plugin_catalog — host-side embedded-manifest discovery +# --------------------------------------------------------------------------- + add_library(pj_plugin_catalog STATIC src/plugin_catalog.cpp ) @@ -294,6 +298,7 @@ target_compile_definitions(plugin_catalog_test PRIVATE PJ_MOCK_DIALOG_PLUGIN_PATH="$" PJ_STATIC_MANIFEST_DIALOG_PLUGIN_PATH="$" PJ_LEGACY_MACRO_DIALOG_PLUGIN_PATH="$" + PJ_MISSING_DIALOG_REQUIRED_SLOTS_PLUGIN_PATH="$" PJ_MISSING_ID_PLUGIN_PATH="$" PJ_INVALID_OPTIONAL_PLUGIN_PATH="$" PJ_MISSING_REQUIRED_SLOTS_PLUGIN_PATH="$" @@ -305,7 +310,8 @@ target_link_libraries(plugin_catalog_test PRIVATE add_dependencies(plugin_catalog_test mock_data_source_plugin mock_json_parser_plugin mock_toolbox_plugin mock_dialog_plugin missing_id_data_source_plugin invalid_optional_manifest_data_source_plugin missing_required_slots_plugin - static_manifest_dialog_plugin legacy_macro_dialog_plugin) + static_manifest_dialog_plugin legacy_macro_dialog_plugin + missing_dialog_required_slots_plugin) add_test(NAME plugin_catalog_test COMMAND plugin_catalog_test) endif() # PJ_BUILD_TESTS diff --git a/pj_plugins/dialog_protocol/CMakeLists.txt b/pj_plugins/dialog_protocol/CMakeLists.txt index b7e4d69..2271971 100644 --- a/pj_plugins/dialog_protocol/CMakeLists.txt +++ b/pj_plugins/dialog_protocol/CMakeLists.txt @@ -44,6 +44,7 @@ target_link_libraries(pj_dialog_library pj_dialog_host pj_base PRIVATE + pj_plugin_loader_detail ${CMAKE_DL_LIBS} ) @@ -55,6 +56,14 @@ add_library(mock_dialog_plugin SHARED examples/mock_dialog.cpp) target_compile_options(mock_dialog_plugin PRIVATE ${PJ_WARNING_FLAGS}) target_link_libraries(mock_dialog_plugin PRIVATE pj_dialog_sdk) +add_library(missing_dialog_abi_plugin SHARED tests/missing_dialog_abi_plugin.cpp) +target_compile_options(missing_dialog_abi_plugin PRIVATE ${PJ_WARNING_FLAGS}) +target_link_libraries(missing_dialog_abi_plugin PRIVATE pj_dialog_protocol) + +add_library(missing_dialog_required_slots_plugin SHARED tests/missing_dialog_required_slots_plugin.cpp) +target_compile_options(missing_dialog_required_slots_plugin PRIVATE ${PJ_WARNING_FLAGS}) +target_link_libraries(missing_dialog_required_slots_plugin PRIVATE pj_dialog_protocol) + # --- Tests --- enable_testing() @@ -107,10 +116,17 @@ add_test(NAME widget_event_builder_test COMMAND widget_event_builder_test) add_executable(dialog_library_test tests/dialog_library_test.cpp) target_compile_definitions(dialog_library_test PRIVATE PJ_MOCK_DIALOG_PLUGIN_PATH="$" + PJ_MISSING_DIALOG_ABI_PLUGIN_PATH="$" + PJ_MISSING_DIALOG_REQUIRED_SLOTS_PLUGIN_PATH="$" ) target_link_libraries(dialog_library_test PRIVATE pj_dialog_library pj_dialog_host pj_dialog_sdk GTest::gtest_main ) +add_dependencies(dialog_library_test + mock_dialog_plugin + missing_dialog_abi_plugin + missing_dialog_required_slots_plugin +) add_test(NAME dialog_library_test COMMAND dialog_library_test) foreach(_test ${PJ_PLUGIN_TESTS}) diff --git a/pj_plugins/dialog_protocol/src/dialog_library.cpp b/pj_plugins/dialog_protocol/src/dialog_library.cpp index 7fc512e..5b8ec27 100644 --- a/pj_plugins/dialog_protocol/src/dialog_library.cpp +++ b/pj_plugins/dialog_protocol/src/dialog_library.cpp @@ -2,66 +2,10 @@ #include -#if defined(_WIN32) -#include -#else -#include -#endif +#include "detail/library_loader.hpp" +#include "detail/vtable_validation.hpp" namespace PJ { -namespace { - -Expected loadLibraryHandle(std::string_view path) { -#if defined(_WIN32) - HMODULE module = LoadLibraryA(std::string(path).c_str()); - if (module == nullptr) { - return unexpected("LoadLibraryA failed"); - } - return reinterpret_cast(module); -#else - void* handle = dlopen(std::string(path).c_str(), RTLD_NOW | RTLD_LOCAL); - if (handle == nullptr) { - const char* error = dlerror(); - return unexpected(error == nullptr ? "" : error); - } - return handle; -#endif -} - -void closeLibraryHandle(void* handle) { - if (handle == nullptr) { - return; - } -#if defined(_WIN32) - FreeLibrary(reinterpret_cast(handle)); -#else - dlclose(handle); -#endif -} - -std::shared_ptr adoptLibraryHandle(void* handle) { - return std::shared_ptr(handle, [](void* loaded_handle) { closeLibraryHandle(loaded_handle); }); -} - -Expected loadEntryPoint(void* handle) { -#if defined(_WIN32) - auto symbol = GetProcAddress(reinterpret_cast(handle), "PJ_get_dialog_vtable"); - if (symbol == nullptr) { - return unexpected("PJ_get_dialog_vtable not found"); - } - return reinterpret_cast(symbol); -#else - dlerror(); - void* symbol = dlsym(handle, "PJ_get_dialog_vtable"); - const char* err = dlerror(); - if (err != nullptr) { - return unexpected(err); - } - return reinterpret_cast(symbol); -#endif -} - -} // namespace DialogLibrary::DialogLibrary(std::shared_ptr handle, const PJ_dialog_vtable_t* vtable, std::string path) : handle_(std::move(handle)), vtable_(vtable), path_(std::move(path)) {} @@ -87,18 +31,23 @@ DialogLibrary& DialogLibrary::operator=(DialogLibrary&& other) noexcept { } Expected DialogLibrary::load(std::string_view path) { - auto raw_handle = loadLibraryHandle(path); + auto raw_handle = detail::loadLibraryHandle(path); if (!raw_handle) { return unexpected(raw_handle.error()); } - auto handle = adoptLibraryHandle(*raw_handle); + auto handle = detail::adoptLibraryHandle(*raw_handle); + + if (auto abi = detail::checkPluginAbiVersion(handle.get()); !abi) { + return unexpected(abi.error()); + } - auto entry = loadEntryPoint(handle.get()); - if (!entry) { - return unexpected(entry.error()); + auto sym = detail::resolveSymbol(handle.get(), "PJ_get_dialog_vtable"); + if (!sym) { + return unexpected(sym.error()); } + auto entry = reinterpret_cast(*sym); - const PJ_dialog_vtable_t* vtable = (*entry)(); + const PJ_dialog_vtable_t* vtable = entry(); if (vtable == nullptr) { return unexpected("PJ_get_dialog_vtable returned null"); } @@ -108,6 +57,9 @@ Expected DialogLibrary::load(std::string_view path) { if (vtable->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { return unexpected("Dialog vtable smaller than v4.0 baseline"); } + if (auto status = detail::validateRequiredSlots(vtable); !status) { + return unexpected(status.error()); + } return DialogLibrary(std::move(handle), vtable, std::string(path)); } diff --git a/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp b/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp index 92342ab..347b4f2 100644 --- a/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp +++ b/pj_plugins/dialog_protocol/tests/dialog_library_test.cpp @@ -10,6 +10,14 @@ #error "PJ_MOCK_DIALOG_PLUGIN_PATH must be defined" #endif +#ifndef PJ_MISSING_DIALOG_ABI_PLUGIN_PATH +#error "PJ_MISSING_DIALOG_ABI_PLUGIN_PATH must be defined" +#endif + +#ifndef PJ_MISSING_DIALOG_REQUIRED_SLOTS_PLUGIN_PATH +#error "PJ_MISSING_DIALOG_REQUIRED_SLOTS_PLUGIN_PATH must be defined" +#endif + namespace { TEST(DialogLibraryTest, LoadAndCreateHandle) { @@ -53,6 +61,18 @@ TEST(DialogLibraryTest, LoadInvalidPath) { EXPECT_FALSE(lib); } +TEST(DialogLibraryTest, RejectsMissingAbiVersionSymbol) { + auto lib = PJ::DialogLibrary::load(PJ_MISSING_DIALOG_ABI_PLUGIN_PATH); + ASSERT_FALSE(lib); + EXPECT_NE(lib.error().find("pj_plugin_abi_version"), std::string::npos); +} + +TEST(DialogLibraryTest, RejectsMissingRequiredSlot) { + auto lib = PJ::DialogLibrary::load(PJ_MISSING_DIALOG_REQUIRED_SLOTS_PLUGIN_PATH); + ASSERT_FALSE(lib); + EXPECT_NE(lib.error().find("Dialog vtable missing required slot: get_ui_content"), std::string::npos); +} + TEST(DialogLibraryTest, HandleKeepsSharedLibraryLoadedAfterLibraryObjectDies) { std::unique_ptr handle; { diff --git a/pj_plugins/dialog_protocol/tests/missing_dialog_abi_plugin.cpp b/pj_plugins/dialog_protocol/tests/missing_dialog_abi_plugin.cpp new file mode 100644 index 0000000..e804733 --- /dev/null +++ b/pj_plugins/dialog_protocol/tests/missing_dialog_abi_plugin.cpp @@ -0,0 +1,68 @@ +#include "pj_plugins/dialog_protocol.h" + +namespace { + +void* create() noexcept { + return reinterpret_cast(0x1); +} + +void destroy(void*) noexcept {} + +const char* getManifest(void*) noexcept { + return R"({"id":"missing-dialog-abi","name":"Missing Dialog ABI","version":"1.0.0"})"; +} + +const char* getUiContent(void*) noexcept { + return ""; +} + +const char* getWidgetData(void*) noexcept { + return "{}"; +} + +bool onWidgetEvent(void*, const char*, const char*, PJ_error_t*) noexcept { + return false; +} + +bool onTick(void*, PJ_error_t*) noexcept { + return false; +} + +void onAccepted(void*, const char*) noexcept {} + +void onRejected(void*) noexcept {} + +bool saveConfig(void*, PJ_string_view_t* out_json, PJ_error_t*) noexcept { + static constexpr const char* kJson = "{}"; + if (out_json != nullptr) { + out_json->data = kJson; + out_json->size = 2; + } + return true; +} + +bool loadConfig(void*, PJ_string_view_t, PJ_error_t*) noexcept { + return true; +} + +} // namespace + +extern "C" PJ_DIALOG_EXPORT const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept { + static const PJ_dialog_vtable_t vt = { + .protocol_version = PJ_DIALOG_PROTOCOL_VERSION, + .struct_size = sizeof(PJ_dialog_vtable_t), + .create = create, + .destroy = destroy, + .get_manifest = getManifest, + .get_ui_content = getUiContent, + .get_widget_data = getWidgetData, + .on_widget_event = onWidgetEvent, + .on_tick = onTick, + .on_accepted = onAccepted, + .on_rejected = onRejected, + .save_config = saveConfig, + .load_config = loadConfig, + .manifest_json = R"({"id":"missing-dialog-abi","name":"Missing Dialog ABI","version":"1.0.0"})", + }; + return &vt; +} diff --git a/pj_plugins/dialog_protocol/tests/missing_dialog_required_slots_plugin.cpp b/pj_plugins/dialog_protocol/tests/missing_dialog_required_slots_plugin.cpp new file mode 100644 index 0000000..cd2df4e --- /dev/null +++ b/pj_plugins/dialog_protocol/tests/missing_dialog_required_slots_plugin.cpp @@ -0,0 +1,66 @@ +#include "pj_plugins/dialog_protocol.h" + +extern "C" PJ_DIALOG_EXPORT const uint32_t pj_plugin_abi_version = PJ_ABI_VERSION; + +namespace { + +void* create() noexcept { + return reinterpret_cast(0x1); +} + +void destroy(void*) noexcept {} + +const char* getManifest(void*) noexcept { + return R"({"id":"missing-dialog-slot","name":"Missing Dialog Slot","version":"1.0.0"})"; +} + +const char* getWidgetData(void*) noexcept { + return "{}"; +} + +bool onWidgetEvent(void*, const char*, const char*, PJ_error_t*) noexcept { + return false; +} + +bool onTick(void*, PJ_error_t*) noexcept { + return false; +} + +void onAccepted(void*, const char*) noexcept {} + +void onRejected(void*) noexcept {} + +bool saveConfig(void*, PJ_string_view_t* out_json, PJ_error_t*) noexcept { + static constexpr const char* kJson = "{}"; + if (out_json != nullptr) { + out_json->data = kJson; + out_json->size = 2; + } + return true; +} + +bool loadConfig(void*, PJ_string_view_t, PJ_error_t*) noexcept { + return true; +} + +} // namespace + +extern "C" PJ_DIALOG_EXPORT const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept { + static const PJ_dialog_vtable_t vt = { + .protocol_version = PJ_DIALOG_PROTOCOL_VERSION, + .struct_size = sizeof(PJ_dialog_vtable_t), + .create = create, + .destroy = destroy, + .get_manifest = getManifest, + .get_ui_content = nullptr, + .get_widget_data = getWidgetData, + .on_widget_event = onWidgetEvent, + .on_tick = onTick, + .on_accepted = onAccepted, + .on_rejected = onRejected, + .save_config = saveConfig, + .load_config = loadConfig, + .manifest_json = R"({"id":"missing-dialog-slot","name":"Missing Dialog Slot","version":"1.0.0"})", + }; + return &vt; +} diff --git a/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp b/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp index 408b717..8f92bc5 100644 --- a/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp +++ b/pj_plugins/dialog_protocol/tests/plugin_lifecycle_test.cpp @@ -10,8 +10,21 @@ // Defined in mock_dialog.cpp, linked statically extern "C" const PJ_dialog_vtable_t* PJ_get_dialog_vtable() noexcept; +static_assert(offsetof(PJ_dialog_vtable_t, protocol_version) == 0, "dialog v4 prefix pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, struct_size) == 4, "dialog v4 prefix pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, create) == 8, "dialog create slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, destroy) == 16, "dialog destroy slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, get_manifest) == 24, "dialog manifest slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, get_ui_content) == 32, "dialog UI slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, get_widget_data) == 40, "dialog widget-data slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, on_widget_event) == 48, "dialog event slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, on_tick) == 56, "dialog tick slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, on_accepted) == 64, "dialog accepted slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, on_rejected) == 72, "dialog rejected slot pinned"); +static_assert(offsetof(PJ_dialog_vtable_t, save_config) == 80, "dialog save-config slot pinned"); static_assert(offsetof(PJ_dialog_vtable_t, load_config) == 88, "v4 dialog baseline slot pinned"); static_assert(PJ_DIALOG_MIN_VTABLE_SIZE == 96, "Dialog MIN vtable size is pinned at v4.0"); +static_assert(PJ_DIALOG_MIN_VTABLE_SIZE <= sizeof(PJ_dialog_vtable_t), "MIN must never exceed current"); static_assert(offsetof(PJ_dialog_vtable_t, manifest_json) == 96, "Dialog static manifest tail slot appended"); static_assert(sizeof(PJ_dialog_vtable_t) == 104, "Dialog vtable size updated deliberately on append"); diff --git a/pj_plugins/src/data_source_library.cpp b/pj_plugins/src/data_source_library.cpp index 904783e..d071fd4 100644 --- a/pj_plugins/src/data_source_library.cpp +++ b/pj_plugins/src/data_source_library.cpp @@ -83,6 +83,9 @@ Expected DataSourceLibrary::resolveDialogVtable() con if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { return unexpected("Dialog vtable smaller than v4.0 baseline"); } + if (auto status = detail::validateRequiredSlots(vt); !status) { + return unexpected(status.error()); + } return vt; } diff --git a/pj_plugins/src/detail/vtable_validation.cpp b/pj_plugins/src/detail/vtable_validation.cpp index f418176..336ccee 100644 --- a/pj_plugins/src/detail/vtable_validation.cpp +++ b/pj_plugins/src/detail/vtable_validation.cpp @@ -79,4 +79,21 @@ Status validateRequiredSlots(const PJ_toolbox_vtable_t* vtable) { }); } +Status validateRequiredSlots(const PJ_dialog_vtable_t* vtable) { + return validateSlots( + "Dialog", { + {"create", vtable->create != nullptr}, + {"destroy", vtable->destroy != nullptr}, + {"get_manifest", vtable->get_manifest != nullptr}, + {"get_ui_content", vtable->get_ui_content != nullptr}, + {"get_widget_data", vtable->get_widget_data != nullptr}, + {"on_widget_event", vtable->on_widget_event != nullptr}, + {"on_tick", vtable->on_tick != nullptr}, + {"on_accepted", vtable->on_accepted != nullptr}, + {"on_rejected", vtable->on_rejected != nullptr}, + {"save_config", vtable->save_config != nullptr}, + {"load_config", vtable->load_config != nullptr}, + }); +} + } // namespace PJ::detail diff --git a/pj_plugins/src/detail/vtable_validation.hpp b/pj_plugins/src/detail/vtable_validation.hpp index ae0d99e..5a0826f 100644 --- a/pj_plugins/src/detail/vtable_validation.hpp +++ b/pj_plugins/src/detail/vtable_validation.hpp @@ -4,11 +4,13 @@ #include "pj_base/expected.hpp" #include "pj_base/message_parser_protocol.h" #include "pj_base/toolbox_protocol.h" +#include "pj_plugins/dialog_protocol.h" namespace PJ::detail { Status validateRequiredSlots(const PJ_data_source_vtable_t* vtable); Status validateRequiredSlots(const PJ_message_parser_vtable_t* vtable); Status validateRequiredSlots(const PJ_toolbox_vtable_t* vtable); +Status validateRequiredSlots(const PJ_dialog_vtable_t* vtable); } // namespace PJ::detail diff --git a/pj_plugins/src/message_parser_library.cpp b/pj_plugins/src/message_parser_library.cpp index d7f1d20..6ba2b76 100644 --- a/pj_plugins/src/message_parser_library.cpp +++ b/pj_plugins/src/message_parser_library.cpp @@ -81,6 +81,9 @@ Expected MessageParserLibrary::resolveDialogVtable() if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { return unexpected("Dialog vtable smaller than v4.0 baseline"); } + if (auto status = detail::validateRequiredSlots(vt); !status) { + return unexpected(status.error()); + } return vt; } diff --git a/pj_plugins/src/plugin_catalog.cpp b/pj_plugins/src/plugin_catalog.cpp index 8f5439a..73d67bf 100644 --- a/pj_plugins/src/plugin_catalog.cpp +++ b/pj_plugins/src/plugin_catalog.cpp @@ -106,10 +106,10 @@ Expected tryDialog(void* handle) { if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { return unexpected("Dialog vtable smaller than v4.0 baseline"); } - if (vt->create == nullptr || vt->destroy == nullptr || vt->get_manifest == nullptr) { - return unexpected("Dialog vtable missing required lifecycle slots"); + if (auto status = detail::validateRequiredSlots(vt); !status) { + return unexpected(status.error()); } - if (PJ_HAS_TAIL_SLOT(PJ_dialog_vtable_t, vt, manifest_json) && vt->manifest_json != nullptr) { + if (PJ_HAS_TAIL_SLOT(PJ_dialog_vtable_t, vt, manifest_json)) { return ManifestCandidate{PluginFamily::kDialog, vt->manifest_json}; } diff --git a/pj_plugins/src/toolbox_library.cpp b/pj_plugins/src/toolbox_library.cpp index 9092ef5..c7a0141 100644 --- a/pj_plugins/src/toolbox_library.cpp +++ b/pj_plugins/src/toolbox_library.cpp @@ -80,6 +80,9 @@ Expected ToolboxLibrary::resolveDialogVtable() const if (vt->struct_size < PJ_DIALOG_MIN_VTABLE_SIZE) { return unexpected("Dialog vtable smaller than v4.0 baseline"); } + if (auto status = detail::validateRequiredSlots(vt); !status) { + return unexpected(status.error()); + } return vt; } diff --git a/pj_plugins/tests/plugin_catalog_test.cpp b/pj_plugins/tests/plugin_catalog_test.cpp index 937be9e..dee400a 100644 --- a/pj_plugins/tests/plugin_catalog_test.cpp +++ b/pj_plugins/tests/plugin_catalog_test.cpp @@ -131,6 +131,18 @@ TEST_F(PluginCatalogTest, MissingRequiredVtableSlotIsReportedAsDiagnostic) { EXPECT_NE(result->diagnostics[0].message.find("missing_required_slots"), std::string::npos); } +TEST_F(PluginCatalogTest, MissingRequiredDialogVtableSlotIsReportedAsDiagnostic) { + copyPlugin(PJ_MISSING_DIALOG_REQUIRED_SLOTS_PLUGIN_PATH, pluginFileName("missing_dialog_slot")); + + auto result = scanPluginDsos(dir_); + ASSERT_TRUE(result.has_value()) << result.error(); + EXPECT_TRUE(result->plugins.empty()); + ASSERT_EQ(result->diagnostics.size(), 1U); + EXPECT_NE( + result->diagnostics[0].message.find("Dialog vtable missing required slot: get_ui_content"), std::string::npos); + EXPECT_NE(result->diagnostics[0].message.find("missing_dialog_slot"), std::string::npos); +} + TEST_F(PluginCatalogTest, ScanContinuesAfterBrokenDso) { copyPlugin(PJ_MOCK_DATA_SOURCE_PLUGIN_PATH, pluginFileName("valid")); std::ofstream(dir_ / pluginFileName("broken")) << "not a shared library"; diff --git a/pj_plugins/tests/source_dialog_integration_test.cpp b/pj_plugins/tests/source_dialog_integration_test.cpp index 6dde153..368b321 100644 --- a/pj_plugins/tests/source_dialog_integration_test.cpp +++ b/pj_plugins/tests/source_dialog_integration_test.cpp @@ -1,5 +1,6 @@ #include +#include #include #include @@ -91,6 +92,32 @@ TEST(SourceDialogIntegration, BorrowedDialogHandleWorks) { EXPECT_FALSE(cfg.empty()); } +TEST(SourceDialogIntegration, BorrowedDialogWorksAfterLibraryObjectDiesWhileSourceLives) { + std::unique_ptr source; + std::unique_ptr dialog; + + { + auto lib = PJ::DataSourceLibrary::load(PJ_MOCK_SOURCE_WITH_DIALOG_PLUGIN_PATH); + ASSERT_TRUE(lib) << lib.error(); + + source = std::make_unique(lib->createHandle()); + ASSERT_TRUE(source->valid()); + + auto borrowed = source->getDialog(); + ASSERT_NE(borrowed.ctx, nullptr); + ASSERT_NE(borrowed.vtable, nullptr); + dialog = std::make_unique(PJ::DialogHandle::fromBorrowed(borrowed)); + } + + std::string wd = dialog->widget_data(); + EXPECT_FALSE(wd.empty()); + auto j = nlohmann::json::parse(wd, nullptr, false); + EXPECT_FALSE(j.is_discarded()); + + dialog.reset(); + source.reset(); +} + // --- Test 6: Shared state --- TEST(SourceDialogIntegration, SharedStateBetweenDialogAndSource) { From c259d7a4902a12ee322a4c6d942c3c06e3416d18 Mon Sep 17 00:00:00 2001 From: Davide Faconti Date: Thu, 7 May 2026 12:02:07 +0200 Subject: [PATCH 10/10] fix: enable MSVC conformant preprocessor PJ_DIALOG_PLUGIN's overload-by-arg-count idiom relies on __VA_ARGS__ splitting on commas inside nested macro calls. MSVC's traditional preprocessor passes __VA_ARGS__ as a single token, causing the 2-argument form to dispatch to the legacy 1-argument macro and generate a bogus dialogVtableFor specialisation. Co-Authored-By: Claude Opus 4.7 (1M context) --- CMakeLists.txt | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 61c4015..1c70db4 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -32,7 +32,10 @@ option(PJ_ENABLE_ABI_CHECK "Enable abidiff-based ABI drift gate (requires libabi # --------------------------------------------------------------------------- if(MSVC) - set(PJ_WARNING_FLAGS /W4 /WX /permissive-) + # /Zc:preprocessor: conformant preprocessor — required so __VA_ARGS__ inside + # nested macro calls (e.g. PJ_DIALOG_PLUGIN's overload-by-arg-count idiom) + # splits on commas instead of being passed as a single token. + set(PJ_WARNING_FLAGS /W4 /WX /permissive- /Zc:preprocessor) else() set(PJ_WARNING_FLAGS -Wall -Wextra -Werror -Wshadow -Wnon-virtual-dtor -Wold-style-cast