Skip to content

🎨 Async/Await migration on bucketGet and objectGetLegalHold #6045

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

Merged
bert-e merged 6 commits into from
Feb 12, 2026
+139 −112
Merged

🎨 Async/Await migration on bucketGet and objectGetLegalHold #6045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1396dd1
♻️ Migrate bucketGet and objectGetLegalHold to async/await
DarkIsDude Feb 4, 2026
da1caeb
♻️ use trampoline function to keep backward compatibility
DarkIsDude Feb 4, 2026
42a6ddc
📝 update JSDoc
DarkIsDude Feb 4, 2026
2095e46
♻️ factorise custom promisify function
DarkIsDude Feb 4, 2026
5e2122d
🐛 update callback to async/await after merge
DarkIsDude Feb 4, 2026
b6e1ad0
♻️ remove a useless error management and simplify the code
DarkIsDude Feb 9, 2026
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
112 changes: 58 additions & 54 deletions lib/api/bucketGet.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const { promisify } = require('util');
const querystring = require('querystring');
const { errors, errorInstances, versioning, s3middleware } = require('arsenal');
const constants = require('../../constants');
Expand Down Expand Up @@ -280,8 +281,7 @@ function processOptionalAttributes(item, optionalAttributes) {
return xml;
}

function handleResult(listParams, requestMaxKeys, encoding, authInfo,
bucketName, list, corsHeaders, log, callback) {
function handleResult(listParams, requestMaxKeys, encoding, authInfo, bucketName, list, log) {
// eslint-disable-next-line no-param-reassign
listParams.maxKeys = requestMaxKeys;
// eslint-disable-next-line no-param-reassign
Expand All @@ -297,7 +297,7 @@ function handleResult(listParams, requestMaxKeys, encoding, authInfo,
pushMetric('listBucket', log, { authInfo, bucket: bucketName });
monitoring.promMetrics('GET', bucketName, '200', 'listBucket');

return callback(null, res, corsHeaders);
return res;
}

/**
Expand All @@ -306,11 +306,17 @@ function handleResult(listParams, requestMaxKeys, encoding, authInfo,
* requester's info
* @param {object} request - http request object
* @param {function} log - Werelogs request logger
* @param {function} callback - callback to respond to http request
* with either error code or xml response body
* @return {undefined}
* @param {function} callback - callback optional to keep backward compatibility
* @return {Promise<object>} - object containing xml and additionalResHeaders
* @throws {Error}
*/
function bucketGet(authInfo, request, log, callback) {
async function bucketGet(authInfo, request, log, callback) {
if (callback) {
return bucketGet(authInfo, request, log)
.then(result => callback(null, ...result))
.catch(err => callback(err, null, err.additionalResHeaders));
}

const params = request.query;
const bucketName = request.bucketName;
const v2 = params['list-type'];
Expand All @@ -322,15 +328,13 @@ function bucketGet(authInfo, request, log, callback) {
.map(attr => attr !== 'RestoreStatus' ? attr.toLowerCase() : attr)
?? [];
if (optionalAttributes.some(attr => !attr.startsWith('x-amz-meta-') && attr != 'RestoreStatus')) {
return callback(
errorInstances.InvalidArgument.customizeDescription('Invalid attribute name specified')
);
throw errorInstances.InvalidArgument.customizeDescription('Invalid attribute name specified');
}

if (v2 !== undefined && Number.parseInt(v2, 10) !== 2) {
return callback(errorInstances.InvalidArgument.customizeDescription('Invalid ' +
'List Type specified in Request'));
throw errorInstances.InvalidArgument.customizeDescription('Invalid List Type specified in Request');
}

if (v2) {
log.addDefaultFields({ action: 'ListObjectsV2' });
if (request.serverAccessLog) {
Expand All @@ -348,18 +352,14 @@ function bucketGet(authInfo, request, log, callback) {
const encoding = params['encoding-type'];
if (encoding !== undefined && encoding !== 'url') {
monitoring.promMetrics('GET', bucketName, 400, 'listBucket');
return callback(errorInstances.InvalidArgument.customizeDescription('Invalid ' +
'Encoding Method specified in Request'));
throw errorInstances.InvalidArgument.customizeDescription('Invalid Encoding Method specified in Request');
}

const requestMaxKeys = params['max-keys'] ? Number.parseInt(params['max-keys'], 10) : 1000;
if (Number.isNaN(requestMaxKeys) || requestMaxKeys < 0) {
monitoring.promMetrics('GET', bucketName, 400, 'listBucket');
return callback(errors.InvalidArgument);
throw errors.InvalidArgument;
}
// AWS only returns 1000 keys even if max keys are greater.
// Max keys stated in response xml can be greater than actual
// keys returned.
const actualMaxKeys = Math.min(constants.listingHardLimit, requestMaxKeys);

const metadataValParams = {
Expand Down Expand Up @@ -388,47 +388,51 @@ function bucketGet(authInfo, request, log, callback) {
listParams.marker = params.marker;
}

standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, (err, bucket) => {
const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket);
let bucket;

if (err) {
log.debug('error processing request', { error: err });
monitoring.promMetrics('GET', bucketName, err.code, 'listBucket');
return callback(err, null, corsHeaders);
}
try {
const standardMetadataValidateBucketPromised = promisify(standardMetadataValidateBucket);
bucket = await standardMetadataValidateBucketPromised(metadataValParams, request.actionImplicitDenies, log);
} catch (err) {
log.debug('error processing request', { err });
monitoring.promMetrics('GET', bucketName, err.code, 'listBucket');
throw err;
}
Copy link
Contributor

@francoisferrand francoisferrand Feb 11, 2026

Choose a reason for hiding this comment

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

not an issue for this PR, but I wonder if we can write this "more cleanly" (e.g. avoid the separate let variable, and make the exception "wrapping" more seamless) with a combination of promise and await? e.g. something like this:

const bucket = await standardMetadataValidateBucketPromised(metadataValParams, request.actionImplicitDenies, log).catch(err => {
	log.debug('error processing request', { err });
    monitoring.promMetrics('GET', bucketName, err.code, 'listBucket');
    throw err;
});

(ideally, I'd rather avoid such "exception wrapping", but may not be possible in every case...)

what do you think: Does it work? Is there some Is this a "pattern" we may or should use?
or should we rather a single try/catch over the whole function in such case (which would avoid the duplicated "catch" block for log & prometheus)?

Copy link
Contributor Author

@DarkIsDude DarkIsDude Feb 11, 2026

Choose a reason for hiding this comment

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

That's a good question and it depends a lot on each case. I prefer for example the try/catch than the mix of promise/await but it's a pure personal opinion. IMO the industry try to move to a more try/catch approach but it's just a feeling. Anyway what you suggest should work.

It's a "migration". So some pattern can not be perfect... A good option to avoid the try/catch can be to type of errors and use that in one catch block (more or less what we did for AWS error). In that case I would suggest to keep the try/catch and don't try to complexify the migration. It's a well balanced between moving forward with the migration and improving our code.


if (params.versions !== undefined) {
listParams.listingType = 'DelimiterVersions';
delete listParams.marker;
listParams.keyMarker = params['key-marker'];
listParams.versionIdMarker = params['version-id-marker'] ?
versionIdUtils.decode(params['version-id-marker']) :
undefined;
}
const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket);

if (!requestMaxKeys) {
const emptyList = {
CommonPrefixes: [],
Contents: [],
Versions: [],
IsTruncated: false,
};
return handleResult(listParams, requestMaxKeys, encoding, authInfo,
bucketName, emptyList, corsHeaders, log, callback);
}
if (params.versions !== undefined) {
listParams.listingType = 'DelimiterVersions';
delete listParams.marker;
listParams.keyMarker = params['key-marker'];
listParams.versionIdMarker = params['version-id-marker'] ?
versionIdUtils.decode(params['version-id-marker']) :
undefined;
}
if (!requestMaxKeys) {
const emptyList = {
CommonPrefixes: [],
Contents: [],
Versions: [],
IsTruncated: false,
};
const res = handleResult(listParams, requestMaxKeys, encoding, authInfo, bucketName, emptyList, log);
return [res, corsHeaders];
}

return services.getObjectListing(bucketName, listParams, log, (err, list) => {
if (err) {
log.debug('error processing request', { error: err });
monitoring.promMetrics('GET', bucketName, err.code, 'listBucket');
return callback(err, null, corsHeaders);
}
let list;
try {
list = await promisify(services.getObjectListing)(bucketName, listParams, log);
} catch (err) {
log.debug('error processing request', { error: err });
monitoring.promMetrics('GET', bucketName, err.code, 'listBucket');

return handleResult(listParams, requestMaxKeys, encoding, authInfo,
bucketName, list, corsHeaders, log, callback);
});
});
return undefined;
err.additionalResHeaders = corsHeaders;
throw err;
}

const res = handleResult(listParams, requestMaxKeys, encoding, authInfo, bucketName, list, log);
return [res, corsHeaders];
}

module.exports = {
Expand Down
130 changes: 72 additions & 58 deletions lib/api/objectGetLegalHold.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
const async = require('async');
const { promisify } = require('util');
const { errors, errorInstances, s3middleware } = require('arsenal');

const { decodeVersionId, getVersionIdResHeader }
Expand All @@ -15,18 +15,23 @@ const { convertToXml } = s3middleware.objectLegalHold;
* @param {AuthInfo} authInfo - Instance of AuthInfo class with requester's info
* @param {object} request - http request object
* @param {object} log - Werelogs logger
* @param {function} callback - callback to server
* @return {undefined}
* @return {Promise<object>} - object containing xml and additionalResHeaders
*/
function objectGetLegalHold(authInfo, request, log, callback) {
async function objectGetLegalHold(authInfo, request, log, callback) {
if (callback) {
return objectGetLegalHold(authInfo, request, log)
.then(result => callback(null, ...result))
.catch(err => callback(err, null, err.additionalResHeaders));
}

log.debug('processing request', { method: 'objectGetLegalHold' });

const { bucketName, objectKey, query } = request;

const decodedVidResult = decodeVersionId(query);
if (decodedVidResult instanceof Error) {
log.trace('invalid versionId query', { versionId: query.versionId, error: decodedVidResult });
return process.nextTick(() => callback(decodedVidResult));
throw decodedVidResult;
}
const versionId = decodedVidResult;

Expand All @@ -40,60 +45,69 @@ function objectGetLegalHold(authInfo, request, log, callback) {
request,
};

return async.waterfall([
next => standardMetadataValidateBucketAndObj(metadataValParams, request.actionImplicitDenies, log,
(err, bucket, objectMD) => {
if (err) {
log.trace('request authorization failed', { method: 'objectGetLegalHold', error: err });
return next(err);
}
if (!objectMD) {
const err = versionId ? errors.NoSuchVersion : errors.NoSuchKey;
log.trace('error no object metadata found', { method: 'objectGetLegalHold', error: err });
return next(err, bucket);
}
if (objectMD.isDeleteMarker) {
if (versionId) {
log.trace('requested version is delete marker', { method: 'objectGetLegalHold' });
// FIXME we should return a `x-amz-delete-marker: true` header, see S3C-7592
return next(errors.MethodNotAllowed);
}
log.trace('most recent version is delete marker', { method: 'objectGetLegalHold' });
// FIXME we should return a `x-amz-delete-marker: true` header, see S3C-7592
return next(errors.NoSuchKey);
}
if (!bucket.isObjectLockEnabled()) {
log.trace('object lock not enabled on bucket', { method: 'objectGetRetention' });
return next(errorInstances.InvalidRequest.customizeDescription(
'Bucket is missing Object Lock Configuration'));
}
return next(null, bucket, objectMD);
}),
(bucket, objectMD, next) => {
const { legalHold } = objectMD;
const xml = convertToXml(legalHold);
if (xml === '') {
return next(errors.NoSuchObjectLockConfiguration);
}
return next(null, bucket, xml, objectMD);
},
], (err, bucket, xml, objectMD) => {
const additionalResHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket);
if (err) {
log.trace('error processing request', { error: err, method: 'objectGetLegalHold' });
} else {
pushMetric('getObjectLegalHold', log, {
authInfo,
bucket: bucketName,
keys: [objectKey],
versionId: objectMD ? objectMD.versionId : undefined,
location: objectMD ? objectMD.dataStoreName : undefined,
});
const verCfg = bucket.getVersioningConfiguration();
additionalResHeaders['x-amz-version-id'] = getVersionIdResHeader(verCfg, objectMD);
let bucket, objectMD;

try {
const standardMetadataValidateBucketAndObjPromised = promisify(standardMetadataValidateBucketAndObj);
({ bucket, objectMD } = await standardMetadataValidateBucketAndObjPromised(
metadataValParams,
request.actionImplicitDenies,
log,
));
} catch (err) {
log.trace('request authorization failed', { method: 'objectGetLegalHold', error: err });
throw err;
}

if (!objectMD) {
const err = versionId ? errors.NoSuchVersion : errors.NoSuchKey;
log.trace('error no object metadata found', { method: 'objectGetLegalHold', error: err });
throw err;
}

if (objectMD.isDeleteMarker) {
if (versionId) {
log.trace('requested version is delete marker', { method: 'objectGetLegalHold' });
// FIXME we should return a `x-amz-delete-marker: true` header, see S3C-7592
throw errors.MethodNotAllowed;
}
return callback(err, xml, additionalResHeaders);

log.trace('most recent version is delete marker', { method: 'objectGetLegalHold' });
// FIXME we should return a `x-amz-delete-marker: true` header, see S3C-7592
throw errors.NoSuchKey;
}

if (!bucket.isObjectLockEnabled()) {
log.trace('object lock not enabled on bucket', { method: 'objectGetRetention' });
throw errorInstances.InvalidRequest.customizeDescription('Bucket is missing Object Lock Configuration');
}

const { legalHold } = objectMD;
const xml = convertToXml(legalHold);
if (xml === '') {
throw errors.NoSuchObjectLockConfiguration;
}

const additionalResHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket);

pushMetric('getObjectLegalHold', log, {
authInfo,
bucket: bucketName,
keys: [objectKey],
versionId: objectMD ? objectMD.versionId : undefined,
location: objectMD ? objectMD.dataStoreName : undefined,
});
const verCfg = bucket.getVersioningConfiguration();
additionalResHeaders['x-amz-version-id'] = getVersionIdResHeader(verCfg, objectMD);

return [xml, additionalResHeaders];
Copy link
Contributor

@francoisferrand francoisferrand Feb 11, 2026

Choose a reason for hiding this comment

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

I think we should explicitly add this corner-case in the TAD, to warn user when migrating functions which pass multiple values to callback?

Copy link
Contributor Author

@DarkIsDude DarkIsDude Feb 11, 2026

Choose a reason for hiding this comment

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

https://scality.atlassian.net/wiki/spaces/OS/pages/3523346468/2025-10-30+-+Async+Await+migration#%E2%9A%A0%EF%B8%8F-Limitation%3A-Multiple-callback-arguments it's already here 🙏

}

module.exports = objectGetLegalHold;
module.exports = (...args) => {
const callback = args.at(-1);
const argsWithoutCallback = args.slice(0, -1);

objectGetLegalHold(...argsWithoutCallback)
.then(result => callback(null, ...result))
.catch(err => callback(err, null, err.additionalResHeaders));
};
9 changes: 9 additions & 0 deletions lib/metadata/metadataUtils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
const { promisify } = require('util');
const async = require('async');
const { errors } = require('arsenal');

Expand Down Expand Up @@ -429,6 +430,14 @@ function standardMetadataValidateBucketAndObj(params, actionImplicitDenies, log,
return callback(null, bucket, objMD);
});
}
standardMetadataValidateBucketAndObj[promisify.custom] = (...args) =>
new Promise((resolve, reject) => {
standardMetadataValidateBucketAndObj(...args, (err, bucket, objectMD) => {
if (err) {return reject(err);}
return resolve({ bucket, objectMD });
});
});

/** standardMetadataValidateBucket - retrieve bucket from metadata and check if user
* is authorized to access it
* @param {object} params - function parameters
Expand Down
Loading