Improve custom schema mapping provider #308
Conversation
iby
commented
Mar 16, 2019
iby
commented
- Improve property and relationship mapping.
- Throw errors when property or relationship cannot be mapped, don't explicitly unwrap.
- Add extensive schema mapping and migration tests.
Simplify the code structure and remove autorelease blocks – they introduce a certain overhead and only make sense on loops with a large number of iterations and memory-intense data that should be disposed at the end of the block. In our case, in 99% cases there will be no more than 10 iterations (attributes and relations per entity) and most results of autorelease blocks are returned and stored into the mapping model, which defies their whole purpose in the first place.
1. This is causing a crash. 2. Based on the documentation it's not clear what it's doing here anyway.
1. Improve property and relationship mapping. 2. Throw errors when property or relationship cannot be mapped, don't explicitly unwrap. 3. Add extensive schema mapping and migration tests.
There was a problem hiding this comment.
I appreciate the PR, thanks! I also apologize for not having the time to fully review your changes for quite a while.
I'm not sure we have the same presumption of how renamingIdentifiers should work. Please take a look at my comments.
Also as much as possible, please give some examples of the use cases where the previous migration logic fails and which this PR resolves.
| sourceModel: sourceModel, | ||
| destinationModel: destinationModel | ||
| ) | ||
| // Todo: What is this? Why is it needed? How does it work? |
There was a problem hiding this comment.
Please try to remove ToDos and if possible keep formatting changes to a minimum.
| entityMapping.sourceEntityName = sourceEntity.name | ||
| entityMapping.sourceEntityVersionHash = sourceEntity.versionHash | ||
| entityMapping.mappingType = .removeEntityMappingType | ||
| entityMapping.sourceExpression = expression(forSource: sourceEntity) |
There was a problem hiding this comment.
The FETCH expression above is important. Is there any reason this was removed?
There was a problem hiding this comment.
Note here: If the migration is a lightweight migration, the database will not be recreated so old records need to be deleted explicitly.
| return relationshipMappings | ||
| } | ||
| entityMapping.attributeMappings = attributeMappings | ||
| entityMapping.relationshipMappings = relationshipMappings |
There was a problem hiding this comment.
This looks like it's doing the exact same thing as the previous code. Please try to avoid refactoring if there are no changes in behavior.
| return relationshipMappings | ||
| } | ||
| entityMapping.attributeMappings = attributeMappings | ||
| entityMapping.relationshipMappings = relationshipMappings |
| entityMapping.sourceEntityName = sourceEntity.name | ||
| entityMapping.sourceEntityVersionHash = sourceEntity.versionHash | ||
| entityMapping.destinationEntityName = destinationEntity.name | ||
| entityMapping.destinationEntityVersionHash = destinationEntity.versionHash | ||
| entityMapping.mappingType = .customEntityMappingType | ||
| entityMapping.sourceExpression = expression(forSource: sourceEntity) | ||
| entityMapping.entityMigrationPolicyClassName = NSStringFromClass(CustomEntityMigrationPolicy.self) | ||
| entityMapping.relationshipMappings = relationshipMappings |
| } | ||
|
|
||
| @nonobjc | ||
| internal func cs_resolveRelationshipNames() -> [String: (relationship: NSRelationshipDescription, versionHash: Data)] { |
There was a problem hiding this comment.
Same comment above regarding cs_resolveAttributeNames
| return self.relationshipsByName.reduce(into: [:], { (result, relationship: (name: String, description: NSRelationshipDescription)) in | ||
| result[relationship.description.renamingIdentifier ?? relationship.name] = (relationship.description, relationship.description.versionHash) | ||
| /// Maps relationships of the current entity with the earlier-version source entity. Todo: Log a warning if the renaming identifier is used but not found in source. | ||
| internal func mapRelationships(in sourceEntity: NSEntityDescription) throws -> [NSRelationshipDescription: NSRelationshipDescription] { |
There was a problem hiding this comment.
Same comment above regarding mapAttributes
| @@ -231,7 +231,7 @@ class SetupTests: BaseTestDataTestCase { | |||
| ) | |||
| try sqliteStore.cs_eraseStorageAndWait( | |||
| metadata: metadata, | |||
| soureModelHint: stack.schemaHistory.schema(for: metadata)?.rawModel() | |||
| sourceModelHint: stack.schemaHistory.schema(for: metadata)?.rawModel() | |||
There was a problem hiding this comment.
Thanks for pointing out this typo. Personally I think it should be in a separate PR though
|
|
||
| var map: [NSAttributeDescription: NSAttributeDescription] = [:] | ||
| XCTAssertNoThrow(map = try dst.mapAttributes(in: src)) | ||
| XCTAssertEqual(map.count, 1) |
There was a problem hiding this comment.
In my understanding, in this case mapAttributes() should throw. The new dst.bar should be completely unrelated to src.bar (dst.bar should be linked to a src.foo)
|
|
||
| class Bar: CoreStoreObject { | ||
| var firstName = Value.Required<String>("firstName", initial: "", renamingIdentifier: "name") | ||
| var lastName = Value.Required<String>("lastName", initial: "", renamingIdentifier: "placeholder") |
There was a problem hiding this comment.
This should never migrate successfully as there is no "placeholder" key in the source entity.
|
Hey, apologies for the delay, have been away. Let's get this rolling!
I'm sure we don't, otherwise this PR wouldn't exist! 😄 I've migrated an existing app to CoreStack from using native migrations, both, lightweight and ones employing mapping models. It turned out there were a few schemas where renaming identifiers were copied over and were not removed across different versions. Standard CoreData tools would gracefully handle such cases, while CoreStack would crash. I described this in #300 and made a #301 PR, which partly addressed the issue, but it turned out it's a little deeper. Currently CoreStack assumes that renaming identifiers, if present, are correct and does explicit unwrapping. Instead, I believe, it should follow what CoreData does and check whether a renaming identifier matches an attribute or entity from a previous version and only then create a mapping. Otherwise, it should fallback to current attribute/entity name or throw an error if mapping cannot be created explaining why the migration failed, but never just crash. So, this is what this PR is about – it attempts to match CoreData's graceful handling of It's hard to overlook issues with renaming identifiers when using dynamic schemas, but very easy when using If you're happy with that, I'll review the rest of the feedback. |
|
@JohnEstropia still hoping we can look over this on some sad past-summer evening? 🍁😄 Would very much appreciate your input. |
|
@ianbytchek Hi, I've been meaning to respond to this but simply keep forgetting. Sorry
I would recommend you use
I do understand that this is a pretty strict decision for a library, but first let me know if
I'm not sure the problem will be avoided by just following Core Data's approach. Even after a schema makes it into production the migration can always be controlled in new versions. (If you do not have the schema for the old one you cannot migrate from it anyway) The one thing we can possibly improve here is to provide a way to automatically create migration unit tests given the schema models. |
|
If this the ideological thing then it's hard to argue. I'll give you that.
I replaced Xcode migrations with Xcode data models seemed, both, easier and more elegant back at the time, unlike the migration mappings. So, I kept the models, but moved to Going forward, I can keep the error handling part and we can take the rest into a separate PR if needed. As an Idea, |
