Merge pull request #9641 from nextcloud/techdep/noid/caching_explicit_in_response

Move caching logic to response

8 files changed, 26 insertions, 102 deletions

diff --git a/apps/theming/lib/Controller/IconController.php b/apps/theming/lib/Controller/IconController.php
index 61df7b0b353..13f385e0bd1 100644
--- a/apps/theming/lib/Controller/IconController.php
+++ b/apps/theming/lib/Controller/IconController.php
@@ -101,11 +101,6 @@ class IconController extends Controller {
 		if ($iconFile !== false) {
 			$response = new FileDisplayResponse($iconFile, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);
 			$response->cacheFor(86400);
-			$expires = new \DateTime();
-			$expires->setTimestamp($this->timeFactory->getTime());
-			$expires->add(new \DateInterval('PT24H'));
-			$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-			$response->addHeader('Pragma', 'cache');
 			return $response;
 		}
 
@@ -146,11 +141,6 @@ class IconController extends Controller {
 			$response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
 		}
 		$response->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$response->addHeader('Pragma', 'cache');
 		return $response;
 	}
 
@@ -187,11 +177,6 @@ class IconController extends Controller {
 			$response = new DataDisplayResponse($this->fileAccessHelper->file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']);
 		}
 		$response->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$response->addHeader('Pragma', 'cache');
 		return $response;
 	}
 }
diff --git a/apps/theming/lib/Controller/ThemingController.php b/apps/theming/lib/Controller/ThemingController.php
index a834bb1c5ab..c50e8a18460 100644
--- a/apps/theming/lib/Controller/ThemingController.php
+++ b/apps/theming/lib/Controller/ThemingController.php
@@ -368,11 +368,6 @@ class ThemingController extends Controller {
 
 		$response = new FileDisplayResponse($file);
 		$response->cacheFor(3600);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$response->addHeader('Pragma', 'cache');
 		$response->addHeader('Content-Type', $this->config->getAppValue($this->appName, $key . 'Mime', ''));
 		$response->addHeader('Content-Disposition', 'attachment; filename="' . $key . '"');
 		return $response;
@@ -403,11 +398,6 @@ class ThemingController extends Controller {
 			$cssFile = $this->scssCacher->getCachedCSS('theming', 'theming.css');
 			$response = new FileDisplayResponse($cssFile, Http::STATUS_OK, ['Content-Type' => 'text/css']);
 			$response->cacheFor(86400);
-			$expires = new \DateTime();
-			$expires->setTimestamp($this->timeFactory->getTime());
-			$expires->add(new \DateInterval('PT24H'));
-			$response->addHeader('Expires', $expires->format(\DateTime::RFC1123));
-			$response->addHeader('Pragma', 'cache');
 			return $response;
 		} catch (NotFoundException $e) {
 			return new NotFoundResponse();
@@ -435,8 +425,6 @@ class ThemingController extends Controller {
 	};
 })();';
 		$response = new DataDownloadResponse($responseJS, 'javascript', 'text/javascript');
-		$response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$response->addHeader('Pragma', 'cache');
 		$response->cacheFor(3600);
 		return $response;
 	}
@@ -470,8 +458,6 @@ class ThemingController extends Controller {
 			'display' => 'standalone'
 		];
 		$response = new Http\JSONResponse($responseJS);
-		$response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$response->addHeader('Pragma', 'cache');
 		$response->cacheFor(3600);
 		return $response;
 	}
diff --git a/apps/theming/tests/Controller/IconControllerTest.php b/apps/theming/tests/Controller/IconControllerTest.php
index 6539c6e0209..f509005d32c 100644
--- a/apps/theming/tests/Controller/IconControllerTest.php
+++ b/apps/theming/tests/Controller/IconControllerTest.php
@@ -102,11 +102,6 @@ class IconControllerTest extends TestCase {
 			->willReturn($file);
 		$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/svg+xml']);
 		$expected->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$expected->addHeader('Pragma', 'cache');
 		$this->assertEquals($expected, $this->iconController->getThemedIcon('core', 'filetypes/folder.svg'));
 	}
 
@@ -139,11 +134,6 @@ class IconControllerTest extends TestCase {
 
 		$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
 		$expected->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$expected->addHeader('Pragma', 'cache');
 		$this->assertEquals($expected, $this->iconController->getFavicon());
 	}
 
@@ -162,11 +152,6 @@ class IconControllerTest extends TestCase {
 			->willReturn(file_get_contents($fallbackLogo));
 		$expected = new DataDisplayResponse(file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/x-icon']);
 		$expected->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$expected->addHeader('Pragma', 'cache');
 		$this->assertEquals($expected, $this->iconController->getFavicon());
 	}
 
@@ -196,11 +181,6 @@ class IconControllerTest extends TestCase {
 
 		$expected = new FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'image/png']);
 		$expected->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$expected->addHeader('Pragma', 'cache');
 		$this->assertEquals($expected, $this->iconController->getTouchIcon());
 	}
 
