Fix the dangling reference left by bundling a boolean remote schema

src/core/jsonschema/bundle.cc

@@ -99,12 +99,43 @@ auto charge(std::uint64_t &remaining, const SchemaFrame &frame) -> void {
   remaining -= frame.location_count();
 }
 
-// A schema of a dialect that cannot name itself answers to nothing once it is
-// embedded, so every reference to one has to spell out where it went. Neither
-// half is known where the reference is found: the embedding settles the
-// position, and elevation can move a schema again afterwards. So the walk only
-// records which identifiers went in unnamed and where each landed, and the
-// references themselves are settled against the finished document
+// A boolean schema carries no keyword at all, so it has nowhere to hold the
+// identifier that JSON Schema 2020-12, section 9.3.1 asks of every embedded
+// resource: "Each embedded JSON Schema Resource MUST identify itself with a
+// URI using the `$id` keyword". Section 4.3.2 names the object that says what
+// each of the two says, "true: Always passes validation, as if the empty
+// schema `{}`" and "false: Always fails validation, as if the schema `{ "not":
+// {} }`", and restating a remote that way is what lets it carry the name,
+// which in turn leaves every reference that found it spelled as it was
+// written, as the same section goes on to require
+auto restate_boolean(JSON &schema) -> void {
+  assert(schema.is_boolean());
+  const auto permissive{schema.to_boolean()};
+  schema.into_object();
+  if (!permissive) {
+    schema.assign("not", JSON::make_object());
+  }
+}
+
+// Only a dialect that gives `not` a meaning can say what `false` says, which
+// the section above notes by placing the keyword "in the subschema application
+// vocabulary". Writing it under a dialect that draws on no such vocabulary
+// would leave an annotation that says nothing, turning the schema that refuses
+// everything into the one that accepts it, so such a remote is pointed at
+// where it lands instead
+auto boolean_can_be_restated(const SchemaWalker &walker,
+                             const SchemaVocabularies &vocabularies,
+                             const JSON &schema) -> bool {
+  assert(schema.is_boolean());
+  return schema.to_boolean() || dialect_defines(walker, vocabularies, "not"sv);
+}
+
+// A schema that cannot name itself answers to nothing once it is embedded, so
+// every reference to one has to spell out where it went. Neither half is known
+// where the reference is found: the embedding settles the position, and
+// elevation can move a schema again afterwards. So the walk only records which
+// identifiers went in unnamed and where each landed, and the references
+// themselves are settled against the finished document
 struct PositionalReferences {
   // Recorded while walking, as that is where the dialect of a remote is known
   std::unordered_set<JSON::String> unnamed;
@@ -874,15 +905,24 @@ auto embed_references(
     JSON::String effective_id{remote_id.empty() ? JSON::String{identifier}
                                                 : JSON::String{remote_id}};
 
-    // Whether the dialect of the remote has a way to name it at all, which
-    // decides between giving it a name and pointing at wherever it lands
-    const auto dialect_allows_naming{dialect_defines_identifier(
-        walker, remote_vocabularies, remote_base_dialect)};
+    // Whether the remote can be made to name itself at all, which decides
+    // between giving it a name and pointing at wherever it lands. A boolean
+    // has to be restated as the object that says the same thing before it can
+    // hold one, which is not something every dialect can say
+    const auto remote_can_name_itself{
+        dialect_defines_identifier(walker, remote_vocabularies,
+                                   remote_base_dialect) &&
+        (remote.is_object() ||
+         boolean_can_be_restated(walker, remote_vocabularies, remote))};
 
     const auto remote_dialect_uri{declared_dialect(remote, default_dialect)};
 
-    if (remote.is_object()) {
-      if (dialect_allows_naming) {
+    if (remote_can_name_itself || remote.is_object()) {
+      if (remote_can_name_itself) {
+        if (remote.is_boolean()) {
+          restate_boolean(remote);
+        }
+
         // Otherwise the embedded resource would be re-interpreted under the
         // dialect of the schema it gets embedded into, which can differ from
         // the default dialect that the remote was resolved with
@@ -904,10 +944,11 @@ auto embed_references(
     if (names_a_resource) {
       // Settled above, and by a name rather than by a place, so none of what
       // follows has anything to add to it
-    } else if (!dialect_allows_naming) {
-      // A dialect that defines no identifier keyword leaves the remote unable
-      // to declare one, so the name it was resolved by is not the name it
-      // answers to afterwards
+    } else if (!remote_can_name_itself) {
+      // A remote that cannot declare an identifier, whether because its
+      // dialect defines no keyword for one or because it says what no object
+      // of that dialect can say, is left by the embedding without the name it
+      // was resolved by
       assert(effective_id == identifier);
       positional.unnamed.insert(identifier);
       ref_rewrites.emplace_back(to_pointer(pointer),

src/core/openapi/bundle.cc

@@ -4,18 +4,19 @@
 #include "document.h"
 #include "helpers.h"
 
-#include <algorithm>    // std::min
-#include <cassert>      // assert
-#include <charconv>     // std::from_chars
-#include <cstddef>      // std::size_t
-#include <cstdint>      // std::uint64_t
-#include <map>          // std::map
-#include <optional>     // std::optional
-#include <set>          // std::set
-#include <string>       // std::to_string
-#include <system_error> // std::errc
-#include <utility>      // std::move, std::make_pair, std::pair
-#include <vector>       // std::vector
+#include <algorithm>     // std::min
+#include <cassert>       // assert
+#include <charconv>      // std::from_chars
+#include <cstddef>       // std::size_t
+#include <cstdint>       // std::uint64_t
+#include <map>           // std::map
+#include <optional>      // std::optional
+#include <set>           // std::set
+#include <string>        // std::to_string
+#include <system_error>  // std::errc
+#include <unordered_set> // std::unordered_set
+#include <utility>       // std::move, std::make_pair, std::pair
+#include <vector>        // std::vector
 
 namespace {
 
@@ -872,7 +873,11 @@ auto bundle_schemas(sourcemeta::core::JSON &document,
   // the revision settles for the same reason
   const auto booleans{
       sourcemeta::core::openapi_schema_object_is_boolean(walk.version)};
-  const auto standalone_resolver{[&resolver, &standalone, &base, booleans,
+  // Which schemas answered as a boolean, which is the only record of the ones
+  // whose dialect has to be declared for them once they have landed
+  std::unordered_set<sourcemeta::core::JSON::String> standalone_booleans;
+  const auto standalone_resolver{[&resolver, &standalone, &base,
+                                  &standalone_booleans, booleans,
                                   nameable](const std::string_view identifier)
                                      -> sourcemeta::core::SchemaResolverResult {
     auto result{resolver(identifier)};
@@ -909,6 +914,16 @@ auto bundle_schemas(sourcemeta::core::JSON &document,
           "A Schema Object must be an object"};
     }
 
+    // Schema bundling restates a boolean as the object saying the same thing
+    // so that it can be named, but the declaration below is one a boolean has
+    // nowhere to hold, so what this hands over cannot carry it. Which of them
+    // arrived this way is remembered instead, and the declaration written once
+    // the object that replaced it has landed
+    if (result.has_value() && result.value().is_boolean()) {
+      standalone_booleans.insert(sourcemeta::core::JSON::String{identifier});
+      return result;
+    }
+
     if (!result.has_value() || !result.value().is_object() ||
         result.value().defines("$schema")) {
       return result;
@@ -943,14 +958,6 @@ auto bundle_schemas(sourcemeta::core::JSON &document,
   }
 
   auto &schemas{sourcemeta::core::get(document, container)};
-  // A resource that travels this way has to say who it is, which is what JSON
-  // Schema 2020-12 Section 9.3.1 asks of a Compound Schema Document: "Each
-  // embedded JSON Schema Resource MUST identify itself with a URI using the
-  // `$id` keyword". A description is not one of those, so nothing binds it
-  // here, but a reference that named a schema by URI goes on naming nothing
-  // unless the schema carries that URI. Section 4.8.24 lets a Schema Object be
-  // a boolean, which carries no keyword at all and so can say nothing, which
-  // writing it out as the object saying the same thing settles
   // What each key became, for the sake of whatever was written to point at the
   // old one
   std::vector<
@@ -959,17 +966,13 @@ auto bundle_schemas(sourcemeta::core::JSON &document,
   for (const auto &entry : landed) {
     const auto &identifier{entry.first};
     const auto &key{entry.second.back().to_property()};
-    auto &schema{schemas.at(key)};
-    if (schema.is_boolean()) {
-      spell_out(schema);
-      // A schema that can neither name its dialect nor name itself answers to
-      // where it sits, and what pointed at it was written as a pointer for
-      // that reason, so there is nothing here for it to be told to answer to
-      // instead
-      if (nameable) {
-        schema.assign("$schema", sourcemeta::core::JSON{standalone});
-        schema.assign("$id", sourcemeta::core::JSON{identifier});
-      }
+    // Section 4.8.24.5 reads "For standalone JSON Schema documents that do not
+    // set `$schema` [...] the dialect SHOULD be assumed to be the OAS
+    // dialect", and schema bundling handed the object that replaced a boolean
+    // the dialect it was bundling under instead, there being nothing on the
+    // boolean to tell it otherwise
+    if (nameable && standalone_booleans.contains(identifier)) {
+      schemas.at(key).assign("$schema", sourcemeta::core::JSON{standalone});
     }
 
     const auto name{schema_name(schemas, identifier, namer)};
@@ -2328,9 +2331,9 @@ auto bundle_internal(JSON &document, const SchemaWalker &walker,
       // being the last thing that happens.
       //
       // Going round again is safe as well as needed. Every schema that lands
-      // says who it is, and the one kind that cannot, a boolean, has whatever
-      // named it written out to name where it went, so a second pass finds
-      // nothing left outside to ask for rather than landing a second copy
+      // says who it is, a boolean being restated as the object that can say
+      // it, so a second pass finds nothing left outside to ask for rather than
+      // landing a second copy
       if (bundle_schemas(document, walk, walker, schema_resolver, base,
                          remaining, options)) {
         continue;

test/jsonschema/bundle/2019-09/a_boolean_remote_is_restated_so_it_can_name_itself.json

@@ -0,0 +1,55 @@
+{
+  "schema": {
+    "properties": {
+      "flag": {
+        "$ref": "https://www.sourcemeta.com/flag"
+      }
+    }
+  },
+  "resolver": {
+    "https://www.sourcemeta.com/flag": true
+  },
+  "defaultDialect": "https://json-schema.org/draft/2019-09/schema",
+  "result": {
+    "nonOfficialMetaschemas": {
+      "properties": {
+        "flag": {
+          "$ref": "https://www.sourcemeta.com/flag"
+        }
+      },
+      "$defs": {
+        "https://www.sourcemeta.com/flag": {
+          "$schema": "https://json-schema.org/draft/2019-09/schema",
+          "$id": "https://www.sourcemeta.com/flag"
+        }
+      }
+    },
+    "references": {
+      "properties": {
+        "flag": {
+          "$ref": "https://www.sourcemeta.com/flag"
+        }
+      },
+      "$defs": {
+        "https://www.sourcemeta.com/flag": {
+          "$schema": "https://json-schema.org/draft/2019-09/schema",
+          "$id": "https://www.sourcemeta.com/flag"
+        }
+      }
+    }
+  },
+  "inserted": {
+    "nonOfficialMetaschemas": [
+      {
+        "from": "https://www.sourcemeta.com/flag",
+        "at": "/$defs/https:~1~1www.sourcemeta.com~1flag"
+      }
+    ],
+    "references": [
+      {
+        "from": "https://www.sourcemeta.com/flag",
+        "at": "/$defs/https:~1~1www.sourcemeta.com~1flag"
+      }
+    ]
+  }
+}

test/jsonschema/bundle/2020-12/a_boolean_remote_from_a_remote_that_names_itself.json

@@ -0,0 +1,93 @@
+{
+  "schema": {
+    "$schema": "https://json-schema.org/draft/2020-12/schema",
+    "properties": {
+      "outer": {
+        "$ref": "https://www.sourcemeta.com/outer"
+      }
+    }
+  },
+  "resolver": {
+    "https://www.sourcemeta.com/outer": {
+      "$schema": "https://json-schema.org/draft/2020-12/schema",
+      "$id": "https://www.sourcemeta.com/outer",
+      "properties": {
+        "flag": {
+          "$ref": "https://www.sourcemeta.com/flag"
+        }
+      }
+    },
+    "https://www.sourcemeta.com/flag": true
+  },
+  "defaultDialect": "https://json-schema.org/draft/2020-12/schema",
+  "result": {
+    "nonOfficialMetaschemas": {
+      "$schema": "https://json-schema.org/draft/2020-12/schema",
+      "properties": {
+        "outer": {
+          "$ref": "https://www.sourcemeta.com/outer"
+        }
+      },
+      "$defs": {
+        "https://www.sourcemeta.com/flag": {
+          "$schema": "https://json-schema.org/draft/2020-12/schema",
+          "$id": "https://www.sourcemeta.com/flag"
+        },
+        "https://www.sourcemeta.com/outer": {
+          "$schema": "https://json-schema.org/draft/2020-12/schema",
+          "$id": "https://www.sourcemeta.com/outer",
+          "properties": {
+            "flag": {
+              "$ref": "https://www.sourcemeta.com/flag"
+            }
+          }
+        }
+      }
+    },
+    "references": {
+      "$schema": "https://json-schema.org/draft/2020-12/schema",
+      "properties": {
+        "outer": {
+          "$ref": "https://www.sourcemeta.com/outer"
+        }
+      },
+      "$defs": {
+        "https://www.sourcemeta.com/flag": {
+          "$schema": "https://json-schema.org/draft/2020-12/schema",
+          "$id": "https://www.sourcemeta.com/flag"
+        },
+        "https://www.sourcemeta.com/outer": {
+          "$schema": "https://json-schema.org/draft/2020-12/schema",
+          "$id": "https://www.sourcemeta.com/outer",
+          "properties": {
+            "flag": {
+              "$ref": "https://www.sourcemeta.com/flag"
+            }
+          }
+        }
+      }
+    }
+  },
+  "inserted": {
+    "nonOfficialMetaschemas": [
+      {
+        "from": "https://www.sourcemeta.com/flag",
+        "at": "/$defs/https:~1~1www.sourcemeta.com~1flag"
+      },
+      {
+        "from": "https://www.sourcemeta.com/outer",
+        "at": "/$defs/https:~1~1www.sourcemeta.com~1outer"
+      }
+    ],
+    "references": [
+      {
+        "from": "https://www.sourcemeta.com/flag",
+        "at": "/$defs/https:~1~1www.sourcemeta.com~1flag"
+      },
+      {
+        "from": "https://www.sourcemeta.com/outer",
+        "at": "/$defs/https:~1~1www.sourcemeta.com~1outer"
+      }
+    ]
+  }
+}