"FCModel 2" non-unique-instances branch

[marcoarment]

Just pushed the first commit e0f9b3d3 of what might be called "FCModel 2" to the "non-unique-instances" branch. This is a big, likely-breaking change that applies a lot of lessons learned since the original design:

Instances are no longer tracked or kept unique in memory. All instances are "detached".

• You can have two instances in memory of the same logical model (same table and primary key).
• Changes made to a model object or table won't affect any other copies in memory.
• If you want your interface to react to changes, you must reload any data in response to FCModel's change notifications.
• dataWasUpdatedExternally, reloads, and conflict resolution are no longer necessary and have been removed.
• allLoadedInstances is no longer possible and has been removed.
• Transactions are now supported, although limited. (Rollbacks don't revert changed models in memory.)
• Autoincrement, formerly deprecated, is now removed and will raise an exception.

All saves and deletes are expected to succeed. Saves shouldn't unexpectedly fail or be blocked.

• Removed should/didInsert/Update/Delete, saveWasRefused, saveDidFail. To customize behavior, override save and call [super save] from the subclass.
• FCModelSaveResult is removed. delete now returns void and save now returns a BOOL to indicate whether changes were made.
• Saves and deletes that fail from database errors now raise exceptions. lastSQLiteError is removed.
• All FCModel exceptions now have name FCModelException.
• saveAll has been removed. Saves should happen intentionally, right after you change the data.
• Custom serializations for NSURL, NSDate, etc. have been removed to avoid a lot of subtle bugs and inconsistencies between formats.

Notifications have been simplified to just a single FCModelChangeNotification for any change to a table.

• FCModelInstanceSetKey has been replaced by FCModelInstanceKey and will always be one instance if known, or unset for many/unknown.
• Notification-batching functions are now redundant and have been removed since transactions also batch notifications.

Take a look and see what you think so far. What other changes should we consider now that we're breaking things?

PLEASE don't ship any apps with this yet.

[NZKoz]

Most of these changes seem great, detached instances is just a much simpler model, and we had some truly weird bugs caused by the uniquing.

However I'm curious what the rationale for killing should/didInsert/Update/Delete and FCModelSaveResult.

We rely on should/did persist to update some cache columns and do some full text search related changes:

-(void) didPersist {
    [SeachThingy addOrUpdateDataForAudioItem:self];
}

As for FCModelSaveResult, we import some data from an API which ... sometimes does stupid things. In particular values which "absolutely completely totally will never be null" are frequently null. We rely on this behaviour at present to know which stuff we should just 'drop on the floor' for a while. We have a tiny macro which emulates the proposed behaviour (approximately):

#define ASSERT_SAVE(obj) NSAssert2([obj save] == FCModelSaveSucceeded, @"Failed to save object %@ at line %i", obj, __LINE__)

The vast bulk of our save calls don't call it.

[marcoarment]

@NZKoz The main goals for the should/didInsert/Update/Delete and FCModelSaveResult removal were:

• Reduce the number of states an object can exist in, and discourage keeping objects around with unsaved changes for very long.
• Assure the caller in more cases that save will always save and delete will always delete.
• Reduce the number of methods you need to look for/at if saves and deletes aren't behaving as expected. The simple, logical place to do these customizations is now just by overriding save and calling [super save] from it as necessary.

For instance, I have an OCFeed model in Overcast. It can exist in two modes defined by an isTransient property. Transient instances are temporary instances from the API (like when browsing the directory) and should not be saved in the local database. Separately, when a locally saved OCFeed is deleted from the database, all of its OCFeedItems should be deleted as well. So I had this code previously in OCFeed:

- (BOOL)shouldInsert { return ! self.isTransient; }
- (BOOL)shouldUpdate { return ! self.isTransient; }
- (BOOL)shouldDelete { return ! self.isTransient; }

- (void)didDelete
{
    [OCFeedItem deleteAllItemsInFeedID:self.id];
}

Here's the new code under "FCModel 2":

- (BOOL)save
{
    if (self.isTransient) return NO;
    return [super save];
}

- (void)delete
{
    [super delete];
    [OCFeedItem deleteAllItemsInFeedID:self.id];
}

You can still do everything you could do before, but I think it makes more sense this way, especially if you're new to FCModel or you've forgotten how it works since you wrote the code 6 months ago and don't know to look for those should/did methods when you're trying to figure out why something's behaving weirdly.

[NZKoz]

I'm neither here nor there on the should methods, the only case I call them was to hook the lifecycle event, they all return YES.

The callbacks for did were a convenient place to stash behaviour without running the, admittedly low, risk of forgetting to call super. I suppose it'd be neither here nor there if they were removed, but most libraries provide these lifecycle hooks

However I can speak from experience that providing developers with those callbacks will be both a blessing and a curse, there's tonnes of brittle rails code caused by callback-mystery-wtf-happenedcode.

[NZKoz]

One other thing, your decision to not try and rollback object state on transaction failure is the right one. We do that and it's essentially impossible to do correctly leading to buggy code with 500 special cases.

[madninja]

Anything I can do to help move this forward? I'm going to want to use non-unique instances (right approach and we actually have an extension writing to the db). Are there gotchas or general directional things you'd rather not see?

[marcoarment]

Sorry, I've been busy trying to get the latest Overcast update out the door.

I'm going to be tackling this in the next major update. I'll be writing the FCModel2/whatever system along with it, probably a couple of months from now.

I have a branch of the Overcast code running with this FCModel branch, and there are still a few big things I'd like to change.

[marcoarment]

Made a few improvements tonight:

• Cross-process data-change triggers so you can have read/write iOS extensions (I think).
• All variadic functions are gone for Swift friendliness and general code/complexity reduction. This is an experiment, not a strong opinion. Let me know if you miss them a lot.

[jlnr]

You could probably support variadic methods and keep FCModel.m short by having every method accept a va_list. Then introduce an Objective C category FCModel+VarArgs that provides a variadic forwarder for each method using C varargs, and a Swift category that does the same using withVaList(). Disclaimer: I haven't tried this in Swift, only seen it on StackOverflow.

That said, NSArray is a lot safer than varargs. I'll miss FCModel's variadic methods, but not "a lot" :)

