From 51785368fe0ba442bc7627159516abbf9c680bd6 Mon Sep 17 00:00:00 2001 From: Nicolas Humbert Date: Thu, 8 Feb 2024 14:21:13 +0100 Subject: [PATCH] ARSN-392 Fix processVersionSpecificPut MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - For backward compatibility (if isNull is undefined), add the nullVersionId field to the master update. The nullVersionId is needed for listing, retrieving, and deleting null versions.
 - For the new null key implementation (if isNull is defined): add the isNull2 field and set it to true to specify that the new version is null AND has been put with a Cloudserver handling null keys (i.e., supporting S3C-7352).
 - Manage scenarios in which a version is marked with the isNull attribute set to true, but without a version ID. This happens after BackbeatClient.putMetadata() is applied to a standalone null master. --- lib/versioning/Version.ts | 76 ++++- lib/versioning/VersioningRequestProcessor.ts | 14 +- tests/unit/versioning/Version.spec.js | 55 ++++ .../VersioningRequestProcessor.spec.js | 280 ++++++++++++++++++ 4 files changed, 414 insertions(+), 11 deletions(-) create mode 100644 tests/unit/versioning/Version.spec.js diff --git a/lib/versioning/Version.ts b/lib/versioning/Version.ts index ff2b1113d..1e0a61be2 100644 --- a/lib/versioning/Version.ts +++ b/lib/versioning/Version.ts @@ -3,7 +3,7 @@ import { VersioningConstants } from './constants'; const VID_SEP = VersioningConstants.VersionId.Separator; /** * Class for manipulating an object version. - * The format of a version: { isNull, isDeleteMarker, versionId, otherInfo } + * The format of a version: { isNull, isNull2, isDeleteMarker, versionId, otherInfo } * * @note Some of these functions are optimized based on string search * prior to a full JSON parse/stringify. (Vinh: 18K op/s are achieved @@ -13,6 +13,7 @@ const VID_SEP = VersioningConstants.VersionId.Separator; export class Version { version: { isNull?: boolean; + isNull2?: boolean; isDeleteMarker?: boolean; versionId?: string; isPHD?: boolean; @@ -22,12 +23,16 @@ export class Version { * Create a new version instantiation from its data object. * @param version - the data object to instantiate * @param version.isNull - is a null version + * @param version.isNull2 - Whether new version is null or not AND has + * been put with a Cloudserver handling null keys (i.e. supporting + * S3C-7352) * @param version.isDeleteMarker - is a delete marker * @param version.versionId - the version id * @constructor */ constructor(version?: { isNull?: boolean; + isNull2?: boolean; isDeleteMarker?: boolean; versionId?: string; isPHD?: boolean; @@ -83,6 +88,31 @@ export class Version { return `{ "isPHD": true, "versionId": "${versionId}" }`; } + /** + * Appends a key-value pair to a JSON object represented as a string. It adds + * a comma if the object is not empty (i.e., not just '{}'). It assumes the input + * string is formatted as a JSON object. + * + * @param {string} stringifiedObject The JSON object as a string to which the key-value pair will be appended. + * @param {string} key The key to append to the JSON object. + * @param {string} value The value associated with the key to append to the JSON object. + * @returns {string} The updated JSON object as a string with the new key-value pair appended. + * @example + * _jsonAppend('{"existingKey":"existingValue"}', 'newKey', 'newValue'); + * // returns '{"existingKey":"existingValue","newKey":"newValue"}' + */ + static _jsonAppend(stringifiedObject: string, key: string, value: string): string { + // stringifiedObject value has the format of '{...}' + let index = stringifiedObject.length - 2; + while (stringifiedObject.charAt(index--) === ' '); + const comma = stringifiedObject.charAt(index + 1) !== '{'; + return ( + `${stringifiedObject.slice(0, stringifiedObject.length - 1)}` + // eslint-disable-line + (comma ? ',' : '') + + `"${key}":"${value}"}` + ); + } + /** * Put versionId into an object in the (cheap) way of string manipulation, * instead of the more expensive alternative parsing and stringification. @@ -93,14 +123,32 @@ export class Version { */ static appendVersionId(value: string, versionId: string): string { // assuming value has the format of '{...}' - let index = value.length - 2; - while (value.charAt(index--) === ' '); - const comma = value.charAt(index + 1) !== '{'; - return ( - `${value.slice(0, value.length - 1)}` + // eslint-disable-line - (comma ? ',' : '') + - `"versionId":"${versionId}"}` - ); + return Version._jsonAppend(value, 'versionId', versionId); + } + + /** + * Updates or appends a `nullVersionId` property to a JSON-formatted string. + * This function first checks if the `nullVersionId` property already exists within the input string. + * If it exists, the function updates the `nullVersionId` with the new value provided. + * If it does not exist, the function appends a `nullVersionId` property with the provided value. + * + * @static + * @param {string} value - The JSON-formatted string that may already contain a `nullVersionId` property. + * @param {string} nullVersionId - The new value for the `nullVersionId` property to be updated or appended. + * @returns {string} The updated JSON-formatted string with the new `nullVersionId` value. + */ + static updateOrAppendNullVersionId(value: string, nullVersionId: string): string { + // Check if "nullVersionId" already exists in the string + const nullVersionIdPattern = /"nullVersionId":"[^"]*"/; + const nullVersionIdExists = nullVersionIdPattern.test(value); + + if (nullVersionIdExists) { + // Replace the existing nullVersionId with the new one + return value.replace(nullVersionIdPattern, `"nullVersionId":"${nullVersionId}"`); + } else { + // Append nullVersionId + return Version._jsonAppend(value, 'nullVersionId', nullVersionId); + } } /** @@ -190,6 +238,16 @@ export class Version { return this; } + /** + * Mark that the null version has been put with a Cloudserver handling null keys (i.e. supporting S3C-7352) + * + * @return - the updated version + */ + setNull2Version() { + this.version.isNull2 = true; + return this; + } + /** * Serialize the version. * diff --git a/lib/versioning/VersioningRequestProcessor.ts b/lib/versioning/VersioningRequestProcessor.ts index 914da35f2..b60cec04a 100644 --- a/lib/versioning/VersioningRequestProcessor.ts +++ b/lib/versioning/VersioningRequestProcessor.ts @@ -496,6 +496,7 @@ export default class VersioningRequestProcessor { const versionIdFromMaster = masterVersion.getVersionId(); if (versionIdFromMaster === undefined || versionIdFromMaster >= versionId) { + let value = request.value; logger.debug('version to put is not older than master'); // new behavior when isNull is defined is to only // update the master key if it is the latest @@ -509,7 +510,7 @@ export default class VersioningRequestProcessor { request.options.isNull === undefined) { // master key is strictly older than the put version let masterVersionId; - if (masterVersion.isNullVersion()) { + if (masterVersion.isNullVersion() && versionIdFromMaster) { logger.debug('master key is a null version'); masterVersionId = versionIdFromMaster; } else if (versionIdFromMaster === undefined) { @@ -537,13 +538,22 @@ export default class VersioningRequestProcessor { '' : masterVersionId; const masterVersionKey = formatVersionKey(key, masterKeyVersionId); masterVersion.setNullVersion(); + // isNull === false means Cloudserver supports null keys, + // so create a null key with the isNull2 flag + if (request.options.isNull === false) { + masterVersion.setNull2Version(); + // else isNull === undefined means Cloudserver does not support null keys, + // hence set/update the new master nullVersionId for backward compatibility + } else { + value = Version.updateOrAppendNullVersionId(request.value, masterVersionId); + } ops.push({ key: masterVersionKey, value: masterVersion.toString() }); } } else { logger.debug('version to put is the master'); } - ops.push({ key, value: request.value }); + ops.push({ key, value: value }); } else { logger.debug('version to put is older than master'); if (request.options.isNull === true && !masterVersion.isNullVersion()) { diff --git a/tests/unit/versioning/Version.spec.js b/tests/unit/versioning/Version.spec.js new file mode 100644 index 000000000..e96297b4e --- /dev/null +++ b/tests/unit/versioning/Version.spec.js @@ -0,0 +1,55 @@ +const { Version } = require('../../../lib/versioning/Version'); + +describe('Version', () => { + describe('appendVersionId', () => { + it('should append versionId to an empty object', () => { + const emptyObject = '{}'; + const versionId = '123'; + const expected = '{"versionId":"123"}'; + const result = Version.appendVersionId(emptyObject, versionId); + expect(result).toEqual(expected); + }); + + it('should append versionId to an object with existing properties', () => { + const existingObject = '{"key":"value"}'; + const versionId = '456'; + const expected = '{"key":"value","versionId":"456"}'; + const result = Version.appendVersionId(existingObject, versionId); + expect(result).toEqual(expected); + }); + + it('should append versionId to an object with existing versionId', () => { + const objectWithVersionId = '{"key":"value","versionId":"old"}'; + const versionId = 'new'; + const expected = '{"key":"value","versionId":"old","versionId":"new"}'; + const result = Version.appendVersionId(objectWithVersionId, versionId); + expect(result).toEqual(expected); + }); + }); + + describe('updateOrAppendNullVersionId', () => { + it('should append nullVersionId when it does not exist', () => { + const initialValue = '{"key":"value"}'; + const nullVersionId = '12345'; + const expectedValue = '{"key":"value","nullVersionId":"12345"}'; + const result = Version.updateOrAppendNullVersionId(initialValue, nullVersionId); + expect(result).toEqual(expectedValue); + }); + + it('should update nullVersionId when it exists', () => { + const initialValue = '{"key":"value","nullVersionId":"initial"}'; + const nullVersionId = 'updated12345'; + const expectedValue = '{"key":"value","nullVersionId":"updated12345"}'; + const result = Version.updateOrAppendNullVersionId(initialValue, nullVersionId); + expect(result).toEqual(expectedValue); + }); + + it('should handle empty string by appending nullVersionId', () => { + const initialValue = '{}'; + const nullVersionId = 'emptyCase12345'; + const expectedValue = '{"nullVersionId":"emptyCase12345"}'; + const result = Version.updateOrAppendNullVersionId(initialValue, nullVersionId); + expect(result).toEqual(expectedValue); + }); + }); +}); diff --git a/tests/unit/versioning/VersioningRequestProcessor.spec.js b/tests/unit/versioning/VersioningRequestProcessor.spec.js index a801b4ebb..240b6b9fd 100644 --- a/tests/unit/versioning/VersioningRequestProcessor.spec.js +++ b/tests/unit/versioning/VersioningRequestProcessor.spec.js @@ -254,6 +254,286 @@ describe('test VRP', () => { }], done); }); + + it('should be able to put Metadata on top of a standalone null version', done => { + const versionId = '00000000000000999999PARIS '; + + async.waterfall([next => { + // simulate the creation of a standalone null version. + const request = { + db: 'foo', + key: 'bar', + value: '{"qux":"quz"}', + options: {}, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + // simulate a BackbeatClient.putMetadata + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}"}`, + options: { + versioning: true, + versionId, + // isNull === false means Cloudserver supports the new "null key" logic. + isNull: false, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + wgm.list({}, logger, next); + }, + (res, next) => { + const expectedListing = [ + // master version should have the provided version id + { + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}"}`, + }, + // The null version will get the highest version number. + // It should have "isNull" and "isNul2" set to true, + // showing it's a null version made by Cloudserver that works with null keys. + { + key: `bar${VID_SEP}`, + value: '{"qux":"quz","versionId":"99999999999999999999PARIS ","isNull":true,"isNull2":true}', + }, + // the new version + { + key: `bar${VID_SEP}${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, + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); + + it('should be able to put Metadata on top of a standalone null version in backward compatibility mode', done => { + const versionId = '00000000000000999999PARIS '; + + async.waterfall([next => { + // simulate the creation of a standalone null version. + const request = { + db: 'foo', + key: 'bar', + value: '{"qux":"quz"}', + options: {}, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + // simulate a BackbeatClient.putMetadata + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}"}`, + options: { + versioning: true, + versionId, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + wgm.list({}, logger, next); + }, + (res, next) => { + const expectedListing = [ + // master version should have the provided version id and a reference of the null version id. + { + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}","nullVersionId":"99999999999999999999PARIS "}`, + }, + // the "internal" master version should have the provided version id. + { + key: `bar${VID_SEP}${versionId}`, + value: `{"qux":"quz2","versionId":"${versionId}"}`, + }, + // should create a version that represents the old null master with the infinite version id and + // the isNull property set to true. + { + key: `bar${VID_SEP}99999999999999999999PARIS `, + value: '{"qux":"quz","versionId":"99999999999999999999PARIS ","isNull":true}', + }, + ]; + assert.deepStrictEqual(res, expectedListing); + const request = { + db: 'foo', + key: 'bar', + }; + vrp.get(request, logger, next); + }, + (res, next) => { + const expectedGet = { + qux: 'quz2', + versionId, + nullVersionId: '99999999999999999999PARIS ', + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); + + it('should be able to put Metadata on top of a null suspended version', 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 + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}"}`, + options: { + versioning: true, + versionId, + // isNull === false means Cloudserver supports the new "null key" logic. + isNull: false, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + wgm.list({}, logger, next); + }, + (res, next) => { + const expectedListing = [ + // master version should have the provided version id + { + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}"}`, + }, + // The null version will get the highest version number. + // It should have "isNull" and "isNul2" set to true, + // showing it's a null version made by Cloudserver that works with null keys. + { + key: `bar${VID_SEP}`, + value: `{"qux":"quz","isNull":true,"versionId":"${nullVersionId}","isNull2":true}`, + }, + // the new version + { + key: `bar${VID_SEP}${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, + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); + + it('should be able to put Metadata on top of a null suspended version in backward compatibility mode', 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 + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}"}`, + options: { + versioning: true, + versionId, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + wgm.list({}, logger, next); + }, + (res, next) => { + const expectedListing = [ + // master version should have the provided version id and a reference of the null version id. + { + key: 'bar', + value: `{"qux":"quz2","versionId":"${versionId}","nullVersionId":"${nullVersionId}"}`, + }, + // the "internal" master version should have the provided version id. + { + key: `bar${VID_SEP}${versionId}`, + value: `{"qux":"quz2","versionId":"${versionId}"}`, + }, + // should create a version that represents the old null master with the infinite version id and + // the isNull property set to true. + { + key: `bar${VID_SEP}${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', + versionId, + nullVersionId, + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); });