Refactor schema initialization a bit

Change schema verification to operate on a pair of Schema objects rather than a
Schema and a Group to eliminate some redundant work done, defer some of the
work done for migrations to within the migration block to avoid doing it
unnecessarily, and make passing in a custom schema in the Config when creating
a Realm entirely equivalent to calling update_schema() afterwards.

src/object-store/object_store.cpp

@@ -136,20 +136,24 @@ TableRef ObjectStore::table_for_object_type_create_if_needed(Group *group, const
     return group->get_or_add_table(table_name_for_object_type(object_type), &created);
 }
 
-static inline bool property_has_changed(Property &p1, Property &p2) {
+static inline bool property_has_changed(Property const& p1, Property const& p2) {
-    return p1.type != p2.type || p1.name != p2.name || p1.object_type != p2.object_type || p1.is_nullable != p2.is_nullable;
+    return p1.type != p2.type
+        || p1.name != p2.name
+        || p1.object_type != p2.object_type
+        || p1.is_nullable != p2.is_nullable;
 }
 
 static bool compare_by_name(ObjectSchema const& lft, ObjectSchema const& rgt) {
     return lft.name < rgt.name;
 }
 
-void ObjectStore::verify_schema(Group *group, Schema &target_schema, bool allow_missing_tables) {
+void ObjectStore::verify_schema(Schema const& actual_schema, Schema &target_schema, bool allow_missing_tables) {
     std::sort(begin(target_schema), end(target_schema), compare_by_name);
 
     std::vector<ObjectSchemaValidationException> errors;
     for (auto &object_schema : target_schema) {
-        if (!table_for_object_type(group, object_schema.name)) {
+        auto matching_schema = std::lower_bound(begin(actual_schema), end(actual_schema), object_schema, compare_by_name);
+        if (matching_schema == end(actual_schema) || matching_schema->name != object_schema.name) {
             if (!allow_missing_tables) {
                 errors.emplace_back(ObjectSchemaValidationException(object_schema.name,
                                     "Missing table for object type '" + object_schema.name + "'."));
@@ -157,7 +161,7 @@ void ObjectStore::verify_schema(Group *group, Schema &target_schema, bool allow_
             continue;
         }
 
-        auto more_errors = verify_object_schema(group, object_schema, target_schema);
+        auto more_errors = verify_object_schema(*matching_schema, object_schema, target_schema);
         errors.insert(errors.end(), more_errors.begin(), more_errors.end());
     }
     if (errors.size()) {
@@ -165,9 +169,8 @@ void ObjectStore::verify_schema(Group *group, Schema &target_schema, bool allow_
     }
 }
 
-std::vector<ObjectSchemaValidationException> ObjectStore::verify_object_schema(Group *group, ObjectSchema &target_schema, Schema &schema) {
+std::vector<ObjectSchemaValidationException> ObjectStore::verify_object_schema(ObjectSchema const& table_schema, ObjectSchema &target_schema, Schema &schema) {
     std::vector<ObjectSchemaValidationException> exceptions;
-    ObjectSchema table_schema(group, target_schema.name);
 
     ObjectSchema cmp;
     auto schema_contains_table = [&](std::string const& name) {
@@ -176,7 +179,7 @@ std::vector<ObjectSchemaValidationException> ObjectStore::verify_object_schema(G
     };
 
     // check to see if properties are the same
-    Property *primary = nullptr;
+    const Property *primary = nullptr;
     for (auto& current_prop : table_schema.properties) {
         auto target_prop = target_schema.property_for_name(current_prop.name);
 
@@ -276,12 +279,27 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update
         ObjectSchema current_schema(group, target_object_schema->name);
         std::vector<Property> &target_props = target_object_schema->properties;
 
+        // remove extra columns
+        size_t deleted = 0;
+        for (auto& current_prop : current_schema.properties) {
+            auto target_prop = target_object_schema->property_for_name(current_prop.name);
+            if (!target_prop || property_has_changed(current_prop, *target_prop)) {
+                table->remove_column(current_prop.table_column - deleted);
+                ++deleted;
+                current_prop.table_column = npos;
+                changed = true;
+            }
+            else {
+                current_prop.table_column -= deleted;
+            }
+        }
+
         // add missing columns
         for (auto& target_prop : target_props) {
             auto current_prop = current_schema.property_for_name(target_prop.name);
 
             // add any new properties (new name or different type)
-            if (!current_prop || property_has_changed(*current_prop, target_prop)) {
+            if (!current_prop || current_prop->table_column == npos) {
                 switch (target_prop.type) {
                         // for objects and arrays, we have to specify target table
                     case PropertyTypeObject:
@@ -298,17 +316,8 @@ bool ObjectStore::create_tables(Group *group, Schema &target_schema, bool update
 
                 changed = true;
             }
-        }
+            else {
-
+                target_prop.table_column = current_prop->table_column;
-        // remove extra columns
-        sort(begin(current_schema.properties), end(current_schema.properties), [](Property &i, Property &j) {
-            return j.table_column < i.table_column;
-        });
-        for (auto& current_prop : current_schema.properties) {
-            auto target_prop = target_object_schema->property_for_name(current_prop.name);
-            if (!target_prop || property_has_changed(current_prop, *target_prop)) {
-                table->remove_column(current_prop.table_column);
-                changed = true;
             }
         }
 
@@ -337,24 +346,22 @@ bool ObjectStore::is_schema_at_version(Group *group, uint64_t version) {
     return old_version == version;
 }
 
-bool ObjectStore::realm_requires_update(Group *group, uint64_t version, Schema const& schema) {
+bool ObjectStore::needs_update(Schema const& old_schema, Schema& schema) {
-    if (!is_schema_at_version(group, version)) {
-        return true;
-    }
-
     for (auto const& target_schema : schema) {
-        TableRef table = table_for_object_type(group, target_schema.name);
+        auto matching_schema = std::lower_bound(begin(old_schema), end(old_schema), target_schema, compare_by_name);
-        if (!table) {
+        if (matching_schema == end(old_schema) || matching_schema->name != target_schema.name) {
+            // Table doesn't exist
             return true;
         }
 
+        if (matching_schema->properties.size() != target_schema.properties.size()) {
+            // If the number of properties don't match then a migration is required
+            return false;
+        }
+
         // Check that all of the property indexes are up to date
-        size_t count = table->get_column_count();
+        for (size_t i = 0, count = target_schema.properties.size(); i < count; ++i) {
-        for (size_t col = 0; col < count; ++col) {
+            if (target_schema.properties[i].is_indexed != matching_schema->properties[i].is_indexed) {
-            StringData name = table->get_column_name(col);
-            bool is_indexed = table->has_search_index(col);
-            auto prop = target_schema.property_for_name(name);
-            if (prop && prop->requires_index() != is_indexed) {
                 return true;
             }
         }
@@ -363,9 +370,8 @@ bool ObjectStore::realm_requires_update(Group *group, uint64_t version, Schema c
     return false;
 }
 
-bool ObjectStore::update_realm_with_schema(Group *group,
+bool ObjectStore::update_realm_with_schema(Group *group, Schema const& old_schema,
-                                           uint64_t version,
+                                           uint64_t version, Schema &schema,
-                                           Schema &schema,
                                            MigrationFunction migration) {
     // Recheck the schema version after beginning the write transaction as
     // another process may have done the migration after we opened the read
@@ -376,7 +382,11 @@ bool ObjectStore::update_realm_with_schema(Group *group,
     bool changed = create_metadata_tables(group);
     changed = create_tables(group, schema, migrating) || changed;
 
-    verify_schema(group, schema);
+    if (!migrating) {
+        // If we aren't migrating, then verify that all of the tables which
+        // were already present are valid (newly created ones always are)
+        verify_schema(old_schema, schema, true);
+    }
 
     changed = update_indexes(group, schema) || changed;
 
@@ -403,6 +413,7 @@ Schema ObjectStore::schema_from_group(Group *group) {
             schema.emplace_back(group, object_type);
         }
     }
+    std::sort(begin(schema), end(schema), compare_by_name);
     return schema;
 }
 
@@ -466,7 +477,7 @@ InvalidSchemaVersionException::InvalidSchemaVersionException(uint64_t old_versio
     m_what = "Provided schema version " + std::to_string(old_version) + " is less than last set version " + std::to_string(new_version) + ".";
 }
 
-DuplicatePrimaryKeyValueException::DuplicatePrimaryKeyValueException(std::string object_type, Property &property) :
+DuplicatePrimaryKeyValueException::DuplicatePrimaryKeyValueException(std::string object_type, Property const& property) :
     m_object_type(object_type), m_property(property)
 {
     m_what = "Primary key property '" + property.name + "' has duplicate values after migration.";
@@ -482,25 +493,25 @@ SchemaValidationException::SchemaValidationException(std::vector<ObjectSchemaVal
     }
 }
 
-PropertyTypeNotIndexableException::PropertyTypeNotIndexableException(std::string object_type, Property &property) :
+PropertyTypeNotIndexableException::PropertyTypeNotIndexableException(std::string object_type, Property const& property) :
     ObjectSchemaPropertyException(object_type, property)
 {
     m_what = "Can't index property " + object_type + "." + property.name + ": indexing a property of type '" + string_for_property_type(property.type) + "' is currently not supported";
 }
 
-ExtraPropertyException::ExtraPropertyException(std::string object_type, Property &property) :
+ExtraPropertyException::ExtraPropertyException(std::string object_type, Property const& property) :
     ObjectSchemaPropertyException(object_type, property)
 {
     m_what = "Property '" + property.name + "' has been added to latest object model.";
 }
 
-MissingPropertyException::MissingPropertyException(std::string object_type, Property &property) :
+MissingPropertyException::MissingPropertyException(std::string object_type, Property const& property) :
     ObjectSchemaPropertyException(object_type, property)
 {
     m_what = "Property '" + property.name + "' is missing from latest object model.";
 }
 
-InvalidNullabilityException::InvalidNullabilityException(std::string object_type, Property &property) :
+InvalidNullabilityException::InvalidNullabilityException(std::string object_type, Property const& property) :
     ObjectSchemaPropertyException(object_type, property)
 {
     if (property.type == PropertyTypeObject) {
@@ -515,13 +526,13 @@ InvalidNullabilityException::InvalidNullabilityException(std::string object_type
     }
 }
 
-MissingObjectTypeException::MissingObjectTypeException(std::string object_type, Property &property) :
+MissingObjectTypeException::MissingObjectTypeException(std::string object_type, Property const& property) :
     ObjectSchemaPropertyException(object_type, property)
 {
     m_what = "Target type '" + property.object_type + "' doesn't exist for property '" + property.name + "'.";
 }
 
-MismatchedPropertiesException::MismatchedPropertiesException(std::string object_type, Property &old_property, Property &new_property) :
+MismatchedPropertiesException::MismatchedPropertiesException(std::string object_type, Property const& old_property, Property const& new_property) :
     ObjectSchemaValidationException(object_type), m_old_property(old_property), m_new_property(new_property)
 {
     if (new_property.type != old_property.type) {

src/object-store/object_store.hpp

@@ -42,26 +42,26 @@ namespace realm {
         // checks if the schema in the group is at the given version
         static bool is_schema_at_version(realm::Group *group, uint64_t version);
 
-        // verify a target schema against tables in the given group
+        // verify that schema from a group and a target schema are compatible
-        // updates the column mapping on all ObjectSchema properties
+        // updates the column mapping on all ObjectSchema properties of the target schema
-        // throws if the schema is invalid or does not match tables in the given group
+        // throws if the schema is invalid or does not match
-        static void verify_schema(Group *group, Schema &target_schema, bool allow_missing_tables = false);
+        static void verify_schema(Schema const& actual_schema, Schema &target_schema, bool allow_missing_tables = false);
 
         // updates the target_column member for all properties based on the column indexes in the passed in group
         static void update_column_mapping(Group *group, ObjectSchema &target_schema);
 
-        // determines if you must call update_realm_with_schema for a given realm.
+        // determines if a realm with the given old schema needs non-migration
-        // returns true if there is a schema version mismatch, if there tables which still need to be created,
+        // changes to make it compatible with the given target schema
-        // or if file format or other changes/updates need to be made
+        static bool needs_update(Schema const& old_schema, Schema& schema);
-        static bool realm_requires_update(Group *group, uint64_t version, Schema const& schema);
         
-        // updates a Realm to a given target schema/version creating tables and updating indexes as necessary
+        // updates a Realm from old_schema to the given target schema, creating and updating tables as needed
         // returns if any changes were made
-        // passed in schema ar updated with the correct column mapping
+        // passed in target schema is updated with the correct column mapping
-        // optionally runs migration function/lambda if schema is out of date
+        // optionally runs migration function if schema is out of date
         // NOTE: must be performed within a write transaction
         typedef std::function<void(Group *, Schema &)> MigrationFunction;
-        static bool update_realm_with_schema(Group *group, uint64_t version, Schema &schema, MigrationFunction migration);
+        static bool update_realm_with_schema(Group *group, Schema const& old_schema, uint64_t version,
+                                             Schema &schema, MigrationFunction migration);
 
         // get a table for an object type
         static realm::TableRef table_for_object_type(Group *group, StringData object_type);
@@ -88,10 +88,10 @@ namespace realm {
         // if update existing is true, updates existing tables, otherwise only adds and initializes new tables
         static bool create_tables(realm::Group *group, Schema &target_schema, bool update_existing);
 
-        // verify a target schema against its table, setting the table_column property on each schema object
+        // verify a target schema against an expected schema, setting the table_column property on each schema object
         // updates the column mapping on the target_schema
         // returns array of validation errors
-        static std::vector<ObjectSchemaValidationException> verify_object_schema(Group *group, ObjectSchema &target_schema, Schema &schema);
+        static std::vector<ObjectSchemaValidationException> verify_object_schema(ObjectSchema const& expected, ObjectSchema &target_schema, Schema &schema);
 
         // get primary key property name for object type
         static StringData get_primary_key_for_object(Group *group, StringData object_type);
@@ -118,7 +118,7 @@ namespace realm {
       public:
         ObjectStoreException() = default;
         ObjectStoreException(const std::string &what) : m_what(what) {}
-        virtual const char* what() const noexcept { return m_what.c_str(); }
+        const char* what() const noexcept override { return m_what.c_str(); }
       protected:
         std::string m_what;
     };
@@ -137,9 +137,9 @@ namespace realm {
 
     class DuplicatePrimaryKeyValueException : public MigrationException {
       public:
-        DuplicatePrimaryKeyValueException(std::string object_type, Property &property);
+        DuplicatePrimaryKeyValueException(std::string object_type, Property const& property);
         std::string object_type() { return m_object_type; }
-        Property &property() { return m_property; }
+        Property const& property() { return m_property; }
       private:
         std::string m_object_type;
         Property m_property;
@@ -166,36 +166,36 @@ namespace realm {
 
     class ObjectSchemaPropertyException : public ObjectSchemaValidationException {
       public:
-        ObjectSchemaPropertyException(std::string object_type, Property &property) :
+        ObjectSchemaPropertyException(std::string object_type, Property const& property) :
             ObjectSchemaValidationException(object_type), m_property(property) {}
-        Property &property() { return m_property; }
+        Property const& property() { return m_property; }
       private:
         Property m_property;
     };
 
     class PropertyTypeNotIndexableException : public ObjectSchemaPropertyException {
       public:
-        PropertyTypeNotIndexableException(std::string object_type, Property &property);
+        PropertyTypeNotIndexableException(std::string object_type, Property const& property);
     };
 
     class ExtraPropertyException : public ObjectSchemaPropertyException {
       public:
-        ExtraPropertyException(std::string object_type, Property &property);
+        ExtraPropertyException(std::string object_type, Property const& property);
     };
 
     class MissingPropertyException : public ObjectSchemaPropertyException {
       public:
-        MissingPropertyException(std::string object_type, Property &property);
+        MissingPropertyException(std::string object_type, Property const& property);
     };
 
     class InvalidNullabilityException : public ObjectSchemaPropertyException {
       public:
-        InvalidNullabilityException(std::string object_type, Property &property);
+        InvalidNullabilityException(std::string object_type, Property const& property);
     };
 
     class MissingObjectTypeException : public ObjectSchemaPropertyException {
     public:
-        MissingObjectTypeException(std::string object_type, Property &property);
+        MissingObjectTypeException(std::string object_type, Property const& property);
     };
 
     class DuplicatePrimaryKeysException : public ObjectSchemaValidationException {
@@ -205,9 +205,9 @@ namespace realm {
 
     class MismatchedPropertiesException : public ObjectSchemaValidationException {
       public:
-        MismatchedPropertiesException(std::string object_type, Property &old_property, Property &new_property);
+        MismatchedPropertiesException(std::string object_type, Property const& old_property, Property const& new_property);
-        Property &old_property() { return m_old_property; }
+        Property const& old_property() { return m_old_property; }
-        Property &new_property() { return m_new_property; }
+        Property const& new_property() { return m_new_property; }
       private:
         Property m_old_property, m_new_property;
     };

src/object-store/shared_realm.cpp

@@ -121,30 +121,36 @@ SharedRealm Realm::get_shared_realm(Config config)
 
     SharedRealm realm(new Realm(std::move(config)));
 
+    auto target_schema = std::move(realm->m_config.schema);
+    auto target_schema_version = realm->m_config.schema_version;
+    realm->m_config.schema_version = ObjectStore::get_schema_version(realm->read_group());
+
     // we want to ensure we are only initializing a single realm at a time
     static std::mutex s_init_mutex;
     std::lock_guard<std::mutex> lock(s_init_mutex);
-
-    uint64_t old_version = ObjectStore::get_schema_version(realm->read_group());
     if (auto existing = s_global_cache.get_any_realm(realm->config().path)) {
         // if there is an existing realm at the current path steal its schema/column mapping
         // FIXME - need to validate that schemas match
         realm->m_config.schema = std::make_unique<Schema>(*existing->m_config.schema);
     }
-    else if (!realm->m_config.schema) {
-        // get schema from group and skip validation
-        realm->m_config.schema_version = old_version;
-        realm->m_config.schema = std::make_unique<Schema>(ObjectStore::schema_from_group(realm->read_group()));
-    }
-    else if (realm->m_config.read_only) {
-        if (old_version == ObjectStore::NotVersioned) {
-            throw UnitializedRealmException("Can't open an un-initialized Realm without a Schema");
-        }
-        ObjectStore::verify_schema(realm->read_group(), *realm->m_config.schema, true);
-    }
     else {
-        // its a non-cached realm so update/migrate if needed
+        // otherwise get the schema from the group
-        realm->update_schema(*realm->m_config.schema, realm->m_config.schema_version);
+        realm->m_config.schema = std::make_unique<Schema>(ObjectStore::schema_from_group(realm->read_group()));
+
+        // if a target schema is supplied, verify that it matches or migrate to
+        // it, as neeeded
+        if (target_schema) {
+            if (realm->m_config.read_only) {
+                if (realm->m_config.schema_version == ObjectStore::NotVersioned) {
+                    throw UnitializedRealmException("Can't open an un-initialized Realm without a Schema");
+                }
+                ObjectStore::verify_schema(*realm->m_config.schema, *target_schema, true);
+                realm->m_config.schema = std::move(target_schema);
+            }
+            else {
+                realm->update_schema(std::move(target_schema), target_schema_version);
+            }
+        }
     }
 
     if (config.cache) {
@@ -153,47 +159,49 @@ SharedRealm Realm::get_shared_realm(Config config)
     return realm;
 }
 
-bool Realm::update_schema(Schema const& schema, uint64_t version)
+bool Realm::update_schema(std::unique_ptr<Schema> schema, uint64_t version)
 {
-    bool changed = false;
+    bool needs_update = !m_config.read_only && (m_config.schema_version != version || ObjectStore::needs_update(*m_config.schema, *schema));
-    Config old_config(m_config);
+    if (!needs_update) {
-
+        ObjectStore::verify_schema(*m_config.schema, *schema, m_config.read_only);
-    // set new version/schema
+        m_config.schema = std::move(schema);
-    if (m_config.schema.get() != &schema) {
+        m_config.schema_version = version;
-        m_config.schema = std::make_unique<Schema>(schema);
+        return false;
     }
+
+    // Store the old config/schema for the migration function, and update
+    // our schema to the new one
+    auto old_schema = std::move(m_config.schema);
+    Config old_config(m_config);
+    old_config.read_only = true;
+    old_config.schema = std::move(old_schema);
+
+    m_config.schema = std::move(schema);
     m_config.schema_version = version;
 
-    try {
+    auto migration_function = [&](Group*,  Schema&) {
-        if (!m_config.read_only && ObjectStore::realm_requires_update(read_group(), version, schema)) {
+        SharedRealm old_realm(new Realm(old_config));
-            // keep old copy to pass to migration function
+        auto updated_realm = shared_from_this();
-            old_config.read_only = true;
+        m_config.migration_function(old_realm, updated_realm);
-            old_config.schema_version = ObjectStore::get_schema_version(read_group());
+    };
-            old_config.schema = std::make_unique<Schema>(ObjectStore::schema_from_group(read_group()));
-            SharedRealm old_realm(new Realm(old_config));
-            auto updated_realm = shared_from_this();
 
-            // update and migrate
+    try {
-            begin_transaction();
+        // update and migrate
-            changed = ObjectStore::update_realm_with_schema(read_group(), version, *m_config.schema,
+        begin_transaction();
-                                                            [=](__unused Group *group, __unused Schema &target_schema) {
+        bool changed = ObjectStore::update_realm_with_schema(read_group(), *old_config.schema,
-                                                                m_config.migration_function(old_realm, updated_realm);
+                                                             version, *m_config.schema,
-                                                            });
+                                                             migration_function);
-            commit_transaction();
+        commit_transaction();
-        }
+        return changed;
-        else {
-            ObjectStore::verify_schema(read_group(), *m_config.schema, m_config.read_only);
-        }
     }
     catch (...) {
         if (is_in_transaction()) {
             cancel_transaction();
         }
         m_config.schema_version = old_config.schema_version;
-        m_config.schema = std::move(old_config.schema);
+        m_config.schema = std::move(m_config.schema);
         throw;
     }
-    return changed;
 }
 
 static void check_read_write(Realm *realm)

src/object-store/shared_realm.hpp

@@ -75,7 +75,7 @@ namespace realm {
         // on the Config, and the resulting Schema and version with updated
         // column mappings are set on the realms config upon success.
         // returns if any changes were made
-        bool update_schema(Schema const& schema, uint64_t version);
+        bool update_schema(std::unique_ptr<Schema> schema, uint64_t version);
 
         static uint64_t get_schema_version(Config const& config);