Make primary keys optional.

Author: chinmaygarde

impeller/archivist/archivable.h

@@ -5,6 +5,7 @@
 #pragma once
 
 #include <cstdint>
+#include <optional>
 #include <string>
 #include <vector>
 
@@ -22,15 +23,15 @@ struct ArchiveDef {
 
 class ArchiveLocation;
 
+using PrimaryKey = std::optional<int64_t>;
+
 //------------------------------------------------------------------------------
 /// @brief      Instances of `Archivable`s can be read from and written to a
 ///             persistent archive.
 ///
 class Archivable {
  public:
-  using ArchiveName = uint64_t;
-
-  virtual ArchiveName GetArchivePrimaryKey() const = 0;
+  virtual PrimaryKey GetPrimaryKey() const = 0;
 
   virtual bool Write(ArchiveLocation& item) const = 0;
 

impeller/archivist/archive.cc

@@ -10,6 +10,7 @@
 #include "impeller/archivist/archive_class_registration.h"
 #include "impeller/archivist/archive_database.h"
 #include "impeller/archivist/archive_location.h"
+#include "impeller/base/validation.h"
 
 namespace impeller {
 
@@ -50,7 +51,13 @@ std::optional<int64_t /* row id */> Archive::ArchiveInstance(
     return std::nullopt;
   }
 
-  auto primary_key = archivable.GetArchivePrimaryKey();
+  auto primary_key = archivable.GetPrimaryKey();
+
+  if (!definition.auto_key && !primary_key.has_value()) {
+    VALIDATION_LOG << "Archive definition specified that primary keys will be "
+                      "explicitly specified but none was provided when asked.";
+    return std::nullopt;
+  }
 
   /*
    *  The lifecycle of the archive item is tied to this scope and there is no
@@ -65,7 +72,7 @@ std::optional<int64_t /* row id */> Archive::ArchiveInstance(
    */
   if (!definition.auto_key &&
       !statement.WriteValue(ArchiveClassRegistration::kPrimaryKeyIndex,
-                            primary_key)) {
+                            primary_key.value())) {
     return std::nullopt;
   }
 
@@ -79,7 +86,8 @@ std::optional<int64_t /* row id */> Archive::ArchiveInstance(
 
   int64_t lastInsert = database_->GetLastInsertRowID();
 
-  if (!definition.auto_key && lastInsert != static_cast<int64_t>(primary_key)) {
+  if (!definition.auto_key &&
+      lastInsert != static_cast<int64_t>(primary_key.value())) {
     return std::nullopt;
   }
 
@@ -93,7 +101,7 @@ std::optional<int64_t /* row id */> Archive::ArchiveInstance(
 }
 
 bool Archive::UnarchiveInstance(const ArchiveDef& definition,
-                                Archivable::ArchiveName name,
+                                PrimaryKey name,
                                 Archivable& archivable) {
   UnarchiveStep stepper = [&archivable](ArchiveLocation& item) {
     archivable.Read(item);
@@ -105,7 +113,7 @@ bool Archive::UnarchiveInstance(const ArchiveDef& definition,
 
 size_t Archive::UnarchiveInstances(const ArchiveDef& definition,
                                    Archive::UnarchiveStep stepper,
-                                   std::optional<int64_t> primary_key) {
+                                   PrimaryKey primary_key) {
   if (!IsValid()) {
     return 0;
   }

impeller/archivist/archive.h

@@ -35,7 +35,7 @@ class Archive {
 
   template <class T,
             class = std::enable_if<std::is_base_of<Archivable, T>::value>>
-  bool Read(Archivable::ArchiveName name, T& archivable) {
+  bool Read(PrimaryKey name, T& archivable) {
     const ArchiveDef& def = T::ArchiveDefinition;
     return UnarchiveInstance(def, name, archivable);
   }
@@ -60,12 +60,12 @@ class Archive {
       const Archivable& archivable);
 
   bool UnarchiveInstance(const ArchiveDef& definition,
-                         Archivable::ArchiveName name,
+                         PrimaryKey name,
                          Archivable& archivable);
 
   size_t UnarchiveInstances(const ArchiveDef& definition,
                             UnarchiveStep stepper,
-                            std::optional<int64_t> primary_key = std::nullopt);
+                            PrimaryKey primary_key = std::nullopt);
 
   FML_DISALLOW_COPY_AND_ASSIGN(Archive);
 };

impeller/archivist/archive_location.cc

@@ -12,15 +12,15 @@ namespace impeller {
 ArchiveLocation::ArchiveLocation(Archive& context,
                                  ArchiveStatement& statement,
                                  const ArchiveClassRegistration& registration,
-                                 std::optional<int64_t> name)
+                                 PrimaryKey name)
     : context_(context),
       statement_(statement),
       registration_(registration),
       name_(name),
       current_class_(registration.GetClassName()) {}
 
-Archivable::ArchiveName ArchiveLocation::GetPrimaryKey() const {
-  return name_.value_or(0u);
+PrimaryKey ArchiveLocation::GetPrimaryKey() const {
+  return name_;
 }
 
 bool ArchiveLocation::Write(ArchiveDef::Member member,
@@ -79,7 +79,7 @@ std::optional<int64_t> ArchiveLocation::WriteVectorKeys(
   return context_.ArchiveInstance(ArchiveVector::ArchiveDefinition, vector);
 }
 
-bool ArchiveLocation::ReadVectorKeys(Archivable::ArchiveName name,
+bool ArchiveLocation::ReadVectorKeys(PrimaryKey name,
                                      std::vector<int64_t>& members) {
   ArchiveVector vector;
   if (!context_.UnarchiveInstance(ArchiveVector::ArchiveDefinition, name,

impeller/archivist/archive_location.h

@@ -20,7 +20,7 @@ class ArchiveStatement;
 
 class ArchiveLocation {
  public:
-  Archivable::ArchiveName GetPrimaryKey() const;
+  PrimaryKey GetPrimaryKey() const;
 
   template <class T, class = std::enable_if<std::is_integral<T>::value>>
   bool Write(ArchiveDef::Member member, T item) {
@@ -166,24 +166,23 @@ class ArchiveLocation {
   Archive& context_;
   ArchiveStatement& statement_;
   const ArchiveClassRegistration& registration_;
-  std::optional<int64_t> name_;
+  PrimaryKey name_;
   std::string current_class_;
 
   friend class Archive;
 
   ArchiveLocation(Archive& context,
                   ArchiveStatement& statement,
                   const ArchiveClassRegistration& registration,
-                  std::optional<int64_t> name);
+                  PrimaryKey name);
 
   bool WriteIntegral(ArchiveDef::Member member, int64_t item);
 
   bool ReadIntegral(ArchiveDef::Member member, int64_t& item);
 
   std::optional<int64_t> WriteVectorKeys(std::vector<int64_t>&& members);
 
-  bool ReadVectorKeys(Archivable::ArchiveName name,
-                      std::vector<int64_t>& members);
+  bool ReadVectorKeys(PrimaryKey name, std::vector<int64_t>& members);
 
   bool Write(ArchiveDef::Member member,
              const ArchiveDef& otherDef,

impeller/archivist/archive_vector.cc

@@ -17,13 +17,14 @@ ArchiveVector::ArchiveVector() {}
 
 const ArchiveDef ArchiveVector::ArchiveDefinition = {
     /* .superClass = */ nullptr,
-    /* .className = */ "Meta_Vector",
+    /* .className = */ "_IPLR_meta_vector_items_",
     /* .autoAssignName = */ true,
     /* .members = */ {0},
 };
 
-Archivable::ArchiveName ArchiveVector::GetArchivePrimaryKey() const {
-  return 0;
+PrimaryKey ArchiveVector::GetPrimaryKey() const {
+  // Archive definition says the keys will be auto assigned.
+  return std::nullopt;
 }
 
 const std::vector<int64_t> ArchiveVector::GetKeys() const {

impeller/archivist/archive_vector.h

@@ -13,7 +13,7 @@ class ArchiveVector : public Archivable {
  public:
   static const ArchiveDef ArchiveDefinition;
 
-  ArchiveName GetArchivePrimaryKey() const override;
+  PrimaryKey GetPrimaryKey() const override;
 
   const std::vector<int64_t> GetKeys() const;
 

impeller/archivist/archivist_unittests.cc

@@ -14,7 +14,7 @@
 namespace impeller {
 namespace testing {
 
-static Archivable::ArchiveName LastSample = 0;
+static int64_t LastSample = 0;
 
 class Sample : public Archivable {
  public:
@@ -23,7 +23,7 @@ class Sample : public Archivable {
   uint64_t GetSomeData() const { return some_data_; }
 
   // |Archivable|
-  ArchiveName GetArchivePrimaryKey() const override { return name_; }
+  PrimaryKey GetPrimaryKey() const override { return name_; }
 
   // |Archivable|
   bool Write(ArchiveLocation& item) const override {
@@ -40,7 +40,7 @@ class Sample : public Archivable {
 
  private:
   uint64_t some_data_;
-  ArchiveName name_ = ++LastSample;
+  PrimaryKey name_ = ++LastSample;
 
   FML_DISALLOW_COPY_AND_ASSIGN(Sample);
 };
@@ -87,12 +87,12 @@ TEST_F(ArchiveTest, ReadData) {
 
   size_t count = 50;
 
-  std::vector<Archivable::ArchiveName> keys;
+  std::vector<PrimaryKey::value_type> keys;
   std::vector<uint64_t> values;
 
   for (size_t i = 0; i < count; i++) {
     Sample sample(i + 1);
-    keys.push_back(sample.GetArchivePrimaryKey());
+    keys.push_back(sample.GetPrimaryKey().value());
     values.push_back(sample.GetSomeData());
     ASSERT_TRUE(archive.Write(sample));
   }
@@ -110,15 +110,15 @@ TEST_F(ArchiveTest, ReadDataWithNames) {
 
   size_t count = 8;
 
-  std::vector<Archivable::ArchiveName> keys;
+  std::vector<PrimaryKey::value_type> keys;
   std::vector<uint64_t> values;
 
   keys.reserve(count);
   values.reserve(count);
 
   for (size_t i = 0; i < count; i++) {
     Sample sample(i + 1);
-    keys.push_back(sample.GetArchivePrimaryKey());
+    keys.push_back(sample.GetPrimaryKey().value());
     values.push_back(sample.GetSomeData());
     ASSERT_TRUE(archive.Write(sample));
   }
@@ -127,7 +127,7 @@ TEST_F(ArchiveTest, ReadDataWithNames) {
     Sample sample;
     ASSERT_TRUE(archive.Read(keys[i], sample));
     ASSERT_EQ(values[i], sample.GetSomeData());
-    ASSERT_EQ(keys[i], sample.GetArchivePrimaryKey());
+    ASSERT_EQ(keys[i], sample.GetPrimaryKey());
   }
 }