From a0cf650d94dc35b1afa4350f1ac25e27f481f058 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Wed, 3 Apr 2019 15:47:40 -0300 Subject: [PATCH 1/3] Simplify key_value bucket Change-Id: I7f698c8f6914027c89923c11123d0bd13286feb8 --- lib/mwUtil.js | 7 +-- sys/key_value.js | 61 +++++++---------------- test/features/buckets/key_value_bucket.js | 9 +++- test/features/router/handlerTemplate.js | 13 +---- test/features/specification/monitoring.js | 1 + test/test_module.yaml | 4 +- v1/definition.yaml | 19 ++++--- v1/summary_new.yaml | 12 +++-- 8 files changed, 53 insertions(+), 73 deletions(-) diff --git a/lib/mwUtil.js b/lib/mwUtil.js index d42fc51ae..cc55799dc 100644 --- a/lib/mwUtil.js +++ b/lib/mwUtil.js @@ -17,12 +17,13 @@ const mwUtil = {}; /** * Create an etag value of the form * "//" - * @param {Integer} rev page revision number - * @param {string} tid page render UUID + * @param {Integer} [rev] page revision number + * @param {string} [tid] page render UUID * @param {string} [suffix] optional suffix * @return {string} the value of the ETag header */ -mwUtil.makeETag = (rev, tid, suffix) => { +mwUtil.makeETag = (rev = 0, tid, suffix) => { + tid = tid || uuid.now(); let etag = `"${rev}/${tid}`; if (suffix) { etag += `/${suffix}`; diff --git a/sys/key_value.js b/sys/key_value.js index eb4366a53..ed800c549 100644 --- a/sys/key_value.js +++ b/sys/key_value.js @@ -4,7 +4,6 @@ * Key-value bucket handler */ -const uuid = require('cassandra-uuid').TimeUuid; const mwUtil = require('../lib/mwUtil'); const HyperSwitch = require('hyperswitch'); const stringify = require('fast-json-stable-stringify'); @@ -19,15 +18,9 @@ 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, + headers: row.headers, body: row.value }; } else { @@ -56,7 +49,7 @@ class KVBucket { } makeSchema(opts) { - const schemaVersionMajor = 5; + const schemaVersionMajor = 6; return { // Combine option & bucket version into a monotonically increasing @@ -76,7 +69,6 @@ class KVBucket { }, attributes: { key: opts.keyType || 'string', - tid: 'timeuuid', headers: 'json', value: opts.valueType || 'blob' }, @@ -88,7 +80,13 @@ class KVBucket { getRevision(hyper, req) { if (mwUtil.isNoCacheRequest(req)) { - throw new HTTPError({ status: 404 }); + throw new HTTPError({ + status: 404, + body: { + type: 'not_found', + description: 'Not attempting to fetch content for no-cache request' + } + }); } const rp = req.params; @@ -98,8 +96,7 @@ class KVBucket { table: rp.bucket, attributes: { key: rp.key - }, - limit: 1 + } } }; return hyper.get(storeReq).then(returnRevision(req)); @@ -114,7 +111,6 @@ class KVBucket { attributes: { key: req.params.key }, - proj: ['tid'], limit: 1000 } }; @@ -132,20 +128,8 @@ class KVBucket { 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 doPut = () => hyper.put({ @@ -154,7 +138,6 @@ class KVBucket { table: rp.bucket, attributes: { key: rp.key, - tid, value: req.body, headers: req.headers } @@ -164,22 +147,15 @@ class KVBucket { if (res.status === 201) { return { status: 201, - headers: { - etag: req.headers && req.headers.etag || mwUtil.makeETag('0', tid) - }, body: { - message: 'Created.', - tid + message: 'Created.' } }; } else { throw res; } }) - .catch((error) => { - hyper.logger.log('error/kv/putRevision', error); - return { status: 400 }; - }); + .tapCatch((error) => hyper.logger.log('error/kv/putRevision', error)); if (req.headers['if-none-hash-match']) { delete req.headers['if-none-hash-match']; @@ -191,14 +167,15 @@ class KVBucket { (!req.headers['content-type'] || req.headers['content-type'] === oldContent.headers['content-type'])) { hyper.metrics.increment(`sys_kv_${req.params.bucket}.unchanged_rev_render`); - return { + throw new HTTPError({ status: 412, - headers: { - etag: oldContent.headers.etag + body: { + type: 'precondition_failed', + description: 'Not replacing existing content' } - }; + }); } - throw new HTTPError({ status: 404 }); + return doPut(); }) .catch({ status: 404 }, doPut); } else { diff --git a/test/features/buckets/key_value_bucket.js b/test/features/buckets/key_value_bucket.js index 73780121c..47c974dd8 100644 --- a/test/features/buckets/key_value_bucket.js +++ b/test/features/buckets/key_value_bucket.js @@ -4,6 +4,7 @@ const preq = require('preq'); const assert = require('../../utils/assert.js'); const Server = require('../../utils/server.js'); const uuid = require('cassandra-uuid').TimeUuid; +const mwUtils = require('../../../lib/mwUtil'); const P = require('bluebird'); const parallel = require('mocha.parallel'); @@ -46,10 +47,14 @@ describe('Key value buckets', () => { }); }); - it('assigns etag to a value', () => { + it('preserves headers', () => { const testData = randomString(100); + const testEtag = mwUtils.makeETag(); return preq.put({ uri: `${bucketBaseURI}/Test3`, + headers: { + etag: testEtag + }, body: new Buffer(testData) }) .then((res) => { @@ -60,7 +65,7 @@ describe('Key value buckets', () => { }) .then((res) => { assert.deepEqual(res.status, 200); - assert.ok(res.headers.etag); + assert.deepEqual(res.headers.etag, testEtag); assert.ok(new RegExp('^"0\/').test(res.headers.etag), true); }); }); diff --git a/test/features/router/handlerTemplate.js b/test/features/router/handlerTemplate.js index 567fdc8a6..81e26298f 100644 --- a/test/features/router/handlerTemplate.js +++ b/test/features/router/handlerTemplate.js @@ -15,17 +15,12 @@ describe('handler template', function() { 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 @@ -40,9 +35,6 @@ describe('handler template', function() { }); }) .then((res) => { - tid2 = res.headers.etag; - assert.notDeepEqual(tid2, tid1); - assert.notDeepEqual(tid2, undefined); hasTextContentType(res); assert.remoteRequests(true); assert.cleanupRecorder(); @@ -56,12 +48,9 @@ describe('handler template', function() { }); }) .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); - }); + }).tapCatch(console.log); }); }); diff --git a/test/features/specification/monitoring.js b/test/features/specification/monitoring.js index 71b6e37f6..f66fe7f8e 100644 --- a/test/features/specification/monitoring.js +++ b/test/features/specification/monitoring.js @@ -193,6 +193,7 @@ describe('Monitoring tests', function() { .then((res) => { validateTestResponse(testCase, res); }, (err) => { + console.log(err); validateTestResponse(testCase, err); }); }); diff --git a/test/test_module.yaml b/test/test_module.yaml index 7a637093f..ef5a16952 100644 --- a/test/test_module.yaml +++ b/test/test_module.yaml @@ -39,9 +39,7 @@ paths: - return_response: return: status: '{{get_from_api.status}}' - headers: - 'content-type': '{{get_from_api.headers.content-type}}' - 'etag': '{{store.headers.etag}}' + headers: '{{get_from_api.headers}}' body: '{{get_from_api.body}}' x-monitor: false diff --git a/v1/definition.yaml b/v1/definition.yaml index a47eee8d6..45b5532e3 100644 --- a/v1/definition.yaml +++ b/v1/definition.yaml @@ -115,17 +115,20 @@ paths: - extract: request: method: get - uri: '{{$$.options.host}}/{domain}/v1/page/definition/{term}' + uri: '{{options.host}}/{domain}/v1/page/definition/{term}' response: - # Define the response to save & return. - headers: - content-type: '{{extract.headers.content-type}}' + status: '{{extract.status}}' + headers: '{{extract.headers}}' 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)}}' + uri: /{domain}/sys/key_value/term.definition-ng/{term} + headers: + content-type: '{{extract.headers.content-type}}' + etag: '{{extract.headers.etag}}' + content-language: '{{extract.headers.content-language}}' + if-none-hash-match: '*' 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 @@ -138,7 +141,7 @@ paths: status: 200 headers: content-type: '{{extract.headers.content-type}}' - etag: '{{store.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' body: '{{extract.body}}' - emit_change_event: @@ -152,7 +155,7 @@ paths: status: 200 headers: content-type: '{{extract.headers.content-type}}' - etag: '{{store.headers.etag}}' + etag: '{{extract.headers.etag}}' cache-control: '{{options.response_cache_control}}' body: '{{extract.body}}' diff --git a/v1/summary_new.yaml b/v1/summary_new.yaml index 0c8fb618c..30a0093ea 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: + content-type: '{{extract.headers.content-type}}' + cache-control: '{{request.headers.cache-control}}' + etag: '{{extract.headers.etag}}' + content-language: '{{extract.headers.content-language}}' + vary: '{{extract.headers.vary}}' + if-none-hash-match: '*' 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 @@ -149,7 +155,7 @@ paths: 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}}' vary: '{{extract.headers.vary}}' @@ -165,7 +171,7 @@ paths: 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}}' vary: '{{extract.headers.vary}}' From 86163e1eeab8835405477f1cf90505bf0c39cfe6 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Wed, 3 Apr 2019 16:21:57 -0300 Subject: [PATCH 2/3] Use key_value bucket for mobile-sections Change-Id: I38f949bcc47453503759b3d86e6f522fe56581b0 --- projects/dev.yaml | 14 ---- projects/wmf_enwiki.yaml | 17 +---- projects/wmf_wikipedia.yaml | 17 +---- projects/wmf_wikivoyage.yaml | 13 ---- projects/wmf_wiktionary.yaml | 19 +---- sys/mobileapps.js | 92 ++++++++----------------- test/features/router/handlerTemplate.js | 2 +- 7 files changed, 37 insertions(+), 137 deletions(-) diff --git a/projects/dev.yaml b/projects/dev.yaml index c29840bfc..664c29d9b 100644 --- a/projects/dev.yaml +++ b/projects/dev.yaml @@ -118,18 +118,4 @@ 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..cb0f3e751 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,23 @@ 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: res.headers, + 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 +121,10 @@ module.exports = (options) => { }, resources: [ { - uri: '/{domain}/sys/mobile_bucket/' + uri: `/{domain}/sys/key_value/${BUCKET_NAME}`, + body: { + valueType: 'json' + } } ] }; diff --git a/test/features/router/handlerTemplate.js b/test/features/router/handlerTemplate.js index 81e26298f..ee2db7b09 100644 --- a/test/features/router/handlerTemplate.js +++ b/test/features/router/handlerTemplate.js @@ -51,6 +51,6 @@ describe('handler template', function() { // Check that there were no remote requests assert.remoteRequests(false); hasTextContentType(res); - }).tapCatch(console.log); + }); }); }); From d477b8e293c973f34d84119b7fc047d44840c4b1 Mon Sep 17 00:00:00 2001 From: Petr Pchelko Date: Thu, 4 Apr 2019 16:36:40 -0300 Subject: [PATCH 3/3] Add default etag and resurrect storing tid in a separate column Change-Id: Idd1200f67699efa7441ef9d1170823f42f036871 --- sys/key_value.js | 82 ++++++++++++----------- test/features/buckets/key_value_bucket.js | 4 +- 2 files changed, 46 insertions(+), 40 deletions(-) diff --git a/sys/key_value.js b/sys/key_value.js index ed800c549..91915a03f 100644 --- a/sys/key_value.js +++ b/sys/key_value.js @@ -4,38 +4,16 @@ * Key-value bucket handler */ +const crypto = require('crypto'); +const stringify = require('fast-json-stable-stringify'); +const TimeUUID = require('cassandra-uuid').TimeUuid; const mwUtil = require('../lib/mwUtil'); const HyperSwitch = require('hyperswitch'); -const stringify = require('fast-json-stable-stringify'); const HTTPError = HyperSwitch.HTTPError; 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]; - 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 - } - }); - } - }; -} - class KVBucket { createBucket(hyper, req) { const schema = this.makeSchema(req.body || {}); @@ -56,19 +34,13 @@ class KVBucket { // combined schema version. By multiplying the bucket version by 1000, // we increase the chance of catching a reset in the option version. version: schemaVersionMajor * 1000 + (opts.version || 0), - options: { - compression: opts.compression || [ - { - algorithm: 'deflate', - block_size: 256 - } - ], - updates: opts.updates || { - pattern: 'timeseries' - } - }, attributes: { key: opts.keyType || 'string', + // Both TID and ETAG are added in case we ever want to support + // CAS using lightweight transactions to support proper + // conditional HTTP requests with `if-modified-since` or `if-match` + tid: 'timeuuid', + etag: 'string', headers: 'json', value: opts.valueType || 'blob' }, @@ -99,7 +71,25 @@ class KVBucket { } } }; - 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 + } + }); + } + }); } listRevisions(hyper, req) { @@ -127,17 +117,31 @@ class KVBucket { } putRevision(hyper, req) { - const rp = req.params; if (mwUtil.isNoStoreRequest(req)) { return { status: 202 }; } + const rp = req.params; + req.headers = req.headers || {}; + + const tid = TimeUUID.now().toString(); + if (!req.headers.etag) { + hyper.logger.log('fatal/kv/putRevision', { + msg: 'No etag header provided to key-value bucket' + }); + req.headers.etag = crypto.createHash('sha256') + .update(stringify(req.body)) + .digest('hex'); + } + const doPut = () => hyper.put({ uri: new URI([rp.domain, 'sys', 'table', rp.bucket, '']), body: { table: rp.bucket, attributes: { key: rp.key, + tid, + etag: req.headers.etag, value: req.body, headers: req.headers } @@ -157,6 +161,8 @@ class KVBucket { }) .tapCatch((error) => hyper.logger.log('error/kv/putRevision', error)); + // TODO: Respect the stored ETag and allow matching on etag - either one provided + // by the client or auto-generated one. if (req.headers['if-none-hash-match']) { delete req.headers['if-none-hash-match']; return hyper.get({ diff --git a/test/features/buckets/key_value_bucket.js b/test/features/buckets/key_value_bucket.js index 47c974dd8..50410053a 100644 --- a/test/features/buckets/key_value_bucket.js +++ b/test/features/buckets/key_value_bucket.js @@ -86,7 +86,7 @@ describe('Key value buckets', () => { const tids = [ uuid.now().toString(), uuid.now().toString(), uuid.now().toString() ]; - return P.each(tids, (tid) => { + return P.each(tids, () => { return preq.put({ uri: `${bucketBaseURI}/List_Test_1`, body: new Buffer(testData), @@ -94,7 +94,7 @@ describe('Key value buckets', () => { 'if-none-hash-match': '*' } }) - .catch(() => {}); + .catch({ status: 412 }, () => {}); }) .then(() => { return preq.get({