Merge 70b6821 into 479f95f

wikimedia · Jun 22, 2018 · 42c0087 · 42c0087
2 parents 479f95f + 70b6821
commit 42c0087
Show file tree

Hide file tree

Showing 8 changed files with 205 additions and 10 deletions.
diff --git a/lib/language_variants_filter.js b/lib/language_variants_filter.js
@@ -39,7 +39,7 @@ module.exports = (hyper, req, next, options, specInfo) => {
                 delete req.headers['accept-language'];
                 return next(hyper, req)
                 // TODO: Eventually we want to store the Vary header we get from Parsoid!
-                .then((res) => mwUtil.canConvertLangVariant(hyper, req, revision.page_language)
+                .then(res => mwUtil.canConvertLangVariant(hyper, req, revision.page_language)
                 .then((canConvert) => {
                     if (canConvert) {
                         res.headers.vary = 'accept-language';
@@ -87,7 +87,10 @@ module.exports = (hyper, req, next, options, specInfo) => {
                     return res;
                 }));
             } else {
-                // TODO: It's something else, so just forward to MCS
+                // TODO: Eventually we want to think of a better way to support
+                // other content pure-fetching, but right now let's use a magic
+                // header.
+                req.headers['cache-control'] = 'no-cache,no-store';
                 return next(hyper, req);
             }
         });

diff --git a/lib/mwUtil.js b/lib/mwUtil.js
@@ -156,9 +156,17 @@ mwUtil.normalizeContentType = (res) => {
 /**
  * Checks whether the request is a 'no-cache' request
  * @param {Object} req
+ * @return {boolean}
  */
 mwUtil.isNoCacheRequest = req => req.headers && /no-cache/i.test(req.headers['cache-control']);
 
+/**
+ * Checks whether the request is a 'no-store' request
+ * @param {Object} req
+ * @return {boolean}
+ */
+mwUtil.isNoStoreRequest = req => req.headers && /no-store/i.test(req.headers['cache-control']);
+
 mwUtil.parseRevision = (rev, bucketName) => {
     if (!/^[0-9]+/.test(rev)) {
         throw new HTTPError({

diff --git a/sys/key_value.js b/sys/key_value.js
@@ -142,6 +142,15 @@ class KVBucket {
             tid = tid || uuid.now().toString();
         }
 
+        if (mwUtil.isNoStoreRequest(req)) {
+            return {
+                status: 202,
+                headers: {
+                    etag: req.headers && req.headers.etag || mwUtil.makeETag('0', tid)
+                }
+            };
+        }
+
         const doPut = () => hyper.put({
             uri: new URI([rp.domain, 'sys', 'table', rp.bucket, '']),
             body: {

diff --git a/sys/mobileapps.js b/sys/mobileapps.js
@@ -110,12 +110,19 @@ class MobileApps {
         }
 
         return hyper.get({
-            uri: new URI(serviceURI)
+            uri: new URI(serviceURI),
+            headers: {
+                'accept-language': req.headers['accept-language']
+            }
         })
         .then((res) => {
+            if (mwUtils.isNoStoreRequest(req)) {
+                return res;
+            }
             return hyper.put({
                 uri: new URI([rp.domain, 'sys', 'mobile_bucket', 'all', rp.title,
-                    res.body.lead.revision]),
+                    res.body.lead.revision,
+                    mwUtils.parseETag(res.headers.etag).tid]),
                 body: {
                     lead: {
                         headers: res.headers,

diff --git a/test/features/pagecontent/language_variants.js b/test/features/pagecontent/language_variants.js
@@ -106,4 +106,161 @@ describe('Language variants', function() {
             assert.deepEqual(/2\. Ovo je testna stranica/.test(res.body), true);
         });
     });
+
+    it('should request summary with no variant and store it', () => {
+        let storedEtag;
+        return preq.get({
+            uri: `${server.config.variantsWikiBucketURL}/summary/${variantsPageTitle}`
+        })
+        .then((res) => {
+            storedEtag = res.headers.etag;
+            assert.deepEqual(res.status, 200);
+            assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control_with_client_caching');
+            // TODO: Pass in MCS assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
+            assert.deepEqual('1. Ово је тестна страница', res.body.extract);
+            // Not try fetching again with a default variant and see if etag matches
+            return preq.get({
+                uri: `${server.config.variantsWikiBucketURL}/summary/${variantsPageTitle}`,
+                headers: {
+                    'accept-language': 'sr'
+                }
+            });
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control_with_client_caching');
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.deepEqual(res.headers.etag, storedEtag);
+            assert.deepEqual('1. Ово је тестна страница', res.body.extract);
+            // Now try the impossible variant and see that stored one is served again.
+            return preq.get({
+                uri: `${server.config.variantsWikiBucketURL}/summary/${variantsPageTitle}`,
+                headers: {
+                    'accept-language': 'sr-this-is-no-a-variant'
+                }
+            });
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control_with_client_caching');
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.deepEqual(res.headers.etag, storedEtag);
+            assert.deepEqual('1. Ово је тестна страница', res.body.extract);
+        });
+    });
+
+    it('should request summary with latin variant and not store it', () => {
+        return preq.get({
+            uri: `${server.config.variantsWikiBucketURL}/summary/${variantsPageTitle}`,
+            headers: {
+                'accept-language': 'sr-el'
+            }
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            // TODO: Pass in MCS assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control_with_client_caching');
+            // TODO: Pass in MCS assert.deepEqual(res.headers['content-language'], 'sr-el');
+            assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
+            assert.deepEqual('1. Ovo je testna stranica', res.body.extract);
+            // Try again without variant to see that stored didn't change
+            return preq.get({
+                uri: `${server.config.variantsWikiBucketURL}/summary/${variantsPageTitle}`,
+                headers: {
+                    'accept-language': 'sr'
+                }
+            });
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control_with_client_caching');
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
+            assert.deepEqual('1. Ово је тестна страница', res.body.extract);
+        });
+    });
+
+    it('should request mobile-sections with no variant and store it', () => {
+        return preq.get({
+            uri: `${server.config.variantsWikiBucketURL}/mobile-sections/${variantsPageTitle}`
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
+            // TODO: Pass in MCS assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.checkString(res.headers.etag, /^(:?W\/)?"\d+\/[a-f0-9-]+"$/);
+            assert.deepEqual(/1\. Ово је тестна страница/.test(JSON.stringify(res.body)), true);
+            assert.deepEqual(/2\. Ovo je testna stranica/.test(JSON.stringify(res.body)), true);
+            // Not try fetching again with a default variant and see if etag matches
+            return preq.get({
+                uri: `${server.config.variantsWikiBucketURL}/mobile-sections/${variantsPageTitle}`,
+                headers: {
+                    'accept-language': 'sr'
+                }
+            });
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.deepEqual(/1\. Ово је тестна страница/.test(JSON.stringify(res.body)), true);
+            assert.deepEqual(/2\. Ovo je testna stranica/.test(JSON.stringify(res.body)), true);
+            // Now try the impossible variant and see that stored one is served again.
+            return preq.get({
+                uri: `${server.config.variantsWikiBucketURL}/mobile-sections/${variantsPageTitle}`,
+                headers: {
+                    'accept-language': 'sr-this-is-no-a-variant'
+                }
+            });
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.deepEqual(/1\. Ово је тестна страница/.test(JSON.stringify(res.body)), true);
+            assert.deepEqual(/2\. Ovo je testna stranica/.test(JSON.stringify(res.body)), true);
+        });
+    });
+
+    it('should request mobile-sections with latin variant and not store it', () => {
+        return preq.get({
+            uri: `${server.config.variantsWikiBucketURL}/mobile-sections/${variantsPageTitle}`,
+            headers: {
+                'accept-language': 'sr-el'
+            }
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            // TODO: Pass in MCS assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
+            // TODO: Pass in MCS assert.deepEqual(res.headers['content-language'], 'sr-el');
+            assert.checkString(res.headers.etag, /^(:?W\/)?"\d+\/[a-f0-9-]+"$/);
+            assert.deepEqual(/1\. Ovo je testna stranica/.test(JSON.stringify(res.body)), true);
+            assert.deepEqual(/2\. Ovo je testna stranica/.test(JSON.stringify(res.body)), true);
+            // Try again without variant to see that stored didn't change
+            return preq.get({
+                uri: `${server.config.variantsWikiBucketURL}/mobile-sections/${variantsPageTitle}`,
+                headers: {
+                    'accept-language': 'sr'
+                }
+            });
+        })
+        .then((res) => {
+            assert.deepEqual(res.status, 200);
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
+            assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
+            // TODO: Pass in MCS, store in RB assert.deepEqual(res.headers['content-language'], 'sr');
+            assert.checkString(res.headers.etag, /^(:?W\/)?"\d+\/[a-f0-9-]+"$/);
+            assert.deepEqual(/1\. Ово је тестна страница/.test(JSON.stringify(res.body)), true);
+            assert.deepEqual(/2\. Ovo je testna stranica/.test(JSON.stringify(res.body)), true);
+        });
+    });
 });
diff --git a/v1/mobileapps.yaml b/v1/mobileapps.yaml
@@ -9,6 +9,7 @@ paths:
         options:
           redirect_cache_control: '{{options.response_cache_control}}'
       - path: lib/ensure_content_type.js
+      - path: lib/language_variants_filter.js
     get:
       tags:
         - Mobile
@@ -83,6 +84,7 @@ paths:
               uri: /{domain}/sys/mobileapps/mobile-sections/{title}{/revision}
               headers:
                 cache-control: '{{cache-control}}'
+                accept-language: '{{accept-language}}'
             return:
               status: '{{from_backend.status}}'
               headers: '{{ merge({"cache-control": options.response_cache_control},
@@ -183,6 +185,7 @@ paths:
               uri: /{domain}/sys/mobileapps/mobile-sections-lead/{title}{/revision}
               headers:
                 cache-control: '{{cache-control}}'
+                accept-language: '{{accept-language}}'
             return:
               status: '{{from_backend.status}}'
               headers: '{{ merge({"cache-control": options.response_cache_control},
@@ -271,6 +274,7 @@ paths:
               uri: /{domain}/sys/mobileapps/mobile-sections-remaining/{title}{/revision}
               headers:
                 cache-control: '{{cache-control}}'
+                accept-language: '{{accept-language}}'
             return:
               status: '{{from_backend.status}}'
               headers: '{{ merge({"cache-control": options.response_cache_control},

diff --git a/v1/summary.yaml b/v1/summary.yaml
@@ -145,15 +145,16 @@ paths:
             request:
               method: put
               uri: /{domain}/sys/key_value/page_summary/{request.params.title}
-              headers: '{{merge({"if-none-hash-match": "*"}, extract.headers)}}'
+              headers: '{{merge({"if-none-hash-match": "*"}, extract.headers, "cache-control": request.headers.cache-control)}}'
               body: '{{extract.body}}'
             # With the if-none-hash-match header the storage will return 412
             # if the content is not changed. In that case, return from the
             # handler completely, and avoid issuing purges.
+            # Also if we didn't really store anything, return as well.
             catch:
-              status: 412
+              status: [ 412, 202 ]
             return_if:
-              status: 412
+              status: [ 412, 202 ]
             return:
               status: 200
               headers:
@@ -164,6 +165,8 @@ paths:
         - emit_change_event:
             request:
               method: post
+              headers:
+                'cache-control': '{{request.headers.cache-control}}'
               uri: /{domain}/sys/events/
               body:
                 - meta:

diff --git a/v1/summary_new.yaml b/v1/summary_new.yaml
@@ -18,6 +18,7 @@ paths:
         options:
           redirect_cache_control: '{{options.response_cache_control}}'
       - path: ./lib/ensure_content_type.js
+      - path: ./lib/language_variants_filter.js
     get:
       tags:
         - Page content
@@ -112,6 +113,8 @@ paths:
         - extract:
             request:
               method: get
+              headers:
+                accept-language: '{{accept-language}}'
               uri: '{{options.host}}/{domain}/v1/page/summary/{title}'
             response:
               status: '{{extract.status}}'
@@ -121,15 +124,16 @@ paths:
             request:
               method: put
               uri: /{domain}/sys/key_value/page_summary/{request.params.title}
-              headers: '{{merge({"if-none-hash-match": "*"}, extract.headers)}}'
+              headers: '{{merge({"if-none-hash-match": "*", "cache-control": request.headers.cache-control}, extract.headers)}}'
               body: '{{extract.body}}'
             # With the if-none-hash-match header the storage will return 412
             # if the content is not changed. In that case, return from the
             # handler completely, and avoid issuing purges.
+            # Also don't emit an event if nothing new was actually stored (code 202)
             catch:
-              status: 412
+              status: [ 202, 412 ]
             return_if:
-              status: 412
+              status: [ 202, 412 ]
             return:
               status: 200
               headers: