From 344c287f1807116b8b4fc4a5706a9cc80cbcc7cd Mon Sep 17 00:00:00 2001 From: Nicolas Humbert Date: Thu, 8 Feb 2024 12:27:09 +0100 Subject: [PATCH 1/4] ARSN-392 Import the V0 processVersionSpecificPut from Metadata This logic is used by CRR replication feature to BackbeatClient.putMetadata on top of a null version (cherry picked from commit 40e271f7e2159df4dcc383d6b76640bf698acd0c) (cherry picked from commit 7be2ca2fedcfb640e35f6d0ba2adae32718ab293) --- lib/versioning/VersioningRequestProcessor.ts | 90 +++++++++++++++++--- 1 file changed, 77 insertions(+), 13 deletions(-) diff --git a/lib/versioning/VersioningRequestProcessor.ts b/lib/versioning/VersioningRequestProcessor.ts index 9267d4528..914da35f2 100644 --- a/lib/versioning/VersioningRequestProcessor.ts +++ b/lib/versioning/VersioningRequestProcessor.ts @@ -1,6 +1,6 @@ import errors, { ArsenalError } from '../errors'; import { Version } from './Version'; -import { generateVersionId as genVID } from './VersionID'; +import { generateVersionId as genVID, getInfVid } from './VersionID'; import WriteCache from './WriteCache'; import WriteGatheringManager from './WriteGatheringManager'; @@ -481,19 +481,83 @@ export default class VersioningRequestProcessor { const versionId = request.options.versionId; const versionKey = formatVersionKey(key, versionId); const ops: any = []; - if (!request.options.isNull) { - ops.push({ key: versionKey, value: request.value }); + const masterVersion = data !== undefined && + Version.from(data); + // push a version key if we're not updating the null + // version (or in legacy Cloudservers not sending the + // 'isNull' parameter, but this has an issue, see S3C-7526) + if (request.options.isNull !== true) { + const versionOp = { key: versionKey, value: request.value }; + ops.push(versionOp); } - if (data === undefined || - (Version.from(data).getVersionId() ?? '') >= versionId) { - // master does not exist or is not newer than put - // version and needs to be updated as well. - // Note that older versions have a greater version ID. - ops.push({ key: request.key, value: request.value }); - } else if (request.options.isNull) { - logger.debug('create or update null key'); - const nullKey = formatVersionKey(key, ''); - ops.push({ key: nullKey, value: request.value }); + if (masterVersion) { + // master key exists + // note that older versions have a greater version ID + const versionIdFromMaster = masterVersion.getVersionId(); + if (versionIdFromMaster === undefined || + versionIdFromMaster >= versionId) { + 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 + // version, old behavior needs to copy master to + // the null version because older Cloudservers + // rely on version-specific PUT to copy master + // contents to a new null version key (newer ones + // use special versionId="null" requests for this + // purpose). + if (versionIdFromMaster !== versionId || + request.options.isNull === undefined) { + // master key is strictly older than the put version + let masterVersionId; + if (masterVersion.isNullVersion()) { + logger.debug('master key is a null version'); + masterVersionId = versionIdFromMaster; + } else if (versionIdFromMaster === undefined) { + logger.debug('master key is nonversioned'); + // master key does not have a versionID + // => create one with the "infinite" version ID + masterVersionId = getInfVid(this.replicationGroupId); + masterVersion.setVersionId(masterVersionId); + } else { + logger.debug('master key is a regular version'); + } + if (request.options.isNull === true) { + if (!masterVersionId) { + // master is a regular version: delete the null key that + // may exist (older null version) + logger.debug('delete null key'); + const nullKey = formatVersionKey(key, ''); + ops.push({ key: nullKey, type: 'del' }); + } + } else if (masterVersionId) { + logger.debug('create version key from master version'); + // isNull === false means Cloudserver supports null keys, + // so create a null key in this case, and a version key otherwise + const masterKeyVersionId = request.options.isNull === false ? + '' : masterVersionId; + const masterVersionKey = formatVersionKey(key, masterKeyVersionId); + masterVersion.setNullVersion(); + ops.push({ key: masterVersionKey, + value: masterVersion.toString() }); + } + } else { + logger.debug('version to put is the master'); + } + ops.push({ key, value: request.value }); + } else { + logger.debug('version to put is older than master'); + if (request.options.isNull === true && !masterVersion.isNullVersion()) { + logger.debug('create or update null key'); + const nullKey = formatVersionKey(key, ''); + const nullKeyOp = { key: nullKey, value: request.value }; + ops.push(nullKeyOp); + // for backward compatibility: remove null version key + ops.push({ key: versionKey, type: 'del' }); + } + } + } else { + // master key does not exist: create it + ops.push({ key, value: request.value }); } return callback(null, ops, versionId); }); From 2868ee5db9d2241ff9b3e9878e9d290e717e3240 Mon Sep 17 00:00:00 2001 From: Nicolas Humbert Date: Thu, 8 Feb 2024 14:21:13 +0100 Subject: [PATCH 2/4] 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. (cherry picked from commit 68204448a14c4c17e5b4ef30fe1c39a77759118c) (cherry picked from commit 8367e8a3657da80f92032ee911f1d907fc0fc3cf) --- lib/versioning/Version.ts | 96 +++- lib/versioning/VersioningRequestProcessor.ts | 32 +- tests/unit/versioning/Version.spec.js | 72 +++ .../VersioningRequestProcessor.spec.js | 437 ++++++++++++++++++ 4 files changed, 626 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..0bc8f2214 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,24 +13,31 @@ const VID_SEP = VersioningConstants.VersionId.Separator; export class Version { version: { isNull?: boolean; + isNull2?: boolean; isDeleteMarker?: boolean; versionId?: string; isPHD?: boolean; + nullVersionId?: string; }; /** * 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; + nullVersionId?: string; }) { this.version = version || {}; } @@ -83,6 +90,33 @@ 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) === ' ') { + index -= 1; + } + const needComma = stringifiedObject.charAt(index) !== '{'; + return ( + `${stringifiedObject.slice(0, stringifiedObject.length - 1)}` + + (needComma ? ',' : '') + + `"${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 +127,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); + } } /** @@ -121,6 +173,19 @@ export class Version { return this.version.isNull ?? false; } + /** + * Check if a version is a null version and has + * been put with a Cloudserver handling null keys (i.e. supporting + * S3C-7352). + * + * @return - stating if the value is a null version and has + * been put with a Cloudserver handling null keys (i.e. supporting + * S3C-7352). + */ + isNull2Version(): boolean { + return this.version.isNull2 ?? false; + } + /** * Check if a stringified object is a delete marker. * @@ -190,6 +255,19 @@ export class Version { return this; } + /** + * Mark that the null version has been put with a Cloudserver handling null keys (i.e. supporting S3C-7352) + * + * If `isNull2` is set, `isNull` is also set to maintain consistency. + * Explicitly setting both avoids misunderstandings and mistakes in future updates or fixes. + * @return - the updated version + */ + setNull2Version() { + this.version.isNull2 = true; + this.version.isNull = true; + return this; + } + /** * Serialize the version. * diff --git a/lib/versioning/VersioningRequestProcessor.ts b/lib/versioning/VersioningRequestProcessor.ts index 914da35f2..c1d79e014 100644 --- a/lib/versioning/VersioningRequestProcessor.ts +++ b/lib/versioning/VersioningRequestProcessor.ts @@ -496,7 +496,26 @@ 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'); + // Delete the deprecated, null key for backward compatibility + // to avoid storing both deprecated and new null keys. + // If master null version was put with an older Cloudserver (or in compat mode), + // there is a possibility that it also has a null versioned key + // associated, so we need to delete it as we write the null key. + // Deprecated null key gets deleted when the new CloudServer: + // - updates metadata of a null master (options.isNull=true) + // - puts metadata on top of a master null key (options.isNull=false) + 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. + if (masterNullVersionId) { + const oldNullVersionKey = formatVersionKey(key, masterNullVersionId); + ops.push({ key: oldNullVersionKey, type: 'del' }); + } + } // new behavior when isNull is defined is to only // update the master key if it is the latest // version, old behavior needs to copy master to @@ -509,7 +528,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 +556,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..883724a52 --- /dev/null +++ b/tests/unit/versioning/Version.spec.js @@ -0,0 +1,72 @@ +const { Version } = require('../../../lib/versioning/Version'); + +describe('Version', () => { + describe('_jsonAppend', () => { + it('should append key-value pair to an empty object', () => { + const result = Version._jsonAppend('{}', 'versionId', '123'); + expect(result).toBe('{"versionId":"123"}'); + }); + + it('should append key-value pair to an object with existing properties', () => { + const result = Version._jsonAppend('{"existingKey":"existingValue"}', 'versionId', '123'); + expect(result).toBe('{"existingKey":"existingValue","versionId":"123"}'); + }); + + it('should append key-value pair to an object with existing key', () => { + const result = Version._jsonAppend('{"versionId":"0"}', 'versionId', '123'); + expect(result).toBe('{"versionId":"0","versionId":"123"}'); + }); + }); + + 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..6b720b28c 100644 --- a/tests/unit/versioning/VersioningRequestProcessor.spec.js +++ b/tests/unit/versioning/VersioningRequestProcessor.spec.js @@ -254,6 +254,443 @@ 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); + }); + + it('should delete the deprecated null key after put Metadata on top of an old null master', 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; + // update metadata of the same null version with compat mode (options.isNull not defined) + // to generate a deprecated null key. + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","isNull":true,"versionId":"${nullVersionId}"}`, + options: { + versionId: nullVersionId, + }, + }; + vrp.put(request, logger, next); + }, + (res, next) => { + // put metadata with the new keys implementation (options.isNull defined) + // on top of the null master with a deprecated null key. + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz3","versionId":"${versionId}"}`, + options: { + versionId, + 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":"quz3","versionId":"${versionId}"}`, + }, + // the null key + { + key: `bar${VID_SEP}`, + value: `{"qux":"quz2","isNull":true,"versionId":"${nullVersionId}",` + + `"nullVersionId":"${nullVersionId}","isNull2":true}`, + }, + // version key + { + key: `bar${VID_SEP}${versionId}`, + value: `{"qux":"quz3","versionId":"${versionId}"}`, + }, + ]; + assert.deepStrictEqual(res, expectedListing); + const request = { + db: 'foo', + key: 'bar', + }; + vrp.get(request, logger, next); + }, + (res, next) => { + const expectedGet = { + qux: 'quz3', + versionId, + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); + + it('should delete the deprecated null key after updating metadata of an old null master', 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; + // update metadata of the same null version with compat mode (options.isNull not defined) + // to generate a deprecated null key. + const request = { + db: 'foo', + key: 'bar', + value: `{"qux":"quz2","isNull":true,"versionId":"${nullVersionId}"}`, + options: { + versionId: nullVersionId, + }, + }; + 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 = [ + // the internal null version should be deleted. + { + key: 'bar', + value: `{"qux":"quz3","isNull2":true,"isNull":true,"versionId":"${nullVersionId}"}`, + }, + ]; + assert.deepStrictEqual(res, expectedListing); + const request = { + db: 'foo', + key: 'bar', + }; + vrp.get(request, logger, next); + }, + (res, next) => { + const expectedGet = { + qux: 'quz3', + isNull2: true, + isNull: true, + versionId: nullVersionId, + }; + assert.deepStrictEqual(JSON.parse(res), expectedGet); + next(); + }], + done); + }); }); From a11af7ed66c283d492fb08bb2706a8043756c84d Mon Sep 17 00:00:00 2001 From: Nicolas Humbert Date: Tue, 18 Jun 2024 12:07:37 +0200 Subject: [PATCH 3/4] ARSN-403 Set nullVersionId to master when replacing a null version. (cherry picked from commit 3f20120c0cdebeaea715af2910185a995f51cc65) --- lib/versioning/VersioningRequestProcessor.ts | 8 +- .../VersioningRequestProcessor.spec.js | 149 +++++++++++++++++- 2 files changed, 152 insertions(+), 5 deletions(-) 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); + }); }); From 9a4fa57da3fca6a81d6473997e856e165c496093 Mon Sep 17 00:00:00 2001 From: Nicolas Humbert Date: Wed, 19 Jun 2024 12:38:31 +0200 Subject: [PATCH 4/4] ARSN-419 bump package version --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 418eaa708..fd335668e 100644 --- a/package.json +++ b/package.json @@ -3,7 +3,7 @@ "engines": { "node": ">=16" }, - "version": "7.70.20", + "version": "7.70.20-1", "description": "Common utilities for the S3 project components", "main": "build/index.js", "repository": {