Fix potential inconsistencies when removing schemas with references

src/build/delta.cc

@@ -212,6 +212,39 @@ auto delta(const BuildPhase phase, const BuildPlan::Type build_type,
     const auto &output_string{output.native()};
     const auto schemas_prefix{output_string + "/" + SCHEMAS_DIRECTORY + "/"};
 
+    auto extract_cross_schema_refs{
+        [&schemas_prefix](
+            const BuildState::Entry *state_entry,
+            std::string_view owner_base) -> std::unordered_set<std::string> {
+          std::unordered_set<std::string> result;
+          if (state_entry == nullptr) {
+            return result;
+          }
+
+          for (const auto &dependency : state_entry->dependencies) {
+            const auto &dependency_path{dependency.native()};
+            if (!dependency_path.starts_with(schemas_prefix)) {
+              continue;
+            }
+
+            const auto sentinel_position{dependency_path.find("/%/")};
+            if (sentinel_position == std::string::npos) {
+              continue;
+            }
+
+            const auto relative{dependency_path.substr(
+                schemas_prefix.size(),
+                sentinel_position - schemas_prefix.size())};
+            if (relative != owner_base) {
+              result.insert(std::string{relative});
+            }
+          }
+
+          return result;
+        }};
+
+    const auto owner_start{schemas_prefix.size()};
+
     std::unordered_set<std::string> affected_schemas;
     for (const auto key : entries.keys()) {
       if (!key.ends_with("/%/dependencies.metapack")) {
@@ -225,38 +258,6 @@ auto delta(const BuildPhase phase, const BuildPlan::Type build_type,
       const auto *new_entry{entries.entry(std::string{key})};
       const auto *old_entry{entries.disk_entry(std::string{key})};
 
-      auto extract_cross_schema_refs{
-          [&schemas_prefix](
-              const BuildState::Entry *state_entry,
-              std::string_view owner_base) -> std::unordered_set<std::string> {
-            std::unordered_set<std::string> result;
-            if (state_entry == nullptr) {
-              return result;
-            }
-
-            for (const auto &dependency : state_entry->dependencies) {
-              const auto &dependency_path{dependency.native()};
-              if (!dependency_path.starts_with(schemas_prefix)) {
-                continue;
-              }
-
-              const auto sentinel_position{dependency_path.find("/%/")};
-              if (sentinel_position == std::string::npos) {
-                continue;
-              }
-
-              const auto relative{dependency_path.substr(
-                  schemas_prefix.size(),
-                  sentinel_position - schemas_prefix.size())};
-              if (relative != owner_base) {
-                result.insert(std::string{relative});
-              }
-            }
-
-            return result;
-          }};
-
-      const auto owner_start{schemas_prefix.size()};
       const auto owner_sentinel{key.find("/%/", owner_start)};
       if (owner_sentinel == std::string_view::npos) {
         continue;
@@ -284,6 +285,33 @@ auto delta(const BuildPhase phase, const BuildPlan::Type build_type,
       }
     }
 
+    for (const auto &deleted_key : entries.deleted_keys()) {
+      if (!deleted_key.ends_with("/%/dependencies.metapack")) {
+        continue;
+      }
+
+      if (!deleted_key.starts_with(schemas_prefix)) {
+        continue;
+      }
+
+      const auto *old_entry{entries.raw_disk_entry(deleted_key)};
+      if (old_entry == nullptr) {
+        continue;
+      }
+
+      const auto owner_sentinel{deleted_key.find("/%/", owner_start)};
+      if (owner_sentinel == std::string::npos) {
+        continue;
+      }
+
+      const auto owner_base{
+          deleted_key.substr(owner_start, owner_sentinel - owner_start)};
+      for (const auto &reference :
+           extract_cross_schema_refs(old_entry, owner_base)) {
+        affected_schemas.insert(reference);
+      }
+    }
+
     BuildPlan plan;
     plan.output = output;
     plan.type = build_type;
@@ -568,6 +596,32 @@ auto delta(const BuildPhase phase, const BuildPlan::Type build_type,
     }
   }
 
+  std::unordered_set<std::string_view> current_schema_bases;
+  current_schema_bases.reserve(active_schemas.size());
+  for (const auto &schema : active_schemas) {
+    current_schema_bases.insert(schema.schema_base);
+  }
+
+  std::unordered_set<std::string> removed_entries;
+  {
+    const auto schemas_prefix{output_string + '/' + SCHEMAS_DIRECTORY + '/'};
+    for (const auto entry_path : entries.keys()) {
+      if (!entry_path.starts_with(schemas_prefix)) {
+        continue;
+      }
+
+      const auto sentinel_pos{entry_path.find("/%/")};
+      if (sentinel_pos == std::string_view::npos) {
+        continue;
+      }
+
+      if (!current_schema_bases.contains(
+              entry_path.substr(0, sentinel_pos + 2))) {
+        removed_entries.emplace(entry_path);
+      }
+    }
+  }
+
   std::unordered_set<std::string> dirty_set;
   {
     for (const auto &schema : active_schemas) {
@@ -618,7 +672,8 @@ auto delta(const BuildPhase phase, const BuildPlan::Type build_type,
           }
 
           for (const auto &dep : state_entry->dependencies) {
-            if (dirty_set.contains(dep.native())) {
+            if (dirty_set.contains(dep.native()) ||
+                removed_entries.contains(dep.native())) {
               dirty_set.insert(target_path);
               propagation_changed = true;
               break;
@@ -692,32 +747,7 @@ auto delta(const BuildPhase phase, const BuildPlan::Type build_type,
     }
   }
 
-  bool has_potential_stale{false};
-  {
-    std::unordered_set<std::string_view> current_schema_bases;
-    current_schema_bases.reserve(schemas.size());
-    for (const auto &schema : active_schemas) {
-      current_schema_bases.insert(schema.schema_base);
-    }
-
-    const auto schemas_prefix{output_string + '/' + SCHEMAS_DIRECTORY + '/'};
-    for (const auto entry_path : entries.keys()) {
-      if (!entry_path.starts_with(schemas_prefix)) {
-        continue;
-      }
-
-      const auto sentinel_pos{entry_path.find("/%/")};
-      if (sentinel_pos == std::string_view::npos) {
-        continue;
-      }
-
-      if (!current_schema_bases.contains(
-              entry_path.substr(0, sentinel_pos + 2))) {
-        has_potential_stale = true;
-        break;
-      }
-    }
-  }
+  const auto has_potential_stale{!removed_entries.empty()};
 
   if (!is_full && dirty_set.empty() && removed_uris.empty() &&
       !has_missing_web && !has_stale_web && !has_potential_stale) {

src/build/include/sourcemeta/one/build_state.h

@@ -68,8 +68,9 @@ class SOURCEMETA_ONE_BUILD_EXPORT BuildState {
 
   [[nodiscard]] auto in_overlay(const std::string &key) const -> bool;
   [[nodiscard]] auto disk_entry(const std::string &key) const -> const Entry *;
+  [[nodiscard]] auto raw_disk_entry(const std::string &key) const
+      -> const Entry *;
 
-private:
   struct TransparentHash {
     using is_transparent = void;
     auto operator()(std::string_view value) const noexcept -> std::size_t {
@@ -85,6 +86,10 @@ class SOURCEMETA_ONE_BUILD_EXPORT BuildState {
     }
   };
 
+  [[nodiscard]] auto deleted_keys() const -> const
+      std::unordered_set<std::string, TransparentHash, TransparentEqual> &;
+
+private:
   auto probe_slot(std::string_view key, std::uint8_t kind) const
       -> const std::uint8_t *;
   auto parse_slot_entry(const std::uint8_t *slot) const -> const Entry &;

src/build/state.cc

@@ -454,6 +454,20 @@ auto BuildState::disk_entry(const std::string &key) const -> const Entry * {
   return &this->parse_slot_entry(slot);
 }
 
+auto BuildState::raw_disk_entry(const std::string &key) const -> const Entry * {
+  const auto *slot{this->probe_slot(key, KIND_OUTPUT)};
+  if (slot == nullptr) {
+    return nullptr;
+  }
+
+  return &this->parse_slot_entry(slot);
+}
+
+auto BuildState::deleted_keys() const -> const
+    std::unordered_set<std::string, TransparentHash, TransparentEqual> & {
+  return this->deleted;
+}
+
 auto BuildState::save(const std::filesystem::path &path) const -> void {
   if (!this->dirty && this->view && path == this->loaded_path) {
     return;

test/cli/CMakeLists.txt

@@ -67,6 +67,8 @@ if(ONE_INDEX)
   sourcemeta_one_test_cli(common index dependents-modify-transitive)
   sourcemeta_one_test_cli(common index dependents-add-schema)
   sourcemeta_one_test_cli(common index dependents-modify-add-ref)
+  sourcemeta_one_test_cli(common index dependents-remove-referenced-schema)
+  sourcemeta_one_test_cli(common index dependents-remove-referencing-schema)
 
   if(ONE_ENTERPRISE)
     sourcemeta_one_test_cli(enterprise index no-options)

test/cli/index/common/dependents-remove-referenced-schema.sh

@@ -0,0 +1,80 @@
+#!/bin/sh
+
+# Removing a schema that is still referenced by another schema should
+# cause the indexer to fail, as the reference cannot be resolved.
+
+set -o errexit
+set -o nounset
+
+TMP="$(mktemp -d)"
+clean() { rm -rf "$TMP"; }
+trap clean EXIT
+
+cat << EOF > "$TMP/one.json"
+{
+  "url": "https://sourcemeta.com/",
+  "contents": {
+    "example": {
+      "contents": {
+        "schemas": {
+          "baseUri": "https://example.com/",
+          "path": "./schemas"
+        }
+      }
+    }
+  }
+}
+EOF
+
+mkdir "$TMP/schemas"
+
+cat << 'EOF' > "$TMP/schemas/a.json"
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "$id": "https://example.com/a"
+}
+EOF
+
+cat << 'EOF' > "$TMP/schemas/b.json"
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "$id": "https://example.com/b",
+  "properties": {
+    "foo": { "$ref": "https://example.com/a" }
+  }
+}
+EOF
+
+remove_threads_information() {
+  expr='s/ \[[^]]*[^a-z-][^]]*\]//g'
+  if [ "$(uname -s)" = "Darwin" ]; then
+    sed -i '' "$expr" "$1"
+  else
+    sed -i "$expr" "$1"
+  fi
+}
+
+# Run 1: full build from scratch
+"$1" --skip-banner "$TMP/one.json" "$TMP/output" --concurrency 1 \
+    > /dev/null 2>&1
+
+# Run 2: remove schema A (which is referenced by B)
+rm "$TMP/schemas/a.json"
+
+"$1" --skip-banner "$TMP/one.json" "$TMP/output" --concurrency 1 \
+    2> "$TMP/output.txt" && CODE="$?" || CODE="$?"
+test "$CODE" = "1" || exit 1
+remove_threads_information "$TMP/output.txt"
+
+cat << EOF > "$TMP/expected.txt"
+Writing output to: $(realpath "$TMP")/output
+Using configuration: $(realpath "$TMP")/one.json
+Detecting: $(realpath "$TMP")/schemas/b.json (#1)
+(  7%) Producing: explorer/%/directory.metapack
+( 15%) Producing: schemas/example/schemas/b/%/dependencies.metapack
+error: Could not resolve the reference to an external schema
+  https://sourcemeta.com/example/schemas/a
+
+Did you forget to register a schema with such URI in the one?
+EOF
+diff "$TMP/output.txt" "$TMP/expected.txt"