Эх сурвалжийг харах

release(v2.4.2): project-scoped views; views were unreachable since v2.3

Client::createView sent `name` bare but `collection` qualified, so
ViewManager keyed its cache on the bare name while Find/Get/Count looked
up the project-qualified one. The keys could never meet, and the miss
fell through to "treat it as a real collection" - 0 docs, no error.

Views were therefore unreachable through the client for EVERY project,
including "default", from v2.3.0 onward. tests/test_views.cpp passed the
whole time because it only unit-tests applyProjection() in-process; the
break lives at the client/server boundary.

Views are now keyed by their qualified <project>:<name> form. The client
qualifies on createView/dropView/getViewInfo and un-qualifies results, so
callers keep using bare names. Two projects may each own a view of the
same name - the global registry previously rejected the second as a
duplicate. ListViewsRequest gained an additive `project` field; empty
means all projects (operator/CLI), clients always set their own.

Legacy bare-named rows in _views are re-keyed into the default project on
load. Idempotent, no operator action.

Find on a name that is neither view nor collection now returns NOT_FOUND,
and the client throws rather than returning {}. The silent empty is what
hid this bug for two releases.

tests/load_test/test_views_multiproject.sh: 18 checks, RED before / green
after. ctest 14/14. TLS/auth e2e 4/4. Upgrade path verified by driving
the installed 2.4.1 client as a legacy-data generator.
fszontagh 1 сар өмнө
parent
commit
6132c2535a

Файлын зөрүү хэтэрхий том тул дарагдсан байна
+ 2 - 0
CLAUDE.md


+ 1 - 1
VERSION

@@ -1 +1 @@
-2.4.1
+2.4.2

+ 35 - 7
client/src/client.cpp

@@ -80,6 +80,17 @@ public:
         return out;
     }
 
+    // Inverse of qualify() for values coming back off the wire. Strips only
+    // OUR project's prefix, so a name from another namespace stays visibly
+    // qualified rather than being silently flattened into ours.
+    std::string unqualify(const std::string& qualified) const {
+        const std::string prefix = config_.project + ":";
+        if (qualified.rfind(prefix, 0) == 0) {
+            return qualified.substr(prefix.size());
+        }
+        return qualified;
+    }
+
     ~Impl() {
         disconnect();
     }
@@ -527,6 +538,13 @@ public:
         setDeadline(context);
 
         auto status = stub_->Find(&context, request, &response);