[marcoarment]

I already hate it without variadics. Putting them back.

[marcoarment]

Just committed fc5b4686 with restored variadic SELECT functions (now coexisting with argument-array variants of each one).

I also removed the "keyed" versions that returned dictionaries (too much code complexity/duplication for too little gain) and the functions that operated on existing FMResultSet objects (too little use, might as well minimize the public header's reliance on FMDB).

[marcoarment]

@jlnr Just committed 7b37ea9b with va_list variants. I don't know enough about building with Swift to integrate that Stack Overflow technique properly in a build and test it — any help/pulls on that are welcome.

[jlnr]

@marcoarment I've played around with my own suggestion now and I'm not happy with it.

I thought it'd be clever to drop all NSArray variants and use only va_list internally, as the lowest common denominator on which variadic methods can be built in Objective C and Swift. That would get rid of the branching everywhere (as in #108), and on a first glance I think it looks cleaner: failed refactoring here.

The whole va_start, va_arg, va_end dance could also be moved into a separate file to clean up FCModel.m even more, like this.

But some methods like +[FCModel instancesWithPrimaryKeyValues:] or +[FCModel cachedInstancesWhere:arguments:ignoreFieldsForInvalidation:] only make sense with NSArray, so that destroys the beauty of a pure va_list approach.

And since you have already added NSArray variants for all FCModel methods, there's probably no good reason to add va_list variants in addition to that. Variadic methods in Swift can be built on top of NSArray arguments just as easily as on va_list (if that's really necessary, passing arrays looks nice enough in Swift). And using NSArray is even a little safer because you can't accidentally pass a raw int or too few arguments (which FMDB would catch). So I think the interface before https://github.com/marcoarment/FCModel/commit/7b37ea9b178f167a20b3afca90b532db5e0ac6a1 is just fine, for both Swift and Objective C. :+1:

[marcoarment]

Thanks for looking into that. I just removed the va_list variants. Also added basic in-memory caching by primary key (but using copies, not shared instances) and a few other fixes.

Here's another (crazy?) idea: Can/should we drop the custom serialization/unserialization for things like NSDate and NSURL and just do basic strings and numbers (which FMDB does anyway)?

Conceptually, it has always been problematic (and a big source of bugs — see all of the pull requests on those) because SQLite doesn't naturally handle those datatypes, and there's issues like what happens if a non-NULL column reads in an invalid URL string from the database that results in a nil NSURL.

