From 61f3b992441d379564a492be6b24d6e735bd59fe Mon Sep 17 00:00:00 2001 From: Michael Tarassov Date: Sun, 23 Aug 2026 14:36:02 +0500 Subject: [PATCH 1/2] =?UTF-8?q?chore:=20dead=20template=20surface=20?= =?UTF-8?q?=E2=80=94=20instantiate=20what=20we=20ship,=20drop=20what=20not?= =?UTF-8?q?hing=20uses?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- docs/CONVENTIONS.md | 16 ++++ src/api/Validation.hpp | 13 ++- src/messaging/Messaging.hpp | 12 --- src/security/Auth.cpp | 23 ----- src/security/Auth.hpp | 6 -- tests/integration/test_database.cpp | 82 ++++++++++++++++++ tests/unit/test_crud_owned.cpp | 126 ++++++++++++++++++++++++++++ tests/unit/test_module_guards.cpp | 63 +++++++++++++- 8 files changed, 296 insertions(+), 45 deletions(-) create mode 100644 tests/unit/test_crud_owned.cpp diff --git a/docs/CONVENTIONS.md b/docs/CONVENTIONS.md index 3fe12d2..06f0133 100644 --- a/docs/CONVENTIONS.md +++ b/docs/CONVENTIONS.md @@ -57,6 +57,22 @@ hand-written variant because it joins roles): - Every write wraps `Database::get().execute_write([&](auto& txn){…})`. The lambda MUST take `auto& txn` (it receives `detail::TracingTxn&`, not a raw `pqxx::work&`). Use `execute_read_primary` for read-after-write. +- Two more `execute_*` variants exist for forks (nothing in the template + itself calls them — don't let that fool you into deleting them): + - `execute_write_idempotent(fn)` — same as `execute_write` but with the + liberal read-style retry classifier, which may REPLAY the whole + transaction after a connection-class error whose commit already landed. + Safe only for writes keyed by a natural key (UPSERT by name, DELETE by + PK) where a replay converges instead of double-applying. + - `execute_transaction(IsolationLevel, fn)` — multi-statement transaction + at `ReadCommitted` / `RepeatableRead` / `Serializable` (one-arg overload + defaults to ReadCommitted). Retry classification matches + `execute_write` (only PG-confirmed 40001/40P01 rollbacks — exactly what + Serializable produces under contention). + + Both are pinned by real-Postgres tests in + `tests/integration/test_database.cpp` and compile-checked against the DI + seam in `tests/unit/test_database_seam.cpp`. - Wrap UNIQUE/FK-tripping writes in `detail::translate_sql(...)` (`repositories/SqlErrors.hpp`) to turn a SQLSTATE into your typed exception — otherwise a constraint violation surfaces as a raw 500. For the ubiquitous diff --git a/src/api/Validation.hpp b/src/api/Validation.hpp index fc2d8e1..0a9341e 100644 --- a/src/api/Validation.hpp +++ b/src/api/Validation.hpp @@ -32,6 +32,7 @@ #include +#include "api/RequestUtils.hpp" #include "utils/ErrorResponse.hpp" namespace Api::Validation { @@ -224,8 +225,16 @@ inline void email(Errors& errs, const json& body, const std::string& field) { } inline void uuid(Errors& errs, const json& body, const std::string& field) { - static const std::regex re("^[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{4}-[0-9a-fA-F]{12}$"); - regex_match(errs, body, field, re, "uuid"); + // Single source of truth for the 8-4-4-4-12 hex shape: the char-walk in + // RequestUtils (Api::detail::is_uuid_segment) — this used to be a second, + // regex-based UUID implementation that could drift from it. Same error + // contract as the regex validators: "bad_format" on mismatch, "not_string" + // on a wrong-typed field, no-op when absent (presence is require()'s job). + const auto* s = detail::as_string(errs, body, field); + if (!s) + return; + if (!Api::detail::is_uuid_segment(*s)) + errs.add(field, "bad_format", "expected format: uuid"); } // --------------------------------------------------------------------------- diff --git a/src/messaging/Messaging.hpp b/src/messaging/Messaging.hpp index 0b8c92a..385760e 100644 --- a/src/messaging/Messaging.hpp +++ b/src/messaging/Messaging.hpp @@ -138,8 +138,6 @@ class KafkaProducer { return true; } - bool produce(const std::string& topic, const std::string& payload) { return produce(topic, "", payload); } - int flush(int timeout_ms = 10000) { check_initialized(); return producer->flush(timeout_ms); @@ -307,16 +305,6 @@ class KafkaConsumer { void stop_consuming() { consuming = false; } - bool commit() { - check_initialized(); - RdKafka::ErrorCode err = consumer->commitSync(); - if (err != RdKafka::ERR_NO_ERROR) { - spdlog::error("Failed to commit offsets: {}", RdKafka::err2str(err)); - return false; - } - return true; - } - void shutdown() { if (initialized) { spdlog::info("Shutting down Kafka consumer"); diff --git a/src/security/Auth.cpp b/src/security/Auth.cpp index c3a2fc8..4e9e4e1 100644 --- a/src/security/Auth.cpp +++ b/src/security/Auth.cpp @@ -281,17 +281,6 @@ bool has_role(const drogon::HttpRequestPtr& req, const std::string& role) { return std::find(p->roles.begin(), p->roles.end(), role) != p->roles.end(); } -bool has_any_role(const drogon::HttpRequestPtr& req, const std::vector& roles) { - auto p = principal_of(req); - if (!p) - return false; - for (const auto& want : roles) { - if (std::find(p->roles.begin(), p->roles.end(), want) != p->roles.end()) - return true; - } - return false; -} - bool auth_enforced() { return is_initialized() && get().config().mode != AuthMode::None; } @@ -304,14 +293,6 @@ drogon::HttpResponsePtr require_role(const drogon::HttpRequestPtr& req, const st return ErrorResponse::make({drogon::k403Forbidden, "forbidden", "", nlohmann::json{{"required_role", role}}}); } -drogon::HttpResponsePtr require_any_role(const drogon::HttpRequestPtr& req, const std::vector& roles) { - if (!auth_enforced()) - return {}; - if (has_any_role(req, roles)) - return {}; - return ErrorResponse::make({drogon::k403Forbidden, "forbidden", "", nlohmann::json{{"required_roles", roles}}}); -} - drogon::HttpResponsePtr require_confirmed(const drogon::HttpRequestPtr& req) { if (!auth_enforced()) return {}; @@ -351,10 +332,6 @@ bool current_user_can(const drogon::HttpRequestPtr& req, std::uint32_t perm) { return (have & perm) == perm; } -bool current_user_is_admin(const drogon::HttpRequestPtr& req) { - return current_user_can(req, kAdminPermissionBits); -} - drogon::HttpResponsePtr require_permission(const drogon::HttpRequestPtr& req, std::uint32_t perm) { if (!auth_enforced()) return {}; diff --git a/src/security/Auth.hpp b/src/security/Auth.hpp index 525e4fb..381924b 100644 --- a/src/security/Auth.hpp +++ b/src/security/Auth.hpp @@ -134,8 +134,6 @@ std::optional principal_of(const drogon::HttpRequestPtr& req); bool has_role(const drogon::HttpRequestPtr& req, const std::string& role); -bool has_any_role(const drogon::HttpRequestPtr& req, const std::vector& roles); - /** * @brief True when auth checks must be enforced — the module is initialized * and not running in AuthMode::None. Shared guard for the require_* @@ -149,8 +147,6 @@ bool auth_enforced(); */ drogon::HttpResponsePtr require_role(const drogon::HttpRequestPtr& req, const std::string& role); -drogon::HttpResponsePtr require_any_role(const drogon::HttpRequestPtr& req, const std::vector& roles); - /** * @brief nullptr if the caller's email is confirmed (or auth is disabled); * 401 if anonymous; 403 if authenticated but unconfirmed. The "confirmed" @@ -178,8 +174,6 @@ std::uint32_t current_permissions(const drogon::HttpRequestPtr& req); bool current_user_can(const drogon::HttpRequestPtr& req, std::uint32_t perm); -bool current_user_is_admin(const drogon::HttpRequestPtr& req); - /** * @brief Returns a 403 response if the request's principal lacks the * permission, or nullptr if it has it (or auth is disabled). diff --git a/tests/integration/test_database.cpp b/tests/integration/test_database.cpp index 87e54c3..aec2941 100644 --- a/tests/integration/test_database.cpp +++ b/tests/integration/test_database.cpp @@ -161,6 +161,88 @@ TEST_F(DatabaseManagerTest, ExecuteWrite) { EXPECT_EQ(result[0][0].template as(), "test_value"); } +// --- execute_transaction / execute_write_idempotent (fork-facing API) --- +// Real-Postgres coverage for the two execute_* variants nothing in the +// template calls in production code — forks do (documented in +// docs/CONVENTIONS.md §3). The unit-level seam only proves they compile and +// enter the hook (test_database_seam.cpp); these prove the actual semantics. + +TEST_F(DatabaseManagerTest, ExecuteTransactionRunsAtRequestedIsolationLevel) { + auto isolation = [](auto& txn) { + auto r = txn.exec("SHOW transaction_isolation"); + return r[0][0].template as(); + }; + + // Convenience overload = ReadCommitted (the Postgres default — no SET + // TRANSACTION is issued on this path). + EXPECT_EQ(Database::get().execute_transaction(isolation), "read committed"); + EXPECT_EQ(Database::get().execute_transaction(Database::IsolationLevel::ReadCommitted, isolation), + "read committed"); + EXPECT_EQ(Database::get().execute_transaction(Database::IsolationLevel::RepeatableRead, isolation), + "repeatable read"); + EXPECT_EQ(Database::get().execute_transaction(Database::IsolationLevel::Serializable, isolation), "serializable"); +} + +TEST_F(DatabaseManagerTest, ExecuteTransactionIsAtomicAcrossStatements) { + Database::get().execute_write([](auto& txn) { + txn.exec("DROP TABLE IF EXISTS test_table"); + txn.exec("CREATE TABLE test_table (id SERIAL PRIMARY KEY, name TEXT)"); + return 0; + }); + + // Happy path: both statements of one transaction land together. + const int inserted = Database::get().execute_transaction([](auto& txn) { + txn.exec("INSERT INTO test_table (name) VALUES ('a')"); + txn.exec("INSERT INTO test_table (name) VALUES ('b')"); + return 2; + }); + EXPECT_EQ(inserted, 2); + + // Failure path: a throw after the first INSERT must roll BOTH back — + // a non-pqxx exception is not classified transient, so no retry either. + EXPECT_THROW(Database::get().execute_transaction([](auto& txn) { + txn.exec("INSERT INTO test_table (name) VALUES ('c')"); + throw std::runtime_error("boom"); + return 0; + }), + std::runtime_error); + + auto r = Database::get().execute_read([](auto& txn) { return txn.exec("SELECT COUNT(*) FROM test_table"); }); + EXPECT_EQ(r[0][0].template as(), 2) << "the failed transaction must leave no partial rows"; +} + +TEST_F(DatabaseManagerTest, ExecuteWriteIdempotentUpsertConvergesOnReplay) { + // execute_write_idempotent uses the liberal (read-style) retry classifier, + // which may replay the whole transaction after a connection-class error a + // commit could already have survived. That is safe ONLY for writes keyed + // by a natural key — replaying converges instead of double-applying. + // Exercise exactly that contract: an UPSERT keyed by name. + Database::get().execute_write([](auto& txn) { + txn.exec("DROP TABLE IF EXISTS test_table"); + txn.exec("CREATE TABLE test_table (name TEXT PRIMARY KEY, val INT NOT NULL)"); + return 0; + }); + + auto upsert = [](int val) { + return Database::get().execute_write_idempotent([val](auto& txn) { + txn.exec_params( + "INSERT INTO test_table (name, val) VALUES ($1, $2) " + "ON CONFLICT (name) DO UPDATE SET val = EXCLUDED.val", + "job-42", + val); + return val; + }); + }; + + EXPECT_EQ(upsert(1), 1); + EXPECT_EQ(upsert(2), 2); // replay of the same logical write — converges + + auto r = + Database::get().execute_read([](auto& txn) { return txn.exec("SELECT COUNT(*), MAX(val) FROM test_table"); }); + EXPECT_EQ(r[0][0].template as(), 1) << "same key must never duplicate"; + EXPECT_EQ(r[0][1].template as(), 2) << "last write wins"; +} + TEST_F(DatabaseLifecycleTest, UnreachableReplicaDoesNotBlockBoot) { // Replica is an optional read optimization: a dead replica URL must not // fail initialization (it used to crash-loop the whole app). Reads then diff --git a/tests/unit/test_crud_owned.cpp b/tests/unit/test_crud_owned.cpp new file mode 100644 index 0000000..522495a --- /dev/null +++ b/tests/unit/test_crud_owned.cpp @@ -0,0 +1,126 @@ +/** + * @file test_crud_owned.cpp + * @brief Compile-time + behavioral pin for CrudBase's ownership-scoped reads + * (find_owned / list_owned / count_owned, unlocked by kOwnerColumn). + * + * These are member templates of a class template: they only + * type-check when something instantiates them. In the template repo + * itself only find_owned is instantiated by production code + * (BillingController.cpp via BillingRepository, kOwnerColumn = + * "user_id"); list_owned/count_owned are instantiated by the output + * of `new-resource.sh --owned` (the generated controller + * calls all three) — i.e. by FORK code, not by anything this repo + * compiles. Without this test, a silently broken signature would + * pass CI here and detonate in every fork's first `--owned` resource. + * + * Runs against the Database DI seam (Database::install_for_testing, + * same fake shape as test_database_seam.cpp) — no Postgres. The fake + * returns EMPTY pqxx::results, which is enough to prove the owner + * predicate lands in the SQL template and the empty-result paths + * behave (nullopt / empty vector). Pure unit. + */ + +#include +#include +#include +#include +#include + +#include + +#include "database/Database.hpp" +#include "repositories/CrudBase.hpp" + +namespace { + +// ---- Database seam fake (mirrors test_database_seam.cpp) ------------------- + +class RecordingTxn : public Database::detail::ErasedTxn { +public: + std::vector queries; + + pqxx::result exec_params_erased(const std::string& query, const pqxx::params& /*params*/) override { + queries.push_back(query); + return pqxx::result{}; + } + + pqxx::result exec_erased(const std::string& query) override { + queries.push_back(query); + return pqxx::result{}; + } +}; + +class FakeDatabase : public Database::DatabaseManager { +public: + RecordingTxn txn; + std::map entered; // "@" -> count + + bool is_initialized() const override { return true; } + bool health_check() override { return true; } + +protected: + Database::detail::ErasedTxn* test_transaction_(const char* op, const char* pool) override { + ++entered[std::string(op) + "@" + pool]; + return &txn; + } +}; + +// ---- Minimal owned repository (the shape new-resource.sh --owned emits) ---- + +struct OwnedNote { + std::string id; + static OwnedNote from_row(const pqxx::row& /*row*/) { return {}; } +}; + +class OwnedNoteRepository : public Repositories::CrudBase { +public: + static constexpr const char* kTable = "owned_notes"; + static constexpr const char* kColumns = "id, owner_id"; + static constexpr const char* kIdColumn = "id"; + static constexpr const char* kOrderBy = "id"; + static constexpr const char* kOwnerColumn = "owner_id"; +}; + +class CrudOwnedTest : public ::testing::Test { +protected: + void SetUp() override { + auto fake = std::make_unique(); + fake_ = fake.get(); + Database::install_for_testing(std::move(fake)); + } + void TearDown() override { Database::reset_for_testing(); } + + FakeDatabase* fake_ = nullptr; + OwnedNoteRepository repo_; +}; + +TEST_F(CrudOwnedTest, FindOwnedScopesByIdAndOwner) { + auto found = repo_.find_owned("note-1", "user-1"); + EXPECT_FALSE(found.has_value()); // empty fake result → nullopt, no from_row + ASSERT_EQ(fake_->txn.queries.size(), 1u); + EXPECT_EQ(fake_->txn.queries[0], "SELECT id, owner_id FROM owned_notes WHERE id = $1 AND owner_id = $2"); + EXPECT_EQ(fake_->entered["db.read@replica"], 1); + + // from_primary=true routes through the read-after-write path. + repo_.find_owned("note-1", "user-1", /*from_primary=*/true); + EXPECT_EQ(fake_->entered["db.read@primary"], 1); +} + +TEST_F(CrudOwnedTest, ListOwnedScopesByOwner) { + auto rows = repo_.list_owned("user-1", 25, 5); + EXPECT_TRUE(rows.empty()); + ASSERT_EQ(fake_->txn.queries.size(), 1u); + EXPECT_EQ(fake_->txn.queries[0], + "SELECT id, owner_id FROM owned_notes WHERE owner_id = $1 ORDER BY id LIMIT $2 OFFSET $3"); +} + +TEST_F(CrudOwnedTest, CountOwnedScopesByOwner) { + // The empty fake result can't carry the COUNT(*) row, so reading it throws + // (seam limitation — see test_database_seam.cpp header). The template still + // fully instantiates and the SQL it issued is observable. + EXPECT_THROW((void)repo_.count_owned("user-1"), std::exception); + ASSERT_EQ(fake_->txn.queries.size(), 1u); + EXPECT_EQ(fake_->txn.queries[0], "SELECT COUNT(*) FROM owned_notes WHERE owner_id = $1"); +} + +} // namespace diff --git a/tests/unit/test_module_guards.cpp b/tests/unit/test_module_guards.cpp index b8a0699..a5b5f5e 100644 --- a/tests/unit/test_module_guards.cpp +++ b/tests/unit/test_module_guards.cpp @@ -1,8 +1,10 @@ /** * @file test_module_guards.cpp * @brief Unit tests for the lifecycle/guard contracts of modules that have - * no other direct coverage: Messaging, Tasks, and the SqlErrors - * translate_sql wrapper. Pure — no Kafka broker, no Postgres. + * no other direct coverage: Messaging (incl. the MessagingSystem / + * producer / consumer pre-init guards), Tasks, Migrations' singleton + * doorway, the JobQueue config accessors, and the SqlErrors + * translate_sql wrapper. Pure — no Kafka broker, no Postgres/Redis. */ #include @@ -10,6 +12,8 @@ #include +#include "database/Migrations.hpp" +#include "jobs/Jobs.hpp" #include "messaging/Messaging.hpp" #include "repositories/SqlErrors.hpp" #include "tasks/Tasks.hpp" @@ -40,6 +44,61 @@ TEST(MessagingGuardTest, InitThrowsOnDoubleInitAndShutdownResets) { EXPECT_THROW(Messaging::get(), std::runtime_error); } +// Smoke over the template-API surface below Messaging::get() — the accessor +// guards that forks hit first when wiring a producer/consumer. None of this +// touches librdkafka objects: everything must throw/report BEFORE any broker +// I/O could happen. +TEST(MessagingGuardTest, SystemAccessorsGuardBeforeComponentInit) { + if (Messaging::is_initialized()) + Messaging::shutdown(); + Messaging::initialize(); + auto& sys = Messaging::get(); + EXPECT_FALSE(sys.has_producer()); + EXPECT_FALSE(sys.has_consumer()); + EXPECT_THROW(sys.get_producer(), std::runtime_error); + EXPECT_THROW(sys.get_consumer(), std::runtime_error); + Messaging::shutdown(); +} + +TEST(MessagingGuardTest, ProducerAndConsumerOpsThrowBeforeInit) { + Messaging::KafkaProducer producer; + EXPECT_FALSE(producer.is_initialized()); + EXPECT_THROW(producer.produce("topic", "key", "payload"), std::runtime_error); + EXPECT_THROW(producer.flush(0), std::runtime_error); + EXPECT_THROW(producer.outq_len(), std::runtime_error); + + Messaging::KafkaConsumer consumer; + EXPECT_FALSE(consumer.is_initialized()); + EXPECT_FALSE(consumer.is_consuming()); + EXPECT_THROW(consumer.consume(0), std::runtime_error); + EXPECT_THROW(consumer.start_consuming([](const std::string&, const std::string&) {}, 0), std::runtime_error); + // stop_consuming/shutdown are deliberately safe no-ops pre-init. + EXPECT_NO_THROW(consumer.stop_consuming()); + EXPECT_NO_THROW(consumer.shutdown()); + EXPECT_NO_THROW(producer.shutdown()); +} + +// ---- Migrations singleton doorway ------------------------------------------ + +TEST(MigrationsGuardTest, GetBeforeInitThrows) { + if (Migrations::is_initialized()) + Migrations::shutdown(); + EXPECT_FALSE(Migrations::is_initialized()); + EXPECT_THROW(Migrations::get(), std::runtime_error); +} + +// ---- JobQueue config accessors --------------------------------------------- + +TEST(JobsAccessorTest, DefaultsVisibleThroughAccessors) { + // A fresh (never-initialized) JobQueue exposes the documented defaults — + // the same values Jobs::initialize() falls back to (JOBS_MAX_RETRIES=3, + // JOBS_RESULT_TTL=86400 in docs/CONFIG.md). No Redis involved. + Jobs::JobQueue q; + EXPECT_FALSE(q.is_initialized()); + EXPECT_EQ(q.default_max_retries(), 3); + EXPECT_EQ(q.result_ttl(), 86400); +} + // ---- Tasks guards ---------------------------------------------------------- TEST(TasksGuardTest, ScheduleBeforeInitThrows) { From 836165c080e1a90d12cd8020aab580ae893ae87e Mon Sep 17 00:00:00 2001 From: Michael Tarassov Date: Sun, 23 Aug 2026 14:42:22 +0500 Subject: [PATCH 2/2] =?UTF-8?q?fix(test):=20fictional=20owned=20entity=20m?= =?UTF-8?q?ust=20use=20the=20templated=20from=5Frow=20convention=20?= =?UTF-8?q?=E2=80=94=20pqxx=20iteration=20yields=20row=5Fref?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- tests/unit/test_crud_owned.cpp | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/tests/unit/test_crud_owned.cpp b/tests/unit/test_crud_owned.cpp index 522495a..4fe6910 100644 --- a/tests/unit/test_crud_owned.cpp +++ b/tests/unit/test_crud_owned.cpp @@ -69,7 +69,13 @@ class FakeDatabase : public Database::DatabaseManager { struct OwnedNote { std::string id; - static OwnedNote from_row(const pqxx::row& /*row*/) { return {}; } + // Templated like every shipped domain struct: pqxx result iteration + // yields row_ref, not row& — a concrete `const pqxx::row&` signature + // fails to compile inside list_owned (this test proved it live). + template + static OwnedNote from_row(const Row& /*row*/) { + return {}; + } }; class OwnedNoteRepository : public Repositories::CrudBase {