fix putData : detect collision on specific provided version id - #6230
fix putData : detect collision on specific provided version id#6230SylvainSenechal wants to merge 1 commit into
Conversation
Hello sylvainsenechal,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
| errorInstances.BadRequest.customizeDescription('bad request: invalid x-scal-version-id header'), | ||
| ); | ||
| } | ||
| if (objMd && objMd.versionId === incomingVersionIdDecoded) { |
There was a problem hiding this comment.
No need for objMd.versionId === incomingVersionIdDecoded anymore as the middleware is already fetching objMd for the specific versionID provided in the header
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
❌ 11 Tests Failed:
View the full list of 11 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
148ca98 to
18d305d
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes Backbeat putData collision detection for replication of non-current object versions by ensuring the metadata lookup targets the specific replicated version (from x-scal-version-id) rather than always using the master version.
Changes:
- Decode
x-scal-version-idin the Backbeat router forPUT /_/backbeat/dataand use it as theversionIdfor metadata lookup soobjMdcorresponds to the replicated version. - Simplify collision detection in
putDatato treat the presence of that specific version’sobjMdas a collision (while explicitly excludingExternalNullVersionId/"null"). - Add functional test coverage for collisions on a non-current version and adjust the
"null"version test to cover cases where a master already exists.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/functional/backbeat/putData.js | Adds/updates functional tests to validate collision behavior for non-current versions and "null" version behavior. |
| lib/routes/routeBackbeat.js | Uses decoded x-scal-version-id to fetch version-specific metadata for putData, fixing collision detection semantics. |
maeldonn
left a comment
maeldonn
left a comment
There was a problem hiding this comment.
Logic is fine, need a little refactor
| } | ||
| if (objMd && objMd.versionId === incomingVersionIdDecoded) { | ||
| // Data already at destination for this version; return 409 with the existing | ||
| if (objMd) { |
There was a problem hiding this comment.
if (objMd) only means "this version exists" because the router's isPutDataApi block fetched the header version instead of the master; that coupling is easy to break. Clearer to drop the router block and fetch the version here with metadataGetObject (returns undefined when missing):
const incomingVersionId = decode(incomingVersionIdEncoded);
// ... BadRequest on decode error ...
return metadataGetObject(request.bucketName, request.objectKey, incomingVersionId, null, log, (err, versionMd) => {
if (err) {
return callback(err);
}
if (versionMd) {
// existing 409 conflict path, using versionMd.microVersionId
}
return writeData();
});| // For putData api: the version to check is passed | ||
| // via x-scal-version-id header, not the URL query. Fetch that specific | ||
| // version so objMd matches the replicated version, not always the master. | ||
| const isPutDataApi = request.method === 'PUT' && request.resourceType === 'data'; |
There was a problem hiding this comment.
The router shouldn't special-case one handler, and these guards duplicate putData's. Move the version lookup into putData itself (see handler comment) so this block goes away.
| const versionIdHeader = request.headers['x-scal-version-id']; | ||
| if (versionIdHeader !== undefined && versionIdHeader !== ExternalNullVersionId) { | ||
| const decoded = decode(versionIdHeader); | ||
| if (!(decoded instanceof Error)) { |
There was a problem hiding this comment.
Decode failure is ignore, should it be handled ?
18d305d to
c3d0e4b
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
c3d0e4b to
c9814a4
Compare
Issue: CLDSRV-953
The putData collision detection previously fetched the master object and compared its versionId against the incoming x-scal-version-id header. This caused issues when replicating a non-current version : the master's versionId wouldn't match, so no collision was detected and data was re-uploaded unnecessarily
Fix : decode the x-scal-version-id header in the router and use it as the metadata lookup key, so objMd is always the specific version being replicated