Răsfoiți Sursa

feat(relations): T4 - restrict/no_action delete-blocking logic, unit-tested

findRelationBlocks() + formatRelationBlockError() + RelationEnforcer in
new service/src/relations/relation_enforcement.{hpp,cpp}: pure, gRPC-free
logic answering "may this parent delete proceed" via the O(1) reverse
index from T2/T3. Only restrict blocks; no_action permits and leaves the
reference dangling (documented escape hatch); cascade/set_null also
permit for now since their destructive behaviour is T12's, not this
task's. relationsEnforced is checked by the caller, not inside
findRelationBlocks, so `relations check` (T7) can still get an honest
answer regardless of the flag.

Not wired into DatabaseGrpcImpl::Delete or DatabaseService - RelationManager
has no live instance/boot loadFromStore()/constructor plumbing anywhere yet,
and that wiring is T6's scope ("boot re-arm"). This task delivers the
brief's unit-level tests only; T6 wires it into Delete, T9 tests it over
the real gRPC boundary.

tests/test_relation_enforcement.cpp: 3 new tests (restrict blocks + names
blockers, cascade/set_null permit for now, relationsEnforced=false skips
the check), pulling in RelationManager + MemoryStore alongside the
existing LMDB-only T2/T3 tests. 27/27 passing.
fszontagh 1 lună în urmă
părinte
comite
b600990d9d

+ 103 - 0
service/src/relations/relation_enforcement.cpp

@@ -0,0 +1,103 @@
+#include "relation_enforcement.hpp"
+
+#include "../project_addressing.hpp"
+#include "../storage/document_store_lmdb.hpp"
+
+#include <sstream>
+
+#include <spdlog/spdlog.h>
+
+namespace smartbotic::database {
+
+std::vector<RelationBlock> findRelationBlocks(
+    RelationManager& relations,
+    smartbotic::db::storage::LmdbDocumentStore& store,
+    const std::string& qualifiedParentCollection,
+    const std::string& parentId) {
+    std::vector<RelationBlock> out;
+
+    for (const auto& r : relations.relationsWithParent(qualifiedParentCollection)) {
+        // Only restrict blocks. no_action is the documented escape hatch;
+        // cascade/set_null are not yet destructive (Task 12) and behave as
+        // permissive until then - see the file header.
+        if (r.onDelete != OnDelete::Restrict) continue;
+
+        // relation_index_* take the BARE relation name; RelationInfo::name
+        // is project-qualified.
+        std::string bareRelation;
+        try {
+            bareRelation = resolveCollection(r.name).collection;
+        } catch (const std::exception&) {
+            // Malformed relation name should never happen (createRelation
+            // validates it), but a delete is not the place to throw over a
+            // data problem in an unrelated declaration - skip it.
+            continue;
+        }
+
+        uint64_t count = store.relation_index_child_count(bareRelation, parentId);
+        if (count == 0) continue; // no reference -> nothing to block on
+
+        RelationBlock block;
+        block.relation = r.name;
+        block.childCollection = r.child;
+        block.childCount = count;
+        block.sampleChildIds = store.relation_index_children(bareRelation, parentId, 5);
+        out.push_back(std::move(block));
+    }
+
+    return out;
+}
+
+std::string formatRelationBlockError(
+    const std::string& qualifiedParentCollection,
+    const std::string& parentId,
+    const std::vector<RelationBlock>& blocks) {
+    if (blocks.empty()) return "";
+
+    std::ostringstream os;
+    os << "cannot delete '" << qualifiedParentCollection << "/" << parentId
+       << "': " << blocks.size() << " relation(s) still reference it -";
+    for (const auto& b : blocks) {
+        os << " relation '" << b.relation << "' has " << b.childCount
+           << " child document(s) in '" << b.childCollection << "' referencing it";
+        if (!b.sampleChildIds.empty()) {
+            os << " (e.g. ";
+            for (size_t i = 0; i < b.sampleChildIds.size(); ++i) {
+                if (i) os << ", ";
+                os << b.sampleChildIds[i];
+            }
+            os << ")";
+        }
+        os << ";";
+    }
+    os << " delete or re-point the referencing document(s) first, or change "
+          "the relation's on_delete to no_action to permit the dangling "
+          "reference.";
+    return os.str();
+}
+
+RelationEnforcer::RelationEnforcer(RelationManager& relations,
+                                    smartbotic::db::storage::LmdbDocumentStore& store,
+                                    const bool& relationsEnforced)
+    : relations_(relations), store_(store), relationsEnforced_(relationsEnforced) {}
+
+bool RelationEnforcer::canDelete(const std::string& qualifiedParentCollection,
+                                  const std::string& parentId,
+                                  std::string& err) {
+    if (!relationsEnforced_) {
+        spdlog::info(
+            "relations: enforcement disabled for '{}' - permitting delete of '{}' "
+            "unchecked. This is NOT retroactive: dangling references this creates "
+            "will not be found by re-enabling enforcement, only by `relations check`.",
+            qualifiedParentCollection, parentId);
+        return true;
+    }
+
+    auto blocks = findRelationBlocks(relations_, store_, qualifiedParentCollection, parentId);
+    if (blocks.empty()) return true;
+
+    err = formatRelationBlockError(qualifiedParentCollection, parentId, blocks);
+    return false;
+}
+
+} // namespace smartbotic::database

