Enforce use_hash flag for types of structured annotations in schema.t…

…hrift

Summary:
The schema.thrift representation constructed by thrift2ast has the [use_hash](https://fburl.com/code/e8a071lt) flag to convert any URIs or name identifiers to unique hashes. This prevents users from having to create & store an external mapping of identifiers to unique IDs in order to e.g. check for type equality.

While the `Annotation` struct itself already hashed its underlying type, all annotation for a definition are also stored in a [map<string, Annotation>](https://www.internalfb.com/code/fbsource/[4ef64d09cf78d72d3bf29ae2b8c12251272133e2]/fbcode/thrift/lib/thrift/schema.thrift?lines=169), where the string is the URI/scoped name of the type (again).

This diff adds a third write path that indexes the annotations by `DefinitionKey`, which going forward is the canonical way of indexing definitions. The other write paths are retained for backwards compatibility until we can migrate all existing use-cases.

#buildall

Reviewed By: praihan, iahs

Differential Revision: D69148104

fbshipit-source-id: 04396567b6851e7fd9f759555930657312b63840

third-party/thrift/src/thrift/compiler/sema/schematizer.cc

@@ -53,16 +53,6 @@ std::unique_ptr<t_const_value> val(std::string_view s) {
   return val(std::string{s});
 }
 
-std::string uri_or_name(const t_named& node) {
-  if (!node.uri().empty()) {
-    return node.uri();
-  }
-  if (node.program()) {
-    return node.program()->scope_name(node);
-  }
-  return node.name();
-}
-
 // Returns the ProtocolObject ValueType of a t_const_value
 t_type::value_type from_const_value_type(
     t_const_value::t_const_value_kind kind) {
@@ -90,15 +80,25 @@ t_type_ref schematizer::std_type(std::string_view uri) {
       static_cast<const t_type*>(scope_.find_by_uri(uri)));
 }
 
+// Returns the `TypeUri` type & the corresponding Uri value for the given node
+schematizer::resolved_uri schematizer::calculate_uri(
+    const t_named& node, const bool use_hash) {
+  if (use_hash) {
+    return {"definitionKey", identify_definition(node)};
+  }
+  if (!node.uri().empty()) {
+    return {"uri", node.uri()};
+  }
+  if (node.program()) {
+    return {"scopedName", node.program()->scope_name(node)};
+  }
+  return {"scopedName", node.name()};
+}
+
 std::unique_ptr<t_const_value> schematizer::type_uri(const t_type& type) {
   auto ret = t_const_value::make_map();
-  if (opts_.use_hash) {
-    ret->add_map(val("definitionKey"), val(identify_definition(type)));
-  } else if (!type.uri().empty()) {
-    ret->add_map(val("uri"), val(type.uri()));
-  } else {
-    ret->add_map(val("scopedName"), val(type.get_scoped_name()));
-  }
+  auto uri = calculate_uri(type, opts_.use_hash);
+  ret->add_map(val(uri.uri_type), val(std::move(uri.value)));
   ret->set_ttype(std_type("facebook.com/thrift/type/TypeUri"));
   return ret;
 }
@@ -123,6 +123,7 @@ void schematizer::add_definition(
       !structured.empty() && opts_.include_annotations) {
     auto annots = t_const_value::make_map();
     auto structured_annots = t_const_value::make_list();
+    auto annots_by_key = t_const_value::make_map();
 
     for (const auto& item : structured) {
       auto annot = t_const_value::make_map();
@@ -154,15 +155,24 @@ void schematizer::add_definition(
       }
 
       annot->set_ttype(std_type("facebook.com/thrift/type/Annotation"));
-      annots->add_map(val(uri_or_name(*item.type())), std::move(annot));
+      auto unhashed_uri = calculate_uri(*item.type(), false /*use_hash*/);
+      // We're not hashing & ignoring the UriType here, as annotations are
+      // stored as map<string, Annotation>.
+      annots->add_map(val(std::move(unhashed_uri.value)), annot->clone());
+
+      auto hashed_uri = calculate_uri(*item.type(), true /*use_hash*/);
+      annots_by_key->add_map(
+          val(std::move(hashed_uri.value)), std::move(annot));
     }
 
     // Double write to deprecated externed path (T161963504).
     if (opts_.double_writes) {
       definition->add_map(
           val("structuredAnnotations"), std::move(structured_annots));
     }
+
     definition->add_map(val("annotations"), std::move(annots));
+    definition->add_map(val("annotationsByKey"), std::move(annots_by_key));
   }
 
   if (auto unstructured = node.annotations();

third-party/thrift/src/thrift/compiler/sema/schematizer.h

@@ -127,6 +127,13 @@ class schematizer {
       intern_func& intern_value);
 
   std::string_view program_checksum(const t_program& program);
+
+  struct resolved_uri {
+    std::string_view uri_type;
+    std::string value;
+  };
+
+  resolved_uri calculate_uri(const t_named& node, bool use_hash);
 };
 
 // Tag for obtaining a compact-encoded schema for the root program via a

third-party/thrift/src/thrift/compiler/test/schematizer_test.cc

@@ -25,19 +25,9 @@
 
 #include <folly/portability/GTest.h>
 
-using apache::thrift::compiler::t_const_value;
-using apache::thrift::compiler::t_enum;
-using apache::thrift::compiler::t_enum_value;
-using apache::thrift::compiler::t_field;
-using apache::thrift::compiler::t_list;
-using apache::thrift::compiler::t_map;
-using apache::thrift::compiler::t_primitive_type;
-using apache::thrift::compiler::t_program;
-using apache::thrift::compiler::t_set;
-using apache::thrift::compiler::t_struct;
-using apache::thrift::compiler::t_type_ref;
-using apache::thrift::compiler::t_typedef;
+using namespace apache::thrift::compiler;
 using apache::thrift::compiler::detail::protocol_value_builder;
+using apache::thrift::compiler::detail::schematizer;
 
 template <typename... Args>
 std::unique_ptr<t_const_value> val(Args&&... args) {
@@ -133,12 +123,17 @@ std::unique_ptr<t_struct> make_foo_bar(const t_program* program) {
   return struct_ty;
 }
 
-TEST(SchematizerTest, wrap_with_protocol_with_struct_ty) {
+std::unique_ptr<t_const_value> make_foo_bar_value() {
   auto strct = t_const_value::make_map();
   strct->add_map(val("foo"), val(1));
   strct->add_map(val("bar"), val(2));
   strct->add_map(val("baz"), val(3.14));
   strct->add_map(val("qux"), val(4.2));
+  return strct;
+}
+
+TEST(SchematizerTest, wrap_with_protocol_with_struct_ty) {
+  auto strct = make_foo_bar_value();
 
   // With the type mapping, integers will be represented by their types in
   // FooBar.
@@ -352,3 +347,185 @@ TEST(SchematizerTest, wrap_with_protocol_map) {
   expectWrappedValue(
       *inner_map_val2, t_const_value::CV_INTEGER, "i32Value", 888);
 }
+
+constexpr auto UTILITY_CLASSES = {
+    std::pair(
+        "StructuredAnnotation",
+        "facebook.com/thrift/type/StructuredAnnotation"),
+    std::pair("Annotation", "facebook.com/thrift/type/Annotation"),
+    std::pair("ProtocolValue", "facebook.com/thrift/protocol/Value"),
+    std::pair{"TypeUri", "facebook.com/thrift/type/TypeUri"},
+};
+
+template <typename F>
+void with_schematizer(schematizer::options opts, F&& f) {
+  source_manager sm;
+  sm.add_virtual_file("my_prog", "abcd");
+
+  t_scope scope{};
+  auto program = std::make_unique<t_program>("my_prog", "my_prog");
+
+  // Add some well-known types to the scope.
+  auto protocol_value_ty = std::make_unique<t_struct>(
+      program.get(), "ProtocolValue"); // Can be empty
+  protocol_value_ty->set_uri("facebook.com/thrift/protocol/Value");
+  std::vector<std::unique_ptr<t_struct>> utility_types;
+  for (const auto& [name, uri] : UTILITY_CLASSES) {
+    utility_types.push_back(std::make_unique<t_struct>(program.get(), name));
+    utility_types.back()->set_uri(uri);
+    scope.add_def(*utility_types.back().get());
+  }
+
+  schematizer schematizer{scope, sm, std::move(opts)};
+  f(*program.get(), schematizer);
+}
+
+template <typename F, typename G, typename H>
+void with_structured_annotation_uris(
+    const t_const_value& struct_schema,
+    const std::string_view struct_name,
+    F&& on_annotation,
+    G&& on_structured_annotation,
+    H&& on_annotation_by_key) {
+  const auto [attrs_key, attrs] = struct_schema.get_map().at(0);
+  EXPECT_EQ(attrs_key->get_string(), "attrs");
+  const auto& definition_attrs = attrs->get_map();
+
+  const auto [name_key, name] = definition_attrs.at(0);
+  EXPECT_EQ(name_key->get_string(), "name");
+  EXPECT_EQ(name->get_string(), struct_name);
+
+  // Visit `structuredAnnotations`
+  {
+    const auto [structured_annotations_key, structured_annotation_ids_set] =
+        definition_attrs.at(1);
+    EXPECT_EQ(
+        structured_annotations_key->get_string(), "structuredAnnotations");
+    const auto& structured_annotations =
+        structured_annotation_ids_set->get_list();
+
+    for (size_t i = 0; i < structured_annotations.size(); ++i) {
+      const auto annotation_id = structured_annotations.at(i)->get_integer();
+      on_structured_annotation(i, annotation_id);
+    }
+  }
+
+  // Visit `annotations`
+  {
+    const auto [annotations_key, annotations_map] = definition_attrs.at(2);
+    EXPECT_EQ(annotations_key->get_string(), "annotations");
+    const auto& annotations = annotations_map->get_map();
+
+    for (size_t i = 0; i < annotations.size(); ++i) {
+      const auto [annotation_name, _] = annotations.at(i);
+      on_annotation(i, annotation_name->get_string());
+    }
+  }
+
+  // Visit `annotionsByKey`
+  {
+    const auto [annotations_key, annotations_map] = definition_attrs.at(3);
+    EXPECT_EQ(annotations_key->get_string(), "annotationsByKey");
+    const auto& annotations = annotations_map->get_map();
+
+    for (size_t i = 0; i < annotations.size(); ++i) {
+      const auto [annotation_def_key, _] = annotations.at(i);
+      on_annotation_by_key(i, annotation_def_key->get_string());
+    }
+  }
+}
+
+template <typename F>
+void with_foo_bar_plus_structured_annotations(t_program& program, F f) {
+  auto foo_bar_with_annotations_ty =
+      std::make_unique<t_struct>(&program, "FooBarWithStructuredAnnotations");
+  auto foo_bar_ty = make_foo_bar(&program);
+  foo_bar_with_annotations_ty->add_structured_annotation(
+      std::make_unique<t_const>(
+          &program,
+          t_type_ref{*foo_bar_ty},
+          "FooBarWithStructuredAnnotations",
+          make_foo_bar_value()));
+
+  f(*foo_bar_with_annotations_ty, *foo_bar_ty);
+}
+
+TEST(SchematizerTest, use_hash_for_structured_annotations) {
+  for (const bool use_hash : {true, false}) {
+    schematizer::options opts;
+    opts.use_hash = use_hash;
+    size_t id = 0;
+    std::vector<std::unique_ptr<t_const_value>> interned_values;
+    opts.intern_value = [&](std::unique_ptr<t_const_value> val, t_program*) {
+      interned_values.push_back(std::move(val));
+      return static_cast<schematizer::value_id>(id++);
+    };
+
+    with_schematizer(opts, [&](t_program& program, schematizer& schematizer) {
+      with_foo_bar_plus_structured_annotations(
+          program,
+          [&](const auto& foo_bar_with_annotations_ty, const auto& foo_bar_ty) {
+            // Generate a schema.thrift representation of
+            // `FooBarWithStructuredAnnotations`
+            const std::unique_ptr<t_const_value> struct_schema =
+                schematizer.gen_schema(foo_bar_with_annotations_ty);
+
+            const auto on_annotation = [&](size_t annotation_idx,
+                                           const std::string& annotation_name) {
+              ASSERT_EQ(annotation_idx, 0); // There's only one annotation
+              EXPECT_EQ(
+                  annotation_name, "my_prog.FooBar"); // This is never hashed
+            };
+
+            const auto on_structured_annotation = [&](size_t annotation_idx,
+                                                      size_t id) {
+              ASSERT_EQ(
+                  annotation_idx,
+                  0); // There's only one structured annotation
+
+              const auto& interned_annotation =
+                  interned_values.at(id)->get_map();
+              const auto [type_key, type_uri] = interned_annotation.at(1);
+              ASSERT_EQ(type_key->get_string(), "type");
+
+              // TypeUri should be a union, so check for the appropriate
+              // field
+              const auto [type_uri_key, type_uri_value] =
+                  type_uri->get_map().at(0);
+              if (use_hash) {
+                EXPECT_EQ(type_uri_key->get_string(), "definitionKey");
+                EXPECT_EQ(
+                    type_uri_value->get_string(),
+                    schematizer.identify_definition(foo_bar_ty));
+              } else {
+                EXPECT_EQ(type_uri_key->get_string(), "scopedName");
+                EXPECT_EQ(
+                    type_uri_value->get_string(),
+                    "my_prog.FooBar"); // This should not be hashed
+              }
+            };
+
+            const auto on_annotation_by_key =
+                [&](size_t annotation_idx, const std::string& definition_key) {
+                  ASSERT_EQ(annotation_idx,
+                            0); // There's only one annotation
+
+                  // This is always hashed
+                  EXPECT_TRUE(
+                      definition_key.find("FooBar") == std::string::npos);
+                  EXPECT_EQ(
+                      definition_key,
+                      schematizer.identify_definition(foo_bar_ty));
+                };
+            // Verify that the URIs of the structured annotations are
+            // hashed
+            with_structured_annotation_uris(
+                *struct_schema,
+                "FooBarWithStructuredAnnotations",
+                on_annotation,
+                on_structured_annotation,
+                on_annotation_by_key);
+          });
+    });
+  }
+}

third-party/thrift/src/thrift/lib/thrift/schema.thrift

@@ -157,17 +157,24 @@ struct DefinitionAttrs {
   2: string uri;
 
   /**
-   * DEPRECATED! Prefer `annotations`.
+   * DEPRECATED! Prefer `annotationsByKey`.
    * The structured annotations associated with this definition.
    */
   3: AnnotationIds structuredAnnotations;
 
   /**
+   * DEPRECATED! Prefer `annotationsByKey`
    * The structured annotations associated with this definition indexed by
-   * their universal names.
+   * their universal names (i.e. Uri or scoped name)
    */
   8: map<string, Annotation> annotations;
 
+  /**
+   * The structured annotations associated with this definitions indexed by
+   * the annotation's definitionKey (~hash).
+   */
+  9: map<DefinitionKey, Annotation> annotationsByKey;
+
   /**
    * The unstructured annotations (deprecated) associated with this definition.
    */