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:
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
... and 2 files with indirect coverage changes @@ Coverage Diff @@
## development/9.4 #6230 +/- ##
===================================================
- Coverage 86.10% 86.06% -0.04%
===================================================
Files 212 212
Lines 14483 14491 +8
===================================================
+ Hits 12470 12472 +2
- Misses 2013 2019 +6
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
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 ?
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