refactor(metadata): improve nodeId precedence and test coverage

- Clarify signedNodeId takes precedence over nodeId
- Add DataProvider for nodeId precedence scenarios
- Add DataProvider for metadata field scenarios
- Remove redundant test cases
- Consolidate metadata defaults testing

Tests now use parameterized tests for better coverage
and clearer test intent. The nodeId resolution logic
is explicitly tested for both precedence and fallback cases.

Signed-off-by: Vitor Mattos <[email protected]>

Author: vitormattos

lib/Service/File/MetadataLoader.php

@@ -33,6 +33,9 @@ public function loadMetadata(?File $file, stdClass $fileData): void {
 
 		try {
 			$fileNode = $this->getFileNode($file);
+			$metadata = $file->getMetadata() ?? [];
+
+			$fileData->metadata = $metadata;
 
 			if (method_exists($fileNode, 'getSize')) {
 				$fileData->size = $fileNode->getSize();
@@ -46,9 +49,15 @@ public function loadMetadata(?File $file, stdClass $fileData): void {
 			}
 
 			$fileData->pages = $this->getPages($file);
+
+			$fileData->totalPages = (int)($metadata['p'] ?? count($fileData->pages ?? []));
+			$fileData->pdfVersion = (string)($metadata['pdfVersion'] ?? '');
 		} catch (\Throwable $e) {
 			$this->logger->warning('Failed to load file metadata: ' . $e->getMessage());
 		}
+
+		$fileData->totalPages ??= 0;
+		$fileData->pdfVersion ??= '';
 	}
 
 	/**

tests/php/Unit/Service/File/MetadataLoaderTest.php

@@ -16,6 +16,7 @@
 use OCP\Files\IMimeTypeDetector;
 use OCP\Files\IRootFolder;
 use OCP\IURLGenerator;
+use PHPUnit\Framework\Attributes\DataProvider;
 use PHPUnit\Framework\MockObject\MockObject;
 use Psr\Log\LoggerInterface;
 use stdClass;
@@ -117,40 +118,6 @@ public function testLoadMetadataWithPages(): void {
 		$this->assertEquals(200, $fileData->pages[1]['resolution']);
 	}
 
-	public function testLoadMetadataUsesContentProviderWhenNoMimeType(): void {
-		$file = new File();
-		$file->setId(1);
-		$file->setUserId('user123');
-		$file->setSignedNodeId(123);
-		$file->setMetadata(['p' => 1, 'd' => [100]]);
-		$file->setUuid('uuid-123');
-
-		// Create a mock that implements the File interface but doesn't define getMimeType
-		$fileNode = $this->createMock(\OCP\Files\File::class);
-		$fileNode->method('getSize')->willReturn(5000);
-
-		$userFolder = $this->createMock(Folder::class);
-		$userFolder->method('getFirstNodeById')->with(123)->willReturn($fileNode);
-
-		$this->root->method('getUserFolder')->with('user123')->willReturn($userFolder);
-
-		// Since the mock will have getMimeType available, we expect it to be called
-		// The test needs to be adjusted - it tests the else branch when method_exists is false
-		// But with mocks, method_exists will always return true
-		// So this test is actually not testing the right scenario
-		// Let's just test that getMimeType is called
-		$fileNode->method('getMimeType')->willReturn('application/pdf');
-
-		$this->urlGenerator->method('linkToRoute')->willReturn('http://example.com/page.pdf');
-
-		$fileData = new stdClass();
-
-		$service = $this->getService();
-		$service->loadMetadata($file, $fileData);
-
-		$this->assertEquals('application/pdf', $fileData->mime);
-	}
-
 	public function testLoadMetadataLogsWarningOnError(): void {
 		$file = new File();
 		$file->setId(1);
@@ -176,11 +143,13 @@ public function testLoadMetadataLogsWarningOnError(): void {
 		$this->assertTrue(true);
 	}
 
-	public function testLoadMetadataUsesSignedNodeIdFirst(): void {
+	#[DataProvider('provideNodeIdPrecedenceScenarios')]
+	public function testNodeIdPrecedenceAndFallback(?int $signedNodeId, ?int $nodeId, int $expectedNodeId): void {
 		$file = new File();
 		$file->setId(1);
 		$file->setUserId('user123');
-		$file->setSignedNodeId(123);
+		$file->setSignedNodeId($signedNodeId);
+		$file->setNodeId($nodeId);
 		$file->setMetadata(['p' => 0, 'd' => []]);
 
 		$fileNode = $this->createMock(\OCP\Files\File::class);
@@ -191,7 +160,7 @@ public function testLoadMetadataUsesSignedNodeIdFirst(): void {
 		$userFolder
 			->expects($this->once())
 			->method('getFirstNodeById')
-			->with(123)
+			->with($expectedNodeId)
 			->willReturn($fileNode);
 
 		$this->root->method('getUserFolder')->with('user123')->willReturn($userFolder);
@@ -206,43 +175,25 @@ public function testLoadMetadataUsesSignedNodeIdFirst(): void {
 		$this->assertEquals(5000, $fileData->size);
 	}
 
-	public function testLoadMetadataFallsBackToNodeId(): void {
-		$file = new File();
-		$file->setId(1);
-		$file->setUserId('user123');
-		$file->setSignedNodeId(null);
-		$file->setNodeId(456);
-		$file->setMetadata(['p' => 0, 'd' => []]);
-
-		$fileNode = $this->createMock(\OCP\Files\File::class);
-		$fileNode->method('getSize')->willReturn(5000);
-		$fileNode->method('getMimeType')->willReturn('application/pdf');
-
-		$userFolder = $this->createMock(Folder::class);
-		$userFolder
-			->expects($this->once())
-			->method('getFirstNodeById')
-			->with(456)
-			->willReturn($fileNode);
-
-		$this->root->method('getUserFolder')->with('user123')->willReturn($userFolder);
-
-		$this->urlGenerator->method('linkToRoute')->willReturn('http://example.com/page.pdf');
-
-		$fileData = new stdClass();
-
-		$service = $this->getService();
-		$service->loadMetadata($file, $fileData);
-
-		$this->assertEquals(5000, $fileData->size);
+	public static function provideNodeIdPrecedenceScenarios(): array {
+		return [
+			'signedNodeId takes precedence when present' => [123, 456, 123],
+			'fallback to nodeId when signedNodeId is null' => [null, 456, 456],
+		];
 	}
 
-	public function testLoadMetadataHandlesNoPages(): void {
+	#[DataProvider('provideMetadataFieldScenarios')]
+	public function testLoadMetadataDefaults(int $pageCount, ?string $pdfVersion, int $expectedPages, string $expectedPdfVersion): void {
 		$file = new File();
 		$file->setId(1);
 		$file->setUserId('user123');
 		$file->setSignedNodeId(123);
-		$file->setMetadata(['p' => 0, 'd' => []]);
+		$metadata = ['p' => $pageCount];
+		if ($pdfVersion !== null) {
+			$metadata['pdfVersion'] = $pdfVersion;
+		}
+		$file->setMetadata($metadata);
+		$file->setUuid('uuid-123');
 
 		$fileNode = $this->createMock(\OCP\Files\File::class);
 		$fileNode->method('getSize')->willReturn(5000);
@@ -253,11 +204,22 @@ public function testLoadMetadataHandlesNoPages(): void {
 
 		$this->root->method('getUserFolder')->with('user123')->willReturn($userFolder);
 
+		$this->urlGenerator->method('linkToRoute')->willReturn('http://example.com/page.pdf');
+
 		$fileData = new stdClass();
 
 		$service = $this->getService();
 		$service->loadMetadata($file, $fileData);
 
-		$this->assertCount(0, $fileData->pages);
+		$this->assertEquals($expectedPages, $fileData->totalPages);
+		$this->assertEquals($expectedPdfVersion, $fileData->pdfVersion);
+	}
+
+	public static function provideMetadataFieldScenarios(): array {
+		return [
+			'no pages with no pdfVersion' => [0, null, 0, ''],
+			'multiple pages with pdf version' => [5, '1.7', 5, '1.7'],
+			'single page with pdf version' => [1, '1.5', 1, '1.5'],
+		];
 	}
 }