From f3fdc9b26ac8d8ad3fc7524fd43e05253a078160 Mon Sep 17 00:00:00 2001 From: Marc Paterno Date: Fri, 23 Oct 2026 16:46:07 -0500 Subject: [PATCH] fix: represent absent and empty creator and suffix explicitly - Model an unqualified creator's plugin and an empty suffix as explicit empty values instead of invalid empty identifiers. - Make a product specification's creator optional; provider construction now requires a present creator. - Reject empty algorithm names, empty plugin components, and an empty configured module_label instead of accepting them silently. - Rename the default source creator from "[Source]" to "phlex_source", which is a valid name under the new parser. - Return borrowed string views from handle creator() and suffix() accessors. - Add tests for parsing, equality/hashing, exact product lookup, provider construction, and handle views. --- phlex/core/consumer.cpp | 2 +- phlex/core/consumer.hpp | 9 +- phlex/core/detail/make_algorithm_name.cpp | 15 ++- phlex/core/provider_node.cpp | 25 +++- phlex/core/upstream_predicates.hpp | 4 + phlex/model/algorithm_name.cpp | 17 +-- phlex/model/algorithm_name.hpp | 22 +++- phlex/model/handle.hpp | 18 ++- phlex/model/product_specification.cpp | 31 +++-- phlex/model/product_specification.hpp | 66 ++++++++-- phlex/model/product_store.cpp | 2 +- phlex/model/product_store.hpp | 5 +- phlex/model/products.cpp | 3 + test/core_misc_test.cpp | 92 +++++++++++-- test/output_products_test.cpp | 150 +++++++++++++++++++++- test/product_handle_test.cpp | 112 ++++++++++++++++ test/product_store_test.cpp | 59 +++++++++ test/products_test.cpp | 73 +++++++++++ test/provider_test.cpp | 50 ++++++++ 19 files changed, 703 insertions(+), 52 deletions(-) diff --git a/phlex/core/consumer.cpp b/phlex/core/consumer.cpp index 9d1cd3764..e7c2d3530 100644 --- a/phlex/core/consumer.cpp +++ b/phlex/core/consumer.cpp @@ -15,7 +15,7 @@ namespace phlex::detail { } phlex::experimental::algorithm_name const& consumer::name() const noexcept { return name_; } - phlex::experimental::identifier const& consumer::plugin() const noexcept + phlex::experimental::identifier_or_empty const& consumer::plugin() const noexcept { return name_.plugin(); } diff --git a/phlex/core/consumer.hpp b/phlex/core/consumer.hpp index aa9e328cc..936b0ec81 100644 --- a/phlex/core/consumer.hpp +++ b/phlex/core/consumer.hpp @@ -2,18 +2,25 @@ #define PHLEX_CORE_CONSUMER_HPP #include "phlex/model/algorithm_name.hpp" +#include "phlex/model/identifier.hpp" #include "phlex/phlex_core_export.hpp" #include #include namespace phlex::detail { + /// Base class of nodes that consume data products. + /// + /// Accessor contract: `name()` is the complete creator name. `plugin()` is that name's plugin + /// component, which is explicitly empty (`empty_string`) for an unqualified name, such as one + /// registered without a configuration; it is never an invalid empty `identifier`. + /// `algorithm()` is the non-empty algorithm component. class PHLEX_CORE_EXPORT consumer { public: consumer(phlex::experimental::algorithm_name name, std::vector predicates); phlex::experimental::algorithm_name const& name() const noexcept; - phlex::experimental::identifier const& plugin() const noexcept; + phlex::experimental::identifier_or_empty const& plugin() const noexcept; phlex::experimental::identifier const& algorithm() const noexcept; std::vector const& when() const noexcept; diff --git a/phlex/core/detail/make_algorithm_name.cpp b/phlex/core/detail/make_algorithm_name.cpp index e4e49d3d4..8c8414650 100644 --- a/phlex/core/detail/make_algorithm_name.cpp +++ b/phlex/core/detail/make_algorithm_name.cpp @@ -4,12 +4,23 @@ #include "phlex/model/algorithm_name.hpp" #include "phlex/model/identifier.hpp" +#include #include namespace phlex::experimental::internal { + // The plugin component is absent (explicitly empty) only when there is no configuration. A + // configuration must supply a non-empty 'module_label': a missing label is an error reported by + // the configuration, and an empty label is rejected here rather than treated as an absent + // plugin. algorithm_name make_algorithm_name(configuration const* config, std::string_view name) { - return {config ? identifier(config->get("module_label")) : ""_id, - identifier(name)}; + if (config == nullptr) { + return {identifier_or_empty{empty_string{}}, identifier(name)}; + } + auto const module_label = config->get("module_label"); + if (module_label.empty()) { + throw std::runtime_error("The configured 'module_label' must not be empty"); + } + return {identifier_or_empty{identifier(module_label)}, identifier(name)}; } } diff --git a/phlex/core/provider_node.cpp b/phlex/core/provider_node.cpp index 77e11797f..bbda61210 100644 --- a/phlex/core/provider_node.cpp +++ b/phlex/core/provider_node.cpp @@ -11,11 +11,32 @@ #include #include #include +#include #include +namespace { + // A provider always names its creator, so an output specification without one is an error. + phlex::experimental::algorithm_name const& creator_of( + phlex::experimental::product_specification const& spec) + { + if (!spec.creator()) { + throw std::runtime_error("Cannot create a provider for the product specification '" + + spec.to_string() + "' because it has no creator"); + } + return *spec.creator(); + } + + phlex::experimental::product_specification require_creator( + phlex::experimental::product_specification spec) + { + creator_of(spec); + return spec; + } +} + namespace phlex::detail { provider_node::provider_node(tbb::flow::graph& g, provider_bundle bundle) : - provider_node{bundle.spec.creator(), + provider_node{creator_of(bundle.spec), bundle.max_concurrency.value, g, std::move(bundle.provider_function), @@ -33,7 +54,7 @@ namespace phlex::detail { phlex::experimental::identifier output_layer, phlex::experimental::identifier stage) : name_{std::move(algo_name)}, - output_{std::move(output_spec)}, + output_{require_creator(std::move(output_spec))}, layer_{std::move(output_layer)}, stage_{std::move(stage)}, provider_{g, diff --git a/phlex/core/upstream_predicates.hpp b/phlex/core/upstream_predicates.hpp index e1e7471c9..7ae7a473a 100644 --- a/phlex/core/upstream_predicates.hpp +++ b/phlex/core/upstream_predicates.hpp @@ -36,6 +36,10 @@ namespace phlex::detail { return experimental_when({std::forward(names)...}); } + // Fluent suffix registration is textual: the strings given here, including the empty string + // and the default used when no suffixes are registered, stay strings in the registrar. They + // become `suffix_t` values (empty text as `empty_string`, non-empty text as a named + // identifier) when `to_product_specifications()` creates the product specifications. template requires(NumberOutputProducts > 0) void output_product_suffixes(std::array outputs) diff --git a/phlex/model/algorithm_name.cpp b/phlex/model/algorithm_name.cpp index 6571e7eb2..d88c0207f 100644 --- a/phlex/model/algorithm_name.cpp +++ b/phlex/model/algorithm_name.cpp @@ -36,15 +36,13 @@ namespace { } namespace phlex::experimental { - algorithm_name::algorithm_name() = default; - algorithm_name::algorithm_name(char const* spec) : algorithm_name{std::string_view{spec}} {} algorithm_name::algorithm_name(std::string const& spec) : algorithm_name{std::string_view{spec}} { } algorithm_name::algorithm_name(std::string_view spec) { *this = create(spec); } - algorithm_name::algorithm_name(identifier plugin, identifier algorithm) : + algorithm_name::algorithm_name(identifier_or_empty plugin, identifier algorithm) : plugin_{std::move(plugin)}, algorithm_{std::move(algorithm)} { } @@ -63,13 +61,16 @@ namespace phlex::experimental { algorithm_name algorithm_name::create(char const* spec) { return create(std::string_view{spec}); } algorithm_name algorithm_name::create(std::string_view spec) { - // Empty spec means nothing is specified if (spec.empty()) { - return {}; + throw std::runtime_error("An algorithm name must not be empty"); } // Do we have a colon? If so, have both plugin and algorithm if (spec.contains(':')) { + if (spec.starts_with(':')) { + throw std::runtime_error( + fmt::format("The specification '{}' has an empty plugin name", spec)); + } if (spec.ends_with(':')) { throw std::runtime_error("Cannot create an algorithm name that ends with a colon (':')"); } @@ -81,14 +82,14 @@ namespace phlex::experimental { throw std::runtime_error( fmt::format("The specification '{}' is not a valid algorithm name.", spec)); } - return {identifier(plugin), identifier(algorithm)}; + return {identifier_or_empty(plugin), identifier(algorithm)}; } - // No colon -- could be plugin or algorithm + // No colon: the plugin component is explicitly empty if (!words_only(spec)) { throw std::runtime_error( fmt::format("The specification '{}' is not a valid algorithm name.", spec)); } - return {"", identifier(spec)}; + return {identifier_or_empty(empty_string{}), identifier(spec)}; } } diff --git a/phlex/model/algorithm_name.hpp b/phlex/model/algorithm_name.hpp index 43772960f..f66fc0a71 100644 --- a/phlex/model/algorithm_name.hpp +++ b/phlex/model/algorithm_name.hpp @@ -7,29 +7,43 @@ #include namespace phlex::experimental { + /// The creator of a product or the name of an algorithm. + /// + /// An algorithm name always has a non-empty algorithm component. Its plugin component is either + /// a non-empty plugin name or explicitly empty (for an unqualified name). A default-constructed + /// algorithm name is not valid, so one must always be constructed from a name. class PHLEX_MODEL_EXPORT algorithm_name { public: - algorithm_name(); + algorithm_name() = delete; // NOLINTBEGIN(google-explicit-constructor) - Implicit conversion is intentional algorithm_name(char const* spec); algorithm_name(std::string const& spec); algorithm_name(std::string_view spec); // NOLINTEND(google-explicit-constructor) - algorithm_name(identifier plugin, identifier algorithm); + algorithm_name(identifier_or_empty plugin, identifier algorithm); std::string to_string() const; - identifier const& plugin() const noexcept { return plugin_; } + identifier_or_empty const& plugin() const noexcept { return plugin_; } identifier const& algorithm() const noexcept { return algorithm_; } bool match(algorithm_name const& other) const; auto operator<=>(algorithm_name const&) const noexcept = default; + /// Parses a creator name. + /// + /// Accepted forms: + /// - `algorithm`: the plugin component is explicitly empty. + /// - `plugin:algorithm`: both components are non-empty names. + /// + /// Each component must consist of word characters only. Rejected inputs include the empty + /// string, a leading colon (`:algorithm`), a trailing colon (`plugin:`), and any empty + /// component. Throws `std::runtime_error` for every rejected input. static algorithm_name create(char const* spec); static algorithm_name create(std::string_view spec); private: - identifier plugin_; + identifier_or_empty plugin_; identifier algorithm_; }; diff --git a/phlex/model/handle.hpp b/phlex/model/handle.hpp index 3c126402c..bf222d9ed 100644 --- a/phlex/model/handle.hpp +++ b/phlex/model/handle.hpp @@ -8,6 +8,8 @@ #include #include +#include +#include #include #include #include @@ -54,6 +56,11 @@ namespace phlex { using const_reference = value_type const&; using const_pointer = value_type const*; + /// Borrowed textual views of the creator's components. Each view refers to the text owned + /// by the product specification that the handle references, so it is valid only while that + /// specification (and thus the handle's source) is alive. No copy is stored. A component that + /// is absent (the plugin of an unqualified creator, or both components when the specification + /// has no creator) is an empty view. struct algorithm_name_view { std::string_view plugin; std::string_view algorithm; @@ -87,12 +94,17 @@ namespace phlex { // NOLINTEND(google-explicit-constructor) auto const& data_cell_index() const noexcept { return *index_; } - // Product specification information + // Product specification information. The handle exposes borrowed textual views, not the + // domain types (`suffix_t`, `identifier_or_empty`), so callers need not depend on them. + /// Views of the creator's plugin and algorithm names. Absent components are empty views. algorithm_name_view creator() const noexcept { - return {std::string_view(spec_->plugin()), std::string_view(spec_->algorithm())}; + auto const* const algorithm = spec_->algorithm(); + return {spec_->plugin().text(), + algorithm ? std::string_view(*algorithm) : std::string_view{}}; } - std::string_view suffix() const noexcept { return std::string_view(spec_->suffix()); } + /// View of the suffix text; empty for the explicit empty suffix. + std::string_view suffix() const noexcept { return spec_->suffix().text(); } std::string_view layer() const noexcept { return std::string_view(index_->layer_name()); } std::string_view stage() const noexcept { return std::string_view(*stage_); } std::string layer_path() const { return index_->layer_path().to_string(); } diff --git a/phlex/model/product_specification.cpp b/phlex/model/product_specification.cpp index cd7f63e4a..c7906c1a6 100644 --- a/phlex/model/product_specification.cpp +++ b/phlex/model/product_specification.cpp @@ -7,6 +7,7 @@ #include #include +#include #include #include #include @@ -25,19 +26,25 @@ namespace phlex::experimental { } product_specification::product_specification(std::string_view name) { *this = create(name); } - product_specification::product_specification(experimental::algorithm_name creator, - experimental::identifier suffix, + product_specification::product_specification(std::optional creator, + experimental::suffix_t suffix, type_id type) : creator_{std::move(creator)}, suffix_{std::move(suffix)}, type_id_{std::move(type)} { } + identifier_or_empty const& product_specification::plugin() const noexcept + { + static identifier_or_empty const absent_plugin{empty_string{}}; + return creator_ ? creator_->plugin() : absent_plugin; + } + std::string product_specification::to_string() const { - if (creator_.plugin().empty() && creator_.algorithm().empty()) { + if (!creator_) { return fmt::format("{}", suffix_); } - return fmt::format("{}/{}", creator_.to_string(), suffix_); + return fmt::format("{}/{}", creator_->to_string(), suffix_); } product_specification product_specification::create(char const* c) @@ -49,11 +56,15 @@ namespace phlex::experimental { { auto forward_slash = s.find('/'); if (forward_slash != std::string_view::npos) { - return {experimental::algorithm_name::create(s.substr(0, forward_slash)), - experimental::identifier(s.substr(forward_slash + 1)), - type_id{}}; + // An empty creator part (e.g. "/hits") is an absent creator, not an invalid creator name. + auto const creator_text = s.substr(0, forward_slash); + std::optional creator; + if (!creator_text.empty()) { + creator = experimental::algorithm_name::create(creator_text); + } + return {std::move(creator), experimental::suffix_t(s.substr(forward_slash + 1)), type_id{}}; } - return {experimental::algorithm_name::create(""), experimental::identifier(s), type_id{}}; + return {std::nullopt, experimental::suffix_t(s), type_id{}}; } } @@ -66,6 +77,8 @@ namespace phlex::detail { { static std::string const default_product_suffix; + // Fluent suffix registration is textual; empty text becomes `empty_string` and non-empty + // text a named identifier here, when the specifications are created. if (output_suffixes.empty()) { // This happens whenever the user does not specify output_product_suffixes(...), so we // assign the default suffix to all output data products. @@ -81,7 +94,7 @@ namespace phlex::detail { return std::views::zip(output_suffixes, output_types) | std::views::transform([&algo_name](auto const& p) { return experimental::product_specification{ - algo_name, experimental::identifier(std::get<0>(p)), std::get<1>(p)}; + algo_name, experimental::suffix_t(std::get<0>(p)), std::get<1>(p)}; }) | std::ranges::to(); } diff --git a/phlex/model/product_specification.hpp b/phlex/model/product_specification.hpp index f9f8d2f3a..4748f86d6 100644 --- a/phlex/model/product_specification.hpp +++ b/phlex/model/product_specification.hpp @@ -8,44 +8,87 @@ #include +#include #include +#include #include #include namespace phlex::experimental { /// Describes a product supplied by an algorithm. /// + /// A product specification is a partial label: it carries the creator, suffix, and type of a + /// product. Its metadata combines with a concrete data-cell/provider layer to form the complete + /// label of a created product, so the layer is deliberately not a member of this class. + /// + /// The creator is optional. A specification without a creator is valid for exact storage and + /// lookup (for example, the suffix-only text `hits`, `/hits`, `""`, or `/`); it is not a + /// wildcard. Equality, ordering, and hashing distinguish an absent creator from every present + /// creator: an absent creator orders before any present creator and equals only another absent + /// creator. Provider construction requires a present creator. + /// + /// The suffix is a `suffix_t`: either the explicit empty value (`empty_string`) or a named + /// `identifier`. Parsing, defaulting, formatting, equality, ordering, and hashing all preserve + /// that representation without manufacturing an empty `identifier`. The default suffix is + /// explicitly empty. It is never an omitted selection criterion, which is `std::nullopt` in a + /// product selector. + /// + /// Textual boundary: suffixes supplied as text (the `creator/suffix` text accepted by + /// `create()`, and the strings given to the fluent `output_product_suffixes(...)` registration + /// API, which `phlex::detail::to_product_specifications()` converts) remain strings. Empty text + /// becomes `empty_string` and non-empty text becomes a named `identifier`, when the + /// specification is created. Non-empty text that is not a valid name is rejected rather than + /// stored as an empty suffix. + /// /// This source-provider construction API is experimental and may change before it is /// promoted to the stable phlex namespace. class PHLEX_MODEL_EXPORT product_specification { public: + /// Constructs a specification with an absent creator and an empty suffix. product_specification(); // NOLINTBEGIN(google-explicit-constructor) - Implicit conversion is intentional product_specification(char const* name); product_specification(std::string const& name); product_specification(std::string_view name); // NOLINTEND(google-explicit-constructor) - product_specification(algorithm_name creator, identifier suffix, type_id type); + /// An absent creator is spelled `std::nullopt`; any `algorithm_name` supplies a present one. + product_specification(std::optional creator, suffix_t suffix, type_id type); + /// Text of the specification: `creator/suffix` for a present creator, and only the suffix + /// text for an absent creator. std::string to_string() const; - algorithm_name const& creator() const noexcept { return creator_; } - identifier const& plugin() const noexcept { return creator_.plugin(); } - identifier const& algorithm() const noexcept { return creator_.algorithm(); } - identifier const& suffix() const noexcept { return suffix_; } + + /// The creator, or `std::nullopt` if it is absent. + std::optional const& creator() const noexcept { return creator_; } + /// True if the creator is present. + bool has_creator() const noexcept { return creator_.has_value(); } + /// The creator's plugin component. It is empty if the creator is absent or unqualified. + identifier_or_empty const& plugin() const noexcept; + /// The creator's algorithm component, or nullptr if the creator is absent. + identifier const* algorithm() const noexcept + { + return creator_ ? &creator_->algorithm() : nullptr; + } + /// The suffix: `empty_string` or a named `identifier`. + suffix_t const& suffix() const noexcept { return suffix_; } type_id type() const noexcept { return type_id_; } void set_type(type_id&& type) { type_id_ = std::move(type); } auto operator<=>(product_specification const&) const = default; + /// Parses `creator/suffix` or suffix-only text. Suffix-only text (`hits`, `/hits`, `""`, and + /// `/`) yields an absent creator. A non-empty creator part must be a valid creator name; + /// otherwise this throws `std::runtime_error`. Empty suffix text yields an explicit + /// `empty_string` suffix; non-empty suffix text yields a named `identifier`. static product_specification create(char const* c); static product_specification create(std::string_view s); friend struct std::hash; private: - experimental::algorithm_name creator_; - experimental::identifier suffix_; // Default suffix is empty string + std::optional creator_; + experimental::suffix_t suffix_; // Default suffix is explicitly empty type_id type_id_{}; }; @@ -64,8 +107,13 @@ template <> struct std::hash { std::size_t operator()(phlex::experimental::product_specification const& spec) const noexcept { - std::size_t hash = spec.creator_.plugin().hash(); - boost::hash_combine(hash, spec.creator_.algorithm().hash()); + // The creator's presence is part of the hash so that an absent creator is distinguished + // from a present one, consistently with equality. + std::size_t hash = spec.creator_.has_value() ? 1 : 0; + if (spec.creator_) { + boost::hash_combine(hash, spec.creator_->plugin().hash()); + boost::hash_combine(hash, spec.creator_->algorithm().hash()); + } boost::hash_combine(hash, spec.suffix_.hash()); boost::hash_combine(hash, phlex::experimental::hash_value(spec.type_id_)); return hash; diff --git a/phlex/model/product_store.cpp b/phlex/model/product_store.cpp index 510a4a92d..8f210f3c7 100644 --- a/phlex/model/product_store.cpp +++ b/phlex/model/product_store.cpp @@ -53,7 +53,7 @@ namespace phlex::experimental { algorithm_name product_store::default_source() { - static algorithm_name const def = algorithm_name::create("[Source]"); + static algorithm_name const def = algorithm_name::create("phlex_source"); return def; } } diff --git a/phlex/model/product_store.hpp b/phlex/model/product_store.hpp index 975df8dfb..f23481d7c 100644 --- a/phlex/model/product_store.hpp +++ b/phlex/model/product_store.hpp @@ -53,7 +53,10 @@ namespace phlex::experimental { template void add_product(product_specification const& key, std::unique_ptr>&& t); - // default Source identifier + /// The creator used by default for products supplied by a source. + /// + /// This is the valid internal creator name `phlex_source`: an unqualified algorithm name + /// whose plugin component is empty. static experimental::algorithm_name default_source(); private: diff --git a/phlex/model/products.cpp b/phlex/model/products.cpp index b45b43780..535ce8bce 100644 --- a/phlex/model/products.cpp +++ b/phlex/model/products.cpp @@ -27,6 +27,9 @@ namespace phlex::detail { gsl::not_null products::find_product(product_specification const& spec) const { + // Lookup is exact: product_specification equality distinguishes an absent creator from every + // present creator, so a creator-less specification neither matches nor is matched by one with + // a creator. auto it = std::ranges::find(products_, spec, [](auto const& p) -> auto const& { return p.first; }); if (it == products_.end()) { diff --git a/test/core_misc_test.cpp b/test/core_misc_test.cpp index 62291a3da..326e45686 100644 --- a/test/core_misc_test.cpp +++ b/test/core_misc_test.cpp @@ -1,5 +1,6 @@ #include "phlex/configuration.hpp" #include "phlex/core/consumer.hpp" +#include "phlex/core/detail/make_algorithm_name.hpp" #include "phlex/core/glue.hpp" #include "phlex/core/registrar.hpp" #include "phlex/model/algorithm_name.hpp" @@ -18,21 +19,33 @@ TEST_CASE("algorithm_name tests", "[model]") using namespace phlex::detail; using namespace phlex::experimental; - SECTION("Default constructor") + SECTION("Empty creator name is rejected") { - algorithm_name const an; - CHECK(an.to_string().empty()); + CHECK_THROWS_AS(algorithm_name::create(""), std::runtime_error); + CHECK_THROWS_AS(algorithm_name(""), std::runtime_error); } SECTION("Create from string with colon") { + using namespace phlex::experimental::literals; auto an = algorithm_name::create("plugin:algo"); CHECK(an.to_string() == "plugin:algo"); + CHECK(an.plugin().text() == "plugin"); + CHECK(an.algorithm() == "algo"_id); } - SECTION("Create from string without colon") + SECTION("Create from string without colon has an explicitly empty plugin") { + using namespace phlex::experimental::literals; auto an = algorithm_name::create("algo"); // For 'either' cases, the word is stored as algorithm_ CHECK(an.to_string() == "algo"); + CHECK(an.plugin().empty()); + CHECK(an.algorithm() == "algo"_id); + } + SECTION("Create rejects invalid components") + { + CHECK_THROWS_AS(algorithm_name::create(":find_hits"), std::runtime_error); + CHECK_THROWS_AS(algorithm_name::create("reco:"), std::runtime_error); + CHECK_THROWS_AS(algorithm_name::create(":"), std::runtime_error); } SECTION("Create from char pointer") { @@ -65,13 +78,72 @@ TEST_CASE("algorithm_name tests", "[model]") TEST_CASE("consumer tests", "[core]") { using namespace phlex::experimental::literals; - auto an = phlex::experimental::algorithm_name::create("p:a"); - phlex::detail::consumer const c(an, {"pred1"}); - CHECK(c.name().to_string() == "p:a"); - CHECK(c.plugin() == "p"_idq); - CHECK(c.algorithm() == "a"_idq); - CHECK(c.when().size() == 1); + SECTION("Qualified creator has a present plugin") + { + auto an = phlex::experimental::algorithm_name::create("p:a"); + phlex::detail::consumer const c(an, {"pred1"}); + + CHECK(c.name().to_string() == "p:a"); + CHECK_FALSE(c.plugin().empty()); + CHECK(c.plugin().text() == "p"); + CHECK(c.algorithm() == "a"_idq); + CHECK(c.when().size() == 1); + } + + SECTION("Unqualified creator has an explicitly empty plugin") + { + auto an = phlex::experimental::algorithm_name::create("a"); + phlex::detail::consumer const c(an, {}); + + CHECK(c.name().to_string() == "a"); + CHECK(c.plugin().empty()); + CHECK(c.plugin() == phlex::experimental::identifier_or_empty{}); + CHECK(c.algorithm() == "a"_idq); + CHECK(c.when().empty()); + } +} + +TEST_CASE("make_algorithm_name tests", "[core]") +{ + using namespace phlex::experimental; + using namespace phlex::experimental::literals; + using phlex::experimental::internal::make_algorithm_name; + + SECTION("A null configuration gives an absent plugin") + { + auto const name = make_algorithm_name(nullptr, "algo"); + CHECK(name.plugin().empty()); + CHECK(name.algorithm() == "algo"_id); + CHECK(name.to_string() == "algo"); + } + + SECTION("A configured module label is the plugin") + { + boost::json::object obj; + obj["module_label"] = "my_module"; + phlex::configuration const config{obj}; + + auto const name = make_algorithm_name(&config, "algo"); + CHECK_FALSE(name.plugin().empty()); + CHECK(name.plugin().text() == "my_module"); + CHECK(name.algorithm() == "algo"_id); + CHECK(name.to_string() == "my_module:algo"); + } + + SECTION("A configuration without a module label is an error, not an absent plugin") + { + phlex::configuration const config{boost::json::object{}}; + CHECK_THROWS_AS(make_algorithm_name(&config, "algo"), std::runtime_error); + } + + SECTION("A configured empty module label is an error, not an absent plugin") + { + boost::json::object obj; + obj["module_label"] = ""; + phlex::configuration const config{obj}; + CHECK_THROWS_AS(make_algorithm_name(&config, "algo"), std::runtime_error); + } } TEST_CASE("verify_name tests", "[core]") diff --git a/test/output_products_test.cpp b/test/output_products_test.cpp index 762e0c2b4..721fa3eb4 100644 --- a/test/output_products_test.cpp +++ b/test/output_products_test.cpp @@ -10,7 +10,10 @@ #include "phlex/core/product_selector.hpp" #include "phlex/core/provider_node.hpp" #include "phlex/core/source.hpp" +#include "phlex/model/algorithm_name.hpp" #include "phlex/model/data_cell_index.hpp" +#include "phlex/model/identifier.hpp" +#include "phlex/model/product_specification.hpp" #include "phlex/model/product_store.hpp" #include "phlex/model/products.hpp" #include "phlex/model/type_id.hpp" @@ -18,9 +21,14 @@ #include +#include +#include +#include #include #include +#include #include +#include #include using namespace phlex; @@ -55,8 +63,9 @@ namespace { provider_bundles bundles; std::string const layer = "spill"; std::string const stage = "previous_process"; + // The source's product has an explicitly empty suffix. experimental::product_specification spec{ - "provide_name", "", experimental::make_type_id()}; + "provide_name", empty_string{}, experimental::make_type_id()}; if (selector.match(spec, identifier{layer}, identifier{stage})) { bundles.push_back(provider_bundle{.provider_function = give_me_a_name, @@ -70,6 +79,145 @@ namespace { }; } +TEST_CASE("Parse suffix-only product specifications", "[data model]") +{ + using experimental::product_specification; + using namespace experimental::literals; + + SECTION("A named suffix has no creator") + { + for (auto const* text : {"hits", "/hits"}) { + auto const spec = product_specification::create(text); + CHECK_FALSE(spec.has_creator()); + CHECK(spec.plugin().empty()); + CHECK(spec.suffix() == "hits"_id); + CHECK(spec.to_string() == "hits"); + } + } + + SECTION("An empty suffix has no creator") + { + for (auto const* text : {"", "/"}) { + auto const spec = product_specification::create(text); + CHECK_FALSE(spec.has_creator()); + CHECK(spec.suffix().empty()); + CHECK(spec.to_string().empty()); + } + } + + SECTION("All creator-less spellings are the same exact key") + { + CHECK(product_specification::create("hits") == product_specification::create("/hits")); + CHECK(product_specification::create("") == product_specification::create("/")); + CHECK(product_specification::create("hits") != product_specification::create("other")); + } + + SECTION("A creator is parsed when present") + { + auto const spec = product_specification::create("reco:find_hits/hits"); + REQUIRE(spec.has_creator()); + CHECK(spec.creator()->to_string() == "reco:find_hits"); + CHECK(spec.plugin().text() == "reco"); + CHECK(spec.suffix() == "hits"_id); + CHECK(spec != product_specification::create("hits")); + } + + SECTION("An invalid creator is rejected rather than read as absent") + { + CHECK_THROWS_AS(product_specification::create("reco:/hits"), std::runtime_error); + CHECK_THROWS_AS(product_specification::create(":find_hits/hits"), std::runtime_error); + } +} + +TEST_CASE("Product specification suffixes are explicit values", "[data model]") +{ + using experimental::algorithm_name; + using experimental::empty_string; + using experimental::product_specification; + using experimental::suffix_t; + using namespace experimental::literals; + + STATIC_CHECK( + std::is_same_v().suffix()), suffix_t const&>); + + SECTION("A default-constructed specification has an explicitly empty suffix") + { + product_specification const spec; + CHECK(spec.suffix() == suffix_t{empty_string{}}); + CHECK(spec.suffix().empty()); + CHECK(spec.suffix() != suffix_t{"hits"_id}); + } + + SECTION("Parsed empty and named suffixes keep their representation") + { + auto const empty = product_specification::create("reco:find_hits/"); + CHECK(empty.suffix().empty()); + CHECK(empty.suffix() == suffix_t{empty_string{}}); + CHECK(empty.to_string() == "reco:find_hits/"); + + auto const named = product_specification::create("reco:find_hits/hits"); + CHECK_FALSE(named.suffix().empty()); + CHECK(named.suffix() == suffix_t{"hits"_id}); + CHECK(named.suffix().text() == "hits"); + CHECK(named.to_string() == "reco:find_hits/hits"); + + CHECK(empty != named); + CHECK((empty <=> named) == std::strong_ordering::less); + CHECK(std::hash{}(empty) != std::hash{}(named)); + CHECK(std::hash{}(empty) == + std::hash{}(product_specification::create("reco:find_hits/"))); + } + + SECTION("Constructed specifications accept the empty and named alternatives") + { + auto const creator = algorithm_name::create("reco:find_hits"); + product_specification const empty{creator, empty_string{}, experimental::make_type_id()}; + product_specification const named{creator, "hits"_id, experimental::make_type_id()}; + CHECK(empty.suffix().empty()); + CHECK(named.suffix().text() == "hits"); + CHECK(empty != named); + } +} + +TEST_CASE("Fluent output suffixes become explicit suffix values", "[data model]") +{ + using experimental::algorithm_name; + using experimental::empty_string; + using experimental::suffix_t; + using namespace experimental::literals; + + auto const creator = algorithm_name::create("reco:find_hits"); + auto const int_id = experimental::make_type_id(); + auto const double_id = experimental::make_type_id(); + + SECTION("No registered suffixes default to explicitly empty suffixes") + { + auto const specs = phlex::detail::to_product_specifications(creator, {}, {int_id, double_id}); + REQUIRE(specs.size() == 2ull); + for (auto const& spec : specs) { + CHECK(spec.suffix() == suffix_t{empty_string{}}); + CHECK(spec.suffix().empty()); + REQUIRE(spec.has_creator()); + CHECK(*spec.creator() == creator); + } + CHECK(specs[0].type() == int_id); + CHECK(specs[1].type() == double_id); + CHECK(specs[0].to_string() == "reco:find_hits/"); + } + + SECTION("Empty and named registered suffixes are converted at specification creation") + { + auto const specs = + phlex::detail::to_product_specifications(creator, {"", "hits"}, {int_id, double_id}); + REQUIRE(specs.size() == 2ull); + CHECK(specs[0].suffix() == suffix_t{empty_string{}}); + CHECK(specs[1].suffix() == suffix_t{"hits"_id}); + CHECK(specs[0].to_string() == "reco:find_hits/"); + CHECK(specs[1].to_string() == "reco:find_hits/hits"); + CHECK(specs[0] != specs[1]); + } +} + TEST_CASE("Output data products", "[graph]") { auto gen = experimental::layer_generator::make(); diff --git a/test/product_handle_test.cpp b/test/product_handle_test.cpp index 590fcedbf..0e0bf976c 100644 --- a/test/product_handle_test.cpp +++ b/test/product_handle_test.cpp @@ -8,6 +8,8 @@ #include #include +#include +#include #include using namespace phlex; @@ -133,6 +135,116 @@ TEST_CASE("Retrieve product specification from handle", "[data model]") CHECK(h.stage() == "test_stage"); } +TEST_CASE("Handle metadata accessors return textual views", "[data model]") +{ + using handle_t = handle; + STATIC_CHECK(std::is_same_v().suffix()), std::string_view>); + STATIC_CHECK(std::is_same_v().layer()), std::string_view>); + STATIC_CHECK(std::is_same_v().stage()), std::string_view>); + STATIC_CHECK( + std::is_same_v().creator().plugin), std::string_view>); + STATIC_CHECK( + std::is_same_v().creator().algorithm), std::string_view>); +} + +TEST_CASE("Handle suffix views distinguish empty and named suffixes", "[data model]") +{ + int const number{3}; + auto const job = data_cell_index::job(); + + SECTION("Explicit empty suffix") + { + spec_t const spec{"creator/"}; + auto const h = make_handle(number, *job, spec); + CHECK(h.suffix().empty()); + CHECK(h.suffix() == ""); + CHECK(h.creator().algorithm == "creator"); + } + + SECTION("Default specification has an empty suffix") + { + spec_t const spec; + auto const h = make_handle(number, *job, spec); + CHECK(h.suffix().empty()); + } + + SECTION("Named suffix") + { + spec_t const spec{"creator/hits"}; + auto const h = make_handle(number, *job, spec); + CHECK_FALSE(h.suffix().empty()); + CHECK(h.suffix() == "hits"); + } +} + +TEST_CASE("Handle creator views for absent and present creators", "[data model]") +{ + int const number{3}; + auto const job = data_cell_index::job(); + + SECTION("Absent creator with a named suffix") + { + spec_t const spec{"hits"}; + REQUIRE_FALSE(spec.has_creator()); + auto const h = make_handle(number, *job, spec); + CHECK(h.creator().plugin.empty()); + CHECK(h.creator().algorithm.empty()); + CHECK(h.suffix() == "hits"); + } + + SECTION("Absent creator with an empty suffix") + { + spec_t const spec{"/"}; + REQUIRE_FALSE(spec.has_creator()); + auto const h = make_handle(number, *job, spec); + CHECK(h.creator().plugin.empty()); + CHECK(h.creator().algorithm.empty()); + CHECK(h.suffix().empty()); + } + + SECTION("Unqualified creator has an empty plugin view") + { + spec_t const spec{"find_hits/hits"}; + REQUIRE(spec.has_creator()); + auto const h = make_handle(number, *job, spec); + CHECK(h.creator().plugin.empty()); + CHECK(h.creator().algorithm == "find_hits"); + } + + SECTION("Qualified creator exposes both components") + { + spec_t const spec{"reco:find_hits/hits"}; + REQUIRE(spec.has_creator()); + auto const h = make_handle(number, *job, spec); + CHECK(h.creator().plugin == "reco"); + CHECK(h.creator().algorithm == "find_hits"); + CHECK(h.suffix() == "hits"); + } +} + +TEST_CASE("Handle views are borrowed from the owning specification", "[data model]") +{ + int const number{3}; + auto const job = data_cell_index::job(); + spec_t const spec{"reco:find_hits/hits"}; + auto const h = make_handle(number, *job, spec); + + // The views alias the text stored in the specification's values, rather than copies. + auto const creator = h.creator(); + CHECK(creator.plugin.data() == spec.plugin().text().data()); + CHECK(creator.algorithm.data() == std::string_view(*spec.algorithm()).data()); + CHECK(h.suffix().data() == spec.suffix().text().data()); + + // Repeated calls refer to the same borrowed storage. + CHECK(h.suffix().data() == h.suffix().data()); + CHECK(h.creator().plugin.data() == creator.plugin.data()); + + // A copy of the handle borrows from the same specification. + auto const copy = h; + CHECK(copy.suffix().data() == spec.suffix().text().data()); + CHECK(copy.creator().algorithm.data() == creator.algorithm.data()); +} + TEST_CASE("Retrieve stage from handle", "[data model]") { int const number{3}; diff --git a/test/product_store_test.cpp b/test/product_store_test.cpp index 9ad38e8e5..210781d37 100644 --- a/test/product_store_test.cpp +++ b/test/product_store_test.cpp @@ -13,6 +13,7 @@ #include #include +#include #include #include #include @@ -61,6 +62,64 @@ TEST_CASE("Product store insertion", "[data model]") CHECK(store->get_product>(gsl::make_not_null(&numbers_spec)) == many_numbers); } +TEST_CASE("Product store lookup of creator-less specifications is exact", "[data model]") +{ + auto const creator = algorithm_name::create("test_algorithm"); + auto const stage = "test_stage"_id; + auto store = product_store::base(gsl::make_not_null(&creator), gsl::make_not_null(&stage)); + + auto const type = make_type_id(); + product_specification const creatorless{std::nullopt, "hits"_id, type}; + product_specification const with_creator{creator, "hits"_id, type}; + product_specification const other_creator{ + algorithm_name::create("reco:find_hits"), "hits"_id, type}; + product_specification const parsed_creatorless{"/hits"}; + + store->add_product(creatorless, 1); + store->add_product(with_creator, 2); + + // Spellings of a creator-less specification all find the same product. + CHECK(parsed_creatorless == creatorless); + CHECK(product_specification{"hits"} == creatorless); + + CHECK(store->get_product(gsl::make_not_null(&creatorless)) == 1); + CHECK(store->get_product(gsl::make_not_null(&parsed_creatorless)) == 1); + CHECK(store->get_product(gsl::make_not_null(&with_creator)) == 2); + + // A creator-less specification is not a wildcard, nor does it match a different creator. + CHECK_THROWS_WITH(store->get_handle(gsl::make_not_null(&other_creator)), + Catch::Matchers::ContainsSubstring( + "No product exists with the specification 'reco:find_hits/hits'.")); + CHECK(store->get_handle(gsl::make_not_null(&creatorless)).suffix() == "hits"); +} + +TEST_CASE("Product store with the default source creator", "[data model]") +{ + auto const source = product_store::default_source(); + CHECK(source.to_string() == "phlex_source"); + CHECK(source.plugin() == empty_string{}); + CHECK(source.algorithm() == "phlex_source"_id); + CHECK(source == algorithm_name::create("phlex_source")); + + auto const stage = "test_stage"_id; + auto store = product_store::base(gsl::make_not_null(&source), gsl::make_not_null(&stage)); + CHECK(store->source() == source); + + // Insertion of a product created by the source, and exact lookup by its specification. + auto const type = make_type_id(); + product_specification const from_source{source, "hits"_id, type}; + product_specification const other_creator{ + algorithm_name::create("test_algorithm"), "hits"_id, type}; + store->add_product(from_source, 7); + + CHECK(store->size() == 1ull); + CHECK(store->get_product(gsl::make_not_null(&from_source)) == 7); + CHECK(store->get_handle(gsl::make_not_null(&from_source)).suffix() == "hits"); + CHECK_THROWS_WITH(store->get_handle(gsl::make_not_null(&other_creator)), + Catch::Matchers::ContainsSubstring( + "No product exists with the specification 'test_algorithm/hits'.")); +} + TEST_CASE("Product retrieval requires exactly one match", "[data model]") { using Catch::Matchers::ContainsSubstring; diff --git a/test/products_test.cpp b/test/products_test.cpp index 4773c24cd..a2465068a 100644 --- a/test/products_test.cpp +++ b/test/products_test.cpp @@ -1,7 +1,17 @@ +#include "phlex/model/algorithm_name.hpp" +#include "phlex/model/identifier.hpp" +#include "phlex/model/product_specification.hpp" #include "phlex/model/products.hpp" +#include "phlex/model/type_id.hpp" #include #include +#include + +#include +#include +#include +#include TEST_CASE("Reject null product pointers", "[data model]") { @@ -9,3 +19,66 @@ TEST_CASE("Reject null product pointers", "[data model]") CHECK_THROWS_WITH(product_for(product_ptr{}), "Cannot store a null product pointer."); CHECK_THROWS_WITH(product_for(nullptr), "Cannot store a null product pointer."); } + +TEST_CASE("Creator-less product specifications are exact keys", "[data model]") +{ + using namespace phlex::experimental; + using namespace phlex::experimental::literals; + using Catch::Matchers::ContainsSubstring; + + auto const type = make_type_id(); + product_specification const creatorless{std::nullopt, "hits"_id, type}; + product_specification const creator_present{ + algorithm_name::create("reco:find_hits"), "hits"_id, type}; + product_specification const other_suffix{std::nullopt, "tracks"_id, type}; + + SECTION("Absence is distinct from every present creator") + { + CHECK_FALSE(creatorless.has_creator()); + CHECK_FALSE(creatorless.creator().has_value()); + CHECK(creatorless.algorithm() == nullptr); + CHECK(creatorless.plugin().empty()); + CHECK(creator_present.has_creator()); + CHECK(creator_present.plugin().text() == "reco"); + REQUIRE(creator_present.algorithm() != nullptr); + CHECK(*creator_present.algorithm() == "find_hits"_id); + + CHECK(creatorless == product_specification{std::nullopt, "hits"_id, type}); + CHECK(creatorless != creator_present); + CHECK(creatorless != other_suffix); + CHECK((creatorless <=> creator_present) == std::strong_ordering::less); + CHECK(std::hash{}(creatorless) == + std::hash{}(product_specification{std::nullopt, "hits"_id, type})); + CHECK(std::hash{}(creatorless) != + std::hash{}(creator_present)); + + std::set const ordered{creatorless, creator_present, other_suffix}; + CHECK(ordered.size() == 3ull); + std::unordered_set const hashed{creatorless, creator_present, other_suffix}; + CHECK(hashed.size() == 3ull); + } + + SECTION("Creator-less text omits the creator and slash") + { + CHECK(creatorless.to_string() == "hits"); + CHECK(creator_present.to_string() == "reco:find_hits/hits"); + } + + SECTION("Lookup is exact and a missing creator is not a wildcard") + { + phlex::detail::products products; + products.add(creatorless, 1); + CHECK(products.get(creatorless) == 1); + CHECK_THROWS_WITH(products.get(creator_present), + ContainsSubstring("No product exists with the specification " + "'reco:find_hits/hits'.")); + + phlex::detail::products with_creator; + with_creator.add(creator_present, 2); + CHECK(with_creator.get(creator_present) == 2); + CHECK_THROWS_WITH(with_creator.get(creatorless), + ContainsSubstring("No product exists with the specification 'hits'.")); + CHECK_THROWS_WITH(with_creator.get(other_suffix), + ContainsSubstring("No product exists with the specification 'tracks'.")); + } +} diff --git a/test/provider_test.cpp b/test/provider_test.cpp index cfd1ceaf3..aa07462f2 100644 --- a/test/provider_test.cpp +++ b/test/provider_test.cpp @@ -14,10 +14,13 @@ #include #include #include +#include #include #include #include +#include +#include #include #include @@ -95,6 +98,53 @@ namespace { unsigned pass_on(vertex_collection const& vertices) { return vertices.data; } } +TEST_CASE("Provider construction requires a creator", "[graph]") +{ + using namespace phlex::experimental; + using namespace phlex::experimental::literals; + + tbb::flow::graph g; + auto const type = make_type_id(); + + SECTION("A bundle with a present creator is accepted") + { + provider_bundle bundle{ + .provider_function = give_me_vertices_erased, + .max_concurrency = concurrency::serial, + .spec = product_specification{algorithm_name::create("reco:find_hits"), "hits"_id, type}, + .layer = "job", + .stage = "test"}; + phlex::detail::provider_node const node{g, std::move(bundle)}; + CHECK(node.name().to_string() == "reco:find_hits"); + CHECK(node.name().plugin().text() == "reco"); + CHECK(node.output_product().to_string() == "reco:find_hits/hits"); + } + + SECTION("A bundle with an absent creator is an error") + { + provider_bundle bundle{.provider_function = give_me_vertices_erased, + .max_concurrency = concurrency::serial, + .spec = product_specification{std::nullopt, "hits"_id, type}, + .layer = "job", + .stage = "test"}; + CHECK_THROWS_WITH(phlex::detail::provider_node(g, std::move(bundle)), + ContainsSubstring("has no creator")); + } + + SECTION("An output specification with an absent creator is an error") + { + CHECK_THROWS_AS( + phlex::detail::provider_node(algorithm_name::create("find_hits"), + 1, + g, + give_me_vertices_erased, + product_specification{std::nullopt, "hits"_id, type}, + "job"_id, + "test"_id), + std::runtime_error); + } +} + TEST_CASE("Explicit providers") { constexpr auto num_spills{3u};