Better handling of missing revision bodies. Fixed a crash.

Fixes #1234
couchbase · May 4, 2016 · a0975fc · a0975fc
1 parent 51b723e
commit a0975fc
Show file tree

Hide file tree

Showing 7 changed files with 55 additions and 31 deletions.
diff --git a/Source/CBLDatabase+Insertion.m b/Source/CBLDatabase+Insertion.m
@@ -317,8 +317,9 @@ - (instancetype) initWithDatabase: (CBLDatabase*)db
 
 
 - (CBL_Revision*) current_Revision {
+    CBLStatus status;
     if (_currentRevision)
-        _currentRevision = [_db revisionByLoadingBody: _currentRevision status: NULL];
+        _currentRevision = [_db revisionByLoadingBody: _currentRevision status: &status];
     return _currentRevision;
 }
 

diff --git a/Source/CBLDatabase+Internal.m b/Source/CBLDatabase+Internal.m
@@ -526,11 +526,12 @@ - (CBL_Revision*) getDocumentWithID: (NSString*)docID
 
 
 - (CBLStatus) loadRevisionBody: (CBL_MutableRevision*)rev {
-    // First check for no-op -- if we just need the default properties and already have them:
+    // First check whether we already have the body (or know it's missing) and the sequence:
     if (rev.sequenceIfKnown) {
-        NSDictionary* props = rev.properties;
-        if (props.cbl_rev && props.cbl_id)
+        if (rev.body)
             return kCBLStatusOK;
+        else if (rev.missing)
+            return kCBLStatusNotFound;
     }
     Assert(rev.docID && rev.revID);
 
@@ -540,20 +541,21 @@ - (CBLStatus) loadRevisionBody: (CBL_MutableRevision*)rev {
 - (CBL_Revision*) revisionByLoadingBody: (CBL_Revision*)rev
                                  status: (CBLStatus*)outStatus
 {
-    // First check for no-op -- if we just need the default properties and already have them:
+    // First check whether we already have the body (or know it's missing) and the sequence:
     if (rev.sequenceIfKnown) {
-        NSDictionary* props = rev.properties;
-        if (props.cbl_rev && props.cbl_id) {
-            if (outStatus)
-                *outStatus = kCBLStatusOK;
+        if (rev.body) {
+            *outStatus = kCBLStatusOK;
             return rev;
+        } else if (rev.missing) {
+            *outStatus = kCBLStatusNotFound;
+            return nil;
         }
     }
+    Assert(rev.docID && rev.revID);
+
     CBL_MutableRevision* nuRev = rev.mutableCopy;
-    CBLStatus status = [self loadRevisionBody: nuRev];
-    if (outStatus)
-        *outStatus = status;
-    if (CBLStatusIsError(status))
+    *outStatus = [_storage loadRevisionBody: nuRev];
+    if (CBLStatusIsError(*outStatus))
         nuRev = nil;
     return nuRev;
 }

diff --git a/Source/CBLForestBridge.mm b/Source/CBLForestBridge.mm
@@ -271,7 +271,7 @@ + (CBL_MutableRevision*) revisionObjectFromForestDoc: (C4Document*)doc
     result.sequence = doc->selectedRev.sequence;
     if (withBody) {
         *outStatus = [self loadBodyOfRevisionObject: result fromSelectedRevision: doc];
-        if (CBLStatusIsError(*outStatus))
+        if (CBLStatusIsError(*outStatus) && *outStatus != kCBLStatusGone)
             result = nil;
     }
     return result;
@@ -281,11 +281,13 @@ + (CBL_MutableRevision*) revisionObjectFromForestDoc: (C4Document*)doc
 + (CBLStatus) loadBodyOfRevisionObject: (CBL_MutableRevision*)rev
                   fromSelectedRevision: (C4Document*)doc
 {
+    rev.sequence = doc->selectedRev.sequence;
     C4Error c4err;
-    if (!c4doc_loadRevisionBody(doc, &c4err))
+    if (!c4doc_loadRevisionBody(doc, &c4err)) {
+        rev.missing = YES;
         return err2status(c4err);
+    }
     rev.asJSON = slice2data(doc->selectedRev.body);
-    rev.sequence = doc->selectedRev.sequence;
     return kCBLStatusOK;
 }
 

diff --git a/Source/CBL_ForestDBStorage.mm b/Source/CBL_ForestDBStorage.mm
@@ -312,7 +312,7 @@ - (CBL_MutableRevision*) getDocumentWithID: (NSString*)docID
     LogTo(Database, @"Read %@ rev %@", docID, inRevID);
 #endif
     *outStatus = selectRev(doc, inRevID, withBody);
-    if (CBLStatusIsError(*outStatus))
+    if (CBLStatusIsError(*outStatus) && *outStatus != kCBLStatusGone)
         return nil;
     if (!inRevID && (doc->selectedRev.flags & kRevDeleted)) {
         *outStatus = kCBLStatusDeleted;
@@ -360,7 +360,7 @@ - (CBLStatus) loadRevisionBody: (CBL_MutableRevision*)rev {
     if (!doc)
         return status;
 
-    status = selectRev(doc, rev.revID, YES);
+    status = selectRev(doc, rev.revID, NO);
     if (CBLStatusIsError(status))
         return status;
     return [CBLForestBridge loadBodyOfRevisionObject: rev fromSelectedRevision: doc];

diff --git a/Source/CBL_Revision.m b/Source/CBL_Revision.m
@@ -296,8 +296,14 @@ - (void) setProperties:(UU NSDictionary *)properties {
 }
 
 - (void) setAsJSON:(UU NSData *)asJSON {
-    _body = [[CBL_Body alloc] initWithJSON: asJSON
-                               addingDocID: _docID revID: _revID deleted: _deleted];
+    if (asJSON) {
+        _body = [[CBL_Body alloc] initWithJSON: asJSON
+                                   addingDocID: _docID revID: _revID deleted: _deleted];
+        _missing = NO;
+    } else {
+        _body = nil;
+        _missing = YES;
+    }
 }
 
 - (void) setObject: (UU id)object forKeyedSubscript: (UU NSString*)key {

diff --git a/Source/CBL_SQLiteStorage.m b/Source/CBL_SQLiteStorage.m
@@ -650,8 +650,10 @@ - (CBL_MutableRevision*) getDocumentWithID: (NSString*)docID
     NSMutableString* sql = [NSMutableString stringWithString: @"SELECT revid, deleted, sequence"];
     if (withBody)
         [sql appendString: @", json"];
+    else
+        [sql appendString: @", json is not null"];
     if (revID)
-        [sql appendString: @" FROM revs WHERE revs.doc_id=? AND revid=? AND json notnull LIMIT 1"];
+        [sql appendString: @" FROM revs WHERE revs.doc_id=? AND revid=? LIMIT 1"];
     else
         [sql appendString: @" FROM revs WHERE revs.doc_id=? and current=1 and deleted=0 "
                             "ORDER BY revid DESC LIMIT 1"];
@@ -668,6 +670,8 @@ - (CBL_MutableRevision*) getDocumentWithID: (NSString*)docID
         result.sequence = [r longLongIntForColumnIndex: 2];
         if (withBody)
             result.asJSON = [r dataNoCopyForColumnIndex: 3];
+        else
+            result.missing = ![r boolForColumnIndex: 3];
         status = kCBLStatusOK;
     }
     [r close];
@@ -704,7 +708,7 @@ - (CBL_MutableRevision*) getDocumentWithID: (NSString*)docID
                                                       revID: revID
                                                     deleted: deleted];
         result.sequence = sequence;
-        result.asJSON =[r dataNoCopyForColumnIndex: 2];
+        result.asJSON = [r dataNoCopyForColumnIndex: 2];
         status = kCBLStatusOK;
     }
     [r close];
@@ -715,7 +719,7 @@ - (CBL_MutableRevision*) getDocumentWithID: (NSString*)docID
 
 
 - (CBLStatus) loadRevisionBody: (CBL_MutableRevision*)rev {
-    if (rev.body && rev.sequence)
+    if (rev.body && rev.sequenceIfKnown)
         return kCBLStatusOK;  // no-op
     Assert(rev.docID && rev.revID);
     SInt64 docNumericID = [self getDocNumericID: rev.docID];
@@ -729,12 +733,11 @@ - (CBLStatus) loadRevisionBody: (CBL_MutableRevision*)rev {
     CBLStatus status = kCBLStatusNotFound;
     if ([r next]) {
         // Found the rev. But the JSON still might be null if the database has been compacted.
+        rev.sequence = [r longLongIntForColumnIndex: 0];
         NSData* json = [r dataNoCopyForColumnIndex: 1];
-        if (json) {
+        rev.asJSON = json;
+        if (json)
             status = kCBLStatusOK;
-            rev.sequence = [r longLongIntForColumnIndex: 0];
-            rev.asJSON = json;
-        }
     }
     [r close];
     return status;
@@ -750,8 +753,7 @@ - (CBL_MutableRevision*) revisionWithDocID: (NSString*)docID
     CBL_MutableRevision* rev = [[CBL_MutableRevision alloc] initWithDocID: docID revID: revID
                                                                   deleted: deleted];
     rev.sequence = sequence;
-    if (json)
-        rev.asJSON = json;
+    rev.asJSON = json;
     return rev;
 }
 

diff --git a/Unit-Tests/DatabaseInternal_Tests.m b/Unit-Tests/DatabaseInternal_Tests.m
@@ -212,7 +212,15 @@ - (void) test01_CRUD {
     Assert([db compact: &error]);
 
     // Make sure old rev is missing:
-    AssertNil([db getDocumentWithID: rev1.docID revisionID: rev1.revID]);
+    CBL_Revision* oldRev = [db getDocumentWithID: rev1.docID revisionID: rev1.revID];
+    Assert(oldRev);
+    Assert(oldRev.missing);
+    AssertNil(oldRev.body);
+
+    // Make sure history still works after compaction:
+    history = [db getRevisionHistory: revD backToRevIDs: nil];
+    Log(@"After compaction, history = %@", history);
+    AssertEqual(history, (@[revD.revID, rev2.revID, rev1.revID]));
 
     [[NSNotificationCenter defaultCenter] removeObserver: observer];
 }
@@ -490,7 +498,10 @@ - (void) test06_RevTree {
 
     // Fetch one of those phantom revisions with no body:
     CBL_Revision* rev2 = [db getDocumentWithID: rev.docID revisionID: @"2-2222".cbl_asRevID];
-    AssertNil(rev2);
+    Assert(rev2.missing);
+    AssertNil(rev2.body);
+
+    AssertNil([db getDocumentWithID: rev.docID revisionID: @"666-6666".cbl_asRevID]);
 
     // Make sure no duplicate rows were inserted for the common revisions:
     // (SQLite storage assigns sequences to inserted ancestor revs, while ForestDB doesn't)