pcs/summary: Use if-unmodified-since to avoid unnecessary rendering

* Compare tid with if-unmodified-since value to avoid rendering old content * Rename the function to avoid confusion with `if-modified-since` header
wikimedia · Jul 30, 2024 · bd6f8c6 · bd6f8c6
1 parent 02f1073
commit bd6f8c6
Show file tree

Hide file tree

Showing 5 changed files with 58 additions and 36 deletions.
diff --git a/lib/mwUtil.js b/lib/mwUtil.js
@@ -182,6 +182,33 @@ mwUtil.isNoCacheRequest = (req) => req.headers && /no-cache/i.test(req.headers['
  */
 mwUtil.isNoStoreRequest = (req) => req.headers && /no-store/i.test(req.headers['cache-control']);
 
+/**
+ * Checks whether the request is an 'if-unmodified-since' request
+ * @param   {Object}    req
+ * @return  {boolean}
+ */
+mwUtil.isUnmodifiedSinceRequest = (req) => Boolean(req.headers && req.headers['if-unmodified-since']);
+
+/**
+ * Checks whether the response has been modified since the timestamp
+ * in `if-unmodified-since` header of the request
+ * @param  {Object} req the request
+ * @param  {Object} res the response
+ * @return {boolean}    true if content has beed modified
+ */
+mwUtil.isUnmodifiedSince = (req, res) => {
+    try {
+        if (req.headers['if-unmodified-since']) {
+            const jobTime = Date.parse(req.headers['if-unmodified-since']);
+            const revInfo = mwUtil.parseETag(res.headers.etag);
+            return revInfo && uuidUtils.getDate(revInfo.tid) < jobTime;
+        }
+    } catch (e) {
+        // Ignore errors from date parsing
+    }
+    return true;
+};
+
 mwUtil.parseRevision = (rev, bucketName) => {
     if (!/^[0-9]+/.test(rev)) {
         throw new HTTPError({

diff --git a/sys/key_value.js b/sys/key_value.js
@@ -57,7 +57,10 @@ class KVBucket {
     }
 
     getRevision(hyper, req) {
-        if (mwUtil.isNoCacheRequest(req)) {
+        const isNoCacheRequest = mwUtil.isNoCacheRequest(req);
+        const isUnmodifiedSinceRequest = mwUtil.isUnmodifiedSinceRequest(req);
+
+        if (isNoCacheRequest && !isUnmodifiedSinceRequest) {
             throw new HTTPError({ status: 404 });
         }
 
@@ -75,11 +78,22 @@ class KVBucket {
         return hyper.get(storeReq).then((dbResult) => {
             if (dbResult.body && dbResult.body.items && dbResult.body.items.length) {
                 const row = dbResult.body.items[0];
-                return {
+                const result = {
                     status: 200,
                     headers: row.headers,
                     body: row.value
                 };
+
+                if (isNoCacheRequest && !mwUtil.isUnmodifiedSince(result)) {
+                    throw new HTTPError({
+                        status: 412,
+                        body: {
+                            type: 'precondition_failed',
+                            detail: 'The precondition failed'
+                        }
+                    });
+                }
+                return result;
             } else {
                 throw new HTTPError({
                     status: 404,

diff --git a/sys/parsoid.js b/sys/parsoid.js
@@ -6,7 +6,6 @@ const URI = HyperSwitch.URI;
 const HTTPError = HyperSwitch.HTTPError;
 
 const uuidv1 = require('uuid').v1;
-const uuidUtils = require('../lib/uuidUtils');
 
 const mwUtil = require('../lib/mwUtil');
 
@@ -45,26 +44,6 @@ function extractTidMeta(html) {
     return tidMatch && (tidMatch[1] || tidMatch[2]);
 }
 
-/**
- *  Checks whether the content has been modified since the timestamp
- *  in `if-unmodified-since` header of the request
- * @param  {Object} req the request
- * @param  {Object} res the response
- * @return {boolean}    true if content has beed modified
- */
-function isModifiedSince(req, res) {
-    try {
-        if (req.headers['if-unmodified-since']) {
-            const jobTime = Date.parse(req.headers['if-unmodified-since']);
-            const revInfo = mwUtil.parseETag(res.headers.etag);
-            return revInfo && uuidUtils.getDate(revInfo.tid) >= jobTime;
-        }
-    } catch (e) {
-        // Ignore errors from date parsing
-    }
-    return false;
-}
-
 /** HTML resource_change event emission
  * @param   {HyperSwitch}   hyper           the hyperswitch router object
  * @param   {Object}        req             the request
@@ -568,7 +547,7 @@ class ParsoidService {
                 contentReq = contentReq.then((res) => {
                     res.headers['x-restbase-cache'] = 'nocache';
 
-                    if (isModifiedSince(req, res)) { // Already up to date, nothing to do.
+                    if (!mwUtil.isUnmodifiedSince(req, res)) { // Already up to date, nothing to do.
                         throw new HTTPError({
                             status: 412,
                             body: {

diff --git a/v1/pcs/stored_endpoint.js b/v1/pcs/stored_endpoint.js
@@ -35,22 +35,23 @@ class PCSEndpoint {
             });
         }
 
-        if (mwUtils.isNoCacheRequest(req)) {
-            return this._fetchFromPCSAndStore(hyper, req)
-            .tap((res) => {
-                this._injectCacheControl.bind(this)(res);
-                hyper.metrics.endTiming([
-                    'pcs_getContent_latency',
-                    'pcs_getContent_latency_no_cache',
-                    `pcs_getContent_latency_${rp.domain}`
-                ], startTime);
-            });
-        }
-
         return hyper.get({
             uri: new URI([rp.domain, 'sys', 'key_value', this._options.name, rp.title])
         })
         .then((res) => {
+            if (mwUtils.isNoCacheRequest(req)) {
+                if (!mwUtils.isUnmodifiedSince(req, res)) {
+                    throw new HyperSwitch.HTTPError({
+                        status: 412,
+                        body: {
+                            type: 'precondition_failed',
+                            detail: 'The precondition failed'
+                        }
+                    });
+                }
+                return this._fetchFromPCSAndStore(hyper, req);
+            }
+
             if (!rp.revision ||
                 `${mwUtils.parseETag(res.headers.etag).rev}` === `${rp.revision}`) {
                 return res;

diff --git a/v1/summary_new.yaml b/v1/summary_new.yaml
@@ -107,6 +107,7 @@ paths:
               method: get
               headers:
                 cache-control: '{{cache-control}}'
+                if-unmodified-since: '{{if-unmodified-since}}'
               uri: /{domain}/sys/key_value/page_summary/{request.params.title}
             catch:
               status: 404