Repository: cloudant/couchbackup (js)

Issue:
Version 2.7.0 cannot restore shallow backups from before 2.4.2
## Bug Description

### 1. Steps to reproduce and the simplest code sample possible to demonstrate the issue

1. Make a shallow backup with a version `<2.4.2`
2. Try to restore the backup with version `2.7.0`

### 2. What you expected to happen

The restore should succeed.

Although we haven't made a statement anywhere about mixing versions of backup and restore I think there is no real reason this shouldn't work and I could easily imagine a backup project running on a cron with an old version and not attempting to do a restore until it was really needed and grabbing the lasest version at the time. The failure mode was introduced in `2.7.0`.

### 3. What actually happened

The restore fails with: `ERROR: Error writing batch.` This happens because:
https://github.com/cloudant/couchbackup/blob/68c802b641673d0a29f560970fb70ce90e18cf56/includes/writer.js#L42
expects the empty array response of `new_edits: false` mode.
However, https://github.com/cloudant/couchbackup/blob/68c802b641673d0a29f560970fb70ce90e18cf56/includes/writer.js#L33
still toggles `new_edits:true` (note: the absense of the parameter gives the default `true`) based on whether there is any `_rev` info in the payload. It is worth noting that all backups produced after 2.4.2 include `_rev` info, but shallow backups created prior to `2.4.2` did not include `_rev` info.

We should either:
Toggle the response checking behaviour based on the `new_edits` parameter
or
Remove the toggle and document something about backup/restore version compatibility

Also the error message here at https://github.com/cloudant/couchbackup/blob/68c802b641673d0a29f560970fb70ce90e18cf56/includes/writer.js#L43  could probably do with a bit more information like indicating if the `result` was missing or non-empty - and potentially in the `new_edits:false` case extracting error responses from the set. (We might also want to consider checking for non `"ok":true` entries in the `new_edits:true` case, but I'm concerned that might have a significant impact on restore performance).

## Environment details
- Version(s) that are affected by this issue.
    > 2.7.0
- Node.js version
    > 12.22.7


Pull request:
Fix restore of old version shallow backups
## Checklist

- [x] Tick to sign-off your agreement to the [Developer Certificate of Origin (DCO) 1.1](../blob/master/DCO1.1.txt)
- [x] Added tests for code changes _or_ test/build only changes
- [x] Updated the change log file (`CHANGES.md`|`CHANGELOG.md`) _or_ test/build only changes
- [x] Completed the PR template below:

## Description

Correct batch error check on restore of shallow backups created with versions <2.4.2

Fixes #379 

## Approach

Correct batch error check on restore wrriterr batch response to honour the `new_edits` toggle in the payload that could be triggered on by shallow backups that don't include rev info (i.e. those created with versions earlier than `2.4.2`).

## Schema & API Changes

- "No change"

## Security and Privacy

- "No change"

## Testing

- Added new tests:
    - `should restore shallow backups without rev info successfully` with fixture of a revisionless shallow backup

## Monitoring and Logging

- Augment the error message to make debugging slightly easier in the case of batch write errors.


Patch:
diff --git a/CHANGES.md b/CHANGES.md
index 48ff06e..e72fdc8 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -1,5 +1,6 @@
 # Unreleased
 - [FIXED] Corrected `user-agent` header on requests.
+- [FIXED] Restore of shallow backups created with versions <=2.4.2.
 
 # 2.7.0 (2021-09-14)
 - [UPGRADED] Cloudant client dependency from `@cloudant/cloudant` to `@ibm-cloud/cloudant`.