@@ -219,11 +199,6 @@ class IconControllerTest extends TestCase {
 			->willReturn(file_get_contents($fallbackLogo));
 		$expected = new DataDisplayResponse(file_get_contents($fallbackLogo), Http::STATUS_OK, ['Content-Type' => 'image/png']);
 		$expected->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$expected->addHeader('Pragma', 'cache');
 		$this->assertEquals($expected, $this->iconController->getTouchIcon());
 	}
 
diff --git a/apps/theming/tests/Controller/ThemingControllerTest.php b/apps/theming/tests/Controller/ThemingControllerTest.php
index cd50061c59a..4a2b780076c 100644
--- a/apps/theming/tests/Controller/ThemingControllerTest.php
+++ b/apps/theming/tests/Controller/ThemingControllerTest.php
@@ -695,11 +695,6 @@ class ThemingControllerTest extends TestCase {
 
 		@$expected = new Http\FileDisplayResponse($file);
 		$expected->cacheFor(3600);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$expected->addHeader('Pragma', 'cache');
 		$expected->addHeader('Content-Type', 'text/svg');
 		$expected->addHeader('Content-Disposition', 'attachment; filename="logo"');
 		@$this->assertEquals($expected, $this->themingController->getImage('logo'));
@@ -728,11 +723,6 @@ class ThemingControllerTest extends TestCase {
 
 		@$expected = new Http\FileDisplayResponse($file);
 		$expected->cacheFor(3600);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$expected->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-		$expected->addHeader('Pragma', 'cache');
 		$expected->addHeader('Content-Type', 'image/png');
 		$expected->addHeader('Content-Disposition', 'attachment; filename="background"');
 		@$this->assertEquals($expected, $this->themingController->getImage('background'));
@@ -749,11 +739,6 @@ class ThemingControllerTest extends TestCase {
 
 		$response = new Http\FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'text/css']);
 		$response->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$response->addHeader('Expires', $expires->format(\DateTime::RFC1123));
-		$response->addHeader('Pragma', 'cache');
 
 		$actual = $this->themingController->getStylesheet();
 		$this->assertEquals($response, $actual);
@@ -782,11 +767,6 @@ class ThemingControllerTest extends TestCase {
 
 		$response = new Http\FileDisplayResponse($file, Http::STATUS_OK, ['Content-Type' => 'text/css']);
 		$response->cacheFor(86400);
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT24H'));
-		$response->addHeader('Expires', $expires->format(\DateTime::RFC1123));
-		$response->addHeader('Pragma', 'cache');
 
 		$actual = $this->themingController->getStylesheet();
 		$this->assertEquals($response, $actual);
@@ -824,8 +804,6 @@ class ThemingControllerTest extends TestCase {
 	};
 })();';
 		$expected = new Http\DataDownloadResponse($expectedResponse, 'javascript', 'text/javascript');
-		$expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$expected->addHeader('Pragma', 'cache');
 		$expected->cacheFor(3600);
 		@$this->assertEquals($expected, $this->themingController->getJavascript());
 	}
@@ -860,8 +838,6 @@ class ThemingControllerTest extends TestCase {
 	};
 })();';
 		$expected = new Http\DataDownloadResponse($expectedResponse, 'javascript', 'text/javascript');
-		$expected->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$expected->addHeader('Pragma', 'cache');
 		$expected->cacheFor(3600);
 		@$this->assertEquals($expected, $this->themingController->getJavascript());
 	}
@@ -908,8 +884,6 @@ class ThemingControllerTest extends TestCase {
 				],
 			'display' => 'standalone'
 		]);
-		$response->addHeader('Expires', date(\DateTime::RFC2822, $this->timeFactory->getTime()));
-		$response->addHeader('Pragma', 'cache');
 		$response->cacheFor(3600);
 		$this->assertEquals($response, $this->themingController->getManifest('core'));
 	}
diff --git a/core/Controller/AvatarController.php b/core/Controller/AvatarController.php
index 14709d65fed..11d81ab00b2 100644
--- a/core/Controller/AvatarController.php
+++ b/core/Controller/AvatarController.php
@@ -141,16 +141,8 @@ class AvatarController extends Controller {
 			return $resp;
 		}
 
-		// Let cache this!
-		$resp->addHeader('Pragma', 'public');
 		// Cache for 30 minutes
 		$resp->cacheFor(1800);
