CoreStore

CoreStore copied to clipboard

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.

Hey, apologies for the delay, have been away. Let's get this rolling!

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.

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 renamingIdentifiers for, both, attributes and entities.

It's hard to overlook issues with renaming identifiers when using dynamic schemas, but very easy when using xcdatamodel files. What's worse is once such a schema makes it into production there's no way back. This is why it's important to stick with the CoreData approach, but perhaps it would also make sense to log warnings in debug builds when renaming identifiers cannot be mapped.

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

It's hard to overlook issues with renaming identifiers when using dynamic schemas, but very easy when using xcdatamodel files.

I would recommend you use XcodeSchemaMappingProvider or even InferredSchemaMappingProvider instead. Those build on the foundations that standard Core Data migration provide by default.

CustomMappingProvider on the other hand was designed mainly for CoreStoreObject schemas, which do not support xcmappingmodel files or even NSEntityMigrationPolicy classes. The biggest point is that CoreStoreObject schema's are designed to be used with Progressive Migration, that is, the MigrationChain is explicitly defined for all target model versions. So regarding the handling of renamingIdentifiers, there is little benefit to simulating the default Core Data handling because the flow of transformations are known. On that same reasoning, since transformations are well-known and well-defined during compile time via SchemaHistory, there should never be "optional" mappings. I guess one thing we can improve here is to allow CustomMappingProvider only if the destination model uses CoreStoreObject entities.

I do understand that this is a pretty strict decision for a library, but first let me know if XcodeSchemaMappingProvider or InferredSchemaMappingProvider helps you.

What's worse is once such a schema makes it into production there's no way back. This is why it's important to stick with the CoreData approach, but perhaps it would also make sense to log warnings in debug builds when renaming identifiers cannot be mapped.

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 do understand that this is a pretty strict decision for a library, but first let me know if XcodeSchemaMappingProvider or InferredSchemaMappingProvider helps you.

I replaced Xcode migrations with CustomSchemaMappingProvider-based ones, so no way to test this out now. At the moment I work around the described issues by providing custom transformers instead of CustomMapping.inferredTransformation, which would crashes, otherwise.

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 CustomMappingProvider, which are beyond awesome. These days and starting afresh I might have gone for the CoreStoreObject, but xcdatamodeld files are here to stay and since CoreStore already works alongside with Core Data, I still feel this PR has some rationale behind it. Especially the part with explicit unwrapping and throwing proper errors.

Going forward, I can keep the error handling part and we can take the rest into a separate PR if needed. As an Idea, CustomMapping.inferredTransformation can have an option for the default strict mode or to follow the Core Data behaviour in graceful mode.