Skip to content

Commit

Permalink
Merge pull request #1308 from johngian/no-storage-pcs
Browse files Browse the repository at this point in the history
restbase-sunset: Make using storage on PCS configurable
  • Loading branch information
johngian committed Oct 17, 2022
2 parents 1a02cdf + 6383126 commit 4a5e62a
Show file tree
Hide file tree
Showing 6 changed files with 159 additions and 6 deletions.
17 changes: 16 additions & 1 deletion config.fullstack.test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -124,6 +124,20 @@ wikipedia_project: &wikipedia_project
options: *proxy_options
/{api:sys}: *default_sys

wikipedia_project_no_storage: &wikipedia_project_no_storage
x-modules:
- spec:
paths:
/{api:v1}:
x-modules:
- path: projects/v1/wikipedia.wmf.yaml
options:
<<: *default_options
disabled_storage: true
- path: projects/proxy.yaml
options: *proxy_options
/{api:sys}: *default_sys

wiktionary_project: &wiktionary_project
x-modules:
- spec:
Expand Down Expand Up @@ -214,7 +228,8 @@ spec_root: &spec_root
# The order is important for tests.
# Redirect tests require en.wiki being not the first wiki in the list.
/{domain:en.wikipedia.beta.wmflabs.org}: *wikipedia_project
/{domain:de.wikipedia.beta.wmflabs.org}: *wikipedia_project
# For testing purpose lets run de-beta with storage disabled
/{domain:de.wikipedia.beta.wmflabs.org}: *wikipedia_project_no_storage
/{domain:zh.wikipedia.beta.wmflabs.org}: *wikipedia_project
/{domain:wikidata.beta.wmflabs.org}: *wikidata_project
# Serbian wiki in beta for language variants tests
Expand Down
6 changes: 4 additions & 2 deletions projects/v1/wikipedia.wmf.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -65,18 +65,20 @@ paths:
options: '{{merge({"response_cache_control": options.purged_cache_control},
options.mobileapps)}}'
- path: v1/summary.js
options: '{{merge({"response_cache_control": options.purged_cache_control_client_cache},
options.summary)}}'
options: '{{merge({"response_cache_control": options.purged_cache_control_client_cache,
"disabled_storage": options.disabled_storage}, options.summary)}}'
- path: v1/pcs/stored_endpoint.js
options:
name: media-list
response_cache_control: '{{options.purged_cache_control}}'
host: '{{options.mobileapps.host}}'
disabled_storage: '{{options.disabled_storage}}'
- path: v1/pcs/stored_endpoint.js
options:
name: mobile-html
response_cache_control: '{{options.purged_cache_control}}'
host: '{{options.mobileapps.host}}'
disabled_storage: '{{options.disabled_storage}}'
- path: v1/pcs/mobile-html-offline-resources.yaml
options:
host: '{{options.mobileapps.host}}'
Expand Down
28 changes: 27 additions & 1 deletion test/features/pagecontent/language_variants.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,17 @@ describe('Language variants', function() {
this.timeout(20000);
const server = new Server();

before(() => server.start());
before(async () => {
// Cleaning require cache because of side-effects
// on the way modules are instantiated in hyperswitch
try {
delete require.cache[require.resolve("../../../v1/summary.js")];
} catch {
console.log("Couldn't delete cached module");
}
await server.start();
});

after(() => server.stop());

it('should request html with impossible variants', () => {
Expand Down Expand Up @@ -137,6 +147,21 @@ describe('Language variants', function() {
});
});

it('should request summary with no variant and not store it (no-storage)', () => {
// de.wikipedia.beta.wmflabs.org is configured to not use storage while testing
return preq.get({
uri: `${server.config.bucketURL('de.wikipedia.beta.wmflabs.org')}/summary/${variantsPageTitle}`
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control_with_client_caching');
assert.deepEqual(res.headers['content-language'], 'de');
assert.deepEqual(res.headers['x-restbase-sunset'] || null, 'true');
assert.checkString(res.headers.etag, /^"\d+\/[a-f0-9-]+"$/);
assert.deepEqual('Das ist eine testseite', res.body.extract);
})
});

it('should request summary with no variant and store it', () => {
let storedEtag;
return preq.get({
Expand All @@ -148,6 +173,7 @@ describe('Language variants', function() {
assert.validateListHeader(res.headers.vary, { require: ['Accept-Language'], disallow: ['Accept'] });
assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control_with_client_caching');
assert.deepEqual(res.headers['content-language'], 'sr');
assert.deepEqual(res.headers['x-restbase-sunset'] || null, null);
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
Expand Down
66 changes: 65 additions & 1 deletion test/features/pcs.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,16 @@ const preq = require('preq');

describe('Page Content Service: /page/media-list', () => {
const server = new Server();
before(() => server.start());
before(async () => {
// Cleaning require cache because of side-effects
// on the way modules are instantiated in hyperswitch
try {
delete require.cache[require.resolve('../../v1/pcs/stored_endpoint.js')]
} catch {
console.log("Couldn't delete cached module")
}
await server.start();
});
after(() => server.stop());

const pageTitle = 'San_Francisco';
Expand All @@ -25,6 +34,20 @@ describe('Page Content Service: /page/media-list', () => {
});
});

it('Should fetch latest media-list without storing it', () => {
// de.wikipedia.beta.wmflabs.org is configured to not use storage while testing
return preq.get({
uri: `${server.config.bucketURL('de.wikipedia.beta.wmflabs.org')}/media-list/Erde`
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(/^application\/json/.test(res.headers['content-type']), true);
assert.deepEqual(res.headers['x-restbase-sunset'] || null, 'true');
assert.deepEqual(!!res.body.items, true);
assert.deepEqual(!!res.headers.etag, true);
});
});

it('Should fetch older media-list', () => {
return preq.get({
uri: `${server.config.bucketURL()}/media-list/${pageTitle}/${pageRev}`
Expand All @@ -38,6 +61,47 @@ describe('Page Content Service: /page/media-list', () => {
});
});

describe('Page Content Service: /page/mobile-html', () => {
const server = new Server();
before(async () => {
// Cleaning require cache because of side-effects
// on the way modules are instantiated in hyperswitch
try {
delete require.cache[require.resolve('../../v1/pcs/stored_endpoint.js')]
} catch {
console.log("Couldn't delete cached module")
}
await server.start();
});
after(() => server.stop());

it('Should fetch latest mobile-html', () => {
const pageTitle = 'Earth';
return preq.get({
uri: `${server.config.bucketURL()}/mobile-html/${pageTitle}`
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(/^text\/html/.test(res.headers['content-type']), true);
assert.deepEqual(res.headers['x-restbase-sunset'] || null, null);
});
});

it('Should fetch latest mobile-html directly from PCS', () => {
// de.wikipedia.beta.wmflabs.org is configured to not use storage while testing
const domain = 'de.wikipedia.beta.wmflabs.org'
const pageTitle = 'Erde';
return preq.get({
uri: `${server.config.bucketURL(domain)}/mobile-html/${pageTitle}`
})
.then((res) => {
assert.deepEqual(res.status, 200);
assert.deepEqual(/^text\/html/.test(res.headers['content-type']), true);
assert.deepEqual(res.headers['x-restbase-sunset'] || null, 'true');
});
});
});

describe('Page Content Service: transforms', () => {
const server = new Server();
before(() => server.start());
Expand Down
16 changes: 16 additions & 0 deletions v1/pcs/stored_endpoint.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ const mwUtils = require('../../lib/mwUtil');
class PCSEndpoint {
constructor(options) {
this._options = options;
this._disabled_storage = options.disabled_storage || false;
}

_injectCacheControl(res) {
Expand All @@ -19,6 +20,21 @@ class PCSEndpoint {
const startTime = Date.now();
const rp = req.params;

// restbase sunset: Make PCS requests passthrough
// to mobileapps service
if (this._disabled_storage) {
return this._fetchFromPCS(hyper, req)
.tap((res) => {
res.headers['x-restbase-sunset'] = true;
this._injectCacheControl.bind(this);
hyper.metrics.timing([
'pcs_getContent_latency',
'pcs_getContent_latency_no_storage',
`pcs_getContent_latency_${rp.domain}`
], startTime);
});
}

if (mwUtils.isNoCacheRequest(req)) {
return this._fetchFromPCSAndStore(hyper, req)
.tap(() => {
Expand Down
32 changes: 31 additions & 1 deletion v1/summary.js
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,38 @@ module.exports = (options) => {
} else if (options.protocol === 'https') {
protocol = 'https://';
}

const disabledStorage = options.disabled_storage || false;

let summarySpec = spec;
if (options.implementation === 'mcs') {
summarySpec = newSpec;

// restbase sunset: Make page summary requests passthrough
// to mobileapps service
if (disabledStorage) {
// Filter out storage related handlers and keep only requests to backend
let handlers = summarySpec.paths['/summary/{title}'].get['x-request-handler'];
handlers = handlers.filter((elem) => Object.keys(elem).includes('extract'));
delete handlers[0].extract.response;
handlers[0].extract.return = {
status: 200,
headers: {
etag: '{{extract.headers.etag}}',
vary: '{{extract.headers.vary}}',
'cache-control': '{{options.response_cache_control}}',
'content-language': '{{extract.headers.content-language}}',
'content-type': '{{extract.headers.content-type}}',
'x-restbase-sunset': 'true'
},
body: '{{changeProtocol(extract.body)}}'
};
summarySpec.paths['/summary/{title}'].get['x-request-handler'] = handlers;
}
}

return {
spec: options.implementation === 'mcs' ? newSpec : spec,
spec: summarySpec,
globals: Object.assign({ options }, functions)
};
};

0 comments on commit 4a5e62a

Please sign in to comment.