/restful/rhizome/append on existing journal

Refactor Rhizome insert/append logic into functions used by both CLI and
RESTful API.  Improve RESTful diagnostic messages.

rhizome.c

@@ -164,6 +164,20 @@ enum rhizome_add_file_result rhizome_manifest_add_file(int appending,
   // Caller must not supply a malformed manifest (but an invalid one is okay because missing
   // fields will be filled in, so we don't check validity here).
   assert(!m->malformed);
+  // If appending to a journal, caller must not supply 'version', 'filesize' or 'filehash' fields,
+  // because these will be calculated by the journal append logic.
+  if (appending) {
+    if (m->version)
+      DEBUG(cause = "Cannot set 'version' field in journal append");
+    else if (m->filesize != RHIZOME_SIZE_UNSET)
+      DEBUG(cause = "Cannot set 'filesize' field in journal append");
+    else if (m->has_filehash)
+      DEBUG(cause = "Cannot set 'filehash' field in journal append");
+    if (cause) {
+      result = RHIZOME_ADD_FILE_INVALID_FOR_JOURNAL;
+      goto error;
+    }
+  }
   if (bid) {
     if (config.debug.rhizome)
       DEBUGF("Reading manifest from database: id=%s", alloca_tohex_rhizome_bid_t(*bid));
@@ -180,8 +194,11 @@ enum rhizome_add_file_result rhizome_manifest_add_file(int appending,
       existing_manifest = NULL;
       break;
     case RHIZOME_BUNDLE_STATUS_SAME:
-      // Found a manifest with the same bundle ID.  Unset its 'version', 'filesize' and 'filehash'
-      // fields unless appending, then overwrite it with the supplied manifest.
+      // Found a manifest with the same bundle ID.  If appending to a journal, then keep the
+      // existing 'version', 'filesize' and 'filehash' (so they can be verified when the existing
+      // payload is copied) and don't allow the supplied manifest to overwrite them.  If not a
+      // journal, then unset the 'version', 'filesize' and 'filehash' fields, then overwrite the
+      // existing manifest with the supplied manifest.
       if (!appending) {
         rhizome_manifest_del_version(existing_manifest);
         rhizome_manifest_del_filesize(existing_manifest);

rhizome.h

@@ -891,6 +891,7 @@ enum rhizome_payload_status rhizome_write_open_journal(struct rhizome_write *wri
 int rhizome_write_file(struct rhizome_write *write, const char *filename);
 void rhizome_fail_write(struct rhizome_write *write);
 enum rhizome_payload_status rhizome_finish_write(struct rhizome_write *write);
+enum rhizome_payload_status rhizome_finish_store(struct rhizome_write *write, rhizome_manifest *m, enum rhizome_payload_status status);
 enum rhizome_payload_status rhizome_import_payload_from_file(rhizome_manifest *m, const char *filepath);
 enum rhizome_payload_status rhizome_import_buffer(rhizome_manifest *m, uint8_t *buffer, size_t length);
 enum rhizome_payload_status rhizome_stat_payload_file(rhizome_manifest *m, const char *filepath);

rhizome_restful.c

@@ -615,20 +615,9 @@ static int insert_mime_part_end(struct http_request *hr)
   }
   else if (r->u.insert.current_part == PART_PAYLOAD) {
     r->u.insert.received_payload = 1;
-    switch (r->payload_status) {
-      case RHIZOME_PAYLOAD_STATUS_NEW:
-	r->payload_status = rhizome_finish_write(&r->u.insert.write);
-	if (r->payload_status == RHIZOME_PAYLOAD_STATUS_ERROR) {
-	  WHYF("rhizome_finish_write() returned status = %d", r->payload_status);
-	  return 500;
-	}
-	break;
-      case RHIZOME_PAYLOAD_STATUS_STORED:
-	// TODO: finish calculating payload hash and compare it with stored payload
-	break;
-      default:
-	break;
-    }
+    if (config.debug.rhizome)
+      DEBUGF("received %s, %zd bytes", PART_PAYLOAD, r->u.insert.payload_size);
+    r->payload_status = rhizome_finish_write(&r->u.insert.write);
   } else
     FATALF("current_part = %s", alloca_str_toprint(r->u.insert.current_part));
   r->u.insert.current_part = NULL;
@@ -644,61 +633,69 @@ static int restful_rhizome_insert_end(struct http_request *hr)
     return http_response_form_part(r, "Missing", PART_PAYLOAD, NULL, 0);
   // Fill in the missing manifest fields and ensure payload and manifest are consistent.
   assert(r->manifest != NULL);
-  assert(r->u.insert.write.file_length != RHIZOME_SIZE_UNSET);
-  int status_valid = 0;
   if (config.debug.rhizome)
-    DEBUGF("r->payload_status=%d", r->payload_status);
+    DEBUGF("r->payload_status=%d %s", r->payload_status, rhizome_payload_status_message(r->payload_status));
+  assert(r->u.insert.write.file_length != RHIZOME_SIZE_UNSET);
+  if (r->u.insert.appending) {
+    // For journal appends, the user cannot supply a 'filesize' field.  This will have been caught
+    // by previous logic.  The existing manifest should have a 'filesize' field.  The new payload
+    // size should be the sum of 'filesize' and the appended portion.
+    assert(r->manifest->is_journal);
+    assert(r->manifest->filesize != RHIZOME_SIZE_UNSET);
+    if (config.debug.rhizome)
+      DEBUGF("file_length=%"PRIu64" filesize=%"PRIu64" payload_size=%"PRIu64,
+	  r->u.insert.write.file_length,
+	  r->manifest->filesize,
+	  r->u.insert.payload_size);
+    if (r->u.insert.write.file_length != r->manifest->filesize + r->u.insert.payload_size)
+      r->payload_status = RHIZOME_PAYLOAD_STATUS_WRONG_SIZE;
+  } else {
+    // The Rhizome CLI 'add file' operation allows the user to supply a 'filesize' field which is
+    // smaller than the supplied file, for convenience, to allow only the first part of a file to be
+    // added as a payload.  But the RESTful interface doesn't allow that.
+    assert(!r->manifest->is_journal);
+    if (r->manifest->filesize != RHIZOME_SIZE_UNSET && r->u.insert.payload_size != r->manifest->filesize)
+      r->payload_status = RHIZOME_PAYLOAD_STATUS_WRONG_SIZE;
+  }
+  r->payload_status = rhizome_finish_store(&r->u.insert.write, r->manifest, r->payload_status);
+  int status_valid = 0;
   switch (r->payload_status) {
     case RHIZOME_PAYLOAD_STATUS_NEW:
-      if (r->manifest->filesize == RHIZOME_SIZE_UNSET)
-	rhizome_manifest_set_filesize(r->manifest, r->u.insert.write.file_length);
-      // fall through
     case RHIZOME_PAYLOAD_STATUS_STORED:
-      assert(r->manifest->filesize != RHIZOME_SIZE_UNSET);
-      // TODO: check that stored hash matches received payload's hash
-      // fall through
     case RHIZOME_PAYLOAD_STATUS_EMPTY:
       status_valid = 1;
-      if (r->manifest->filesize == RHIZOME_SIZE_UNSET)
-	rhizome_manifest_set_filesize(r->manifest, 0);
-      if (r->u.insert.payload_size == r->manifest->filesize)
-	break;
-      // fall through
+      break;
     case RHIZOME_PAYLOAD_STATUS_WRONG_SIZE:
-      r->payload_status = RHIZOME_PAYLOAD_STATUS_WRONG_SIZE;
       r->bundle_status = RHIZOME_BUNDLE_STATUS_INCONSISTENT;
       {
 	strbuf msg = strbuf_alloca(200);
 	strbuf_sprintf(msg, "Payload size (%"PRIu64") contradicts manifest (filesize=%"PRIu64")", r->u.insert.payload_size, r->manifest->filesize);
-	return http_request_rhizome_response(r, 403, NULL, strbuf_str(msg));
+	return http_request_rhizome_response(r, 403, "Inconsistent filesize", strbuf_str(msg));
       }
     case RHIZOME_PAYLOAD_STATUS_WRONG_HASH:
       r->bundle_status = RHIZOME_BUNDLE_STATUS_INCONSISTENT;
-      return http_request_rhizome_response(r, 403, NULL, NULL);
+      {
+	strbuf msg = strbuf_alloca(200);
+	strbuf_sprintf(msg, "Payload hash (%s) contradicts manifest (filehash=%s)",
+	    alloca_tohex_rhizome_filehash_t(r->u.insert.write.id),
+	    alloca_tohex_rhizome_filehash_t(r->manifest->filehash));
+	return http_request_rhizome_response(r, 403, "Inconsistent filehash", strbuf_str(msg));
+      }
     case RHIZOME_PAYLOAD_STATUS_CRYPTO_FAIL:
       r->bundle_status = RHIZOME_BUNDLE_STATUS_READONLY;
       return http_request_rhizome_response(r, 403, "Missing bundle secret", NULL);
     case RHIZOME_PAYLOAD_STATUS_TOO_BIG:
+      r->bundle_status = RHIZOME_BUNDLE_STATUS_NO_ROOM;
+      return http_request_rhizome_response(r, 403, "Bundle too big", NULL);
     case RHIZOME_PAYLOAD_STATUS_EVICTED:
       r->bundle_status = RHIZOME_BUNDLE_STATUS_NO_ROOM;
-      // fall through
+      return http_request_rhizome_response(r, 403, "Bundle evicted", NULL);
     case RHIZOME_PAYLOAD_STATUS_ERROR:
-      return http_request_rhizome_response(r, 403, NULL, NULL);
-  }
-  if (!status_valid) {
-    WHYF("r->payload_status = %d", r->payload_status);
-    return http_request_rhizome_response(r, 500, NULL, NULL);
+      return http_request_rhizome_response(r, 500, NULL, NULL);
   }
+  if (!status_valid)
+    FATALF("rhizome_finish_store() returned status = %d", r->payload_status);
   // Finalise the manifest and add it to the store.
-  if (r->u.insert.appending)
-    rhizome_manifest_set_version(r->manifest, r->manifest->filesize);
-
-  if (r->manifest->filesize) {
-    if (!r->manifest->has_filehash)
-      rhizome_manifest_set_filehash(r->manifest, &r->u.insert.write.id);
-    else
-      assert(cmp_rhizome_filehash_t(&r->u.insert.write.id, &r->manifest->filehash) == 0);
-  }
   const char *invalid_reason = rhizome_manifest_validate_reason(r->manifest);
   if (invalid_reason) {
     r->bundle_status = RHIZOME_BUNDLE_STATUS_INVALID;

rhizome_store.c

@@ -310,6 +310,9 @@ int rhizome_store_cleanup(struct rhizome_cleanup_report *report)
 
 enum rhizome_payload_status rhizome_open_write(struct rhizome_write *write, const rhizome_filehash_t *expectedHashp, uint64_t file_length)
 {
+  if (config.debug.rhizome_store)
+    DEBUGF("file_length=%"PRIu64, file_length);
+
   if (file_length == 0)
     return RHIZOME_PAYLOAD_STATUS_EMPTY;
 
@@ -681,6 +684,9 @@ void rhizome_fail_write(struct rhizome_write *write)
 
 enum rhizome_payload_status rhizome_finish_write(struct rhizome_write *write)
 {
+  if (config.debug.rhizome_store)
+    DEBUGF("blob_fd=%d file_offset=%"PRIu64"", write->blob_fd, write->file_offset);
+
   enum rhizome_payload_status status = RHIZOME_PAYLOAD_STATUS_NEW;
   
   // Once the whole file has been processed, we should finally know its length
@@ -789,6 +795,7 @@ enum rhizome_payload_status rhizome_finish_write(struct rhizome_write *write)
     }
     if (config.debug.rhizome_store)
       DEBUGF("Payload id=%s already present, removed id='%"PRIu64"'", alloca_tohex_rhizome_filehash_t(write->id), write->temp_id);
+    status = RHIZOME_PAYLOAD_STATUS_STORED;
   }else{
     if (sqlite_exec_void_retry(&retry, "BEGIN TRANSACTION;", END) == -1)
       goto dbfailure;
@@ -991,33 +998,11 @@ enum rhizome_payload_status rhizome_store_payload_file(rhizome_manifest *m, cons
   }
   if (!status_ok)
     FATALF("rhizome_write_open_manifest() returned status = %d", status);
-  if (rhizome_write_file(&write, filepath) == -1) {
-    rhizome_fail_write(&write);
-    return RHIZOME_PAYLOAD_STATUS_ERROR;
-  }
-  status = rhizome_finish_write(&write);
-  switch (status) {
-    case RHIZOME_PAYLOAD_STATUS_EMPTY:
-      assert(write.file_length == 0);
-      assert(m->filesize == 0);
-      return status;
-    case RHIZOME_PAYLOAD_STATUS_NEW:
-      assert(m->filesize == write.file_length);
-      if (m->has_filehash)
-	assert(cmp_rhizome_filehash_t(&m->filehash, &write.id) == 0);
-      else
-	rhizome_manifest_set_filehash(m, &write.id);
-      return status;
-    case RHIZOME_PAYLOAD_STATUS_ERROR:
-    case RHIZOME_PAYLOAD_STATUS_STORED:
-    case RHIZOME_PAYLOAD_STATUS_WRONG_SIZE:
-    case RHIZOME_PAYLOAD_STATUS_WRONG_HASH:
-    case RHIZOME_PAYLOAD_STATUS_CRYPTO_FAIL:
-    case RHIZOME_PAYLOAD_STATUS_TOO_BIG:
-    case RHIZOME_PAYLOAD_STATUS_EVICTED:
-      return status;
-  }
-  FATALF("rhizome_finish_write() returned status = %d", status);
+  if (rhizome_write_file(&write, filepath) == -1)
+    status = RHIZOME_PAYLOAD_STATUS_ERROR;
+  else
+    status = rhizome_finish_write(&write);
+  return rhizome_finish_store(&write, m, status);
 }
 
 /* Return RHIZOME_PAYLOAD_STATUS_STORED if file blob found, RHIZOME_PAYLOAD_STATUS_NEW if not found.
@@ -1617,14 +1602,16 @@ enum rhizome_payload_status rhizome_write_open_journal(struct rhizome_write *wri
       rstatus_valid = 1;
       break;
     case RHIZOME_PAYLOAD_STATUS_ERROR:
+    case RHIZOME_PAYLOAD_STATUS_TOO_BIG:
+      rstatus_valid = 1;
+      status = rstatus;
+      break;
     case RHIZOME_PAYLOAD_STATUS_WRONG_SIZE:
     case RHIZOME_PAYLOAD_STATUS_WRONG_HASH:
     case RHIZOME_PAYLOAD_STATUS_CRYPTO_FAIL:
-    case RHIZOME_PAYLOAD_STATUS_TOO_BIG:
     case RHIZOME_PAYLOAD_STATUS_EVICTED:
-      rstatus_valid = 1;
-      status = RHIZOME_PAYLOAD_STATUS_ERROR;
-      break;
+      // rhizome_journal_pipe() should not return any of these codes
+      FATALF("rhizome_journal_pipe() returned %d %s", rstatus, rhizome_payload_status_message(rstatus));
     }
     if (!rstatus_valid)
       FATALF("rstatus = %d", rstatus);
@@ -1640,13 +1627,67 @@ enum rhizome_payload_status rhizome_write_open_journal(struct rhizome_write *wri
   return status;
 }
 
-// Call to finish any write started with rhizome_write_open_journal()
-static void rhizome_finish_journal(struct rhizome_write *write, rhizome_manifest *m)
+// Call to finish any payload store operation
+enum rhizome_payload_status rhizome_finish_store(struct rhizome_write *write, rhizome_manifest *m, enum rhizome_payload_status status)
 {
-  assert(m->is_journal);
-  rhizome_manifest_set_filesize(m, write->file_length);
-  rhizome_manifest_set_version(m, write->file_length);
-  rhizome_manifest_set_filehash(m, &write->id);
+  if (config.debug.rhizome)
+    DEBUGF("write=%p m=manifest[%d], status=%d %s", write, m->manifest_record_number, status, rhizome_payload_status_message_nonnull(status));
+  switch (status) {
+  case RHIZOME_PAYLOAD_STATUS_NEW:
+    break;
+  default:
+    break;
+  }
+  int status_valid = 0;
+  switch (status) {
+  case RHIZOME_PAYLOAD_STATUS_EMPTY:
+    status_valid = 1;
+    assert(write->file_length == 0);
+    break;
+  case RHIZOME_PAYLOAD_STATUS_NEW:
+    assert(write->file_length != RHIZOME_SIZE_UNSET);
+    status_valid = 1;
+    break;
+  case RHIZOME_PAYLOAD_STATUS_STORED:
+    assert(write->file_length != RHIZOME_SIZE_UNSET);
+    status_valid = 1;
+    // TODO: check that stored hash matches received payload's hash
+    break;
+  case RHIZOME_PAYLOAD_STATUS_WRONG_SIZE:
+  case RHIZOME_PAYLOAD_STATUS_WRONG_HASH:
+  case RHIZOME_PAYLOAD_STATUS_TOO_BIG:
+  case RHIZOME_PAYLOAD_STATUS_CRYPTO_FAIL:
+  case RHIZOME_PAYLOAD_STATUS_EVICTED:
+  case RHIZOME_PAYLOAD_STATUS_ERROR:
+    status_valid = 1;
+    rhizome_fail_write(write);
+    return status;
+  }
+  if (!status_valid)
+    FATALF("status = %d", status);
+  // Fill in missing manifest fields and check consistency with existing fields.
+  if (m->is_journal || m->filesize == RHIZOME_SIZE_UNSET)
+    rhizome_manifest_set_filesize(m, write->file_length);
+  else if (m->filesize != write->file_length) {
+    if (config.debug.rhizome)
+      DEBUGF("m->filesize=%"PRIu64", write->file_length=%"PRIu64, m->filesize, write->file_length);
+    return RHIZOME_PAYLOAD_STATUS_WRONG_SIZE;
+  }
+  if (m->is_journal)
+    rhizome_manifest_set_version(m, m->filesize);
+  if (m->filesize) {
+    if (m->is_journal || !m->has_filehash)
+      rhizome_manifest_set_filehash(m, &write->id);
+    else if (cmp_rhizome_filehash_t(&write->id, &m->filehash) != 0) {
+      if (config.debug.rhizome)
+	DEBUGF("m->filehash=%s, write->id=%s", alloca_tohex_rhizome_filehash_t(m->filehash), alloca_tohex_rhizome_filehash_t(write->id));
+      return RHIZOME_PAYLOAD_STATUS_WRONG_HASH;
+    }
+  } else if (m->is_journal)
+    rhizome_manifest_del_filehash(m);
+  else if (m->has_filehash)
+    return RHIZOME_PAYLOAD_STATUS_WRONG_HASH;
+  return status;
 }
 
 enum rhizome_payload_status rhizome_append_journal_buffer(rhizome_manifest *m, uint64_t advance_by, uint8_t *buffer, size_t len)
@@ -1656,17 +1697,11 @@ enum rhizome_payload_status rhizome_append_journal_buffer(rhizome_manifest *m, u
   enum rhizome_payload_status status = rhizome_write_open_journal(&write, m, advance_by, (uint64_t) len);
   if (status != RHIZOME_PAYLOAD_STATUS_NEW)
     return status;
-  if (buffer && len && rhizome_write_buffer(&write, buffer, len) == -1) {
-    rhizome_fail_write(&write);
-    return RHIZOME_PAYLOAD_STATUS_ERROR;
-  }
-  status = rhizome_finish_write(&write);
-  if (status != RHIZOME_PAYLOAD_STATUS_NEW) {
-    rhizome_fail_write(&write);
-    return status;
-  }
-  rhizome_finish_journal(&write, m);
-  return status;
+  if (buffer && len && rhizome_write_buffer(&write, buffer, len) == -1)
+    status = RHIZOME_PAYLOAD_STATUS_ERROR;
+  else
+    status = rhizome_finish_write(&write);
+  return rhizome_finish_store(&write, m, status);
 }
 
 enum rhizome_payload_status rhizome_append_journal_file(rhizome_manifest *m, uint64_t advance_by, const char *filename)
@@ -1679,15 +1714,9 @@ enum rhizome_payload_status rhizome_append_journal_file(rhizome_manifest *m, uin
   enum rhizome_payload_status status = rhizome_write_open_journal(&write, m, advance_by, stat.st_size);
   if (status != RHIZOME_PAYLOAD_STATUS_NEW)
     return status;
-  if (stat.st_size != 0 && rhizome_write_file(&write, filename) == -1) {
-    rhizome_fail_write(&write);
-    return RHIZOME_PAYLOAD_STATUS_ERROR;
-  }
-  status = rhizome_finish_write(&write);
-  if (status != RHIZOME_PAYLOAD_STATUS_NEW) {
-    rhizome_fail_write(&write);
-    return status;
-  }
-  rhizome_finish_journal(&write, m);
-  return status;
+  if (stat.st_size != 0 && rhizome_write_file(&write, filename) == -1)
+    status = RHIZOME_PAYLOAD_STATUS_ERROR;
+  else
+    status = rhizome_finish_write(&write);
+  return rhizome_finish_store(&write, m, status);
 }

tests/rhizomerestful

@@ -1169,8 +1169,11 @@ setup_RhizomeJournalAppend() {
    file1_size=$(cat file1 | wc -c)
    >manifest1
    echo "service=anything" >>manifest1
-   echo "filesize=$file1_size" >>manifest1
    echo "name=hoopla" >>manifest1
+   echo "random=rubbish" >>manifest1
+   echo 'File two two two' >file2
+   >manifest2
+   file2_size=$(cat file2 | wc -c)
 }
 test_RhizomeJournalAppend() {
    execute curl \
@@ -1225,6 +1228,19 @@ test_RhizomeJournalAppend() {
    assert_rhizome_list file1
    executeOk_servald rhizome extract file "$BID" file1x
    assert --message="extracted payload is correct" diff file1 file1x
+   execute curl \
+         --silent --show-error --write-out '%{http_code}' \
+         --output file2.manifest \
+         --dump-header http.header \
+         --basic --user harry:potter \
+         --form "bundle-id=$BID" \
+         --form "bundle-author=$SIDA" \
+         --form "manifest=@manifest2;type=rhizome/manifest;format=\"text+binarysig\"" \
+         --form "payload=@file2" \
+         "http://$addr_localhost:$PORTA/restful/rhizome/append"
+   tfw_cat http.header file2.manifest
+   assertExitStatus == 0
+   assertStdoutIs 201
 }
 
 runTests "$@"