+ 126 - 0
service/src/relations/relation_enforcement.hpp

@@ -0,0 +1,126 @@
+// v2.11.0 T4 — restrict/no_action enforcement on parent delete.
+//
+// The pure decision logic for "may this delete proceed", kept free of
+// grpc::ServerContext and the pb:: types entirely so it is unit-testable
+// against just a RelationManager + LmdbDocumentStore (see
+// tests/test_relation_enforcement.cpp). Task 6 wires a live RelationManager
+// into DatabaseGrpcImpl and calls into this from Delete(), after the access
+// gate and before store_.remove(); Task 9 exercises it over the real gRPC
+// boundary.
+//
+// Scope: only `restrict` blocks. `no_action` permits the delete and leaves
+// the reference dangling - the documented escape hatch. A relation
+// declaring `cascade` or `set_null` ALSO permits for now (behaves exactly
+// like no_action) because the destructive write-path work those policies
+// need is Task 12's, not this one's - do not read the permissive behaviour
+// here as their final semantics.
+
+#pragma once
+
+#include <cstdint>
+#include <string>
+#include <vector>
+
+#include "relation_manager.hpp"
+
+namespace smartbotic::db::storage {
+class LmdbDocumentStore;
+}
+
+namespace smartbotic::database {
+
+// One relation that is currently blocking a delete.
+struct RelationBlock {
+    std::string relation;        // qualified name, e.g. "default:exec_wf"
+    std::string childCollection; // qualified, e.g. "default:executions"
+    uint64_t childCount = 0;
+    std::vector<std::string> sampleChildIds;   // at most five
+};
+
+// Every relation that blocks deleting qualifiedParentCollection/parentId
+// right now. Empty means the delete may proceed as far as relations are
+// concerned.
+//
+// Deliberately does NOT consult relationsEnforced - that is the caller's
+// decision about whether to invoke this at all. A caller that always needs
+// the true answer regardless of the flag exists too: `relations check`
+// (Task 7) has to find dangling references produced while enforcement was
+// off, so the query itself must stay honest about the flag being someone
+// else's business.
+//
+// Only Restrict relations are considered; see the file header for why
+// Cascade/SetNull are treated as permissive here. Absent and null child
+// references are not represented in the index at all (Task 3), so a
+// relation with zero matching postings is silently skipped - there is no
+// second code path that reasons about "no reference" separately from "zero
+// count".
+//
+// Costs no document reads: relation_index_child_count is backed by
+// mdb_cursor_count. `relation` is looked up via relationsWithParent(), and
+// its bare name (stripped of the project qualifier) is what the index
+// calls expect.
+std::vector<RelationBlock> findRelationBlocks(
+    RelationManager& relations,
+    smartbotic::db::storage::LmdbDocumentStore& store,
+    const std::string& qualifiedParentCollection,
+    const std::string& parentId);
+
+// Render findRelationBlocks() output into the operator-facing
+// FAILED_PRECONDITION message text: names every blocking relation, its
+// child count, and up to five sample blocking ids, so an operator learns
+// what to do next from the message alone. Returns "" for an empty list
+// (defined total for clarity; callers should not invoke this when
+// findRelationBlocks() returned nothing).
+std::string formatRelationBlockError(
+    const std::string& qualifiedParentCollection,
+    const std::string& parentId,
+    const std::vector<RelationBlock>& blocks);
+
+// Thin, stateful wrapper around findRelationBlocks()/formatRelationBlockError()
+// that also applies the relationsEnforced switch (Task 8). Exists so the
+// decision is directly unit-testable (see tests/test_relation_enforcement.cpp)
+// and directly reusable, unmodified, from Task 6's Delete handler.
+//
+// Holds references only, no ownership. `relationsEnforced` is bound by
+// reference (normally CollectionCfg::relationsEnforced, read per call via
+// config_manager_.configFor(qualifiedCollection)) so it is re-read live,
+// not snapshotted at construction - matching how every other per-call
+// config read in this codebase behaves.
+//
+// ⚠ Lifetime: relationsEnforced must outlive the RelationEnforcer. Bind it
+// to a named local, e.g.:
+//     auto cfg = config_manager_.configFor(qualified);
+//     RelationEnforcer enforcer(relations_, *store, cfg.relationsEnforced);
+// NOT to a temporary's subobject (config_manager_.configFor(qualified).relationsEnforced
+// used directly as the constructor argument) - configFor() returns
+// CollectionCfg BY VALUE, so that temporary is destroyed at the end of the
+// full expression and the reference would dangle for every call after
+// construction.
+class RelationEnforcer {
+public:
+    RelationEnforcer(RelationManager& relations,
+                      smartbotic::db::storage::LmdbDocumentStore& store,
+                      const bool& relationsEnforced);
+
+    // Returns true if the delete of qualifiedParentCollection/parentId may
+    // proceed. False means at least one restrict relation still has
+    // children; err is populated with a message naming the relation(s),
+    // child count(s) and sample blocking ids - see formatRelationBlockError().
+    //
+    // relationsEnforced == false skips the check entirely and returns true
+    // - deliberately, and NOT retroactively: dangling references created
+    // while it was off are not found by turning it back on, only by
+    // `relations check` (Task 7). Logged at INFO on every skip, because
+    // DeleteResponse has no message field to carry this to the caller - the
+    // server log is the only place an operator can learn it happened.
+    bool canDelete(const std::string& qualifiedParentCollection,
+                   const std::string& parentId,
+                   std::string& err);
+
+private:
+    RelationManager& relations_;
+    smartbotic::db::storage::LmdbDocumentStore& store_;
+    const bool& relationsEnforced_;
+};
+
+} // namespace smartbotic::database

