diff --git a/CHANGELOG.md b/CHANGELOG.md index 8be5f99..2bd9815 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,17 @@ All notable changes to `plotjuggler_sdk` are recorded here. Versioning policy is ## [0.21.0] +### Fix: convenience registerService honors its documented assertion (PATCH) + +The non-returning `ServiceRegistryBuilder::registerService` overloads documented +a debug assertion but discarded the `tryRegisterService` status in every build +type, so duplicate or null registrations were silent. A rejection now trips +`PJ_ASSERT`, and the doc-comments state the exact NDEBUG behavior (status +dropped; callers that must report failures use `tryRegisterService`). Adds the +first regression tests for the builder's registration rules +(`service_registry_builder_test`, built with `PJ_ASSERT_THROWS` so the +invariant is observable in every build type). + ### Feature: pure-functional MessageParser C extension (MINOR) MessageParser scalar/object results no longer require new hosts to cast an diff --git a/pj_plugins/CMakeLists.txt b/pj_plugins/CMakeLists.txt index 08b3280..bb245f8 100644 --- a/pj_plugins/CMakeLists.txt +++ b/pj_plugins/CMakeLists.txt @@ -344,6 +344,18 @@ target_link_libraries(object_ingest_policy_test PRIVATE target_include_directories(object_ingest_policy_test PRIVATE include) add_test(NAME object_ingest_policy_test COMMAND object_ingest_policy_test) +# Unit test: ServiceRegistryBuilder registration rules. PJ_ASSERT_THROWS makes +# the convenience overload's invariant observable in every build type — a plain +# assert() is compiled away under NDEBUG, which is where it would go unchecked. +add_executable(service_registry_builder_test tests/service_registry_builder_test.cpp) +target_compile_definitions(service_registry_builder_test PRIVATE PJ_ASSERT_THROWS) +target_compile_options(service_registry_builder_test PRIVATE ${PJ_WARNING_FLAGS}) +target_link_libraries(service_registry_builder_test PRIVATE + pj_base GTest::gtest_main +) +target_include_directories(service_registry_builder_test PRIVATE include) +add_test(NAME service_registry_builder_test COMMAND service_registry_builder_test) + # TODO(v3-port): delegated_ingest_integration_test.cpp uses old bindWriteHost / # bindRuntimeHost methods and get_last_error slots removed in v3. Pending port # to the service registry + PJ_error_t* pattern. Its coverage (parser binding + 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 e27ceca..2b7b81b 100644 --- a/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp +++ b/pj_plugins/include/pj_plugins/host/service_registry_builder.hpp @@ -9,6 +9,7 @@ #include #include +#include "pj_base/assert.hpp" #include "pj_base/expected.hpp" #include "pj_base/plugin_data_api.h" @@ -58,16 +59,21 @@ class ServiceRegistryBuilder { } /// Non-returning convenience overload for callers that know the inputs are - /// valid (mocks, tests). Asserts in debug builds; no-op on failure in - /// release (i.e. do NOT rely on this for untrusted inputs — use - /// tryRegisterService instead). + /// valid (mocks, tests). A rejection trips `PJ_ASSERT`, which aborts while + /// `assert()` is live and throws under `PJ_ASSERT_THROWS` — but NDEBUG + /// compiles it away, so in a release build the registration is silently + /// dropped and the reason is lost. Anything whose inputs are not statically + /// known must call tryRegisterService and handle the Status. void registerService(std::string_view name, uint32_t protocol_version, PJ_service_t service) { - auto status = tryRegisterService(name, protocol_version, service); - (void)status; + [[maybe_unused]] const ::PJ::Status status = tryRegisterService(name, protocol_version, service); + PJ_ASSERT(status.has_value(), "registerService: rejected (duplicate name, or null ctx/vtable)"); } /// Typed overload using a service-traits class (see sdk/service_traits.hpp). /// The traits provide the canonical name and a default protocol version. + /// Inherits the overload above's release-build behavior: a rejection is + /// dropped without a trace, so a host that must report it registers through + /// tryRegisterService with the same expansion. template void registerService(typename Traits::Raw service) { registerService( diff --git a/pj_plugins/tests/service_registry_builder_test.cpp b/pj_plugins/tests/service_registry_builder_test.cpp new file mode 100644 index 0000000..b55d568 --- /dev/null +++ b/pj_plugins/tests/service_registry_builder_test.cpp @@ -0,0 +1,91 @@ +// Copyright 2026 Davide Faconti +// SPDX-License-Identifier: Apache-2.0 + +#include "pj_plugins/host/service_registry_builder.hpp" + +#include + +#include +#include + +#include "pj_base/plugin_data_api.h" +#include "pj_base/sdk/service_traits.hpp" + +// This TU is compiled with PJ_ASSERT_THROWS so the convenience overload's +// invariant is observable in every build type (a plain assert() is compiled +// away under NDEBUG, which is exactly how the missing check went unnoticed). +#ifndef PJ_ASSERT_THROWS +#error "service_registry_builder_test must be built with PJ_ASSERT_THROWS" +#endif + +namespace { + +// The builder only stores the fat pointer; it never dereferences the vtable, +// so a zeroed one is enough to make a service "valid". +const PJ_source_write_host_vtable_t kWriteVtable{}; +int ctx_a = 0; +int ctx_b = 0; + +PJ_source_write_host_t makeWriteHost(int* ctx) { + return PJ_source_write_host_t{ctx, &kWriteVtable}; +} + +constexpr const char* kName = PJ::sdk::SourceWriteHostService::kName; + +TEST(ServiceRegistryBuilderTest, FirstRegistrationSucceeds) { + PJ::ServiceRegistryBuilder builder; + + const PJ::Status status = builder.tryRegisterService(kName, 1, PJ_service_t{&ctx_a, &kWriteVtable}); + + EXPECT_TRUE(status.has_value()) << status.error(); + EXPECT_EQ(builder.size(), 1U); +} + +TEST(ServiceRegistryBuilderTest, DuplicateNameIsRejectedAndKeepsTheFirstEntry) { + PJ::ServiceRegistryBuilder builder; + ASSERT_TRUE(builder.tryRegisterService(kName, 1, PJ_service_t{&ctx_a, &kWriteVtable}).has_value()); + + const PJ::Status status = builder.tryRegisterService(kName, 2, PJ_service_t{&ctx_b, &kWriteVtable}); + + ASSERT_FALSE(status.has_value()); + EXPECT_NE(status.error().find("duplicate name"), std::string::npos) << status.error(); + EXPECT_NE(status.error().find(kName), std::string::npos) << status.error(); + EXPECT_EQ(builder.size(), 1U); +} + +TEST(ServiceRegistryBuilderTest, NullCtxOrVtableIsRejected) { + PJ::ServiceRegistryBuilder builder; + + EXPECT_FALSE(builder.tryRegisterService(kName, 1, PJ_service_t{nullptr, &kWriteVtable}).has_value()); + EXPECT_FALSE(builder.tryRegisterService(kName, 1, PJ_service_t{&ctx_a, nullptr}).has_value()); + EXPECT_EQ(builder.size(), 0U); +} + +// The convenience overload documents that it asserts; without that assert a +// duplicate silently changes the service surface a plugin binds against. +TEST(ServiceRegistryBuilderTest, ConvenienceOverloadAssertsOnDuplicate) { + PJ::ServiceRegistryBuilder builder; + builder.registerService(kName, 1, PJ_service_t{&ctx_a, &kWriteVtable}); + + EXPECT_THROW(builder.registerService(kName, 1, PJ_service_t{&ctx_b, &kWriteVtable}), std::runtime_error); + EXPECT_EQ(builder.size(), 1U); +} + +TEST(ServiceRegistryBuilderTest, ConvenienceOverloadAssertsOnNullService) { + PJ::ServiceRegistryBuilder builder; + + EXPECT_THROW(builder.registerService(kName, 1, PJ_service_t{nullptr, nullptr}), std::runtime_error); + EXPECT_EQ(builder.size(), 0U); +} + +// The templated path is the one every host actually calls, so it must inherit +// the same invariant rather than quietly dropping the second registration. +TEST(ServiceRegistryBuilderTest, TemplatedOverloadAssertsOnDuplicate) { + PJ::ServiceRegistryBuilder builder; + builder.registerService(makeWriteHost(&ctx_a)); + + EXPECT_THROW(builder.registerService(makeWriteHost(&ctx_b)), std::runtime_error); + EXPECT_EQ(builder.size(), 1U); +} + +} // namespace