I'd love to get rid of it and just leave devs to implement their own serializations as necessary in didInit and the new save-overriding convention.

[johncblandii]

NSDate handling is the single most common crash in our app. I'm 100% fine w/ keeping them as strings. If we need to, we can always provide a helper method.

[johncblandii]

On top of that, you could provide an update in the README showing how a simple category could be used to add serializing OR, even sweeter, allow a way for use of Mantle or similar.

[marcoarment]

Alright, I'm going to run my usual test on serialization removal: trying to convert Overcast to it and seeing how much it sucks.

[johncblandii]

:+1:

[marcoarment]

It was actually pretty easy (9173939b). What do you think?

[johncblandii]

I'll test it out first thing in the morning but with a cursory view it looks good.

[lickel]

Could -serializedDatabaseRepresentationOfValue:forPropertyNamed: and its converse be kept, but remove the default implementation? That allows for easy transforms, but requires subclasses to handle URLs and Dates.

[marcoarment]

I don't know — I think the whole concept was flawed. Object properties that map to database columns should always reflect what those column values are or should be. I think the role of serialization is better left to implementors and custom methods.

[ilyannn]

That's a reasonable point about object properties. But I'm thinking about providing the serialized accessors automatically via +resolveInstanceMethod:, does that make sense for FCModel?

[lickel]

That's certainly a fair point, though it's not uncommon for model frameworks to do trivial value transformations.

The downside of not having transforms is that you push statefulness down to subclasses. There's certainly a place for that; for example, I save server-defined enums as strings and expose an enum value or SomeEnumTypeUnknown.

Unfortunately, it adds a lot of boilerplate code for common patterns. In order to maintain KVO compliance, you're kind of left with one of:

• Lazy/shim properties and +keyPathsForValuesAffectingFoo methods
• Override both setters and update both underlying ivars (which may not even be an option in swift)

It's not terrible, but it's additional work that could be avoided if subclasses could perform transformations when a snapshot is made to generate the INSERT or UPDATE.

[marcoarment]

FWIW, I just fixed the last known Overcast bug with using this branch — doing the conversion was surprisingly easy. There were only a few spots in the code where I was relying on instances being unique in memory. I'm going to ship the next update with this codebase after a (hopefully brief) beta.

I think it's time to give this a home and make it official. What do you think is the right way to do this? New "FCModel2" project? Or just release it as v2.0 and hope people are paying attention?

[NZKoz]

If people are using cocoapods correctly, they'll be manually opting in to the update. It'll only get bumped if they say pod update FCModel and haven't tied themselves to an earlier version with something like this in their Podfile

pod 'FCModel', '~> 1.0.0'

I'd just push the 2.0 version, but maintain a 1-0-stable branch or something on the off chance that people notice a bug and want it patched.

[jasonsilberman]

I agree with @NZKoz because then if you absolutely need to use an older version you still can.

[mirion]

I think that the framework should be able to offer support for easy values transformation. As @lickel mentioned above, this is a highly repetitive task. Indeed not having it is not terrible but annoying and for me it is close to bad.

This is related to another aspect: for the time being the data model is created through inspection of the database table and class structures. This is not always very confortable. It would be nice to declare a sort of descriptor for a given pair class/table that is able to map attributes to columns. This descriptor may also declare the transformers. But before this descriptor, restoring the ability to transform data is (in my opinion) something really necessary.

[ahti]

I agree with @mirion that value transformations would be nice to have.