+ 22 - 5
tests/CMakeLists.txt

@@ -594,11 +594,11 @@ endif()
 
 add_test(NAME test_relation_index COMMAND test_relation_index)
 
-# v2.11.0 T3 — relation reverse-index maintenance on child writes.
-# Exercises LmdbDocumentStore::put()/del() maintaining the reverse index
-# declared via set_relations(), inside the same write transaction as the
-# document. Same source list as test_relation_index: no relations/
-# relation_manager.cpp - the storage layer stays free of that dependency.
+# v2.11.0 T3/T4 — relation reverse-index maintenance on child writes (T3),
+# plus restrict/no_action delete enforcement (T4). T4 needs the declaration
+# registry (RelationManager, backed by a MemoryStore) alongside the LMDB
+# reverse index, so unlike test_relation_index this target does pull in
+# relations/relation_manager.cpp and memory_store.cpp and their dependencies.
 add_executable(test_relation_enforcement
     test_relation_enforcement.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/storage/lmdb_env.cpp
@@ -608,6 +608,14 @@ add_executable(test_relation_enforcement
     ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/storage/document_store_lmdb.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/storage/secondary_index.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/relations/relation_index.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/relations/relation_manager.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/relations/relation_enforcement.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/memory_store.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/views/view_manager.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/views/projection.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/config/collection_config_manager.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/persistence/history_store.cpp
+    ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/persistence/wal.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/json_parse.cpp
     ${CMAKE_CURRENT_SOURCE_DIR}/../service/src/doc_binary.cpp
 )
@@ -627,6 +635,15 @@ else()
     target_include_directories(test_relation_enforcement PRIVATE ${NLOHMANN_JSON_INCLUDE_DIRS})
 endif()
 
+if(TARGET spdlog::spdlog)
+    target_link_libraries(test_relation_enforcement PRIVATE spdlog::spdlog)
+else()
+    target_link_libraries(test_relation_enforcement PRIVATE ${SPDLOG_LIBRARIES})
+    target_include_directories(test_relation_enforcement PRIVATE ${SPDLOG_INCLUDE_DIRS})
+endif()
+find_package(Threads REQUIRED)
+target_link_libraries(test_relation_enforcement PRIVATE Threads::Threads)
+
 add_test(NAME test_relation_enforcement COMMAND test_relation_enforcement)
 
 # v2.9.0 — secondary index key encoding. Pins the one property that matters:

+ 156 - 0
tests/test_relation_enforcement.cpp

@@ -4,6 +4,12 @@
 // reverse index declared via set_relations(), using the same set-difference
 // discipline as maintainIndexes(). No enforcement (Task 4) is involved here
 // - relation_index_child_count is used purely as the observation point.
+//
+// v2.11.0 T4 adds restrict/no_action delete enforcement below
+// (test_restrict_blocks_and_names_the_blockers,
+// test_relations_enforced_false_skips_the_check) - unit-level, against
+// RelationManager + RelationEnforcer + LmdbDocumentStore directly, no
+// grpc::ServerContext. The RPC-level check lands in Task 6/9.
 
 #include <atomic>
 #include <cstdio>
@@ -16,12 +22,22 @@
 #include <nlohmann/json.hpp>
 
 #include "document.hpp"
+#include "memory_store.hpp"
+#include "relations/relation_enforcement.hpp"
+#include "relations/relation_manager.hpp"
 #include "storage/document_store_lmdb.hpp"
 #include "storage/lmdb_env.hpp"
 
 namespace fs = std::filesystem;
 
 using smartbotic::database::Document;
+using smartbotic::database::MemoryStore;
+using smartbotic::database::OnDelete;
+using smartbotic::database::RelationBlock;
+using smartbotic::database::RelationEnforcer;
+using smartbotic::database::RelationInfo;
+using smartbotic::database::RelationManager;
+using smartbotic::database::findRelationBlocks;
 using smartbotic::db::storage::LmdbDocumentStore;
 using smartbotic::db::storage::LmdbEnv;
 using smartbotic::db::storage::LmdbEnvOpts;
@@ -150,6 +166,143 @@ void test_posting_visible_after_reopen() {
           "posting survives a fresh LmdbDocumentStore over the same env");
 }
 
