Mholloway has uploaded a new change for review.

  https://gerrit.wikimedia.org/r/315955

Change subject: Return 204s for empty responses (for RESTBase aggregated 
requests)
......................................................................

Return 204s for empty responses (for RESTBase aggregated requests)

Currently we're returning these as 200s but a 204 (no content) response
code is more accurate.

This is in preparation for patches strengthening feed response format
checking on the server side.

Change-Id: Ibe419c58c24cb96180ea49df30b1fd4a6e47bcb2
---
M lib/feed/featured-image.js
M lib/feed/featured.js
M lib/feed/most-read.js
M lib/feed/news.js
M routes/featured-image.js
M routes/featured.js
M routes/most-read.js
M routes/news.js
M spec.yaml
M test/features/featured/pagecontent.js
M test/features/most-read/most-read.js
M test/features/news/news.js
12 files changed, 22 insertions(+), 22 deletions(-)


  git pull ssh://gerrit.wikimedia.org:29418/mediawiki/services/mobileapps 
refs/changes/55/315955/1

diff --git a/lib/feed/featured-image.js b/lib/feed/featured-image.js
index 8e01366..f6bc226 100644
--- a/lib/feed/featured-image.js
+++ b/lib/feed/featured-image.js
@@ -126,7 +126,7 @@
     }).catch(function(err) {
         if (err.status === 504) {
             if (aggregated) {
-                return BBPromise.resolve({ payload: undefined, meta: undefined 
});
+                return BBPromise.resolve({ empty: true });
             }
             throw new HTTPError({
                 status: 404,
diff --git a/lib/feed/featured.js b/lib/feed/featured.js
index 11c05c4..d89047d 100644
--- a/lib/feed/featured.js
+++ b/lib/feed/featured.js
@@ -76,14 +76,14 @@
 
     if (!dateUtil.validate(dateUtil.hyphenDelimitedDateString(req))) {
         if (aggregated) {
-            return BBPromise.resolve({});
+            return BBPromise.resolve({ empty: true });
         }
         dateUtil.throwDateError();
     }
 
     if (req.params.domain.indexOf('en') !== 0 || 
req.params.domain.indexOf('beta.wmflabs.org') > 0) {
         if (aggregated) {
-            return BBPromise.resolve({});
+            return BBPromise.resolve({ empty: true });
         } else {
             throw new HTTPError({
                 status: 501,
diff --git a/lib/feed/most-read.js b/lib/feed/most-read.js
index 3fcd4fd..ccd55de 100644
--- a/lib/feed/most-read.js
+++ b/lib/feed/most-read.js
@@ -52,7 +52,7 @@
 
     if (!dateUtil.validate(dateUtil.hyphenDelimitedDateString(req))) {
         if (aggregated) {
-            return BBPromise.resolve({});
+            return BBPromise.resolve({ empty: true });
         }
         dateUtil.throwDateError();
     }
@@ -112,7 +112,7 @@
         // Catch and handle the error if this is an aggregated request and the
         // pageview data are not yet loaded.
         if (aggregated && err.status === 404) {
-            return BBPromise.resolve({});
+            return BBPromise.resolve({ empty: true });
         }
         throw err;
     });
diff --git a/lib/feed/news.js b/lib/feed/news.js
index 4932193..d03bf10 100644
--- a/lib/feed/news.js
+++ b/lib/feed/news.js
@@ -41,7 +41,7 @@
     var aggregated = !!req.query.aggregated;
     if (!newsTemplates[lang]) {
         if (aggregated) {
-            return BBPromise.resolve({ payload: undefined, meta: undefined });
+            return BBPromise.resolve({ empty: true });
         }
         throw new HTTPError({
             status: 501,
diff --git a/routes/featured-image.js b/routes/featured-image.js
index bace4c6..ea47d38 100644
--- a/routes/featured-image.js
+++ b/routes/featured-image.js
@@ -26,7 +26,7 @@
 router.get('/image/featured/:yyyy/:mm/:dd', function (req, res) {
     return featured.promise(app, req)
         .then(function (response) {
-            res.status(200);
+            res.status(response.empty ? 204 : 200);
             mUtil.setETagToValue(res, response.meta && response.meta.etag);
             res.json(response.payload || null).end();
         });
diff --git a/routes/featured.js b/routes/featured.js
index e83fb0a..9588462 100644
--- a/routes/featured.js
+++ b/routes/featured.js
@@ -26,7 +26,7 @@
 router.get('/featured/:yyyy/:mm/:dd', function (req, res) {
     return featured.promise(app, req)
         .then(function (response) {
-            res.status(200);
+            res.status(response.empty ? 204 : 200);
             mUtil.setETagToValue(res, response.meta && response.meta.etag);
             mUtil.setContentType(res, mUtil.CONTENT_TYPES.unpublished);
             res.json(response.payload || null).end();
diff --git a/routes/most-read.js b/routes/most-read.js
index 4d6da4e..2da7385 100644
--- a/routes/most-read.js
+++ b/routes/most-read.js
@@ -24,7 +24,7 @@
 router.get('/most-read/:yyyy/:mm/:dd', function (req, res) {
     return mostRead.promise(app, req)
     .then(function (response) {
-        res.status(200);
+        res.status(response.empty ? 204 : 200);
         mUtil.setETagToValue(res, response.meta && response.meta.etag);
         mUtil.setContentType(res, mUtil.CONTENT_TYPES.unpublished);
         res.json(response.payload || null).end();
diff --git a/routes/news.js b/routes/news.js
index fa4811c..0a8b031 100644
--- a/routes/news.js
+++ b/routes/news.js
@@ -23,7 +23,7 @@
 router.get('/news', function (req, res) {
     return news.promise(app, req)
     .then(function (response) {
-        res.status(200);
+        res.status(response.empty ? 204 : 200);
         mUtil.setETagToValue(res, response.meta && response.meta.etag);
         res.json(response.payload || null).end();
     });
diff --git a/spec.yaml b/spec.yaml
index 8510a91..c3acd69 100644
--- a/spec.yaml
+++ b/spec.yaml
@@ -108,7 +108,7 @@
             query:
               aggregated: true
           response:
-            status: 200
+            status: 204
             body: ""
   # from routes/featured-image.js
   /{domain}/v1/media/image/featured/{yyyy}/{mm}/{dd}:
@@ -238,7 +238,7 @@
             query:
               aggregated: true
           response:
-            status: 200
+            status: 204
             body: ""
   # from routes/random.js
   /{domain}/v1/page/random/title:
@@ -282,7 +282,7 @@
             query:
               aggregated: true
           response:
-            status: 200
+            status: 204
             body: ""
   # from routes/media.js
   /{domain}/v1/page/media/{title}:
diff --git a/test/features/featured/pagecontent.js 
b/test/features/featured/pagecontent.js
index e07afb6..b074726 100644
--- a/test/features/featured/pagecontent.js
+++ b/test/features/featured/pagecontent.js
@@ -60,14 +60,14 @@
             });
     });
 
-    it('unsupported language with aggregated=true should return 200', 
function() {
+    it('unsupported language with aggregated=true should return 204', 
function() {
         return preq.get({
             uri: server.config.uri + 
'zh.wikipedia.org/v1/page/featured/2016/04/15',
             query: { aggregated: true }
         })
         .then(function(res) {
-            assert.status(res, 200);
-            assert.deepEqual(!!res.body, false, 'Expected the body to be 
empty');
+            assert.status(res, 204);
+            assert.deepEqual(!!res.body.data, false, 'Expected the body to be 
empty');
         });
     });
 
diff --git a/test/features/most-read/most-read.js 
b/test/features/most-read/most-read.js
index 0e37f47..52b989f 100644
--- a/test/features/most-read/most-read.js
+++ b/test/features/most-read/most-read.js
@@ -66,12 +66,12 @@
         });
     });
 
-    it('404 for future date should be suppressed when aggregated flag is set', 
function() {
+    it('Request for future date should return 204 when aggregated flag is 
set', function() {
         return preq.get({ uri: server.config.uri + 
'en.wikipedia.org/v1/page/most-read/' + futureDateString,
                         query: { aggregated: true }})
           .then(function(res) {
-            assert.status(res, 200);
-            assert.deepEqual(!!res.body, false, 'Expected the body to be 
empty');
+            assert.status(res, 204);
+            assert.deepEqual(!!res.body.data, false, 'Expected the body to be 
empty');
         });
     });
 });
diff --git a/test/features/news/news.js b/test/features/news/news.js
index a5eb076..5624461 100644
--- a/test/features/news/news.js
+++ b/test/features/news/news.js
@@ -63,14 +63,14 @@
         });
     });
 
-    it('unsupported language with aggregated=true should return 200', 
function() {
+    it('unsupported language with aggregated=true should return 204', 
function() {
         return preq.get({
             uri: server.config.uri + 'is.wikipedia.org/v1/page/news',
             query: { aggregated: true }
         })
         .then(function(res) {
-            assert.status(res, 200);
-            assert.deepEqual(!!res.body, false, 'Expected the body to be 
empty');
+            assert.status(res, 204);
+            assert.deepEqual(!!res.body.data, false, 'Expected the body to be 
empty');
         });
     });
 

-- 
To view, visit https://gerrit.wikimedia.org/r/315955
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings

Gerrit-MessageType: newchange
Gerrit-Change-Id: Ibe419c58c24cb96180ea49df30b1fd4a6e47bcb2
Gerrit-PatchSet: 1
Gerrit-Project: mediawiki/services/mobileapps
Gerrit-Branch: master
Gerrit-Owner: Mholloway <mhollo...@wikimedia.org>

_______________________________________________
MediaWiki-commits mailing list
MediaWiki-commits@lists.wikimedia.org
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits

Reply via email to