Restore: go on after a partial write; created tables stay once the database is committed

lib/Db/DbDumper.php

@@ -225,21 +225,6 @@ final class DbDumper {
 			$this->restoreSequences($manifest['sequences'] ?? [], $report['missing'], array_merge($report['restored'], $report['skipped']));
 			$this->resumeForeignKeys($foreignKeys);
 			$this->db->commit();
-			// Foreign keys of the tables created above, now their rows are in. Not inside the
-			// transaction: MySQL commits on DDL. A failure is reported, not fatal: the data is
-			// restored either way.
-			foreach ($report['created'] as $table) {
-				$structure = $manifest['tables'][$table]['schema'] ?? [];
-				foreach ($structure['foreignKeys'] ?? [] as $fk) {
-					try {
-						$this->db->createSchemaManager()->createForeignKey(
-							new \Doctrine\DBAL\Schema\ForeignKeyConstraint($fk['columns'], $fk['table'], $fk['foreign'], $fk['name'], $fk['options'] ?? []),
-							$table);
-					} catch (\Throwable $e) {
-						$report['foreignKeyErrors'][] = $table . '.' . $fk['name'] . ': ' . $e->getMessage();
-					}
-				}
-			}
 		} catch (\Throwable $e) {
 			$this->db->rollBack();
 			// PostgreSQL rolls the dropped foreign keys back with the transaction; MySQL's session
@@ -250,6 +235,21 @@ final class DbDumper {
 			$this->dropCreatedTables($report);
 			throw $e;
 		}
+		// Committed. Foreign keys of the tables created above, now their rows are in. Not inside the
+		// transaction: MySQL commits on DDL. A failure is reported, not fatal: the data is restored
+		// either way, and the created tables stay.
+		foreach ($report['created'] as $table) {
+			$structure = $manifest['tables'][$table]['schema'] ?? [];
+			foreach ($structure['foreignKeys'] ?? [] as $fk) {
+				try {
+					$this->db->createSchemaManager()->createForeignKey(
+						new \Doctrine\DBAL\Schema\ForeignKeyConstraint($fk['columns'], $fk['table'], $fk['foreign'], $fk['name'], $fk['options'] ?? []),
+						$table);
+				} catch (\Throwable $e) {
+					$report['foreignKeyErrors'][] = $table . '.' . $fk['name'] . ': ' . $e->getMessage();
+				}
+			}
+		}
 		return $total;
 	}
 

lib/Repository/Repository.php

@@ -451,12 +451,17 @@ final class Repository {
 
 	/**
 	 * Write all of $data, or stop: a short write leaves a file that looks damaged only by its size.
+	 * A write may take only part of the data and still be fine; one that takes nothing is not.
 	 *
 	 * @param resource $out
 	 */
 	public static function writeAll($out, string $data, string $path): void {
-		if (@fwrite($out, $data) !== strlen($data)) {
-			throw new RestoreTargetException('Cannot write ' . $path . ' to the restore directory (disk full?)');
+		while ($data !== '') {
+			$written = @fwrite($out, $data);
+			if ($written === false || $written === 0) {
+				throw new RestoreTargetException('Cannot write ' . $path . ' to the restore directory (disk full?)');
+			}
+			$data = substr($data, $written);
 		}
 	}
 

tests/Unit/Repository/RestoreTargetTest.php

@@ -12,6 +12,24 @@ use OCA\NgBackup\Repository\RestoreTargetException;
 use OCA\NgBackup\Tests\Unit\TempDirTrait;
 use PHPUnit\Framework\TestCase;
 
+/** A stream that takes at most 3 bytes per write. */
+class PartialWriteStream {
+	public static string $written = '';
+	/** @var resource|null set by PHP */
+	public $context;
+
+	public function stream_open(string $path, string $mode, int $options, ?string &$opened): bool {
+		self::$written = '';
+		return true;
+	}
+
+	public function stream_write(string $data): int {
+		$part = substr($data, 0, 3);
+		self::$written .= $part;
+		return strlen($part);
+	}
+}
+
 /**
  * What goes wrong where a restore writes to is not damage in the backup: it stops the restore, also
  * with --salvage, and the message does not point at --salvage.
@@ -48,6 +66,18 @@ class RestoreTargetTest extends TestCase {
 		Repository::writeAll($out, 'data', 'dir/a.bin');
 	}
 
+	public function testAWriteThatTakesPartOfTheDataGoesOn(): void {
+		stream_wrapper_register('ngbpartial', PartialWriteStream::class);
+		try {
+			$out = fopen('ngbpartial://x', 'wb');
+			Repository::writeAll($out, 'abcdefgh', 'dir/a.bin');
+			fclose($out);
+			$this->assertSame('abcdefgh', PartialWriteStream::$written);
+		} finally {
+			stream_wrapper_unregister('ngbpartial');
+		}
+	}
+
 	public function testATargetErrorDoesNotLookLikeDamage(): void {
 		$this->assertFalse(Restore::backupLooksDamaged(new RestoreTargetException('Cannot write')));
 	}