diff --git a/includes/writer.js b/includes/writer.js
index dd3e6b5..8602b55 100644
--- a/includes/writer.js
+++ b/includes/writer.js
@@ -32,6 +32,7 @@ module.exports = function(db, bufferSize, parallelism, ee) {
     // if we are restoring known revisions, we need to supply new_edits=false
     if (payload.docs && payload.docs[0] && payload.docs[0]._rev) {
       payload.new_edits = false;
+      debug('Using new_edits false mode.');
     }
 
     if (!didError) {
@@ -39,8 +40,9 @@ module.exports = function(db, bufferSize, parallelism, ee) {
         db: db.db,
         bulkDocs: payload
       }).then(response => {
-        if (!response.result || response.result.length > 0) {
-          throw new Error('Error writing batch.');
+        if (!response.result || (payload.new_edits === false && response.result.length > 0)) {
+          throw new Error(`Error writing batch with new_edits:${payload.new_edits !== false}` +
+            ` and ${response.result ? response.result.length : 'unavailable'} items`);
         }
         written += payload.docs.length;
         writer.emit('restored', { documents: payload.docs.length, total: written });


Test patch:
diff --git a/test/fixtures/animaldb_old_shallow.json b/test/fixtures/animaldb_old_shallow.json
new file mode 100644
index 0000000..2f4a277
--- /dev/null
+++ b/test/fixtures/animaldb_old_shallow.json
@@ -0,0 +1,1 @@
+[{"_id":"_design/views101","views":{"latin_name_jssum":{"map":"function(doc) {\n  if(doc.latin_name){\n    emit(doc.latin_name, doc.latin_name.length);\n  }\n}","reduce":"function (key, values, rereduce){\n  return sum(values);\n}"},"latin_name":{"map":"function(doc) {\n  if(doc.latin_name){\n    emit(doc.latin_name, doc.latin_name.length);\n  }\n}"},"diet_sum":{"map":"function(doc) {\n  if(doc.diet){\n    emit(doc.diet, 1);\n  }\n}","reduce":"_sum"},"diet_count":{"map":"function(doc) {\n  if(doc.diet && doc.latin_name){\n    emit(doc.diet, doc.latin_name);\n  }\n}","reduce":"_count"},"complex_count":{"map":"function(doc){\n  if(doc.class && doc.diet){\n    emit([doc.class, doc.diet], 1);\n  }\n}","reduce":"_count"},"diet":{"map":"function(doc) {\n  if(doc.diet){\n    emit(doc.diet, 1);\n  }\n}"},"complex_latin_name_count":{"map":"function(doc){\n  if(doc.latin_name){\n    emit([doc.class, doc.diet, doc.latin_name], doc.latin_name.length)\n  }\n}","reduce":"_count"},"diet_jscount":{"map":"function(doc) {\n  if(doc.diet){\n    emit(doc.diet, 1);\n  }\n}","reduce":"function (key, values, rereduce){\n  return values.length;\n}"},"latin_name_count":{"map":"function(doc) {\n  if(doc.latin_name){\n    emit(doc.latin_name, doc.latin_name.length);\n  }\n}","reduce":"_count"},"latin_name_sum":{"map":"function(doc) {\n  if(doc.latin_name){\n    emit(doc.latin_name, doc.latin_name.length);\n  }\n}","reduce":"_sum"}},"indexes":{"animals":{"index":"function(doc){\n  index(\"default\", doc._id);\n  if(doc.min_length){\n    index(\"min_length\", doc.min_length, {\"store\": \"yes\"});\n  }\n  if(doc.diet){\n    index(\"diet\", doc.diet, {\"store\": \"yes\"});\n  }\n  if (doc.latin_name){\n    index(\"latin_name\", doc.latin_name, {\"store\": \"yes\"});\n  }\n  if (doc['class']){\n    index(\"class\", doc['class'], {\"store\": \"yes\"});\n  }\n}"}}},{"_id":"aardvark","min_weight":40,"max_weight":65,"min_length":1,"max_length":2.2,"latin_name":"Orycteropus afer","wiki_page":"http://en.wikipedia.org/wiki/Aardvark","class":"mammal","diet":"omnivore"},{"_id":"badger","wiki_page":"http://en.wikipedia.org/wiki/Badger","min_weight":7,"max_weight":30,"min_length":0.6,"max_length":0.9,"latin_name":"Meles meles","class":"mammal","diet":"omnivore"},{"_id":"elephant","wiki_page":"http://en.wikipedia.org/wiki/African_elephant","min_weight":4700,"max_weight":6050,"min_length":3.2,"max_length":4,"class":"mammal","diet":"herbivore"},{"_id":"giraffe","min_weight":830,"min_length":5,"max_weight":1600,"max_length":6,"wiki_page":"http://en.wikipedia.org/wiki/Giraffe","class":"mammal","diet":"herbivore"},{"_id":"kookaburra","min_length":0.28,"max_length":0.42,"wiki_page":"http://en.wikipedia.org/wiki/Kookaburra","class":"bird","diet":"carnivore","latin_name":"Dacelo novaeguineae"},{"_id":"lemur","wiki_page":"http://en.wikipedia.org/wiki/Ring-tailed_lemur","min_weight":2.2,"max_weight":2.2,"min_length":0.95,"max_length":1.1,"class":"mammal","diet":"omnivore"},{"_id":"llama","min_weight":130,"max_weight":200,"min_length":1.7,"max_length":1.8,"latin_name":"Lama glama","wiki_page":"http://en.wikipedia.org/wiki/Llama","class":"mammal","diet":"herbivore"},{"_id":"panda","wiki_page":"http://en.wikipedia.org/wiki/Panda","min_weight":75,"max_weight":115,"min_length":1.2,"max_length":1.8,"class":"mammal","diet":"carnivore"},{"_id":"snipe","min_weight":0.08,"max_weight":0.14,"min_length":0.25,"max_length":0.27,"latin_name":"Gallinago gallinago","wiki_page":"http://en.wikipedia.org/wiki/Common_Snipe","class":"bird","diet":"omnivore"},{"_id":"zebra","wiki_page":"http://en.wikipedia.org/wiki/Plains_zebra","min_length":2,"max_length":2.5,"min_weight":175,"max_weight":387,"class":"mammal","diet":"herbivore"}]
diff --git a/test/writer.js b/test/writer.js
index a7d7ca5..ffb389c 100644
--- a/test/writer.js
+++ b/test/writer.js
@@ -26,8 +26,8 @@ const liner = require('../includes/liner.js');
 // The writer expects a line-by-line stream so this utility function does that
 // processing for the tests (which normally happens in the internal restore
 // function).
-function testLinestream() {
-  return fs.createReadStream('./test/fixtures/animaldb_expected.json')
+function testLinestream(fixture = './test/fixtures/animaldb_expected.json') {
+  return fs.createReadStream(fixture)
     .pipe(liner());
 }
 
@@ -110,4 +110,36 @@ describe('#unit Check database restore writer', function() {
         done();
       });
   });
+
+  it('should restore shallow backups without rev info successfully', function(done) {
+    nock(dbUrl)
+      .post('/_bulk_docs')
+      .reply(200, [{ ok: true, id: 'foo', rev: '1-abc' }]); // success
+
+    testLinestream('./test/fixtures/animaldb_old_shallow.json')
+      .pipe(writer(db, 500, 1, noopEmitter))
+      .on('error', function(err) {
+        done(err);
+      })
+      .on('finished', function(data) {
+        assert.strictEqual(data.total, 11);
+        assert.ok(nock.isDone());
+        done();
+      });
+  });
+
+  it('should get a batch error for non-empty array response with new_edits false', function(done) {
+    nock(dbUrl)
+      .post('/_bulk_docs')
+      .reply(200, [{ id: 'foo', error: 'foo', reason: 'bar' }]);
+
+    testLinestream()
+      .pipe(writer(db, 500, 1, noopEmitter))
+      .on('error', function(err) {
+        assert.strictEqual(err.name, 'Error');
+        assert.strictEqual(err.message, 'Error writing batch with new_edits:false and 1 items');
+        assert.ok(nock.isDone());
+        done();
+      });
+  });
 });