Skip to content

Commit

Permalink
Merge 08a08d8 into 479f95f
Browse files Browse the repository at this point in the history
  • Loading branch information
@Pchelolo
Pchelolo committed Jun 22, 2018
2 parents 479f95f + 08a08d8 commit a66d93e
Show file tree
Hide file tree
Showing 6 changed files with 92 additions and 22 deletions.
12 changes: 4 additions & 8 deletions lib/language_variants_filter.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,7 @@ module.exports = (hyper, req, next, options, specInfo) => {
if (!acceptLanguage || mwUtil.isNoCacheRequest(req)) {
delete req.headers['accept-language'];
return next(hyper, req)
// TODO: Store the Vary header so that if the page is possible
// to be converted we return a proper Vary even when accept-language
// is not specified.
// For now we hacky check by domain.
// TODO: For now we hacky check by domain as the vary header is not stored for everything.
.then((res) => {
const langCode = req.params.domain.substring(0, req.params.domain.indexOf('.'));
return mwUtil.canConvertLangVariant(hyper, req, langCode)
Expand All @@ -38,9 +35,10 @@ module.exports = (hyper, req, next, options, specInfo) => {
if (!shouldConvert) {
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) => {
// TODO: For now we hacky check by domain as the vary header
// is not stored for everything.
if (canConvert) {
res.headers.vary = 'accept-language';
}
Expand Down Expand Up @@ -72,8 +70,6 @@ module.exports = (hyper, req, next, options, specInfo) => {
}
}
})
// TODO: Now we fix up the Vary header cause Parsoid
// sets content-type in there which is wrong.
.then((res) => {
// Parsoid transformation doesn't know about our caching policies
// or additional headers RESTBase sets, so merge headers from the original
Expand Down
9 changes: 6 additions & 3 deletions sys/multi_content_bucket.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,10 @@ function returnRevision(req) {
return (dbResult) => {
if (dbResult.body && dbResult.body.items && dbResult.body.items.length) {
const row = dbResult.body.items[0];
const headers = {
const headers = Object.assign({}, row.headers || {}, {
etag: mwUtil.makeETag(row.rev, row.tid),
'content-type': row['content-type']
};
});
return {
status: 200,
headers,
Expand Down Expand Up @@ -87,6 +87,7 @@ class MultiContentBucket {
rev,
tid,
'content-type': req.body[cTypeSpec.name].headers['content-type'],
headers: req.body[cTypeSpec.name].headers,
value: req.body[cTypeSpec.name].body
}
}
Expand All @@ -101,6 +102,7 @@ class MultiContentBucket {
rev,
tid,
'content-type': req.body[mainCTypeName].headers['content-type'],
headers: req.body[mainCTypeName].headers,
value: req.body[mainCTypeName].body
}
}
Expand Down Expand Up @@ -218,7 +220,7 @@ class MultiContentBucket {
}

makeSchema(opts) {
const schemaVersionMajor = 3;
const schemaVersionMajor = 4;

return {
table: opts.table,
Expand All @@ -245,6 +247,7 @@ class MultiContentBucket {
// Redirect
'content-location': 'string',
'content-type': 'string',
headers: 'json',
tags: 'set<string>'
},
index: [
Expand Down
16 changes: 15 additions & 1 deletion sys/parsoid.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,16 @@ function compileReRenderBlacklist(blacklist) {
return result;
}

function stripAcceptFromVary(res) {
if (res.headers.vary) {
res.headers.vary = res.headers.vary.split(',')
.map(header => header.trim())
.filter(header => !/^accept$/i.test(header))
.join(',');
}
return res;
}

class ParsoidService {
constructor(options) {
this.options = options = options || {};
Expand Down Expand Up @@ -356,6 +366,7 @@ class ParsoidService {
hyper.metrics.increment('sys_parsoid_generateAndSave.unchanged_rev_render');
return currentContentRes;
} else if (res.status === 200) {
stripAcceptFromVary(res.body.html);
const resp = {
status: res.status,
headers: res.body[format].headers,
Expand Down Expand Up @@ -516,10 +527,13 @@ class ParsoidService {
return contentReq
.then((res) => {
mwUtil.normalizeContentType(res);
res.headers = res.headers || {};
if (this.options.response_cache_control) {
if (!res.headers) { res.headers = {}; }
res.headers['cache-control'] = this.options.response_cache_control;
}
// Strip Accept from Vary headers since RESTBase doesn't do content-type
// transformations
res = stripAcceptFromVary(res);
if (/^null$/.test(res.headers.etag)) {
hyper.logger.log('error/parsoid/response_etag_missing', {
msg: 'Detected a null etag in the response!'
Expand Down
33 changes: 23 additions & 10 deletions test/features/pagecontent/language_variants.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,25 +19,32 @@ describe('Language variants', function() {
return preq.get({ uri: `${server.config.labsBucketURL}/html/Main_Page`})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual((res.headers.vary || '').indexOf('accept-language'), -1);
assert.varyNotContains(res, 'accept');
assert.varyNotContains(res, 'accept-language');
assert.deepEqual(res.headers['content-language'], 'en');
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
});
});

let storedEtag;

it('should request html with no variants', () => {
return preq.get({ uri: `${server.config.variantsWikiBucketURL}/html/${variantsPageTitle}`})
.then((res) => {
storedEtag = res.headers.etag;
assert.deepEqual(res.status, 200);
assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
assert.varyNotContains(res, 'accept');
assert.varyContains(res, 'accept-language');
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
assert.deepEqual(res.headers['content-language'], 'sr');
assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
assert.deepEqual(/1\. Ово је тестна страница/.test(res.body), true);
assert.deepEqual(/2\. Ovo je testna stranica/.test(res.body), true);
});
});

it('should request html with default variant', () => {
it('should request html with default variant, from storage', () => {
return preq.get({
uri: `${server.config.variantsWikiBucketURL}/html/${variantsPageTitle}`,
headers: {
Expand All @@ -46,15 +53,17 @@ describe('Language variants', function() {
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
assert.varyNotContains(res, 'accept');
assert.varyContains(res, 'accept-language');
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
assert.deepEqual(res.headers['content-language'], 'sr');
assert.deepEqual(res.headers.etag, storedEtag);
assert.deepEqual(/1\. Ово је тестна страница/.test(res.body), true);
assert.deepEqual(/2\. Ovo je testna stranica/.test(res.body), true);
});
});

it('should request html with wrong variant', () => {
it('should request html with wrong variant, from storage', () => {
return preq.get({
uri: `${server.config.variantsWikiBucketURL}/html/${variantsPageTitle}`,
headers: {
Expand All @@ -63,9 +72,11 @@ describe('Language variants', function() {
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
assert.varyNotContains(res, 'accept');
assert.varyContains(res, 'accept-language');
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
assert.deepEqual(res.headers['content-language'], 'sr');
assert.deepEqual(res.headers.etag, storedEtag);
assert.deepEqual(/1\. Ово је тестна страница/.test(res.body), true);
assert.deepEqual(/2\. Ovo je testna stranica/.test(res.body), true);
});
Expand All @@ -80,7 +91,8 @@ describe('Language variants', function() {
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
assert.varyNotContains(res, 'accept');
assert.varyContains(res, 'accept-language');
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
assert.deepEqual(res.headers['content-language'], 'sr-ec');
assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
Expand All @@ -98,7 +110,8 @@ describe('Language variants', function() {
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(res.headers.vary.toLowerCase(), 'accept-language');
assert.varyNotContains(res, 'accept');
assert.varyContains(res, 'accept-language');
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control');
assert.deepEqual(res.headers['content-language'], 'sr-el');
assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
Expand Down
4 changes: 4 additions & 0 deletions test/features/pagecontent/pagecontent.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ describe('item requests', function() {
})
.then(function(res) {
assert.deepEqual(res.status, 200);
assert.varyNotContains(res, 'accept');
return preq.get({
uri: server.config.labsBucketURL + '/html/Main_Page/'
});
Expand All @@ -66,6 +67,7 @@ describe('item requests', function() {
})
.then(function(res) {
assert.deepEqual(res.status, 200);
assert.varyNotContains(res, 'accept');
});
});

Expand Down Expand Up @@ -96,6 +98,7 @@ describe('item requests', function() {
})
.then(function(res) {
assert.deepEqual(res.status, 200);
assert.varyNotContains(res, 'accept');
rev2Etag = res.headers.etag.replace(/^"(.*)"$/, '$1');
});
});
Expand All @@ -107,6 +110,7 @@ describe('item requests', function() {
.then(function(res) {
assert.deepEqual(res.status, 200);
assert.contentType(res, contentTypes.html);
assert.varyNotContains(res, 'accept');
return preq.get({
uri: server.config.labsBucketURL + '/data-parsoid/Foobar/'
+ res.headers.etag.replace(/^"(.*)"$/, '$1')
Expand Down
40 changes: 40 additions & 0 deletions test/utils/assert.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -160,6 +160,44 @@ function checkString(result, expected, message) {
}
}

function varyNotContains(res, header) {
if (!res.headers) {
throw new assert.AssertionError({
message: `Empty headers verifying vary doesn't contain ${header}`
});
}
if (!res.headers.vary) {
return;
}
const varyAsList = res.headers.vary.split(',')
.map(header => header.trim().toLowerCase());
if (varyAsList.includes(header.toLowerCase())) {
throw new assert.AssertionError({
message: `Vary header contains ${header} while it must not`
});
}
}

function varyContains(res, header) {
if (!res.headers) {
throw new assert.AssertionError({
message: `Empty headers verifying vary contains ${header}`
});
}
if (!res.headers.vary) {
throw new assert.AssertionError({
message: `Empty vary header verifying vary contains ${header}`
});
}
const varyAsList = res.headers.vary.split(',')
.map(header => header.trim().toLowerCase());
if (!varyAsList.includes(header.toLowerCase())) {
throw new assert.AssertionError({
message: `Vary header does not contain ${header}`
});
}
}

module.exports.ok = assert.ok;
module.exports.AssertionError = assert.AssertionError;
module.exports.fails = fails;
Expand All @@ -172,3 +210,5 @@ module.exports.localRequests = localRequests;
module.exports.remoteRequests = remoteRequests;
module.exports.findParsoidRequest = findParsoidRequest;
module.exports.checkString = checkString;
module.exports.varyNotContains = varyNotContains;
module.exports.varyContains = varyContains;

0 comments on commit a66d93e

Please sign in to comment.