Even though I removed my one implementation of -unserializedDatabaseRepresentationOfValue: (in one of ten model classes) recently (see #104), I still rely on the current implementation handling NSDate for me, and think writing all the boilerplate to handle transforming properly would be annoying and error-prone.

I think an approach similar to how CoreData handles this could turn out nicely. Basically it allows you to specify a NSValueTransformer, and by default uses a transformer that knows how to handle objects implementing NSCoding.

The NSCoding part would cover all the Classes the old implementation handled, plus a big bunch of others.

[andreyvit]

I just went to open a ticket asking about typical expected FCModel concurrency patterns, and found this.

My two cents:

1. Loving FCModel, been using it a lot since last summer. (Also loving Overcast and atp.)
2. Detached instances are a great idea, because otherwise there's no convenient way to perform background operations involving DB instance modifications while the UI keeps reading them. I actually wonder how Overcast handled this before. Did you make all your properties atomic and rely on no fatal inconsistencies arising along the way?

3. I love built-in serialization. I've actually used those overridable methods to customize serialization formats in the past, to support my own calendar-related class that acted like a value. NSDates serialized as numbers can probably always be parsed back, no nil issues there.

   I disagree that wrapper properties result in a simpler approach overall, compared to serialization and deserialization methods. Like @mirion said, it's a highly repetitive task that is worth automating.

   I also disagree that removing serialization is a correct approach conceptually. I am storing a value whose domain is e.g. “anything representable by NSURL”. The fact that I can't make some sort of a check constraint on SQLite side to enforce the domain doesn't mean that I cannot rely on the fact that the values are correct; I'm just pushing the enforcement to the application layer, and FCModel should make sure that anything that can be serialized can also be deserialized.

   If someone bypasses FCModel serialization and puts an incorrect value into their database, they've destroyed the integrity of their data and should enjoy a crash when reading it back.

4. Releasing as v2.0 of the pod makes perfect sense, and is the only right approach. Major version changes are supposed to introduce breaking changes.

I wonder if Overcast is already shipping FCModel 2.0? Any particular reason why we shouldn't ship our apps with it as well, provided that we're prepared to debug the problems?

[andreyvit]

Oh, one more thing. I'm using those keyed access methods a lot; am I the only one? Of course, this is a minor issue (I can use my collection grouping categories, or I can add a category to FCModel), but I wonder why @marcoarment finds them to bring “too little gain”.

My use case is bulk-fetching of data (often on a background queue) before processing it (on the main queue).

[mirion]

Some more comments:

1. I missed the removal of instancesFromResultSet. Since FCModel is not providing any method to execute a random query and extract the results into an array, I'm finding this decision very inappropriate. Also the argument that it simplifies the imports is superfluous because FCModel already needs DMDatabase.h
2. Also, another unfortunate decision, why raise exceptions? This kind of error management is completely useless for those using this library in Swift. And beyond Swift, raising exceptions is a little bit "oldie". Why change the errors management when the library that FCModel wraps is using error codes?

FCModel is a highly useful library, please don't dumb it down. Of course, we can fork, but it is not the same. Thanks

[marcoarment]

Sorry, been traveling a lot and haven't had time to respond. Some quick notes, though:

@mirion 1: The general idea here was to ensure that every column is always selected (SELECT *), with no joins or aliases or anything that could mess up the decoding of the result set into saturated model instances. This is less important to core functionality since the move to non-unique instances, but it's still important for caching opportunities and avoiding weird bugs.

@mirion 2: We're using exceptions idiomatically like Cocoa: they're intended to signify errors during development, are not intended to be caught, and are expected not to occur in normal operation of the app. Error codes, NSError out params, etc. are for more common, expected, and/or recoverable errors.

@andreyvit: My idea for removing serialization has not been well-received, and some counterarguments are strong. I'm considering the use of NSValueTransformer to handle custom serialization needs, with FCModel likely providing default implementations for NSURL and NSDate like before, but in a better-structured way.

Overcast has been shipping with this branch since version 1.1.2 a few months ago. I had some concurrency bugs initially, but the move to using reloadAndSave for all writes fixed them all and has been rock-solid. It's so good that I'll likely remove external access to save entirely, forcing all writes to go through reloadAndSave, since it's so much better and eliminates a lot of potential bugs.

I'd love for all model instances to be immutable outside a reloadAndSave context, but I'm not sure if there's a great way to do that without clunky hacks or custom code in each model subclass. Ideally, the properties would all be declared readonly so Xcode and the compiler would enforce it, but that makes writes potentially hacky. Suggestions welcome.

I'm also considering bringing back one of the benefits of unique instances in a way, but optionally. We'd have something like a persistentCopy method on each model instance that would keep itself updated and have its properties be KVO-observable.

So before this is merged and declared 2.0, I'd like to experiment with:

• NSValueTransformer for value serialization
• Immutable-model possibilities
• persistentCopy

...which I'll start doing shortly. Again, suggestions welcome.

[ahti]

Regarding NSValueTransformer things: CoreData uses, by default, a reversed version of [NSValueTransformer valueTransformerForName:NSKeyedUnarchiveFromDataTransformerName], so that would probably be all that's needed for a default implementation by FCModel.

[johncblandii]

How's this looking @marcoarment? I've been riding on "1.0" but am seeing some bad performance for larger data sets (really not that big, less than 100 objects) and hoping this could help.

Any updates?

[johncblandii]

Dude, integrated non-unique and the performance went from taking 40 seconds to save everything to 1.4s. Yummy!

[mirion]

In fact, your problem is triggered by the huge number of notifications that are generated by FCModel. Just modify the code to disable the notifications. II already have an implementation, if you need it, let me know and I will make a pull request.

[johncblandii]

The timing issues were because of dataWasUpdatedExternally. The biggest FCModel time suck now is around notifications but it is sub-500ms so I'm not sweating it too much right now.

[mirion]

@marcoarment are you still maintaining FCModel 1? If yes, I have a several corrections into my fork. If you want to integrate something, please let me know to make PRs.

[johncblandii]

No, I'm on non-unique now and not turning back! :-D

[mirion]

@johncblandii Indeed everything starts with dataWasUpdatedExternally, but dig deeper, you will see that the problem is eventually produced by the notifications. I profiled the code.

In my opinion, (at least for my use case), v2 has some big flaws and is not ready for prime time. Your experience my be different.

[johncblandii]

It isn't a full release so I expect some issues (like the dropping of serialization which I'm addressing now) but overall the performance is worth it. Seeing as Overcast is using it in a large way, I'm confident I can make it work for this app.