+        if (status.error_code() == grpc::StatusCode::NOT_FOUND) {
+            // Surfaced rather than logged: an unknown collection or view is a
+            // caller bug, and returning {} here is indistinguishable from a
+            // legitimately empty result. Other failures keep the old
+            // log-and-return-empty behaviour.
+            throw std::runtime_error(status.error_message());
+        }
         if (!status.ok()) {
             spdlog::error("Client::find failed: {}", status.error_message());
             return {};
@@ -577,6 +595,9 @@ public:
         setDeadline(context);
 
         auto status = stub_->Find(&context, request, &response);
+        if (status.error_code() == grpc::StatusCode::NOT_FOUND) {
+            throw std::runtime_error(status.error_message());  // see find()
+        }
         if (!status.ok()) {
             spdlog::error("Client::findWithMetrics failed: {}", status.error_message());
             return {};
@@ -995,7 +1016,11 @@ public:
                     const std::vector<Client::Filter>& where,
                     const std::optional<Client::Sort>& defaultSort) {
         smartbotic::databasepb::CreateViewRequest request;
-        request.set_name(name);
+        // Both fields are namespaced. Qualifying `collection` but not `name`
+        // is what broke view lookups in v2.3: the registry keyed on the bare
+        // name while every read path sent the qualified one, so the keys
+        // could never meet.
+        request.set_name(qualify(name));
         request.set_collection(qualify(collection));
         for (const auto& p : include) request.add_include(p);
         for (const auto& p : exclude) request.add_exclude(p);
@@ -1031,7 +1056,7 @@ public:
 
     bool dropView(const std::string& name) {
         smartbotic::databasepb::DropViewRequest request;
-        request.set_name(name);
+        request.set_name(qualify(name));
         smartbotic::databasepb::DropViewResponse response;
         grpc::ClientContext context;
         setDeadline(context);
@@ -1046,6 +1071,8 @@ public:
 
     std::vector<Client::ViewDefinition> listViews() {
         smartbotic::databasepb::ListViewsRequest request;
+        // Scope the listing to this client's workspace.
+        request.set_project(config_.project);
         smartbotic::databasepb::ListViewsResponse response;
         grpc::ClientContext context;
         setDeadline(context);
@@ -1058,8 +1085,9 @@ public:
         }
         for (const auto& pb : response.views()) {
             Client::ViewDefinition v;
-            v.name = pb.name();
-            v.collection = pb.collection();
+            // Round-trip: the caller created "adults", so list it as "adults".
+            v.name = unqualify(pb.name());
+            v.collection = unqualify(pb.collection());
             for (const auto& p : pb.include()) v.include.push_back(p);
             for (const auto& p : pb.exclude()) v.exclude.push_back(p);
             for (const auto& pbf : pb.where()) {
@@ -1085,7 +1113,7 @@ public:
 
     std::optional<Client::ViewDefinition> getViewInfo(const std::string& name) {
         smartbotic::databasepb::GetViewInfoRequest request;
-        request.set_name(name);
+        request.set_name(qualify(name));
         smartbotic::databasepb::GetViewInfoResponse response;
         grpc::ClientContext context;
         setDeadline(context);
@@ -1095,8 +1123,8 @@ public:
             return std::nullopt;
         }
         Client::ViewDefinition v;
-        v.name = response.view().name();
-        v.collection = response.view().collection();
+        v.name = unqualify(response.view().name());
+        v.collection = unqualify(response.view().collection());
         for (const auto& p : response.view().include()) v.include.push_back(p);
         for (const auto& p : response.view().exclude()) v.exclude.push_back(p);
         for (const auto& pbf : response.view().where()) {

+ 28 - 0
docs/integration-guide.md

@@ -307,6 +307,34 @@ db.dropView("users_public");
 | **Nested paths** | Dot-notation supported: `user.profile.name`. Stops at arrays. |
 | **Writes** | Rejected — `insert()`, `update()`, `patch()`, `upsert()`, `remove()` on a view name return an error. |
 | **View-of-view** | Not allowed — views must target real collections. |
+| **Project scope** (2.4.2+) | Views belong to a project, exactly like collections. A view created by a client with `project = "acme"` is addressable only from that project, and two projects may each own a view of the same name. `listViews()` returns only the calling project's views. |
+| **Unknown name** (2.4.2+) | A name matching neither a view nor a collection makes `find()` throw `std::runtime_error` (gRPC `NOT_FOUND`). Before 2.4.2 it returned an empty result, which was indistinguishable from an empty collection. |
+
+#### Views and projects (2.4.2+)
+
+View names are namespaced with the client's `Config::project`, so you keep
+using the bare name and the client qualifies it:
+
+```cpp
+Client::Config cfg;
+cfg.project = "acme";
+Client db(cfg);
+
+db.createView("adults", "users", {"name", "age"});  // -> acme:adults over acme:users
+auto rows = db.find("adults", {});                  // resolves within acme only
+```
+
+A different workspace may define its own `adults` over its own `users`
+without colliding, and neither can read through the other's view.
+
+> **Upgrading from <= 2.4.1.** Views were unreachable through the client on
+> 2.3.0-2.4.1: `createView` registered the bare name while every read path
+> sent the project-qualified one, so lookups always missed and returned an
+> empty result with no error. Existing view definitions are re-keyed into the
+> `default` project automatically on first boot of 2.4.2 (idempotent, logged
+> as `ViewManager: re-keyed N legacy view(s)`). No operator action required.
+> Callers that treated the empty result as "view has no rows" will start
+> seeing real rows.
 
 #### Via migrations (recommended)
 

+ 6 - 1
proto/database.proto

@@ -796,7 +796,12 @@ message DropViewResponse {
     string error = 2;
 }
 
-message ListViewsRequest {}
+message ListViewsRequest {
+    // Restrict the listing to one project namespace. Empty lists every
+    // project (operator/CLI use). Clients always set it so a workspace
+    // never sees another workspace's views.
+    string project = 1;
+}
 
 message ListViewsResponse {
     repeated ViewDefinition views = 1;

+ 14 - 1
service/src/database_grpc_impl.cpp

@@ -736,6 +736,12 @@ grpc::Status DatabaseGrpcImpl::Find(
     if (view_manager_.isView(request->collection())) {
         view = view_manager_.getView(request->collection());
         targetCollection = view->collection;
+    } else if (!store_.collectionExists(targetCollection)) {
+        // Neither a view nor a real collection. Returning an empty result here
+        // is how the v2.3 view-addressing break stayed invisible: a misspelled
+        // or mis-qualified name looked exactly like an empty collection.
+        return grpc::Status(grpc::StatusCode::NOT_FOUND,
+                            "no collection or view named '" + targetCollection + "'");
     }
 
     Query query = fromProtoQuery(*request);
@@ -1733,11 +1739,18 @@ grpc::Status DatabaseGrpcImpl::DropView(
 
 grpc::Status DatabaseGrpcImpl::ListViews(
     grpc::ServerContext* /*context*/,
-    const pb::ListViewsRequest* /*request*/,
+    const pb::ListViewsRequest* request,
     pb::ListViewsResponse* response
 ) {
+    // Empty project = list everything (operator / CLI). Clients always send
+    // their own project so one workspace never enumerates another's views.
+    const std::string& wantProject = request->project();
     auto views = view_manager_.listViews();
     for (const auto& v : views) {
+        if (!wantProject.empty()
+            && smartbotic::database::resolveCollection(v.name).project != wantProject) {
+            continue;
+        }
         auto* out = response->add_views();
         out->set_name(v.name);
         out->set_collection(v.collection);

+ 34 - 2
service/src/views/view_manager.cpp

@@ -1,6 +1,7 @@
 #include "view_manager.hpp"
 
 #include "../memory_store.hpp"
+#include "../project_addressing.hpp"
 
 #include <chrono>
 #include <nlohmann/json.hpp>
@@ -79,14 +80,42 @@ void ViewManager::loadFromStore() {
     CollectionOptions opts;
     store_.createCollection(SYSTEM_COLLECTION, opts);
 
-    // Load all view definitions
+    // Load all view definitions. Views are keyed by their qualified
+    // "<project>:<name>" form so two workspaces can each own a view of the
+    // same name without colliding.
+    //
+    // Pre-v2.4.2 rows carry a bare name (views predate multi-project and were
+    // stored globally). Re-key those into the default project on load, and
+    // rewrite the row so the migration happens once rather than on every boot.
     Query q;
     auto result = store_.find(SYSTEM_COLLECTION, q);
+    size_t migrated = 0;
     for (const auto& doc : result.documents) {
         ViewInfo v = fromJson(doc.data());
         if (v.name.empty()) continue;
+
+        if (v.name.find(kProjectSeparator) == std::string::npos) {
+            v.name = resolveCollection(v.name).qualified;
+            Document rekeyed;
+            rekeyed.id = v.name;
+            rekeyed.set_data(toJson(v));
+            try {
+                store_.remove(SYSTEM_COLLECTION, doc.id);
+                store_.insert(SYSTEM_COLLECTION, rekeyed);
+                ++migrated;
+            } catch (const std::exception& e) {
+                // Keep serving the view from cache even if the rewrite fails;
+                // the next boot will retry.
+                spdlog::warn("ViewManager: could not re-key legacy view '{}': {}",
+                             doc.id, e.what());
+            }
+        }
         cache_[v.name] = v;
     }
+    if (migrated > 0) {
+        spdlog::info("ViewManager: re-keyed {} legacy view(s) into the default project",
+                     migrated);
+    }
     spdlog::info("ViewManager: loaded {} views from {}", cache_.size(), SYSTEM_COLLECTION);
 }
 
@@ -103,7 +132,10 @@ bool ViewManager::createView(const ViewInfo& view, std::string& errorOut) {
         errorOut = "view name cannot equal collection name";
         return false;
     }
-    if (!view.name.empty() && view.name[0] == '_') {
+    // Names arrive qualified ("<project>:<name>"), so the reserved-prefix
+    // check has to look at the local part - "_foo" and "acme:_foo" are both
+    // reserved, and testing view.name[0] would miss the second.
+    if (resolveCollection(view.name).collection.front() == '_') {
         errorOut = "view name cannot start with '_' (reserved for system collections)";
         return false;
     }

+ 172 - 0
tests/load_test/test_views_multiproject.sh

@@ -0,0 +1,172 @@
+#!/usr/bin/env bash
+# Views over the RPC path, across project namespaces.
+#
+# Regression test for the v2.3 view-addressing break: Client::createView sent
+# `name` bare but `collection` qualified, while every read path qualified, so
+# ViewManager's cache key could never match the lookup key. Views were
+# unreachable via the client for EVERY project, including "default", and the
+# failure was silent (0 docs, no error).
+#
+# tests/test_views.cpp unit-tests applyProjection() in-process and passed
+# throughout. The break lives at the client/server boundary, so the test that
+# catches it has to cross that boundary.
+#
+# Scenarios:
+#   1. default project      - view is queryable, projection applied
+#   2. custom project       - same, independently
+#   3. name reuse           - same view name in two projects stays isolated
+#   4. cross-project read   - workspace A must not see workspace B's rows
+#   5. unknown name         - NOT_FOUND, not a silent empty result
+
+set -euo pipefail
+
+cd "$(dirname "$0")"
+
+ROOT=/data/smartbotic-database
+DIR=/tmp/sbdb-views-multiproject
+PORT=9078
+
+rm -rf "$DIR"
+mkdir -p "$DIR/data"
+
+cat > "$DIR/config.json" <<EOF
+{
+  "storage": {
+    "data_directory": "$DIR/data",
+    "listeners": [
+      { "bind": "127.0.0.1", "port": $PORT,
+        "tls": { "enabled": false }, "auth": { "required": false } }
+    ],
+    "encryption": { "enabled": false },
+    "migrations": { "enabled": false },
+    "replication": { "enabled": false }
+  }
+}
+EOF
+
+"$ROOT/build/service/smartbotic-database" --config "$DIR/config.json" \
+    > "$DIR/server.log" 2>&1 &
+PID=$!
+trap "kill $PID 2>/dev/null || true" EXIT
+sleep 3
+
+cat > "$DIR/driver.cpp" <<'CPP'
+#include <smartbotic/database/client.hpp>
+#include <iostream>
+#include <string>
+
+using smartbotic::database::Client;
+
+static int failures = 0;
+
+static void check(bool ok, const std::string& what) {
+    std::cout << (ok ? "  PASS  " : "  FAIL  ") << what << "\n";
+    if (!ok) ++failures;
+}
+
+// Each workspace gets its own client, its own collection, and a view named
+// "adults" - deliberately the SAME view name in every project, so a global
+// registry would collide and a leaking one would cross-serve.
+static Client makeClient(const std::string& project) {
+    Client::Config cfg;
+    cfg.address = "127.0.0.1:9078";
+    cfg.project = project;
+    return Client(cfg);
+}
+
+static void seed(Client& c, const std::string& coll, const std::string& who) {
+    c.createCollection(coll);
+    c.insert(coll, {{"name", who + "_adult"}, {"age", 30}, {"secret", "classified"}});
+    c.insert(coll, {{"name", who + "_child"}, {"age", 12}, {"secret", "classified"}});
+}
+
+int main() {
+    // ---- scenario 1 + 2: view is queryable in default AND a custom project ----
+    for (const std::string project : {std::string("default"), std::string("acme")}) {
+        std::cout << "\n[project = " << project << "]\n";
+        Client c = makeClient(project);
+        if (!c.connect()) { check(false, "connect"); continue; }
+
+        seed(c, "users", project);
+        check(c.createView("adults", "users", {"name", "age"}, {},
+                           {Client::Filter::gt("age", 18)}),
+              "createView(\"adults\")");
+
+        auto rows = c.find("adults", Client::QueryOptions{});
+        check(rows.size() == 1, "find(\"adults\") returns 1 row (got " +
+                                std::to_string(rows.size()) + ")");
+        if (rows.size() == 1) {
+            check(rows[0].contains("name"), "projection kept \"name\"");
+            check(!rows[0].contains("secret"), "projection dropped \"secret\"");
+            check(rows[0]["name"] == project + "_adult",
+                  "row belongs to this project");
+        }
+
+        // listViews must round-trip the name the caller supplied.
+        auto views = c.listViews();
+        bool found = false;
+        for (const auto& v : views) if (v.name == "adults") found = true;
+        check(found, "listViews() reports \"adults\" un-prefixed");
+        check(views.size() == 1, "listViews() shows only this project's views (got " +
+                                 std::to_string(views.size()) + ")");
+    }
+
+    // ---- scenario 3 + 4: isolation between two workspaces ----
+    std::cout << "\n[isolation]\n";
+    Client a = makeClient("default");
+    Client b = makeClient("acme");
+    if (a.connect() && b.connect()) {
+        auto ra = a.find("adults", Client::QueryOptions{});
+        auto rb = b.find("adults", Client::QueryOptions{});
+        check(ra.size() == 1 && rb.size() == 1, "both workspaces resolve their own \"adults\"");
+        if (ra.size() == 1 && rb.size() == 1) {
+            check(ra[0]["name"] != rb[0]["name"],
+                  "same view name serves different rows per project");
+            check(ra[0]["name"] == "default_adult" && rb[0]["name"] == "acme_adult",
+                  "no cross-project bleed");
+        }
+    } else {
+        check(false, "isolation clients connected");
+    }
+
+    // ---- scenario 5: unknown name is an error, not a silent empty ----
+    std::cout << "\n[unknown name]\n";
+    Client c = makeClient("acme");
+    if (c.connect()) {
+        bool threw = false;
+        try {
+            auto rows = c.find("no_such_thing", Client::QueryOptions{});
+            // If it does not throw, an empty result is the old silent failure.
+            check(false, "find on unknown name signalled an error (got " +
+                         std::to_string(rows.size()) + " rows, no error)");
+        } catch (const std::exception&) {
+            threw = true;
+        }
+        if (threw) check(true, "find on unknown name signalled an error");
+    }
+
+    std::cout << "\n" << (failures ? "FAILED: " + std::to_string(failures) + " check(s)"
+                                   : "ALL CHECKS PASSED") << "\n";
+    return failures ? 1 : 0;
+}
+CPP
+
+g++ -std=c++20 -O2 \
+    -I"$ROOT/client/include" \
+    "$DIR/driver.cpp" \
+    -L"$ROOT/build/client" -lsmartbotic-db-client \
+    $(pkg-config --libs grpc++) \
+    -lspdlog -lfmt -pthread \
+    -Wl,-rpath,"$ROOT/build/client" \
+    -o "$DIR/driver"
+
+set +e
+"$DIR/driver"
+RC=$?
+set -e
+
+echo ""
+echo "=== server log: view lines ==="
+grep -iE "ViewManager" "$DIR/server.log" | tail -10 || echo "(none)"
+
+exit $RC

Энэ ялгаанд хэт олон файл өөрчлөгдсөн тул зарим файлыг харуулаагүй болно