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

Open
DarkIsDude wants to merge 5 commits into
base: feature/CLDSRV-823/formatting-only
Choose a base branch
from
+144 −112
Open

🎨 Async/Await migration on bucketGet and objectGetLegalHold #6045

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
757f7e6
♻️ Migrate bucketGet and objectGetLegalHold to async/await
DarkIsDude Feb 4, 2026
a24d60c
♻️ use trampoline function to keep backward compatibility
DarkIsDude Feb 4, 2026
d6af6e2
📝 update JSDoc
DarkIsDude Feb 4, 2026
7bed447
♻️ factorise custom promisify function
DarkIsDude Feb 4, 2026
7b03be0
🐛 update callback to async/await after merge
DarkIsDude Feb 4, 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
117 changes: 63 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 @@ -236,8 +237,7 @@ function processMasterVersions(bucketName, listParams, list) {
return xml.join('');
}

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 @@ -250,7 +250,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 @@ -259,27 +259,31 @@ 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'];

const optionalAttributes =
request.headers['x-amz-optional-object-attributes']?.split(',').map(attr => attr.trim()) ?? [];
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 @@ -297,18 +301,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 @@ -336,47 +336,56 @@ 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 error;
let bucket;
Copy link
Contributor

@francoisferrand francoisferrand Feb 5, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

bucket will not be set in case standardMetadataValidateBucketPromised throws:

  • is it already the behavior of standardMetadataValidateBucketPromised' continuation callback, or did it always provide the bucket ? In that case promisify does not work... 😢
  • does collectCorsHeaders work when bucket is not provider?

Copy link
Contributor Author

@DarkIsDude DarkIsDude Feb 5, 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. I remember encounter an issue with that some weeks (not for this PR).

The answer can be found inside the collectCorsHeaders:

    // NOTE: Because collecting CORS headers requires making a call to
    // metadata to retrieve the bucket's CORS configuration, we opt not to
    // return the CORS headers if the request encounters an error before
    // the api method retrieves the bucket from metadata (an example
    // being if a request is not properly authenticated). This is a slight
    // deviation from AWS compatibility, but has the benefit of avoiding
    // additional backend calls for an invalid request. Also, we anticipate
    // that the preflight OPTIONS route will serve most client needs regarding
    // CORS.
    if (!origin || !bucket) {
        return {};
    }

So the answer is yes, collectCorsHeaders will continue to work but no CORS will be returned. So I'm not sure to understand your first point. The bucket will be defined the same as before (can be, if all goes right), and will not if we have an error. Can you help me to clarify 🙏 ?


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) {
error = err;
}

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 (error) {
log.debug('error processing request', { error });
monitoring.promMetrics('GET', bucketName, error.code, 'listBucket');
error.additionalResHeaders = corsHeaders;
throw error;
}
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];
}

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