diff --git a/lib/versioning/VersioningRequestProcessor.ts b/lib/versioning/VersioningRequestProcessor.ts index c1d79e014..67d67a1f0 100644 --- a/lib/versioning/VersioningRequestProcessor.ts +++ b/lib/versioning/VersioningRequestProcessor.ts @@ -509,8 +509,8 @@ export default class VersioningRequestProcessor { if (request.options.isNull !== undefined && // new null key behavior when isNull is defined. masterVersion.isNullVersion() && // master is null !masterVersion.isNull2Version()) { // master does not support the new null key behavior yet. - const masterNullVersionId = masterVersion.getNullVersionId(); - // The deprecated null key is referenced in the "nullVersionId" property of the master key. + const masterNullVersionId = masterVersion.getVersionId(); + // The deprecated null key is referenced in the "versionId" property of the master key. if (masterNullVersionId) { const oldNullVersionKey = formatVersionKey(key, masterNullVersionId); ops.push({ key: oldNullVersionKey, type: 'del' }); @@ -561,8 +561,10 @@ export default class VersioningRequestProcessor { if (request.options.isNull === false) { masterVersion.setNull2Version(); // else isNull === undefined means Cloudserver does not support null keys, + // and versionIdFromMaster !== versionId means that a version is PUT on top of a null version // hence set/update the new master nullVersionId for backward compatibility - } else { + } else if (versionIdFromMaster !== versionId) { + // => set the nullVersionId to the master version if put version on top of null version. value = Version.updateOrAppendNullVersionId(request.value, masterVersionId); } ops.push({ key: masterVersionKey, diff --git a/tests/unit/versioning/VersioningRequestProcessor.spec.js b/tests/unit/versioning/VersioningRequestProcessor.spec.js index 6b720b28c..6661f0b7b 100644 --- a/tests/unit/versioning/VersioningRequestProcessor.spec.js +++ b/tests/unit/versioning/VersioningRequestProcessor.spec.js @@ -535,6 +535,70 @@ describe('test VRP', () => { done); }); + it('should be able to update a null suspended version in backward compatibility mode', done => { + let nullVersionId; + + async.waterfall([next => { + // simulate the creation of a null suspended version. + const request = { + db: 'foo', + key: 'bar', + value: '{"qux":"quz","isNull":true}', + options: { + versionId: '', + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + nullVersionId = JSON.parse(res).versionId; + // simulate update null version with BackbeatClient.putMetadata + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","isNull":true,"versionId":"${nullVersionId}"}`, + options: { + versioning: true, + versionId: nullVersionId, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + wgm.list({}, logger, next); + }, + (res, next) => { + const expectedListing = [ + // NOTE: should not set nullVersionId to the master version if updating a null version. + { + key: 'bar', + value: `{"qux":"quz2","isNull":true,"versionId":"${nullVersionId}"}`, + }, + { + key: `bar\x00${nullVersionId}`, + value: `{"qux":"quz","isNull":true,"versionId":"${nullVersionId}"}`, + }, + ]; + assert.deepStrictEqual(res, expectedListing); + + const request = { + db: 'foo', + key: 'bar', + }; + vrp.get(request, logger, next); + }, + (res, next) => { + const expectedGet = { + qux: 'quz2', + isNull: true, + versionId: nullVersionId, + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); + it('should delete the deprecated null key after put Metadata on top of an old null master', done => { const versionId = '00000000000000999999PARIS '; let nullVersionId; @@ -592,8 +656,7 @@ describe('test VRP', () => { // the null key { key: `bar${VID_SEP}`, - value: `{"qux":"quz2","isNull":true,"versionId":"${nullVersionId}",` + - `"nullVersionId":"${nullVersionId}","isNull2":true}`, + value: `{"qux":"quz2","isNull":true,"versionId":"${nullVersionId}","isNull2":true}`, }, // version key { @@ -691,6 +754,88 @@ describe('test VRP', () => { }], done); }); + + it('should delete the deprecated null key after updating a non-latest null key', done => { + const versionId = '00000000000000999999PARIS '; + let nullVersionId; + + async.waterfall([next => { + // simulate the creation of a null suspended version. + const request = { + db: 'foo', + key: 'bar', + value: '{"qux":"quz","isNull":true}', + options: { + versionId: '', + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + nullVersionId = JSON.parse(res).versionId; + // simulate a BackbeatClient.putMetadata + // null key is not the latest = master is not null. + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}"}`, + options: { + versioning: true, + versionId, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + // update the null version metadata with the new keys implementation (options.isNull defined) + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz3","isNull2":true,"isNull":true,"versionId":"${nullVersionId}"}`, + options: { + versionId: nullVersionId, + isNull: true, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + wgm.list({}, logger, next); + }, + (res, next) => { + const expectedListing = [ + { + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}","nullVersionId":"${nullVersionId}"}`, + }, + { + key: 'bar\x00', + value: `{"qux":"quz3","isNull2":true,"isNull":true,"versionId":"${nullVersionId}"}`, + }, + { + key: `bar\x00${versionId}`, + value: `{"qux":"quz2","versionId":"${versionId}"}`, + }, + ]; + assert.deepStrictEqual(res, expectedListing); + + const request = { + db: 'foo', + key: 'bar', + }; + vrp.get(request, logger, next); + }, + (res, next) => { + const expectedGet = { + qux: 'quz2', + versionId, + nullVersionId, + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); });