From 662010f66ce6230498ef4c3efe88c631f1cda9a6 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Sun, 14 Apr 2019 10:45:12 -0700 Subject: [PATCH 1/3] Remove listings endpoints from buckets. Bug: https://phabricator.wikimedia.org/T220010 Change-Id: I13faf1170f79303d68046373f1a5da2cb6e03bcd --- config.example.wikimedia.yaml | 4 +- sys/key_rev_value.js | 31 --------------- sys/key_rev_value.yaml | 4 -- sys/key_value.js | 26 ------------- sys/key_value.yaml | 4 -- sys/multi_content_bucket.js | 3 +- sys/multi_content_bucket.yaml | 4 -- sys/parsoid.js | 30 --------------- sys/parsoid.yaml | 33 ---------------- test/features/buckets/key_value_bucket.js | 21 +++++----- test/features/buckets/revisioned_bucket.js | 23 ----------- test/features/pagecontent/pagecontent.js | 7 ++-- test/features/pagecontent/redirects.js | 12 ------ v1/content.yaml | 45 ---------------------- 14 files changed, 17 insertions(+), 230 deletions(-) diff --git a/config.example.wikimedia.yaml b/config.example.wikimedia.yaml index 6feaa10a4..e4c4bc5a0 100644 --- a/config.example.wikimedia.yaml +++ b/config.example.wikimedia.yaml @@ -16,9 +16,11 @@ default_project: &default_project storage_groups: - name: default.group.local domains: /./ + # ignored in cassandra, but useful in SQLite testing. only used in tests. + dbname: test.db.sqlite3 parsoid: host: https://parsoid-beta.wmflabs.org - grace_ttl: 1 + grace_ttl: 1000000 action: apiUriTemplate: "{{'https://{domain}/w/api.php'}}" baseUriTemplate: "{{'https://{domain}/api/rest_v1'}}" diff --git a/sys/key_rev_value.js b/sys/key_rev_value.js index 3de670b43..02807b655 100644 --- a/sys/key_rev_value.js +++ b/sys/key_rev_value.js @@ -106,36 +106,6 @@ class KRVBucket { return hyper.get(storeReq).then(returnRevision(req)); } - listRevisions(hyper, req) { - const rp = req.params; - return hyper.get({ - uri: new URI([rp.domain, 'sys', 'table', rp.bucket, '']), - body: { - table: req.params.bucket, - attributes: { - key: req.params.key - }, - proj: ['rev', 'tid'], - limit: mwUtil.getLimit(hyper, req) - } - }) - .then((res) => ({ - status: 200, - - headers: { - 'content-type': 'application/json' - }, - - body: { - items: res.body.items.map((row) => ({ - revision: row.rev, - tid: row.tid - })), - next: res.body.next - } - })); - } - putRevision(hyper, req) { const rp = req.params; const rev = mwUtil.parseRevision(rp.revision, 'key_rev_value'); @@ -185,7 +155,6 @@ module.exports = (options) => { spec, // Re-export from spec module operations: { createBucket: krvBucket.createBucket.bind(krvBucket), - listRevisions: krvBucket.listRevisions.bind(krvBucket), getRevision: krvBucket.getRevision.bind(krvBucket), putRevision: krvBucket.putRevision.bind(krvBucket) } diff --git a/sys/key_rev_value.yaml b/sys/key_rev_value.yaml index c3f2e9951..a22cff16c 100644 --- a/sys/key_rev_value.yaml +++ b/sys/key_rev_value.yaml @@ -14,10 +14,6 @@ paths: put: operationId: putRevision - /{bucket}/{key}/: - get: - operationId: listRevisions - /{bucket}/{key}/{revision}: <<: *bucket_key diff --git a/sys/key_value.js b/sys/key_value.js index eb4366a53..8a7b3ec9a 100644 --- a/sys/key_value.js +++ b/sys/key_value.js @@ -105,31 +105,6 @@ class KVBucket { return hyper.get(storeReq).then(returnRevision(req)); } - listRevisions(hyper, req) { - const rp = req.params; - const storeRequest = { - uri: new URI([rp.domain, 'sys', 'table', rp.bucket, '']), - body: { - table: req.params.bucket, - attributes: { - key: req.params.key - }, - proj: ['tid'], - limit: 1000 - } - }; - return hyper.get(storeRequest) - .then((res) => ({ - status: 200, - headers: { - 'content-type': 'application/json' - }, - body: { - items: res.body.items.map((row) => row.tid) - } - })); - } - putRevision(hyper, req) { const rp = req.params; let tid = rp.tid && mwUtil.coerceTid(rp.tid, 'key_value'); @@ -213,7 +188,6 @@ module.exports = (options) => { spec, // Re-export from spec module operations: { createBucket: kvBucket.createBucket.bind(kvBucket), - listRevisions: kvBucket.listRevisions.bind(kvBucket), getRevision: kvBucket.getRevision.bind(kvBucket), putRevision: kvBucket.putRevision.bind(kvBucket) } diff --git a/sys/key_value.yaml b/sys/key_value.yaml index b3c112b66..8a49f0e7c 100644 --- a/sys/key_value.yaml +++ b/sys/key_value.yaml @@ -7,13 +7,9 @@ paths: /{bucket}: put: operationId: createBucket - /{bucket}/{key}: get: operationId: getRevision put: operationId: putRevision - /{bucket}/{key}/: - get: - operationId: listRevisions diff --git a/sys/multi_content_bucket.js b/sys/multi_content_bucket.js index a8c83fa5a..e74ee1cc2 100644 --- a/sys/multi_content_bucket.js +++ b/sys/multi_content_bucket.js @@ -490,8 +490,7 @@ module.exports = (options) => { operations: { createBucket: mkBucket.createBucket.bind(mkBucket), getRevision: mkBucket.getRevision.bind(mkBucket), - putRevision: mkBucket.putRevision.bind(mkBucket), - listRevisions: mkBucket.listRevisions.bind(mkBucket) + putRevision: mkBucket.putRevision.bind(mkBucket) } }; }; diff --git a/sys/multi_content_bucket.yaml b/sys/multi_content_bucket.yaml index 25befc740..76eaf164f 100644 --- a/sys/multi_content_bucket.yaml +++ b/sys/multi_content_bucket.yaml @@ -11,10 +11,6 @@ paths: get: operationId: getRevision - /{content}/{key}/: - get: - operationId: listRevisions - /{content}/{key}/{revision}: <<: *content_key diff --git a/sys/parsoid.js b/sys/parsoid.js index 98f819430..8b7be5445 100644 --- a/sys/parsoid.js +++ b/sys/parsoid.js @@ -192,10 +192,6 @@ class ParsoidService { getHtml: this.getFormat.bind(this, 'html'), getDataParsoid: this.getFormat.bind(this, 'data-parsoid'), getLintErrors: this.getLintErrors.bind(this), - // Listings - listWikitextRevisions: this.listRevisions.bind(this, 'wikitext'), - listHtmlRevisions: this.listRevisions.bind(this, 'html'), - listDataParsoidRevisions: this.listRevisions.bind(this, 'data-parsoid'), // Transforms transformHtmlToHtml: this.makeTransform('html', 'html'), transformHtmlToWikitext: this.makeTransform('html', 'wikitext'), @@ -514,32 +510,6 @@ class ParsoidService { }); } - listRevisions(format, hyper, req) { - const rp = req.params; - const revReq = { - uri: new URI([rp.domain, 'sys', 'parsoid_bucket', format, rp.title, '']), - body: { - limit: hyper.config.default_page_size - } - }; - - if (req.query.page) { - revReq.body.next = mwUtil.decodePagingToken(hyper, req.query.page); - } - - return hyper.get(revReq) - .then((res) => { - if (res.body.next) { - res.body._links = { - next: { - href: `?page=${mwUtil.encodePagingToken(hyper, res.body.next)}` - } - }; - } - return res; - }); - } - _getStashedContent(hyper, req, etag) { const rp = req.params; const getStash = (format) => hyper.get({ diff --git a/sys/parsoid.yaml b/sys/parsoid.yaml index c3ef160ff..e4e6356a5 100644 --- a/sys/parsoid.yaml +++ b/sys/parsoid.yaml @@ -26,17 +26,6 @@ paths: summary: Retrieve the wikitext for a title and revision. operationId: getWikitext - /wikitext/{title}/: - get: - summary: List Wikitext revisions. - operationId: listWikitextRevisions - parameters: - - name: page - in: query - description: The next page token - type: string - required: false - /wikitext/{title}/{revision}: <<: *wikitext_title @@ -48,17 +37,6 @@ paths: summary: Retrieve the HTML for title and revision. operationId: getHtml - /html/{title}/: - get: - summary: List HTML revisions. - operationId: listHtmlRevisions - parameters: - - name: page - in: query - description: The next page token - type: string - required: false - /html/{title}/{revision}: <<: *html_title @@ -70,17 +48,6 @@ paths: summary: Retrieve the data-parsoid JSON for title & revision. operationId: getDataParsoid - /data-parsoid/{title}/: - get: - summary: List data-parsoid revisions. - operationId: listDataParsoidRevisions - parameters: - - name: page - in: query - description: The next page token - type: string - required: false - /data-parsoid/{title}/{revision}: <<: *data-parsoid_title diff --git a/test/features/buckets/key_value_bucket.js b/test/features/buckets/key_value_bucket.js index 73780121c..2b8618456 100644 --- a/test/features/buckets/key_value_bucket.js +++ b/test/features/buckets/key_value_bucket.js @@ -78,30 +78,29 @@ describe('Key value buckets', () => { it('key_value should not overwrite same content with ignore_duplicates', () => { const testData = randomString(100); - const tids = [ uuid.now().toString(), + const originalEtag = uuid.now().toString(); + const etags = [ originalEtag, uuid.now().toString(), uuid.now().toString() ]; - return P.each(tids, (tid) => { - return preq.put({ + return P.each(etags, (etag) => preq.put({ uri: `${bucketBaseURI}/List_Test_1`, body: new Buffer(testData), headers: { - 'if-none-hash-match': '*' + 'if-none-hash-match': '*', + etag } }) - .catch(() => {}); - }) + .catch(() => { + }) + ) .then(() => { return preq.get({ - uri: `${bucketBaseURI}/List_Test_1/`, - query: { - limit: 10 - } + uri: `${bucketBaseURI}/List_Test_1` }); }) .then((res) => { assert.deepEqual(res.status, 200); - assert.deepEqual(res.body.items.length, 1); + assert.deepEqual(res.headers.etag, originalEtag); }); }); } diff --git a/test/features/buckets/revisioned_bucket.js b/test/features/buckets/revisioned_bucket.js index 4eee7397a..d0d04dbdf 100644 --- a/test/features/buckets/revisioned_bucket.js +++ b/test/features/buckets/revisioned_bucket.js @@ -126,29 +126,6 @@ describe('Revisioned buckets', () => { }); }); - it('lists revisions', () => { - const testData = randomString(100); - return P.each([1, 2, 3], (revNumber) => { - return preq.put({ - uri: `${bucketBaseURI}/Test5/${revNumber}`, - body: new Buffer(testData) - }); - }) - .then(() => { - return preq.get({ - uri: `${bucketBaseURI}/Test5/`, - query: { - limit: 10 - } - }); - }) - .then((res) => { - assert.deepEqual(res.status, 200); - assert.deepEqual(res.body.items.length, 3); - assert.deepEqual(res.body.items.map((r) => { return r.revision; }), [3, 2, 1]); - }); - }); - it('throws error on invalid revision', () => { const testData = randomString(100); return preq.put({ diff --git a/test/features/pagecontent/pagecontent.js b/test/features/pagecontent/pagecontent.js index 69b5daf71..5dcad76a5 100644 --- a/test/features/pagecontent/pagecontent.js +++ b/test/features/pagecontent/pagecontent.js @@ -61,13 +61,12 @@ describe('item requests', function() { assert.deepEqual(res.status, 200); assert.validateListHeader(res.headers.vary, { require: ['Accept'], disallow: [''] }); return preq.get({ - uri: `${server.config.bucketURL('en.wikipedia.beta.wmflabs.org')}/html/Main_Page/` + uri: `${server.config.bucketURL('en.wikipedia.beta.wmflabs.org')}/html/Main_Page` }); }) .then((res) => { - if (res.body.items.length !== 1) { - throw new Error('Expected a single revision for Main_Page'); - } + assert.deepEqual(res.status, 200); + assert.validateListHeader(res.headers.vary, { require: ['Accept'], disallow: [''] }); }); }); it('should transparently create a new HTML revision with id 252937', () => { diff --git a/test/features/pagecontent/redirects.js b/test/features/pagecontent/redirects.js index 730708be4..0133099db 100644 --- a/test/features/pagecontent/redirects.js +++ b/test/features/pagecontent/redirects.js @@ -47,18 +47,6 @@ describe('redirects', () => { }); }); - it('should preserve parameters while redirecting to a normalized version of a title, #2', () => { - return preq.get({ - uri: `${server.config.bucketURL()}/html/Main%20Page/`, - followRedirect: false - }) - .then((res) => { - assert.deepEqual(res.status, 301); - assert.deepEqual(res.headers.location, '../Main_Page/'); - assert.deepEqual(res.headers['cache-control'], 'test_purged_cache_control'); - }); - }); - it('should not redirect to a normalized version of a title, no-cache', () => { return preq.get({ uri: `${server.config.bucketURL()}/html/Main%20Page?test=mwAQ`, diff --git a/v1/content.yaml b/v1/content.yaml index 4cc1046e1..2579e3031 100644 --- a/v1/content.yaml +++ b/v1/content.yaml @@ -375,51 +375,6 @@ paths: body: '{{request.body}}' x-monitor: false - /html/{title}/: - get: - tags: - - Page content - summary: List HTML revisions for a title. - description: | - This currently only lists revisions that are already stored in RESTBase. - - Stability: [experimental](https://www.mediawiki.org/wiki/API_versioning#Experimental) - produces: - - application/json - - application/problem+json - parameters: - - name: title - in: path - description: "Page title. Use underscores instead of spaces. Example: `Main_Page`." - type: string - required: true - - name: page - in: query - description: The next page token, provided by _links.next.href property. - type: string - required: false - responses: - '200': - description: A list of revisions for the given title. - schema: - $ref: '#/definitions/revisions' - default: - description: Error - schema: - $ref: '#/definitions/problem' - x-request-handler: - - get_from_backend: - request: - uri: /{domain}/sys/parsoid/html/{title}/ - headers: - cache-control: '{{cache-control}}' - if-unmodified-since: '{{if-unmodified-since}}' - x-restbase-mode: '{{x-restbase-mode}}' - x-restbase-parentrevision: '{{x-restbase-parentrevision}}' - query: - page: '{{page}}' - x-monitor: false - /html/{title}/{revision}: x-route-filters: &html_title_revision_filters - path: ./lib/access_check_filter.js From e6be1c2c646ebe4a0705b629f495b5f6cc21fe75 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Sun, 14 Apr 2019 12:04:01 -0700 Subject: [PATCH 2/3] Prepare key_value storage for RB split Change-Id: I55d9cc893519a095fc378d2890cb7ccb72ae0d59 --- sys/key_value.js | 108 ++++++++-------------- sys/key_value.yaml | 6 +- sys/mathoid.js | 19 +++- sys/post_data.js | 3 - test/features/buckets/key_value_bucket.js | 44 +++++---- test/features/router/handlerTemplate.js | 67 -------------- test/features/specification/monitoring.js | 8 +- test/test_module.yaml | 39 -------- v1/definition.yaml | 28 +++--- v1/summary.yaml | 14 ++- v1/summary_new.yaml | 16 +++- 11 files changed, 122 insertions(+), 230 deletions(-) delete mode 100644 test/features/router/handlerTemplate.js diff --git a/sys/key_value.js b/sys/key_value.js index 8a7b3ec9a..10635e5ed 100644 --- a/sys/key_value.js +++ b/sys/key_value.js @@ -13,36 +13,6 @@ const URI = HyperSwitch.URI; const spec = HyperSwitch.utils.loadSpec(`${__dirname}/key_value.yaml`); -// Format a revision response. Shared between different ways to retrieve a -// revision (latest & with explicit revision). -function returnRevision(req) { - return (dbResult) => { - if (dbResult.body && dbResult.body.items && dbResult.body.items.length) { - const row = dbResult.body.items[0]; - let headers = { - etag: row.headers.etag || mwUtil.makeETag('0', row.tid) - }; - if (row.headers) { - headers = Object.assign(headers, row.headers); - } - return { - status: 200, - headers, - body: row.value - }; - } else { - throw new HTTPError({ - status: 404, - body: { - type: 'not_found', - uri: req.uri, - method: req.method - } - }); - } - }; -} - class KVBucket { createBucket(hyper, req) { const schema = this.makeSchema(req.body || {}); @@ -102,27 +72,42 @@ class KVBucket { limit: 1 } }; - return hyper.get(storeReq).then(returnRevision(req)); + return hyper.get(storeReq).then((dbResult) => { + if (dbResult.body && dbResult.body.items && dbResult.body.items.length) { + const row = dbResult.body.items[0]; + return { + status: 200, + headers: row.headers, + body: row.value + }; + } else { + throw new HTTPError({ + status: 404, + body: { + type: 'not_found', + uri: req.uri, + method: req.method + } + }); + } + }); } putRevision(hyper, req) { - const rp = req.params; - let tid = rp.tid && mwUtil.coerceTid(rp.tid, 'key_value'); - - if (!tid) { - tid = (mwUtil.parseETag(req.headers && req.headers.etag) || {}).tid; - tid = tid || uuid.now().toString(); - } - if (mwUtil.isNoStoreRequest(req)) { - return { - status: 202, - headers: { - etag: req.headers && req.headers.etag || mwUtil.makeETag('0', tid) - } - }; + return { status: 202 }; } + const rp = req.params; + const tid = uuid.now().toString(); + + const headersToStore = {}; + Object.keys(req.headers) + .filter(headerName => headerName.startsWith('x-store-')) + .forEach((headerName => { + headersToStore[headerName.replace(/^x-store-/, '')] = req.headers[headerName]; + })); + const doPut = () => hyper.put({ uri: new URI([rp.domain, 'sys', 'table', rp.bucket, '']), body: { @@ -130,30 +115,10 @@ class KVBucket { attributes: { key: rp.key, tid, - value: req.body, - headers: req.headers + headers: headersToStore, + value: req.body } } - }) - .then((res) => { - if (res.status === 201) { - return { - status: 201, - headers: { - etag: req.headers && req.headers.etag || mwUtil.makeETag('0', tid) - }, - body: { - message: 'Created.', - tid - } - }; - } else { - throw res; - } - }) - .catch((error) => { - hyper.logger.log('error/kv/putRevision', error); - return { status: 400 }; }); if (req.headers['if-none-hash-match']) { @@ -162,15 +127,14 @@ class KVBucket { uri: new URI([rp.domain, 'sys', 'key_value', rp.bucket, rp.key]) }) .then((oldContent) => { + // TODO: proper etag-based compare. if (stringify(req.body) === stringify(oldContent.body) && - (!req.headers['content-type'] || - req.headers['content-type'] === oldContent.headers['content-type'])) { + (!headersToStore['content-type'] || + headersToStore['content-type'] === oldContent.headers['content-type'])) { hyper.metrics.increment(`sys_kv_${req.params.bucket}.unchanged_rev_render`); return { status: 412, - headers: { - etag: oldContent.headers.etag - } + headers: oldContent.headers }; } throw new HTTPError({ status: 404 }); diff --git a/sys/key_value.yaml b/sys/key_value.yaml index 8a49f0e7c..734376fea 100644 --- a/sys/key_value.yaml +++ b/sys/key_value.yaml @@ -2,7 +2,11 @@ swagger: '2.0' info: version: '1.0.0' title: RESTBase key-value module - description: Revisioned blob storage with HTTP interface, backed by table storage + description: > + Blob storage with HTTP interface, backed by table storage. + + All headers prepended with `x-store-` are saved on write and returned + with the content upon retrieval. All the not prepended headers not stored. paths: /{bucket}: put: diff --git a/sys/mathoid.js b/sys/mathoid.js index b691a95a4..f0deab768 100644 --- a/sys/mathoid.js +++ b/sys/mathoid.js @@ -4,6 +4,7 @@ const P = require('bluebird'); const HyperSwitch = require('hyperswitch'); const URI = HyperSwitch.URI; const HTTPError = HyperSwitch.HTTPError; +const mwUtil = require('../lib/mwUtil'); const FORMATS = ['mml', 'svg', 'png']; @@ -25,7 +26,7 @@ class MathoidService { }).then((res) => { hash = origHash = res.body; // short-circuit if it's a no-cache request - if (req.headers && /no-cache/.test(req.headers['cache-control'])) { + if (mwUtil.isNoCacheRequest(req)) { return P.reject(new HTTPError({ status: 404 })); } // check the post storage @@ -83,7 +84,11 @@ class MathoidService { return P.join( hyper.put({ uri: new URI([rp.domain, 'sys', 'key_value', 'mathoid_ng.check', hash]), - headers: checkRes.headers, + headers: { + 'x-store-x-resource-location': hash, + 'x-store-cache-control': 'no-cache', + 'x-store-content-type': 'application/json' + }, body: checkRes.body }), indirectionP, @@ -114,10 +119,16 @@ class MathoidService { })); } // construct the request object that will be emitted + Object.assign(completeBody[format].headers, { + 'x-store-x-resource-location': hash + }); + const storeHeaders = {}; + Object.keys(completeBody[format].headers).forEach((header) => { + storeHeaders[`x-store-${header}`] = completeBody[format].headers[header]; + }); const reqObj = { uri: new URI([domain, 'sys', 'key_value', `mathoid_ng.${format}`, hash]), - headers: Object.assign( - completeBody[format].headers, { 'x-resource-location': hash }), + headers: storeHeaders, body: completeBody[format].body }; if (format === 'png' && reqObj.body && reqObj.body.type === 'Buffer') { diff --git a/sys/post_data.js b/sys/post_data.js index bd5ef6fcf..a671ed383 100644 --- a/sys/post_data.js +++ b/sys/post_data.js @@ -67,9 +67,6 @@ class PostDataBucket { getRevision(hyper, req) { const rp = req.params; const path = [rp.domain, 'sys', 'key_value', rp.bucket, rp.key]; - if (rp.tid) { - path.push(rp.tid); - } return hyper.get({ uri: new URI(path), headers: { diff --git a/test/features/buckets/key_value_bucket.js b/test/features/buckets/key_value_bucket.js index 2b8618456..cf4799365 100644 --- a/test/features/buckets/key_value_bucket.js +++ b/test/features/buckets/key_value_bucket.js @@ -29,15 +29,15 @@ describe('Key value buckets', () => { after(() => server.stop()); it('stores a content in a bucket and gets it back', () => { - const testData = randomString(60000); + const testData = randomString(100); return preq.put({ - uri: `${bucketBaseURI}/Test1`, + uri: `${bucketBaseURI}/${testData}`, body: new Buffer(testData) }) .then((res) => { assert.deepEqual(res.status, 201); return preq.get({ - uri: `${bucketBaseURI}/Test1` + uri: `${bucketBaseURI}/${testData}` }); }) .then((res) => { @@ -46,33 +46,37 @@ describe('Key value buckets', () => { }); }); - it('assigns etag to a value', () => { + it('throws 404 error if key not found', () => { + return preq.get({ + uri: `${bucketBaseURI}/some_not_existing_key` + }) + .then(() => { + throw new Error('Error should be thrown'); + }, (e) => { + assert.deepEqual(e.status, 404); + }); + }); + + it('Preserves x-store headers and removes others', () => { const testData = randomString(100); return preq.put({ - uri: `${bucketBaseURI}/Test3`, + uri: `${bucketBaseURI}/${testData}`, + headers: { + 'x-store-preserved': 'this_will_be_stored', + 'non-preserved': 'this_will_not_be_stored' + }, body: new Buffer(testData) }) .then((res) => { assert.deepEqual(res.status, 201); return preq.get({ - uri: `${bucketBaseURI}/Test3` + uri: `${bucketBaseURI}/${testData}` }); }) .then((res) => { assert.deepEqual(res.status, 200); - assert.ok(res.headers.etag); - assert.ok(new RegExp('^"0\/').test(res.headers.etag), true); - }); - }); - - it('throws 404 error if key not found', () => { - return preq.get({ - uri: `${bucketBaseURI}/some_not_existing_key` - }) - .then(() => { - throw new Error('Error should be thrown'); - }, (e) => { - assert.deepEqual(e.status, 404); + assert.deepEqual(res.headers['preserved'], 'this_will_be_stored'); + assert.deepEqual(res.headers['non-preserved'], undefined); }); }); @@ -87,7 +91,7 @@ describe('Key value buckets', () => { body: new Buffer(testData), headers: { 'if-none-hash-match': '*', - etag + 'x-store-etag': etag } }) .catch(() => { diff --git a/test/features/router/handlerTemplate.js b/test/features/router/handlerTemplate.js deleted file mode 100644 index 567fdc8a6..000000000 --- a/test/features/router/handlerTemplate.js +++ /dev/null @@ -1,67 +0,0 @@ -'use strict'; - -const assert = require('../../utils/assert.js'); -const Server = require('../../utils/server.js'); -const preq = require('preq'); -const P = require('bluebird'); - -describe('handler template', function() { - this.timeout(20000); - const server = new Server(); - before(() => server.start()); - after(() => server.stop()); - - function hasTextContentType(res) { - assert.deepEqual(/^text\/html/.test(res.headers['content-type']), true); - } - - let slice; - - it('retrieve content from backend service', () => { - let tid1; - let tid2; - return preq.get({ - uri: `${server.config.baseURL()}/service/test/User:GWicke%2fDate` - }) - .then((res) => { - assert.deepEqual(res.status, 200); - tid1 = res.headers.etag; - hasTextContentType(res); - - // Delay for 1s to make sure that the content differs on - // re-render, then force a re-render and check that it happened. - assert.recordRequests(); - return P.delay(1100) - .then(() => { - return preq.get({ - uri: `${server.config.baseURL()}/service/test/User:GWicke%2fDate`, - headers: { 'cache-control': 'no-cache' } - }); - }); - }) - .then((res) => { - tid2 = res.headers.etag; - assert.notDeepEqual(tid2, tid1); - assert.notDeepEqual(tid2, undefined); - hasTextContentType(res); - assert.remoteRequests(true); - assert.cleanupRecorder(); - // delay for 1s to let the content change on re-render - // Check retrieval of a stored render - return P.delay(1100) - .then(() => { - return preq.get({ - uri: `${server.config.baseURL()}/service/test/User:GWicke%2fDate`, - }); - }); - }) - .then((res) => { - const tid3 = res.headers.etag; - assert.deepEqual(tid3, tid2); - assert.notDeepEqual(tid3, undefined); - // Check that there were no remote requests - assert.remoteRequests(false); - hasTextContentType(res); - }); - }); -}); diff --git a/test/features/specification/monitoring.js b/test/features/specification/monitoring.js index 71b6e37f6..7cabd1f4e 100644 --- a/test/features/specification/monitoring.js +++ b/test/features/specification/monitoring.js @@ -1,6 +1,6 @@ 'use strict'; -const parallel = require('mocha.parallel'); +const parallel = describe;//require('mocha.parallel'); const preq = require('preq'); const assert = require('../../utils/assert.js'); const Server = require('../../utils/server.js'); @@ -180,7 +180,7 @@ describe('Monitoring tests', function() { return res.body; }) .then((spec) => { - parallel(`Monitoring routes, ${options.domain} domain`, () => { + const defineTests = () => { constructTests(spec, options, server).forEach((testCase) => { it(testCase.title, () => { const missingParam = /\/{(.+)}/.exec(testCase.request.uri); @@ -197,7 +197,9 @@ describe('Monitoring tests', function() { }); }); }); - }); + }; + parallel(`Monitoring routes, ${options.domain} domain, new content`, defineTests); + parallel(`Monitoring routes, ${options.domain} domain, from storage`, defineTests); after(() => server.stop()); }); }); diff --git a/test/test_module.yaml b/test/test_module.yaml index 1b5928828..8b8780dcb 100644 --- a/test/test_module.yaml +++ b/test/test_module.yaml @@ -10,45 +10,6 @@ paths: title: Wikimedia testing APIs x-is-api-root: true paths: - /service/test/{title}: &service_test_title - get: - x-setup-handler: - - init_storage: - uri: /{domain}/sys/key_value/testservice.test - - x-request-handler: - - get_from_storage: - request: - uri: /{domain}/sys/key_value/testservice.test/{title} - headers: - 'cache-control': '{{cache-control}}' - catch: - status: 404 - return_if: - status: 200 - - get_from_api: - request: - uri: http://en.wikipedia.org/wiki/{+title} - body: '{{request.body}}' - - store: - request: - method: put - uri: /{domain}/sys/key_value/testservice.test/{title} - headers: '{{get_from_api.headers}}' - body: '{{get_from_api.body}}' - - return_response: - return: - status: '{{get_from_api.status}}' - headers: - 'content-type': '{{get_from_api.headers.content-type}}' - 'etag': '{{store.headers.etag}}' - body: '{{get_from_api.body}}' - - x-monitor: false - - /service/test/{title}/{revision}: - <<: *service_test_title - /service/test_parallel/{key1}/{key2}: get: x-request-handler: diff --git a/v1/definition.yaml b/v1/definition.yaml index a47eee8d6..0c8c38e81 100644 --- a/v1/definition.yaml +++ b/v1/definition.yaml @@ -109,23 +109,25 @@ paths: content-type: '{{storage.headers.content-type}}' etag: '{{storage.headers.etag}}' cache-control: '{{options.response_cache_control}}' + content-language: '{{storage.headers.content-language}}' + vary: '{{storage.headers.vary}}' body: '{{storage.body}}' - # Storage miss. Call mobile content service to get the definition. - extract: request: method: get uri: '{{$$.options.host}}/{domain}/v1/page/definition/{term}' - response: - # Define the response to save & return. - headers: - content-type: '{{extract.headers.content-type}}' - body: '{{extract.body}}' - store: request: method: put uri: /{domain}/sys/key_value/term.definition-ng/{request.params.term} - headers: '{{merge({"if-none-hash-match": "*"}, extract.headers)}}' + headers: + if-none-hash-match": '*' + cache-control: '{{request.headers.cache-control}}' + x-store-etag: '{{extract.headers.etag}}' + x-store-content-language: '{{extract.headers.content-language}}' + x-store-content-type: '{{extract.headers.content-type}}' + x-store-vary: '{{extract.headers.vary}}' body: '{{extract.body}}' # With the if-none-hash-match header the storage will return 412 # if the content is not changed. In that case, return from the @@ -137,9 +139,11 @@ paths: return: status: 200 headers: - content-type: '{{extract.headers.content-type}}' - etag: '{{store.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' + content-language: '{{extract.headers.content-language}}' + content-type: '{{extract.headers.content-type}}' + vary: '{{extract.headers.vary}}' body: '{{extract.body}}' - emit_change_event: request: @@ -151,9 +155,11 @@ paths: return: status: 200 headers: - content-type: '{{extract.headers.content-type}}' - etag: '{{store.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' + content-language: '{{extract.headers.content-language}}' + content-type: '{{extract.headers.content-type}}' + vary: '{{extract.headers.vary}}' body: '{{extract.body}}' x-monitor: true diff --git a/v1/summary.yaml b/v1/summary.yaml index 2200009bd..358c518e9 100644 --- a/v1/summary.yaml +++ b/v1/summary.yaml @@ -154,7 +154,11 @@ paths: request: method: put uri: /{domain}/sys/key_value/page_summary/{request.params.title} - headers: '{{merge({"if-none-hash-match": "*"}, extract.headers, {"cache-control": request.headers.cache-control})}}' + headers: + if-none-hash-match": '*' + cache-control: '{{request.headers.cache-control}}' + x-store-etag: '{{extract.headers.etag}}' + x-store-content-type: '{{extract.headers.content-type}}' body: '{{extract.body}}' # With the if-none-hash-match header the storage will return 412 # if the content is not changed. In that case, return from the @@ -167,9 +171,9 @@ paths: return: status: 200 headers: - content-type: '{{extract.headers.content-type}}' - etag: '{{store_and_return.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' + content-type: '{{extract.headers.content-type}}' body: '{{extract.body}}' - emit_change_event: request: @@ -183,9 +187,9 @@ paths: return: status: 200 headers: - content-type: '{{extract.headers.content-type}}' - etag: '{{store_and_return.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' + content-type: '{{extract.headers.content-type}}' body: '{{extract.body}}' x-monitor: true diff --git a/v1/summary_new.yaml b/v1/summary_new.yaml index 0c8fb618c..3b62427b9 100644 --- a/v1/summary_new.yaml +++ b/v1/summary_new.yaml @@ -135,7 +135,13 @@ paths: request: method: put uri: /{domain}/sys/key_value/page_summary/{request.params.title} - headers: '{{merge({"if-none-hash-match": "*", "cache-control": request.headers.cache-control}, extract.headers)}}' + headers: + if-none-hash-match": '*' + cache-control: '{{request.headers.cache-control}}' + x-store-etag: '{{extract.headers.etag}}' + x-store-content-language: '{{extract.headers.content-language}}' + x-store-content-type: '{{extract.headers.content-type}}' + x-store-vary: '{{extract.headers.vary}}' body: '{{extract.body}}' # With the if-none-hash-match header the storage will return 412 # if the content is not changed. In that case, return from the @@ -148,10 +154,10 @@ paths: return: status: 200 headers: - content-type: '{{extract.headers.content-type}}' - etag: '{{store_and_return.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' content-language: '{{extract.headers.content-language}}' + content-type: '{{extract.headers.content-type}}' vary: '{{extract.headers.vary}}' body: '{{extract.body}}' - emit_change_event: @@ -164,10 +170,10 @@ paths: return: status: 200 headers: - content-type: '{{extract.headers.content-type}}' - etag: '{{store_and_return.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' content-language: '{{extract.headers.content-language}}' + content-type: '{{extract.headers.content-type}}' vary: '{{extract.headers.vary}}' body: '{{extract.body}}' From 4a37241f41e2c24f5d483268b6aa3f1339f103a1 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Sun, 14 Apr 2019 12:16:13 -0700 Subject: [PATCH 3/3] Make mobile-sections use key_value instead of multi-content bucket Change-Id: Ifd57c445d36dfeb7e286ae42dd867c74afcab828 --- projects/dev.yaml | 13 ----- projects/wmf_enwiki.yaml | 17 +------ projects/wmf_wikipedia.yaml | 17 +------ projects/wmf_wikivoyage.yaml | 13 ----- projects/wmf_wiktionary.yaml | 19 ++----- sys/mobileapps.js | 97 +++++++++++++----------------------- 6 files changed, 41 insertions(+), 135 deletions(-) diff --git a/projects/dev.yaml b/projects/dev.yaml index c29840bfc..b210e63c7 100644 --- a/projects/dev.yaml +++ b/projects/dev.yaml @@ -118,18 +118,5 @@ paths: - path: sys/mobileapps.js options: '{{merge({"response_cache_control": options.purged_cache_control}, options.mobileapps)}}' - /mobile_bucket: - x-modules: - - path: sys/multi_content_bucket.js - options: - grace_ttl: '{{default(options.parsoid.grace_ttl, 86400)}}' - delete_probability: '{{default(options.parsoid.delete_probability, 1)}}' - table_name_prefix: mobile - main_content_type: - name: lead - value_type: json - dependent_content_types: - - name: remaining - value_type: json options: '{{options}}' diff --git a/projects/wmf_enwiki.yaml b/projects/wmf_enwiki.yaml index 5f9ef3a58..3b4d7b338 100644 --- a/projects/wmf_enwiki.yaml +++ b/projects/wmf_enwiki.yaml @@ -18,12 +18,12 @@ paths: allows us to contact you quickly. Email addresses or URLs of contact pages work well. - By using this API, you agree to Wikimedia's + By using this API, you agree to Wikimedia's [Terms of Use](https://wikimediafoundation.org/wiki/Terms_of_Use) and [Privacy Policy](https://wikimediafoundation.org/wiki/Privacy_policy). Unless otherwise specified in the endpoint documentation below, content accessed via this API is licensed under the - [CC-BY-SA 3.0](https://creativecommons.org/licenses/by-sa/3.0/) + [CC-BY-SA 3.0](https://creativecommons.org/licenses/by-sa/3.0/) and [GFDL](https://www.gnu.org/copyleft/fdl.html) licenses, and you irrevocably agree to release modifications or additions made through this API under these licenses. @@ -221,19 +221,6 @@ paths: - path: sys/mobileapps.js options: '{{merge({"response_cache_control": options.purged_cache_control}, options.mobileapps)}}' - /mobile_bucket: - x-modules: - - path: sys/multi_content_bucket.js - options: - grace_ttl: '{{default(options.parsoid.grace_ttl, 86400)}}' - delete_probability: '{{default(options.parsoid.delete_probability, 1)}}' - table_name_prefix: mobile_ng - main_content_type: - name: lead - value_type: json - dependent_content_types: - - name: remaining - value_type: json /events: x-modules: - path: sys/events.js diff --git a/projects/wmf_wikipedia.yaml b/projects/wmf_wikipedia.yaml index 88bae251f..6db2153c7 100644 --- a/projects/wmf_wikipedia.yaml +++ b/projects/wmf_wikipedia.yaml @@ -18,12 +18,12 @@ paths: allows us to contact you quickly. Email addresses or URLs of contact pages work well. - By using this API, you agree to Wikimedia's + By using this API, you agree to Wikimedia's [Terms of Use](https://wikimediafoundation.org/wiki/Terms_of_Use) and [Privacy Policy](https://wikimediafoundation.org/wiki/Privacy_policy). Unless otherwise specified in the endpoint documentation below, content accessed via this API is licensed under the - [CC-BY-SA 3.0](https://creativecommons.org/licenses/by-sa/3.0/) + [CC-BY-SA 3.0](https://creativecommons.org/licenses/by-sa/3.0/) and [GFDL](https://www.gnu.org/copyleft/fdl.html) licenses, and you irrevocably agree to release modifications or additions made through this API under these licenses. @@ -242,19 +242,6 @@ paths: - path: sys/mobileapps.js options: '{{merge({"response_cache_control": options.purged_cache_control}, options.mobileapps)}}' - /mobile_bucket: - x-modules: - - path: sys/multi_content_bucket.js - options: - grace_ttl: '{{default(options.parsoid.grace_ttl, 86400)}}' - delete_probability: '{{default(options.parsoid.delete_probability, 1)}}' - table_name_prefix: mobile_ng - main_content_type: - name: lead - value_type: json - dependent_content_types: - - name: remaining - value_type: json /events: x-modules: - path: sys/events.js diff --git a/projects/wmf_wikivoyage.yaml b/projects/wmf_wikivoyage.yaml index efbb26074..de6c1676a 100644 --- a/projects/wmf_wikivoyage.yaml +++ b/projects/wmf_wikivoyage.yaml @@ -179,19 +179,6 @@ paths: x-modules: - path: sys/mobileapps.js options: '{{merge({"response_cache_control": options.purged_cache_control}, options.mobileapps)}}' - /mobile_bucket: - x-modules: - - path: sys/multi_content_bucket.js - options: - grace_ttl: '{{default(options.parsoid.grace_ttl, 86400)}}' - delete_probability: '{{default(options.parsoid.delete_probability, 1)}}' - table_name_prefix: mobile_ng - main_content_type: - name: lead - value_type: json - dependent_content_types: - - name: remaining - value_type: json /events: x-modules: - path: sys/events.js diff --git a/projects/wmf_wiktionary.yaml b/projects/wmf_wiktionary.yaml index b399ba58e..6306375f2 100644 --- a/projects/wmf_wiktionary.yaml +++ b/projects/wmf_wiktionary.yaml @@ -19,15 +19,15 @@ paths: allows us to contact you quickly. Email addresses or URLs of contact pages work well. - By using this API, you agree to Wikimedia's + By using this API, you agree to Wikimedia's [Terms of Use](https://wikimediafoundation.org/wiki/Terms_of_Use) and [Privacy Policy](https://wikimediafoundation.org/wiki/Privacy_policy). Unless otherwise specified in the endpoint documentation below, content accessed via this API is licensed under the - [CC-BY-SA 3.0](https://creativecommons.org/licenses/by-sa/3.0/) + [CC-BY-SA 3.0](https://creativecommons.org/licenses/by-sa/3.0/) and [GFDL](https://www.gnu.org/copyleft/fdl.html) licenses, and you irrevocably agree to release modifications or - additions made through this API under these licenses. + additions made through this API under these licenses. See https://www.mediawiki.org/wiki/REST_API for background and details. ### Endpoint documentation @@ -174,19 +174,6 @@ paths: - path: sys/mobileapps.js options: '{{merge({"response_cache_control": options.purged_cache_control}, options.mobileapps)}}' - /mobile_bucket: - x-modules: - - path: sys/multi_content_bucket.js - options: - grace_ttl: '{{default(options.parsoid.grace_ttl, 86400)}}' - delete_probability: '{{default(options.parsoid.delete_probability, 1)}}' - table_name_prefix: mobile_ng - main_content_type: - name: lead - value_type: json - dependent_content_types: - - name: remaining - value_type: json /events: x-modules: - path: sys/events.js diff --git a/sys/mobileapps.js b/sys/mobileapps.js index 63784e308..8f32e921f 100644 --- a/sys/mobileapps.js +++ b/sys/mobileapps.js @@ -1,12 +1,13 @@ 'use strict'; -const P = require('bluebird'); const HyperSwitch = require('hyperswitch'); const URI = HyperSwitch.URI; const mwUtils = require('../lib/mwUtil'); const spec = HyperSwitch.utils.loadSpec(`${__dirname}/mobileapps.yaml`); +const BUCKET_NAME = 'mobile-sections'; + class MobileApps { constructor(options) { this._options = options; @@ -18,56 +19,28 @@ class MobileApps { } const rp = req.params; - const fetchPaths = { - lead: [rp.domain, 'sys', 'mobile_bucket', 'lead', rp.title], - remaining: [rp.domain, 'sys', 'mobile_bucket', 'remaining', rp.title] - }; - if (rp.revision) { - fetchPaths.lead.push(rp.revision); - fetchPaths.remaining.push(rp.revision); - } - return P.join( - hyper.get({ - uri: new URI(fetchPaths.lead) - }), - hyper.get({ - uri: new URI(fetchPaths.remaining) - }) - ).spread((lead, remaining) => ({ - status: 200, - headers: lead.headers, - body: { - lead: lead.body, - remaining: remaining.body + return hyper.get({ + uri: new URI([rp.domain, 'sys', 'key_value', BUCKET_NAME, rp.title]) + }) + .then((res) => { + if (!rp.revision || + `${mwUtils.parseETag(res.headers.etag).rev}` === `${rp.revision}`) { + return res; } - })) + return this._fetchFromMCS(hyper, req); + }) .catch({ status: 404 }, () => this._fetchFromMCSAndStore(hyper, req)); } getPart(part, hyper, req) { - const rp = req.params; - const fetchAndReturnPart = () => this._fetchFromMCSAndStore(hyper, req) + return this.getSections(hyper, req) .then((res) => { return { - status: 200, + status: res.status, headers: res.headers, body: res.body[part] }; }); - - if (mwUtils.isNoCacheRequest(req)) { - return fetchAndReturnPart(); - } - - const fetchPath = [rp.domain, 'sys', 'mobile_bucket', part, rp.title]; - if (rp.revision) { - fetchPath.push(rp.revision); - } - - return hyper.get({ - uri: new URI(fetchPath) - }) - .catch({ status: 404 }, fetchAndReturnPart); } _purgeURIs(hyper, req, revision, purgeLatest) { @@ -101,7 +74,7 @@ class MobileApps { }); } - _fetchFromMCSAndStore(hyper, req) { + _fetchFromMCS(hyper, req) { const rp = req.params; let serviceURI = `${this._options.host}/${rp.domain}/v1/page/mobile-sections`; serviceURI += `/${encodeURIComponent(rp.title)}`; @@ -114,33 +87,28 @@ class MobileApps { headers: { 'accept-language': req.headers['accept-language'] } - }) + }); + } + + _fetchFromMCSAndStore(hyper, req) { + const rp = req.params; + + return this._fetchFromMCS(hyper, req) .then((res) => { if (mwUtils.isNoStoreRequest(req)) { return res; } return hyper.put({ - uri: new URI([rp.domain, 'sys', 'mobile_bucket', 'all', rp.title, - res.body.lead.revision, - mwUtils.parseETag(res.headers.etag).tid]), - body: { - lead: { - headers: res.headers, - body: res.body.lead - }, - remaining: { - headers: res.headers, - body: res.body.remaining - } - } + uri: new URI([rp.domain, 'sys', 'key_value', BUCKET_NAME, rp.title]), + headers: { + 'x-store-etag': res.headers.etag, + 'x-store-content-language': res.headers['content-language'], + 'x-store-content-type': res.headers['content-type'], + 'x-store-vary': res.headers.vary + }, + body: res.body }) - .tap(() => - this._purgeURIs(hyper, req, res.body.lead.revision, true)) - // TODO: This means we never store older revisions for mobile! - // Need to add the fallback when mobile-references get implemented! - .catch({ status: 412 }, () => - // 412 means that it's an older revision - this._purgeURIs(hyper, req, res.body.lead.revision, false)) + .tap(() => this._purgeURIs(hyper, req, res.body.lead.revision, true)) .thenReturn(res); }); } @@ -158,7 +126,10 @@ module.exports = (options) => { }, resources: [ { - uri: '/{domain}/sys/mobile_bucket/' + uri: `/{domain}/sys/key_value/${BUCKET_NAME}`, + body: { + valueType: 'json' + } } ] };