[marcoarment]

@mirion: I don't plan any more updates to v1 — v2 is the way forward. I'd love to hear the flaws you've identified in it.

[mirion]

@marcoarment I already mentioned my problems:

1. the removal of instancesFromResultSet. This is useful for complex queries, for example for FTS. I think that removing functionality in order to protect the developers is not really the right path.
2. the use of exceptions on save/delete. For me this is a very big issue. You may get runtime errors for example related to PKs or other constraints that are not predictable during the development phase. For example after a sync into a distributed environment, etc.
3. the serialization issue

There were others, but I can't remember right now. Will write again if I'll remember. In my case, all these issues are blocking. It looks that for others the implications are much smaller.

Regarding v1, I have some corrections/improvements that may be useful for v2: https://github.com/mirion/FCModel/tree/mirion_dev/fix_inExpectedWrite https://github.com/mirion/FCModel/tree/mirion_dev/write_transaction_on_queue and also views support: https://github.com/mirion/FCModel/tree/mirion_dev/views_file_support

[johncblandii]

I'm curious @mirion, are you trying to use a remote value as a PK?

[marcoarment]

Sounds like we just disagree on the meaning of "has some big flaws and is not ready for prime time". I took that to mean bugs, while you appear to be referring to features and implementation details on which we disagree.

[lynns]

@marcoarment Do you have a sense for when the non-unique branch will be ready for production use? We're anxious to switch over but have been holding off for the final changes to be done before we do. Is the branch pretty stable as is or should we keep holding off for a while longer?

Thanks for all the work you've done on this, we've really enjoyed using FCModel in our apps.

[marcoarment]

@lynns I haven't had time to formally publish and document it as such, but I consider this branch stable for production. I've been shipping it in Overcast for months with no problems.

Once WWDC settles down, I'll make it formal.

[lynns]

@marcoarment Sounds good to me. We'll start trying it out and see how it goes for us. Thanks!

[johncblandii]

@lynns I ported to non-unique last night and I can't speak highly enough about it. You'll need to address serialization (I just used setValue:forKey: to deal w/ one date issue) and that was pretty much it.

It's well worth at least a spike.

@marcoarment, I think the community could help document it. Hopefully these projects wind down in the next week or so and I can lend a hand in that area.

[mirion]

Please don't understand me wrongly. My words may sound too strong, I really think that you did a great job with this library. I like it, I'm using it and thanks a lot for all your work. If I'm commenting your decisions is not to criticize you, but just because I like your code, I want to use it in the future, therefore I want to see it in the best shape for as many of us (including for advanced use), not just for some cases.

I used the word "flaw" because some design decisions (every api that was removed represent in the end a design decision) are beyond the term "bugs". Of course, these design decisions are in the end easy to revert and solve the bugs ;-)

@johncblandii I had a problem (in v1) with records sent from the server, where because of some networking problems, the server was pushing them two times. Just a pipe problem, completely unpredictable.