Fix for Issue 648

[24601]

Improved fix for #648

Adding a "bottom of the totem pole" check for generating methods for getter implementation in unhandled cases of NSObject subclasses.

Essentially, defaults to "Nil" behavior if we can't get a better/more specific implementation for NSObject subclasses.

[snej]

This patch would let you create a property with any object class and it would try to store the value in the document, whether or not it can be serialized to JSON. That will create errors from NSJSONSerialization trying to save, which will confuse people (we already get a fair number of questions about this from people storing things into CBLDocuments directly.)

For example, you could declare a UIView* valued property and CBLModel wouldn't complain.

A correct fix should only allow the case where the class is Nil (i.e. id) because that's what you get if you use an interface type.

[24601]

@snej, certainly makes sense, and I'm eager to rework this to accommodate the very understandable and proper failure case you mention - I am struggling with understanding then how to handle this, because there is already a nil check, but that doesn't appear to be sufficient to avoid the bug demonstrated in https://github.com/24601/CBL-Bug-Demo, but code with my PR does appear to fix the issue, in cases where class is Nil, the existing code checks for this early, this appears to be a case where class is not Nil yet not caught by any of the intervening code and the last catch-all else treatment isn't appropriate.

It is certainly possible there is a different failure mode/pathology at play here and I'm misunderstanding the execution paths and the state machine - I am a novice in the insides of couchbxse-lite-ios (but an eager one :) ), and am happy to better understand what is going on how the correct fix should be implemented - I am simply struggling to envision that given the existing (seemingly early & proper) check for Nil and that the final catch-all else treatment creating this crash, whereas the PR (which, you are right, creates other issues/failure situations that I agree we should avoid) does appear to fix this issue.

Bottom line - I'm all ears, especially with the disclaimer that my understanding of what is going on here (and why this problem is fixed by my PR) may very well be flawed.

[snej]

Can you send me some sort of test case that triggers the issue? It's not showing up in any unit tests even though I've tried to address the situation. It might have something to do with slight differences in the metadata of a class implemented in Swift.

[24601]

Hi @snej - absolutely, from my comment above, I have created a small app that demonstrates this error and is available at https://github.com/24601/CBL-Bug-Demo, is that sufficient?

[snej]

The app doesn't build, because it needs a module "FXForms".

But I think I see what the problem is — the property that causes the error is

    @NSManaged var anArbitraryString : String

String isn't a class known to Objective-C. Try changing it to NSString. (The two classes are bridged, but they're not the same class; it's not like NSString and CFString for example.)

[snej]

Ah, I also found the remaining problem in CBLModel — the code to generate the getter method doesn't have the same fix for NSObject-typed properties that the setter does. And that wasn't caught because the unit test was only setting the id-valued property, not getting it.

I can't check in the fix right now because I'm working on a branch, but I'll check it in sometime today. Here's the patch:

diff --git a/Source/API/CBLModel+Properties.m b/Source/API/CBLModel+Properties.m
index 62e1290..480d409 100644
--- a/Source/API/CBLModel+Properties.m
+++ b/Source/API/CBLModel+Properties.m
@@ -201,7 +201,7 @@ static ValueConverter arrayValueConverter(ValueConverter itemConverter) {
 + (IMP) impForGetterOfProperty: (NSString*)property ofClass: (Class)propertyClass {
     id (^impBlock)(CBLModel*) = nil;

-    if (propertyClass == Nil) {
+    if (propertyClass == Nil || propertyClass == [NSObject class]) {
         // Untyped
         return [super impForGetterOfProperty: property ofClass: propertyClass];
     } else if (propertyClass == [NSString class]
diff --git a/Unit-Tests/Model_Tests.m b/Unit-Tests/Model_Tests.m
index 09dbe0a..a816f9d 100644
--- a/Unit-Tests/Model_Tests.m
+++ b/Unit-Tests/Model_Tests.m
@@ -344,6 +344,7 @@
         AssertEqual(model.url, url);
         AssertEq(model.other, model3);
         AssertEqual(model.others, (@[model2, model3]));
+        AssertEqual(model.untypedObject, @"this is an id");

         // Save it and make sure the save didn't trigger a reload:
         AssertEqual(db.unsavedModels, @[model]);

[zgramana]

@snej can you please close this, or comment on what needs to happen next?

[snej]

Looks like I accidentally left this open despite having committed the above fix on 4 May as eb4ff92.