Skip to content

Commit 1b7b0e2

Browse files
SylvainSenechalSylvainSenechal
committed
fix putData : detect collision on specific provided version id
Issue: CLDSRV-953
1 parent d52ba09 commit 1b7b0e2

3 files changed

Lines changed: 115 additions & 41 deletions

File tree

‎lib/api/apiUtils/object/versioning.js‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const { errorInstances, versioning } = require('arsenal');
2+
const { ExternalNullVersionId } = versioning.VersioningConstants;
23
const async = require('async');
34

45
const metadata = require('../../../metadata/wrapper');
@@ -18,7 +19,7 @@ const nonVersionedObjId = versionIdUtils.getInfVid(config.replicationGroupId);
1819
* fails due to improper format, otherwise undefined or the decoded version id
1920
*/
2021
function decodeVID(versionId) {
21-
if (versionId === 'null') {
22+
if (versionId === ExternalNullVersionId) {
2223
return versionId;
2324
}
2425

‎lib/routes/routeBackbeat.js‎

Lines changed: 26 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ const locationConstraintCheck = require('../api/apiUtils/object/locationConstrai
2929
const locationStorageCheck = require('../api/apiUtils/object/locationStorageCheck');
3030
const { dataStore } = require('../api/apiUtils/object/storeObject');
3131
const prepareRequestContexts = require('../api/apiUtils/authorization/prepareRequestContexts');
32-
const { decodeVersionId } = require('../api/apiUtils/object/versioning');
32+
const { decodeVersionId, decodeVID } = require('../api/apiUtils/object/versioning');
3333
const getReplicationInfo = require('../api/apiUtils/object/getReplicationInfo');
3434
const locationKeysHaveChanged = require('../api/apiUtils/object/locationKeysHaveChanged');
3535
const { standardMetadataValidateBucketAndObj, metadataGetObject } = require('../metadata/metadataUtils');
@@ -433,37 +433,25 @@ function putData(request, response, bucketInfo, objMd, log, callback) {
433433
}
434434

435435
const incomingVersionIdEncoded = request.headers['x-scal-version-id'];
436-
if (incomingVersionIdEncoded !== undefined) {
437-
const incomingVersionIdDecoded =
438-
incomingVersionIdEncoded !== 'null' ? decode(incomingVersionIdEncoded) : 'null';
439-
if (incomingVersionIdDecoded instanceof Error) {
440-
log.error('crr putData: failed to decode x-scal-version-id header', {
441-
method: 'putData',
442-
error: incomingVersionIdDecoded.message,
443-
});
444-
return callback(
445-
errorInstances.BadRequest.customizeDescription('bad request: invalid x-scal-version-id header'),
446-
);
447-
}
448-
if (objMd && objMd.versionId === incomingVersionIdDecoded) {
449-
// Data already at destination for this version; return 409 with the existing
450-
// microVersionId so backbeat can decide if putMetadata is still needed.
451-
log.debug('crr putData: version already at destination', {
452-
method: 'putData',
453-
bucketName: request.bucketName,
454-
objectKey: request.objectKey,
455-
hasMicroVersionId: !!objMd.microVersionId,
456-
});
457-
request.resume();
458-
return _respondWithHeaderCrrConflict(
459-
response,
460-
log,
461-
callback,
462-
VersionIdCollisionException.name,
463-
'version id already at destination',
464-
objMd.microVersionId,
465-
);
466-
}
436+
if (incomingVersionIdEncoded !== undefined && objMd) {
437+
// objMd is the specific version requested by the header x-scal-version-id.
438+
// Its existence means the data is already at the destination. Return 409 with the
439+
// existing microVersionId so backbeat can decide if putMetadata is still needed.
440+
log.debug('crr putData: version already at destination', {
441+
method: 'putData',
442+
bucketName: request.bucketName,
443+
objectKey: request.objectKey,
444+
hasMicroVersionId: !!objMd.microVersionId,
445+
});
446+
request.resume();
447+
return _respondWithHeaderCrrConflict(
448+
response,
449+
log,
450+
callback,
451+
VersionIdCollisionException.name,
452+
'version id already at destination',
453+
objMd.microVersionId,
454+
);
467455
}
468456

469457
writeContinue(request, response);
@@ -2039,10 +2027,13 @@ function routeBackbeat(clientIP, request, response, log) {
20392027
if (!isObjectRequest) {
20402028
return routeNonObjectRequest(request, response, userInfo, log, next);
20412029
}
2042-
const decodedVidResult = decodeVersionId(request.query);
2030+
const versionIdHeader = request.headers['x-scal-version-id'];
2031+
const decodedVidResult = versionIdHeader !== undefined
2032+
? decodeVID(versionIdHeader)
2033+
: decodeVersionId(request.query);
20432034
if (decodedVidResult instanceof Error) {
2044-
log.trace('invalid versionId query', {
2045-
versionId: request.query.versionId,
2035+
log.trace('invalid versionId', {
2036+
versionId: versionIdHeader ?? request.query.versionId,
20462037
error: decodedVidResult,
20472038
});
20482039
return next(errors.InvalidArgument);

‎tests/functional/backbeat/putData.js‎

Lines changed: 87 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -14,6 +14,7 @@ const { BackbeatRoutesClient, PutDataCommand, VersionIdCollisionException } = re
1414
const { generateVersionId, encode: encodeVersionId } = versioning.VersionID;
1515

1616
const TEST_BUCKET = `bucket-putdata-${uuidv4().split('-')[0]}`;
17+
const TEST_BUCKET_UNVERSIONED = `bucket-putdata-unver-${uuidv4().split('-')[0]}`;
1718
const OBJECT_BODY = 'imAboutToBeCascadedWitNoParachuteInMyBack';
1819
const OBJECT_MD5_HEX = createHash('md5').update(OBJECT_BODY).digest('hex');
1920
const CANONICAL_ID = '79a59df900b949e55d96a1e698fbacedfd6e09d98eacf8f8d5218e7cd47ef2be';
@@ -55,6 +56,8 @@ before(async () => {
5556
VersioningConfiguration: { Status: 'Enabled' },
5657
}),
5758
);
59+
60+
await s3.send(new CreateBucketCommand({ Bucket: TEST_BUCKET_UNVERSIONED }));
5861
});
5962

6063
describe('putData : VersionId collision detection', () => {
@@ -106,17 +109,63 @@ describe('putData : VersionId collision detection', () => {
106109
const output = await putData(key, { versionId: differentVersionId });
107110
assert.ok(output.Location, 'should return a Location when data is written normally');
108111
});
112+
113+
it('should throw VersionIdCollisionException when a non-current version already exists', async () => {
114+
const key = 'putdata-non-current-collision';
115+
116+
const v1Result = await s3.send(
117+
new PutObjectCommand({
118+
Bucket: TEST_BUCKET,
119+
Key: key,
120+
Body: Buffer.from(OBJECT_BODY),
121+
ContentType: 'text/plain',
122+
}),
123+
);
124+
const v1VersionId = v1Result.VersionId;
125+
assert.ok(v1VersionId, 'first PutObject should return a VersionId');
126+
127+
const v2Result = await s3.send(
128+
new PutObjectCommand({
129+
Bucket: TEST_BUCKET,
130+
Key: key,
131+
Body: Buffer.from(OBJECT_BODY),
132+
ContentType: 'text/plain',
133+
}),
134+
);
135+
assert.ok(v2Result.VersionId, 'second PutObject should return a VersionId');
136+
137+
// putData on v1 (non-current) must detect the collision, not just compare against master
138+
try {
139+
await putData(key, { versionId: v1VersionId });
140+
assert.fail('expected VersionIdCollisionException');
141+
} catch (err) {
142+
assert.ok(
143+
err instanceof VersionIdCollisionException,
144+
`expected VersionIdCollisionException, got ${err.constructor.name}`,
145+
);
146+
assert.strictEqual(err.microVersionId, '', 'microVersionId should be empty for original write state');
147+
}
148+
});
109149
});
110150

111151
describe('putData : null-version objects (ExternalNullVersionId)', () => {
112-
// Null-version objects created before versioning was enabled use Arsenal constant ExternalNullVersionId = 'null'
113-
// getEncodedVersionId() returns 'null' as-is (no base62 encoding), and objMd.versionId is
114-
// undefined in metadata : collision detection is not possible, so putData must write normally.
115-
it('should write normally when VersionId is "null" (ExternalNullVersionId)', async () => {
152+
it('should write normally when VersionId is "null" and destination has no null version', async () => {
153+
// Versioned bucket: existing object has a real versionId, not a null version.
154+
// Fetching with ExternalNullVersionId returns no objMd => no collision => write normally.
155+
const key = 'putdata-null-version-no-collision';
156+
await s3.send(
157+
new PutObjectCommand({
158+
Bucket: TEST_BUCKET,
159+
Key: key,
160+
Body: Buffer.from(OBJECT_BODY),
161+
ContentType: 'text/plain',
162+
}),
163+
);
164+
116165
const output = await backbeatClient.send(
117166
new PutDataCommand({
118167
Bucket: TEST_BUCKET,
119-
Key: 'putdata-null-version',
168+
Key: key,
120169
ContentMD5: OBJECT_MD5_HEX,
121170
CanonicalID: CANONICAL_ID,
122171
VersioningRequired: true,
@@ -126,6 +175,39 @@ describe('putData : null-version objects (ExternalNullVersionId)', () => {
126175
);
127176
assert.ok(output.Location, 'putData with null-version versionId should write normally');
128177
});
178+
179+
it('should throw VersionIdCollisionException when a null version already exists at destination', async () => {
180+
// Unversioned bucket: objects have no versionId in metadata (they are null versions).
181+
// putData with ExternalNullVersionId must detect the collision.
182+
const key = 'putdata-null-version-collision';
183+
await s3.send(
184+
new PutObjectCommand({
185+
Bucket: TEST_BUCKET_UNVERSIONED,
186+
Key: key,
187+
Body: Buffer.from(OBJECT_BODY),
188+
ContentType: 'text/plain',
189+
}),
190+
);
191+
192+
try {
193+
await backbeatClient.send(
194+
new PutDataCommand({
195+
Bucket: TEST_BUCKET_UNVERSIONED,
196+
Key: key,
197+
ContentMD5: OBJECT_MD5_HEX,
198+
CanonicalID: CANONICAL_ID,
199+
VersionId: ExternalNullVersionId,
200+
Body: Buffer.from(OBJECT_BODY),
201+
}),
202+
);
203+
assert.fail('expected VersionIdCollisionException');
204+
} catch (err) {
205+
assert.ok(
206+
err instanceof VersionIdCollisionException,
207+
`expected VersionIdCollisionException, got ${err.constructor.name}`,
208+
);
209+
}
210+
});
129211
});
130212

131213
describe('putData : baseline (no cascade headers)', () => {

0 commit comments

Comments
 (0)