diff --git a/lib/language_variants_filter.js b/lib/language_variants_filter.js index 7060f6841..423354e7d 100644 --- a/lib/language_variants_filter.js +++ b/lib/language_variants_filter.js @@ -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) @@ -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'; } @@ -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 diff --git a/sys/multi_content_bucket.js b/sys/multi_content_bucket.js index d57a96522..25f59bd34 100644 --- a/sys/multi_content_bucket.js +++ b/sys/multi_content_bucket.js @@ -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, @@ -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 } } @@ -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 } } @@ -218,7 +220,7 @@ class MultiContentBucket { } makeSchema(opts) { - const schemaVersionMajor = 3; + const schemaVersionMajor = 4; return { table: opts.table, @@ -245,6 +247,7 @@ class MultiContentBucket { // Redirect 'content-location': 'string', 'content-type': 'string', + headers: 'json', tags: 'set' }, index: [ diff --git a/sys/parsoid.js b/sys/parsoid.js index 6a9574cd3..13aa9b023 100644 --- a/sys/parsoid.js +++ b/sys/parsoid.js @@ -166,6 +166,16 @@ function compileReRenderBlacklist(blacklist) { return result; } +function stripAcceptFromVary(res) { + if (res && res.headers && 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 || {}; @@ -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, @@ -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!' diff --git a/test/features/pagecontent/language_variants.js b/test/features/pagecontent/language_variants.js index 9ef0f3466..4e8d8799f 100644 --- a/test/features/pagecontent/language_variants.js +++ b/test/features/pagecontent/language_variants.js @@ -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: { @@ -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: { @@ -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); }); @@ -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-]+"$/); @@ -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-]+"$/); diff --git a/test/features/pagecontent/pagecontent.js b/test/features/pagecontent/pagecontent.js index a8c8d2c89..b60187c25 100644 --- a/test/features/pagecontent/pagecontent.js +++ b/test/features/pagecontent/pagecontent.js @@ -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/' }); @@ -66,6 +67,7 @@ describe('item requests', function() { }) .then(function(res) { assert.deepEqual(res.status, 200); + assert.varyNotContains(res, 'accept'); }); }); @@ -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'); }); }); @@ -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') diff --git a/test/utils/assert.js b/test/utils/assert.js index 89f051d83..4513f3009 100644 --- a/test/utils/assert.js +++ b/test/utils/assert.js @@ -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; @@ -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;