jenkins-bot has submitted this change and it was merged. (
https://gerrit.wikimedia.org/r/322084 )
Change subject: Quote the ETag being set and fix it up in some endpoints
......................................................................
Quote the ETag being set and fix it up in some endpoints
Some endpoints, like the featured image, featured page, most read and
news were emitting an ETag containing only the revision number. This
commit adds the time UUID portion needed to distinguish different
renderings of the same revision content. Additionally, the ETag needs to
be quoted as to ensure it's being interpreted as a string.
Bug: T150886
Bug: T150969
Change-Id: I4fbf8b0a7639f298213b1fbe475c522cf6e2aa19
---
M lib/feed/featured-image.js
M lib/feed/most-read.js
M lib/mobile-util.js
M routes/announcements.js
M routes/definition.js
M routes/featured-image.js
M routes/featured.js
M routes/media.js
M routes/mobile-sections.js
M routes/mobile-text.js
M routes/most-read.js
M routes/news.js
M routes/random.js
M test/features/featured-image/pagecontent.js
M test/features/featured/pagecontent.js
M test/features/media/pagecontent.js
M test/features/mobile-sections/pagecontent.js
M test/utils/headers.js
18 files changed, 38 insertions(+), 77 deletions(-)
Approvals:
Mobrovac: Looks good to me, approved
BearND: Looks good to me, approved
jenkins-bot: Verified
diff --git a/lib/feed/featured-image.js b/lib/feed/featured-image.js
index 8eb3327..727d87c 100644
--- a/lib/feed/featured-image.js
+++ b/lib/feed/featured-image.js
@@ -116,9 +116,14 @@
return ret;
}
-function buildEtag(page) {
- return page
- && `${page.pageid}/${mwapi.getRevisionFromExtract(page)}`;
+function buildMeta(page) {
+ if (!page) {
+ return {};
+ }
+ return {
+ revision: page.pageid,
+ tid: mwapi.getRevisionFromExtract(page)
+ };
}
/** @param {!domino.Document} doc
@@ -146,7 +151,7 @@
if (!dateUtil.validate(dateUtil.hyphenDelimitedDateString(req))) {
if (aggregated) {
- return BBPromise.resolve({});
+ return BBPromise.resolve({ meta: {} });
}
dateUtil.throwDateError();
}
@@ -160,9 +165,7 @@
const page = getPageObject(response);
ret = {
payload: buildPayload(page, lang),
- meta: {
- etag: buildEtag(page)
- }
+ meta: buildMeta(page)
};
const parsoidReq = Object.create(req);
@@ -187,7 +190,7 @@
}).catch((err) => {
if (err.status === 504) {
if (aggregated) {
- return BBPromise.resolve({});
+ return BBPromise.resolve({ meta: {} });
}
throw new HTTPError({
status: 404,
diff --git a/lib/feed/most-read.js b/lib/feed/most-read.js
index 17aa1f7..56980f7 100644
--- a/lib/feed/most-read.js
+++ b/lib/feed/most-read.js
@@ -93,7 +93,7 @@
if (!dateUtil.validate(dateUtil.hyphenDelimitedDateString(req))) {
if (aggregated) {
- return BBPromise.resolve({});
+ return BBPromise.resolve({ meta: {} });
}
dateUtil.throwDateError();
}
@@ -181,7 +181,7 @@
articles: results
},
meta: {
- etag: mUtil.getDateStringEtag(dateUtil.dateStringFrom(req))
+ revision: dateUtil.dateStringFrom(req)
}
};
}
@@ -190,7 +190,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({ meta: {} });
}
throw err;
});
diff --git a/lib/mobile-util.js b/lib/mobile-util.js
index d3497a7..37bf3f8 100644
--- a/lib/mobile-util.js
+++ b/lib/mobile-util.js
@@ -64,38 +64,24 @@
};
/**
- * Sets the ETag header on the response object to a specified value.
- *
- * @param {Object} response the HTTPResponse object on which to set the header
- * @param {Object} value to set the ETag to
- */
-mUtil.setETagToValue = function(response, value) {
- if (value) {
- response.set('etag', `${value}`);
- }
-};
-
-/**
- * Sets the ETag header on the response object. First, the request object is
- * checked for the X-Restbase-ETag header. If present, that is used as the ETag
- * header. Otherwise, a new ETag is created, comprised of the revision ID and
+ * Sets the ETag header on the response objecti, comprised of the revision ID
and
* the time UUID. If the latter is not given, the current time stamp is used to
* generate it.
*
- * @param {Object} request the HTTPRequest object to check for the
X-Restbase-ETag header
* @param {Object} response the HTTPResponse object on which to set the header
* @param {integer} revision the revision ID to use
* @param {string} tid the time UUID to use; optional
*/
-mUtil.setETag = function(request, response, revision, tid) {
- if (request && request.headers && request.headers['x-restbase-etag']) {
- response.set('etag', request.headers['x-restbase-etag']);
+mUtil.setETag = function(response, revision, tid) {
+ // we want to bail out if the revision hasn't been supplied, except
+ // in the case revision === 0 because 0 is actually a valid rev_id
+ if (!revision && revision !== 0) {
return;
}
if (!tid) {
tid = uuid.now().toString();
}
- mUtil.setETagToValue(response, `${revision}/${tid}`);
+ response.set('etag', `"${revision}/${tid}"`);
};
/**
@@ -182,14 +168,6 @@
}
}
}
-};
-
-/**
- * Construct an etag using the date from the feed endpoint request.
- * Example: '2016/03/05' -> '20160305/bb6b7552-2cea-11e6-8490-df3f275c37a6'
- */
-mUtil.getDateStringEtag = function(dateString) {
- return `${dateString}/${uuid.now().toString()}`;
};
mUtil.getRbPageSummaryUrl = function(restbaseTpl, domain, title) {
diff --git a/routes/announcements.js b/routes/announcements.js
index 524e302..e47fb03 100644
--- a/routes/announcements.js
+++ b/routes/announcements.js
@@ -65,7 +65,7 @@
res.status(200);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.announcements);
- mUtil.setETag(req, res, hash);
+ mUtil.setETag(res, hash);
res.set('cache-control', 'public, max-age=7200, s-maxage=14400');
res.json(json);
});
diff --git a/routes/definition.js b/routes/definition.js
index 4147a75..6bb39a5 100644
--- a/routes/definition.js
+++ b/routes/definition.js
@@ -26,7 +26,7 @@
return parsoid.definitionPromise(app, req)
.then((response) => {
res.status(200);
- mUtil.setETag(req, res, response.meta.revision);
+ mUtil.setETag(res, response.meta.revision);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.definition);
res.json(response.payload).end();
});
diff --git a/routes/featured-image.js b/routes/featured-image.js
index d6ffb65..0ee352f 100644
--- a/routes/featured-image.js
+++ b/routes/featured-image.js
@@ -27,7 +27,7 @@
return featured.promise(app, req)
.then((response) => {
res.status(!response.payload ? 204 : 200);
- mUtil.setETagToValue(res, response.meta && response.meta.etag);
+ mUtil.setETag(res, response.meta.revision, response.meta.tid);
res.json(response.payload || null).end();
});
});
diff --git a/routes/featured.js b/routes/featured.js
index 7235e15..a5807aa 100644
--- a/routes/featured.js
+++ b/routes/featured.js
@@ -27,7 +27,7 @@
return featured.promise(app, req)
.then((response) => {
res.status(!response.payload ? 204 : 200);
- mUtil.setETagToValue(res, response.meta && response.meta.etag);
+ mUtil.setETag(res, response.meta && response.meta.etag);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.unpublished);
res.json(response.payload || null).end();
});
diff --git a/routes/media.js b/routes/media.js
index d4282a1..3b722c8 100644
--- a/routes/media.js
+++ b/routes/media.js
@@ -26,7 +26,7 @@
media: gallery.collectionPromise(app, req)
}).then((response) => {
res.status(200);
- mUtil.setETag(req, res, response.page.revision);
+ mUtil.setETag(res, response.page.revision);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.unpublished);
res.json(response.media).end();
});
diff --git a/routes/mobile-sections.js b/routes/mobile-sections.js
index 3f1ef2f..97094ba 100644
--- a/routes/mobile-sections.js
+++ b/routes/mobile-sections.js
@@ -212,7 +212,7 @@
}).then((response) => {
response = buildAll(response, removeNodes);
res.status(200);
- mUtil.setETag(req, res, response.lead.revision);
+ mUtil.setETag(res, response.lead.revision);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.mobileSections);
res.json(response).end();
});
@@ -231,7 +231,7 @@
}).then((response) => {
response = buildLead(response, removeNodes);
res.status(200);
- mUtil.setETag(req, res, response.revision);
+ mUtil.setETag(res, response.revision);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.mobileSections);
res.json(response).end();
});
@@ -261,7 +261,7 @@
page: parsoid.pageContentPromise(app, req)
}).then((response) => {
res.status(200);
- mUtil.setETag(req, res, response.page.revision);
+ mUtil.setETag(res, response.page.revision);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.mobileSections);
res.json(buildRemaining(response)).end();
});
@@ -276,7 +276,7 @@
page: parsoid.pageContentPromise(app, req)
}).then((response) => {
res.status(200);
- mUtil.setETag(req, res, response.page.revision);
+ mUtil.setETag(res, response.page.revision);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.mobileSections);
res.json(buildReferences(response)).end();
});
diff --git a/routes/mobile-text.js b/routes/mobile-text.js
index 2ce0ded..109325f 100644
--- a/routes/mobile-text.js
+++ b/routes/mobile-text.js
@@ -143,7 +143,7 @@
}
res.status(200);
- mUtil.setETag(req, res, apiRes.body.mobileview.revision);
+ mUtil.setETag(res, apiRes.body.mobileview.revision);
res.json(apiRes.body.mobileview).end();
});
});
diff --git a/routes/most-read.js b/routes/most-read.js
index c6a72a2..1206fce 100644
--- a/routes/most-read.js
+++ b/routes/most-read.js
@@ -25,7 +25,7 @@
return mostRead.promise(app, req)
.then((response) => {
res.status(!response.payload ? 204 : 200);
- mUtil.setETagToValue(res, response.meta && response.meta.etag);
+ mUtil.setETag(res, response.meta.revision);
mUtil.setContentType(res, mUtil.CONTENT_TYPES.unpublished);
res.json(response.payload || null).end();
});
diff --git a/routes/news.js b/routes/news.js
index 512c97d..75ed0f0 100644
--- a/routes/news.js
+++ b/routes/news.js
@@ -24,7 +24,7 @@
return news.promise(app, req)
.then((response) => {
res.status(!response.payload ? 204 : 200);
- mUtil.setETagToValue(res, response.meta && response.meta.etag);
+ mUtil.setETag(res, response.meta && response.meta.etag);
res.json(response.payload || null).end();
});
});
diff --git a/routes/random.js b/routes/random.js
index 6e3daf2..fbf078f 100644
--- a/routes/random.js
+++ b/routes/random.js
@@ -33,7 +33,7 @@
return randomPage.promise(app, req)
.then((result) => {
res.status(200);
- mUtil.setETag(req, res, result.meta.etag);
+ mUtil.setETag(res, result.meta.etag);
res.json(mwapi.buildTitleResponse(result.payload)).end();
});
});
@@ -52,7 +52,7 @@
return randomPage.promise(app, req)
.then((result) => {
res.status(200);
- mUtil.setETag(req, res, result.meta.etag);
+ mUtil.setETag(res, result.meta.etag);
res.json(result.payload).end();
});
});
diff --git a/test/features/featured-image/pagecontent.js
b/test/features/featured-image/pagecontent.js
index e29b0b2..4b7cfdd 100644
--- a/test/features/featured-image/pagecontent.js
+++ b/test/features/featured-image/pagecontent.js
@@ -23,7 +23,7 @@
.then((res) => {
assert.status(res, 200);
// the page id should be stable but not the revision:
- assert.ok(res.headers.etag.indexOf('42184395/') === 0);
+ assert.ok(res.headers.etag.startsWith('"42184395/'));
assert.equal(res.body.title, title);
assert.equal(res.body.thumbnail.source,
'https://upload.wikimedia.org/wikipedia/commons/thumb/e/e3/Iglesia_de_La_Compa%C3%B1%C3%ADa%2C_Quito%2C_Ecuador%2C_2015-07-22%2C_DD_116-118_HDR.JPG/640px-Iglesia_de_La_Compa%C3%B1%C3%ADa%2C_Quito%2C_Ecuador%2C_2015-07-22%2C_DD_116-118_HDR.JPG');
assert.equal(res.body.image.source,
'https://upload.wikimedia.org/wikipedia/commons/e/e3/Iglesia_de_La_Compa%C3%B1%C3%ADa%2C_Quito%2C_Ecuador%2C_2015-07-22%2C_DD_116-118_HDR.JPG');
diff --git a/test/features/featured/pagecontent.js
b/test/features/featured/pagecontent.js
index d2a7386..8a3f50a 100644
--- a/test/features/featured/pagecontent.js
+++ b/test/features/featured/pagecontent.js
@@ -30,7 +30,7 @@
return preq.get({ uri:
`${server.config.uri}en.wikipedia.org/v1/page/featured/2016/04/15` })
.then((res) => {
assert.status(res, 200);
- assert.ok(res.headers.etag.indexOf('50089449') === 0);
+ assert.ok(res.headers.etag.startsWith('"50089449/'));
assert.equal(res.body.$merge,
'https://en.wikipedia.org/api/rest_v1/page/summary/Cosmic_Stories_and_Stirring_Science_Stories');
});
});
@@ -39,7 +39,7 @@
return preq.get({ uri:
`${server.config.uri}en.wikipedia.org/v1/page/featured/2016/04/29` })
.then((res) => {
assert.status(res, 200);
- assert.ok(res.headers.etag.indexOf('50282338') === 0);
+ assert.ok(res.headers.etag.startsWith('"50282338/'));
assert.equal(res.body.$merge,
'https://en.wikipedia.org/api/rest_v1/page/summary/Lightning_(Final_Fantasy)');
});
});
diff --git a/test/features/media/pagecontent.js
b/test/features/media/pagecontent.js
index d8c790f..2915565 100644
--- a/test/features/media/pagecontent.js
+++ b/test/features/media/pagecontent.js
@@ -21,16 +21,6 @@
return
headers.checkHeaders(`${server.config.uri}en.wikipedia.org/v1/page/media/Foobar`);
});
- it('return the sent ETag', () => {
- return preq.get({
- uri: `${server.config.uri}en.wikipedia.org/v1/page/media/Foobar`,
- headers: { 'x-restbase-etag':
'123456/c3421381-7109-11e5-ac43-8c7f067c3520' }
- }).then((res) => {
- assert.status(res, 200);
- assert.deepEqual(res.headers.etag,
'123456/c3421381-7109-11e5-ac43-8c7f067c3520');
- });
- });
-
it('Sections/deep page should have no media items', () => {
const uri =
`${server.config.uri}test.wikipedia.org/v1/page/media/Sections%2Fdeep`;
return preq.get({ uri })
diff --git a/test/features/mobile-sections/pagecontent.js
b/test/features/mobile-sections/pagecontent.js
index ee67b4d..82e656a 100644
--- a/test/features/mobile-sections/pagecontent.js
+++ b/test/features/mobile-sections/pagecontent.js
@@ -16,16 +16,6 @@
return headers.checkHeaders(uri);
});
- it('return the sent ETag', () => {
- return preq.get({
- uri:
`${server.config.uri}en.wikipedia.org/v1/page/mobile-sections/Foobar`,
- headers: { 'x-restbase-etag':
'123456/c3421381-7109-11e5-ac43-8c7f067c3520' }
- }).then((res) => {
- assert.status(res, 200);
- assert.deepEqual(res.headers.etag,
'123456/c3421381-7109-11e5-ac43-8c7f067c3520');
- });
- });
-
it('Sections/deep page should have a lead object with expected
properties', () => {
const title = 'Sections%2Fdeep';
const uri =
`${server.config.uri}test.wikipedia.org/v1/page/mobile-sections/${title}`;
diff --git a/test/utils/headers.js b/test/utils/headers.js
index f208dcd..2b9c773 100644
--- a/test/utils/headers.js
+++ b/test/utils/headers.js
@@ -12,7 +12,7 @@
expContentType = expContentType || '^application/json;
charset=utf-8; '
+
'profile="https://www.mediawiki.org/wiki/Specs/[a-z-]+/\\d+\\.\\d+\\.\\d+"$';
assert.contentType(res, expContentType);
- assert.deepEqual(!!res.headers.etag, true, 'No ETag header
present');
+ assert.deepEqual(res.headers.etag, '^"[^/"]+\/[^/"]+"$', 'The ETag
header is not present or invalid');
assert.deepEqual(res.headers.etag.indexOf('undefined'), -1, 'etag
should not contain "undefined"');
assert.deepEqual(res.headers['access-control-allow-origin'], '*');
assert.deepEqual(res.headers['access-control-allow-headers'],
'accept, x-requested-with, content-type');
--
To view, visit https://gerrit.wikimedia.org/r/322084
To unsubscribe, visit https://gerrit.wikimedia.org/r/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: I4fbf8b0a7639f298213b1fbe475c522cf6e2aa19
Gerrit-PatchSet: 7
Gerrit-Project: mediawiki/services/mobileapps
Gerrit-Branch: master
Gerrit-Owner: Mobrovac <[email protected]>
Gerrit-Reviewer: BearND <[email protected]>
Gerrit-Reviewer: Dbrant <[email protected]>
Gerrit-Reviewer: Fjalapeno <[email protected]>
Gerrit-Reviewer: GWicke <[email protected]>
Gerrit-Reviewer: Jdlrobson <[email protected]>
Gerrit-Reviewer: Jhernandez <[email protected]>
Gerrit-Reviewer: Mholloway <[email protected]>
Gerrit-Reviewer: Mhurd <[email protected]>
Gerrit-Reviewer: Mobrovac <[email protected]>
Gerrit-Reviewer: Niedzielski <[email protected]>
Gerrit-Reviewer: Ppchelko <[email protected]>
Gerrit-Reviewer: jenkins-bot <>
_______________________________________________
MediaWiki-commits mailing list
[email protected]
https://lists.wikimedia.org/mailman/listinfo/mediawiki-commits