+// v2.11.0 T4 — restrict blocks a delete and names the blockers; no_action
+// permits it and leaves the reference dangling, deliberately.
+//
+// RelationManager's declaration registry lives in a MemoryStore's
+// `_relations` collection (see test_relation_manager.cpp's Fixture); the
+// reverse index it queries lives in a separate per-project LmdbDocumentStore
+// (T2/T3, exercised above). This test wires both together the way Task 6's
+// Delete handler eventually will.
+void test_restrict_blocks_and_names_the_blockers() {
+    MemoryStore mstore(MemoryStore::Config{});
+    mstore.start();
+    RelationManager rm(mstore);
+    rm.loadFromStore();
+
+    TmpEnv t("rel-enforce-restrict");
+    LmdbDocumentStore store(t.env);
+    store.set_relations("executions", {{"exec_wf", "workflowId"}});
+
+    auto put = [&](const std::string& id, const nlohmann::json& data) {
+        Document d; d.id = id; d.collection = "executions"; d.set_data(data);
+        store.put("executions", id, d);
+    };
+    put("e1", {{"workflowId", "wf-1"}});
+    put("e2", {{"workflowId", "wf-1"}});
+
+    RelationInfo rel;
+    rel.name = "default:exec_wf";
+    rel.child = "default:executions";
+    rel.childField = "workflowId";
+    rel.parent = "default:workflows";
+    rel.onDelete = OnDelete::Restrict;
+
+    std::string mgrErr;
+    check(rm.createRelation(rel, mgrErr), "declared the restrict relation");
+
+    bool enforced = true;
+    RelationEnforcer enforcer(rm, store, enforced);
+
+    std::string err;
+    check(!enforcer.canDelete("default:workflows", "wf-1", err), "restrict blocks");
+    check(err.find("exec_wf") != std::string::npos, "names the relation");
+    check(err.find("2") != std::string::npos, "gives the child count");
+    check(err.find("e1") != std::string::npos, "samples a blocking id");
+
+    // no_action permits it and leaves the reference dangling, deliberately.
+    // RelationManager has no update-in-place; re-declare with the new
+    // policy the same way an operator would via dropRelation + createRelation.
+    check(rm.dropRelation(rel.name, mgrErr), "dropped to change on_delete");
+    rel.onDelete = OnDelete::NoAction;
+    check(rm.createRelation(rel, mgrErr), "re-declared as no_action");
+
+    err.clear();
+    check(enforcer.canDelete("default:workflows", "wf-1", err), "no_action permits");
+
+    mstore.stop();
+}
+
+// Cascade/set_null are NOT yet destructive (Task 12) - a relation declaring
+// either one must behave exactly like no_action here, not block, and not
+// half-implement destruction.
+void test_cascade_and_set_null_permit_for_now() {
+    MemoryStore mstore(MemoryStore::Config{});
+    mstore.start();
+    RelationManager rm(mstore);
+    rm.loadFromStore();
+
+    TmpEnv t("rel-enforce-cascade");
+    LmdbDocumentStore store(t.env);
+    store.set_relations("executions", {{"exec_wf", "workflowId"}});
+
+    Document d; d.id = "e1"; d.collection = "executions";
+    d.set_data({{"workflowId", "wf-1"}});
+    store.put("executions", "e1", d);
+
+    RelationInfo rel;
+    rel.name = "default:exec_wf";
+    rel.child = "default:executions";
+    rel.childField = "workflowId";
+    rel.parent = "default:workflows";
+    rel.onDelete = OnDelete::Cascade;
+
+    std::string mgrErr;
+    check(rm.createRelation(rel, mgrErr), "declared a cascade relation");
+
+    auto blocks = findRelationBlocks(rm, store, "default:workflows", "wf-1");
+    check(blocks.empty(), "cascade does not block - not yet destructive (Task 12)");
+
+    check(rm.dropRelation(rel.name, mgrErr), "dropped to change on_delete");
+    rel.onDelete = OnDelete::SetNull;
+    check(rm.createRelation(rel, mgrErr), "re-declared as set_null");
+
+    blocks = findRelationBlocks(rm, store, "default:workflows", "wf-1");
+    check(blocks.empty(), "set_null does not block - not yet destructive (Task 12)");
+
+    mstore.stop();
+}
+
+// relationsEnforced=false (CollectionCfg, Task 8) skips the check entirely,
+// even though a restrict relation with live children exists. Deliberately
+// not retroactive - see RelationEnforcer::canDelete's doc comment.
+void test_relations_enforced_false_skips_the_check() {
+    MemoryStore mstore(MemoryStore::Config{});
+    mstore.start();
+    RelationManager rm(mstore);
+    rm.loadFromStore();
+
+    TmpEnv t("rel-enforce-disabled");
+    LmdbDocumentStore store(t.env);
+    store.set_relations("executions", {{"exec_wf", "workflowId"}});
+
+    Document d; d.id = "e1"; d.collection = "executions";
+    d.set_data({{"workflowId", "wf-1"}});
+    store.put("executions", "e1", d);
+
+    RelationInfo rel;
+    rel.name = "default:exec_wf";
+    rel.child = "default:executions";
+    rel.childField = "workflowId";
+    rel.parent = "default:workflows";
+    rel.onDelete = OnDelete::Restrict;
+
+    std::string mgrErr;
+    check(rm.createRelation(rel, mgrErr), "declared the restrict relation");
+
+    bool enforced = false;   // per-collection switch, Task 8
+    RelationEnforcer enforcer(rm, store, enforced);
+
+    std::string err;
+    check(enforcer.canDelete("default:workflows", "wf-1", err),
+          "enforcement disabled for this collection lets the delete through - and "
+          "the resulting dangling references will NOT be found by re-enabling it, "
+          "only by `relations check`");
+    check(err.empty(), "no error text is populated on a permitted delete");
+
+    mstore.stop();
+}
+
 }  // namespace
 
 int main() {
@@ -158,6 +311,9 @@ int main() {
     test_undeclared_collection_maintains_nothing();
     test_unrelated_update_leaves_the_posting_alone();
     test_posting_visible_after_reopen();
+    test_restrict_blocks_and_names_the_blockers();
+    test_cascade_and_set_null_permit_for_now();
+    test_relations_enforced_false_skips_the_check();
 
     std::cout << "passed: " << g_pass << ", failed: " << g_fail << "\n";
     return g_fail == 0 ? 0 : 1;