-
-		$expires = new \DateTime();
-		$expires->setTimestamp($this->timeFactory->getTime());
-		$expires->add(new \DateInterval('PT30M'));
-		$resp->addHeader('Expires', $expires->format(\DateTime::RFC1123));
-
 		return $resp;
 	}
 
diff --git a/core/Controller/PreviewController.php b/core/Controller/PreviewController.php
index e18487363a1..3dfc4872d01 100644
--- a/core/Controller/PreviewController.php
+++ b/core/Controller/PreviewController.php
@@ -174,17 +174,7 @@ class PreviewController extends Controller {
 		try {
 			$f = $this->preview->getPreview($node, $x, $y, !$a, $mode);
 			$response = new FileDisplayResponse($f, Http::STATUS_OK, ['Content-Type' => $f->getMimeType()]);
-
-			// Let cache this!
-			$response->addHeader('Pragma', 'public');
-
-			// Cache previews for 24H
 			$response->cacheFor(3600 * 24);
-			$expires = new \DateTime();
-			$expires->setTimestamp($this->timeFactory->getTime());
-			$expires->add(new \DateInterval('P1D'));
-			$response->addHeader('Expires', $expires->format(\DateTime::RFC2822));
-
 			return $response;
 		} catch (NotFoundException $e) {
 			return new DataResponse([], Http::STATUS_NOT_FOUND);
diff --git a/lib/public/AppFramework/Http/Response.php b/lib/public/AppFramework/Http/Response.php
index 512b312dae1..a6f5afd3c18 100644
--- a/lib/public/AppFramework/Http/Response.php
+++ b/lib/public/AppFramework/Http/Response.php
@@ -35,6 +35,7 @@
 namespace OCP\AppFramework\Http;
 
 use OCP\AppFramework\Http;
+use OCP\AppFramework\Utility\ITimeFactory;
 
 /**
  * Base class for responses. Also used to just send headers.
@@ -95,12 +96,23 @@ class Response {
 	 * @return $this
 	 * @since 6.0.0 - return value was added in 7.0.0
 	 */
-	public function cacheFor($cacheSeconds) {
-
+	public function cacheFor(int $cacheSeconds) {
 		if($cacheSeconds > 0) {
 			$this->addHeader('Cache-Control', 'max-age=' . $cacheSeconds . ', must-revalidate');
+
+			// Old scool prama caching
+			$this->addHeader('Pragma', 'public');
+
+			// Set expires header
+			$expires = new \DateTime();
+			/** @var ITimeFactory $time */
+			$time = \OC::$server->query(ITimeFactory::class);
+			$expires->setTimestamp($time->getTime());
+			$expires->add(new \DateInterval('PT'.$cacheSeconds.'S'));
+			$this->addHeader('Expires', $expires->format(\DateTime::RFC2822));
 		} else {
 			$this->addHeader('Cache-Control', 'no-cache, no-store, must-revalidate');
+			unset($this->headers['Expires'], $this->headers['Pragma']);
 		}
 
 		return $this;
diff --git a/tests/lib/AppFramework/Http/ResponseTest.php b/tests/lib/AppFramework/Http/ResponseTest.php
index 9267d862600..5c867388369 100644
--- a/tests/lib/AppFramework/Http/ResponseTest.php
+++ b/tests/lib/AppFramework/Http/ResponseTest.php
@@ -27,6 +27,7 @@ namespace Test\AppFramework\Http;
 
 use OCP\AppFramework\Http\Response;
 use OCP\AppFramework\Http;
+use OCP\AppFramework\Utility\ITimeFactory;
 
 
 class ResponseTest extends \Test\TestCase {
@@ -222,15 +223,24 @@ class ResponseTest extends \Test\TestCase {
 
 		$headers = $this->childResponse->getHeaders();
 		$this->assertEquals('no-cache, no-store, must-revalidate', $headers['Cache-Control']);
+		$this->assertFalse(isset($headers['Pragma']));
+		$this->assertFalse(isset($headers['Expires']));
 	}
 
 
 	public function testCacheSeconds() {
+		$time = $this->createMock(ITimeFactory::class);
+		$time->method('getTime')
+			->willReturn('1234567');
+
+		$this->overwriteService(ITimeFactory::class, $time);
+
 		$this->childResponse->cacheFor(33);
 
 		$headers = $this->childResponse->getHeaders();
-		$this->assertEquals('max-age=33, must-revalidate',
-			$headers['Cache-Control']);
+		$this->assertEquals('max-age=33, must-revalidate', $headers['Cache-Control']);
+		$this->assertEquals('public', $headers['Pragma']);
+		$this->assertEquals('Thu, 15 Jan 1970 06:56:40 +0000', $headers['Expires']);
 	}