Skip to content

Add mongodb driver checks #2363

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: development/8.1
Choose a base branch
from
Open

Add mongodb driver checks #2363

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ebb20a
Add checks in MongoClientInterface
williamlardier Apr 24, 2025
80fcfbd
Bump Arsenal
williamlardier May 2, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 70 additions & 9 deletions lib/storage/metadata/mongoclient/MongoClientInterface.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,11 @@ class MongoClientInterface {
}, payload, {
upsert: true,
})
.then(() => {
.then(result => {
if (result.matchedCount === 0 && result.modifiedCount === 0 && result.upsertedCount === 0) {
log.debug('createBucket: failed to create bucket', { bucketName, result });
return cb(errors.InternalError);
}
// caching bucket vFormat
this.bucketVFormatCache.add(bucketName, payload.$set.vFormat);
// NOTE: We do not need to create a collection for
Expand Down Expand Up @@ -583,7 +587,13 @@ class MongoClientInterface {
}, {
upsert: true,
})
.then(() => cb(null))
.then(result => {
if (result.matchedCount === 0 && result.modifiedCount === 0 && result.upsertedCount === 0) {
log.debug('putBucketAttributes: failed to update bucket', { bucketName });
return cb(errors.NoSuchBucket);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit:

  • this is really a case which "should not" (but does) happen: may be best to keep it as InternalError, possibly customized with an indication of the "mongo" issue (errors.InternalError.customize("mongo failed to match, update and upsert"))
  • maybe we can dedup this case of error detection after an update operation, by introducing a processUpdateResult(result, cb) function?

}
return cb(null);
})
.catch(err => {
log.error(
'putBucketAttributes: error putting bucket attributes',
Expand Down Expand Up @@ -623,7 +633,13 @@ class MongoClientInterface {
},
}, {
upsert: true,
}).then(() => cb(null)).catch(err => {
}).then(result => {
if (result.matchedCount === 0 && result.modifiedCount === 0 && result.upsertedCount === 0) {
log.error('putBucketAttributesCapabilities: failed to update bucket', { bucketName });
return cb(errors.InternalError);
}
return cb(null);
}).catch(err => {
log.error(
'putBucketAttributesCapabilities: error putting bucket attributes',
{ error: err.message });
Expand Down Expand Up @@ -657,7 +673,14 @@ class MongoClientInterface {
$unset: {
[updateString]: '',
},
}).then(() => cb(null)).catch(err => {
}).then(result => {
if (result.matchedCount === 0 && result.modifiedCount === 0) {
log.debug('deleteBucketAttributesCapability: bucket not found or capability not present',
{ bucketName });
return cb(errors.NoSuchBucket);
}
return cb(null);
}).catch(err => {
if (err) {
log.error(
'deleteBucketAttributesCapability: error deleting bucket attributes',
Expand Down Expand Up @@ -1967,7 +1990,19 @@ class MongoClientInterface {
if (params && params.doesNotNeedOpogUpdate) {
// If flag is true, directly delete object
return collection.deleteOne(deleteFilter)
.then(() => cb(null, undefined))
.then(result => {
// In case of race conditions (e.g. two concurrent deletes), or invalid state
// (e.g. object is already deleted), the result.deletedCount will be 0
// without error, and we need to catch it because the API might continue and
// delete the actual data, leading to an invalid state, where MongoDB references
// and object not in the data layers anymore.
if (!result || result?.deletedCount != 1) {
log.debug('internalDeleteObject: object not found or already deleted',
{ bucket: bucketName, object: key });
return cb(errors.NoSuchKey);
}
return cb(null, undefined);
})
.catch(err => {
log.error('internalDeleteObject: error deleting object',
{ bucket: bucketName, object: key, error: err.message });
Expand Down Expand Up @@ -2030,10 +2065,24 @@ class MongoClientInterface {
filter: updateDeleteFilter,
},
},
], { ordered: true }).then(() => next(null)).catch(err => next(err)),
], { ordered: true }).then(result => {
// in case of race conditions, the bulk operation might fail
// in this case we return a DeleteConflict error
if (!result || !result.ok) {
log.debug('internalDeleteObject: bulk operation failed',
{ bucket: bucketName, object: key });
return next(errors.DeleteConflict);
}
if (result.deletedCount === 0) {
log.debug('internalDeleteObject: object not found or already deleted',
{ bucket: bucketName, object: key });
return next(errors.DeleteConflict);
}
return next(null);
}).catch(err => next(err)),
], (err, res) => {
if (err) {
if (err instanceof ArsenalError && err.is.NoSuchKey) {
if (err instanceof ArsenalError) {
return cb(err);
}
log.error('internalDeleteObject: error deleting object',
Expand Down Expand Up @@ -2396,7 +2445,13 @@ class MongoClientInterface {
i.insertOne(<InfostoreDocument>{
_id: __UUID,
value: uuid,
}, {}).then(() => cb(null)) // FIXME: shoud we check for result.ok === 1 ?
}, {}).then(result => {
if (!result || !result.acknowledged) {
log.debug('writeUUIDIfNotExists: insertion failed');
return cb(errors.InternalError);
}
return cb(null);
})
.catch(err => {
if (err.code === 11000) {
// duplicate key error
Expand Down Expand Up @@ -3147,7 +3202,13 @@ class MongoClientInterface {
deleteBucketIndexes(bucketName: string, indexSpecs: { name: string }[], log: werelogs.Logger, cb: ArsenalCallback<void>) {
const c = this.getCollection<ObjectMetastoreDocument>(bucketName);
async.each(indexSpecs,
(spec, next) => c.dropIndex(spec.name).then(() => next()).catch(err => next(err)),
(spec, next) => c.dropIndex(spec.name).then(result => {
if (!result || !result.ok) {
log.debug('deleteBucketIndexes: failed to drop index', { indexName: spec.name });
return next(errors.DeleteConflict);
}
return next();
}).catch(err => next(err)),
err => {
if (err) {
if (err instanceof MongoServerError && err.codeName === 'NamespaceNotFound') {
Expand Down
4 changes: 4 additions & 0 deletions lib/storage/metadata/mongoclient/utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,6 +222,10 @@ function indexFormatMongoArrayToObject(mongoIndexArray: MongoIndex[]): Index[] {
}

function indexFormatObjectToMongoArray(indexObj: Index[]): MongoIndex[] {
if (!indexObj || !Array.isArray(indexObj)) {
return [];
}

return indexObj.map(idx => {
const key = new Map();
idx.keys.forEach(k => key.set(k.key, k.order));
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
"engines": {
"node": ">=16"
},
"version": "8.1.152",
"version": "8.1.153",
"description": "Common utilities for the S3 project components",
"main": "build/index.js",
"repository": {
Expand Down
Loading
Loading