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: development/9.2
Choose a base branch
from
+167 −176
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
82b8424
🎨 migrate buckgetGet to async/await
DarkIsDude Jan 15, 2026
1305c89
♻️ migrate objectGetLegalHold to async/await syntax
DarkIsDude Jan 15, 2026
354a81a
♻️ use callbackify instead of manual callback
DarkIsDude Jan 23, 2026
250de6f
♻️ move to manual callbackify
DarkIsDude Jan 23, 2026
7a14efc
♻️ remove wrapper and use promisify
DarkIsDude Jan 29, 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
194 changes: 93 additions & 101 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 All @@ -8,14 +9,9 @@ const escapeForXml = s3middleware.escapeForXml;
const { pushMetric } = require('../utapi/utilities');
const versionIdUtils = versioning.VersionID;
const monitoring = require('../utilities/monitoringHandler');
const { generateToken, decryptToken }
= require('../api/apiUtils/object/continueToken');
const { generateToken, decryptToken } = require('../api/apiUtils/object/continueToken');

// do not url encode the continuation tokens
const skipUrlEncoding = new Set([
'ContinuationToken',
'NextContinuationToken',
]);
const xmlParamsToSkipUrlEncoding = new Set(['ContinuationToken', 'NextContinuationToken']);

/* Sample XML response for GET bucket objects V2:
<ListBucketResult xmlns="http://s3.amazonaws.com/doc/2006-03-01/">
Expand Down Expand Up @@ -122,17 +118,16 @@ function processVersions(bucketName, listParams, list) {
{ tag: 'IsTruncated', value: isTruncated },
];

const escapeXmlFn = listParams.encoding === 'url' ?
querystring.escape : escapeForXml;
const escapeXmlFn = listParams.encoding === 'url' ? querystring.escape : escapeForXml;
xmlParams.forEach(p => {
if (p.value) {
const val = p.tag !== 'NextVersionIdMarker' || p.value === 'null' ?
p.value : versionIdUtils.encode(p.value);
p.value :
versionIdUtils.encode(p.value);
Copy link
Contributor

@francoisferrand francoisferrand Jan 20, 2026

Choose a reason for hiding this comment

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

Does not seem related to async/await: kind of dilutes the message here (ie show casing the migration)

Copy link
Contributor

@francoisferrand francoisferrand Jan 22, 2026

Choose a reason for hiding this comment

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

(significant part of this change seems to be in that case...)

Copy link
Contributor Author

@DarkIsDude DarkIsDude Jan 23, 2026

Choose a reason for hiding this comment

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

indeed, I re-read the whole file and try to simplify at the same time 🙏

Copy link
Contributor

@francoisferrand francoisferrand Jan 26, 2026

Choose a reason for hiding this comment

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

can we split the PR, to keep the focus?

Copy link
Contributor Author

@DarkIsDude DarkIsDude Jan 29, 2026

Choose a reason for hiding this comment

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

Does it make really sense ? +160 −171 the PR is pretty small already ?

xml.push(`<${p.tag}>${escapeXmlFn(val)}</${p.tag}>`);
}
});
let lastKey = listParams.keyMarker ?
escapeXmlFn(listParams.keyMarker) : undefined;
let lastKey = listParams.keyMarker ? escapeXmlFn(listParams.keyMarker) : undefined;
list.Versions.forEach(item => {
const v = item.value;
const objectKey = escapeXmlFn(item.key);
Expand All @@ -143,7 +138,8 @@ function processVersions(bucketName, listParams, list) {
`<Key>${objectKey}</Key>`,
'<VersionId>',
(v.IsNull || v.VersionId === undefined) ?
'null' : versionIdUtils.encode(v.VersionId),
'null'
: versionIdUtils.encode(v.VersionId),
'</VersionId>',
`<IsLatest>${isLatest}</IsLatest>`,
`<LastModified>${v.LastModified}</LastModified>`,
Expand Down Expand Up @@ -182,31 +178,19 @@ function processMasterVersions(bucketName, listParams, list) {
];

if (listParams.v2) {
xmlParams.push(
{ tag: 'StartAfter', value: listParams.startAfter || '' });
xmlParams.push(
{ tag: 'FetchOwner', value: `${listParams.fetchOwner}` });
xmlParams.push({
tag: 'ContinuationToken',
value: generateToken(listParams.continuationToken) || '',
});
xmlParams.push({
tag: 'NextContinuationToken',
value: generateToken(list.NextContinuationToken),
});
xmlParams.push({
tag: 'KeyCount',
value: list.Contents ? list.Contents.length : 0,
});
xmlParams.push({ tag: 'StartAfter', value: listParams.startAfter || '' });
xmlParams.push({ tag: 'FetchOwner', value: `${listParams.fetchOwner}` });
xmlParams.push({ tag: 'ContinuationToken', value: generateToken(listParams.continuationToken) || '', });
Copy link

@maeldonn maeldonn Jan 29, 2026

Choose a reason for hiding this comment

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

trailing comma 👀

xmlParams.push({ tag: 'NextContinuationToken', value: generateToken(list.NextContinuationToken), });
xmlParams.push({ tag: 'KeyCount', value: list.Contents ? list.Contents.length : 0, });
} else {
xmlParams.push({ tag: 'Marker', value: listParams.marker || '' });
xmlParams.push({ tag: 'NextMarker', value: list.NextMarker });
}

const escapeXmlFn = listParams.encoding === 'url' ?
querystring.escape : escapeForXml;
const escapeXmlFn = listParams.encoding === 'url' ? querystring.escape : escapeForXml;
xmlParams.forEach(p => {
if (p.value && skipUrlEncoding.has(p.tag)) {
if (p.value && xmlParamsToSkipUrlEncoding.has(p.tag)) {
xml.push(`<${p.tag}>${p.value}</${p.tag}>`);
} else if (p.value || p.tag === 'KeyCount' || p.tag === 'MaxKeys') {
xml.push(`<${p.tag}>${escapeXmlFn(p.value)}</${p.tag}>`);
Expand Down Expand Up @@ -246,15 +230,14 @@ function processMasterVersions(bucketName, listParams, list) {
);
});
list.CommonPrefixes.forEach(item => {
const val = escapeXmlFn(item);
xml.push(`<CommonPrefixes><Prefix>${val}</Prefix></CommonPrefixes>`);
xml.push(`<CommonPrefixes><Prefix>${escapeXmlFn(item)}</Prefix></CommonPrefixes>`);
});
xml.push('</ListBucketResult>');

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 @@ -267,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 @@ -278,28 +261,25 @@ function handleResult(listParams, requestMaxKeys, encoding, authInfo,
* @param {function} log - Werelogs request logger
* @param {function} callback - callback to respond to http request
Copy link

@maeldonn maeldonn Jan 29, 2026

Choose a reason for hiding this comment

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

Suggested change
* @param {function} callback - callback to respond to http request

* with either error code or xml response body
* @return {undefined}
* @return {Promise<object>} - object containing xml and additionalResHeaders
*/
function bucketGet(authInfo, request, log, callback) {
async function bucketGet(authInfo, request, log) {
const params = request.query;
const bucketName = request.bucketName;
const v2 = params['list-type'];

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',
});
log.addDefaultFields({ action: 'ListObjectsV2', });
if (request.serverAccessLog) {
// eslint-disable-next-line no-param-reassign
request.serverAccessLog.analyticsAction = 'ListObjectsV2';
}
} else if (params.versions !== undefined) {
log.addDefaultFields({
action: 'ListObjectVersions',
});
log.addDefaultFields({ action: 'ListObjectVersions', });
if (request.serverAccessLog) {
// eslint-disable-next-line no-param-reassign
request.serverAccessLog.analyticsAction = 'ListObjectVersions';
Expand All @@ -308,21 +288,15 @@ function bucketGet(authInfo, request, log, callback) {
log.debug('processing request', { method: 'bucketGet' });
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'));
monitoring.promMetrics('GET', bucketName, 400, 'listBucket');
throw errorInstances.InvalidArgument.customizeDescription('Invalid Encoding Method specified in Request');
}
const requestMaxKeys = params['max-keys'] ?
Number.parseInt(params['max-keys'], 10) : 1000;

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);
monitoring.promMetrics('GET', bucketName, 400, 'listBucket');
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 All @@ -344,56 +318,74 @@ function bucketGet(authInfo, request, log, callback) {
if (v2) {
listParams.v2 = true;
listParams.startAfter = params['start-after'];
listParams.continuationToken =
decryptToken(params['continuation-token']);
listParams.continuationToken = decryptToken(params['continuation-token']);
listParams.fetchOwner = params['fetch-owner'] === 'true';
} else {
listParams.marker = params.marker;
}

standardMetadataValidateBucket(metadataValParams, request.actionImplicitDenies, log, (err, bucket) => {
const corsHeaders = collectCorsHeaders(request.headers.origin,
request.method, bucket);
if (err) {
log.debug('error processing request', { error: err });
monitoring.promMetrics(
'GET', bucketName, err.code, 'listBucket');
return callback(err, null, corsHeaders);
}
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,
};
return handleResult(listParams, requestMaxKeys, encoding, authInfo,
bucketName, emptyList, corsHeaders, log, callback);
}
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);
}
return handleResult(listParams, requestMaxKeys, encoding, authInfo,
bucketName, list, corsHeaders, log, callback);
});
});
return undefined;
let error;
let bucket;

try {
const standardMetadataValidateBucketPromised = promisify(standardMetadataValidateBucket);
Copy link

@maeldonn maeldonn Jan 29, 2026

Choose a reason for hiding this comment

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

In a migration context, I think it's easier to keep all promisified functions at the top of the file.

Copy link

@maeldonn maeldonn Jan 29, 2026

Choose a reason for hiding this comment

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

And for performance of course

bucket = await standardMetadataValidateBucketPromised(metadataValParams, request.actionImplicitDenies, log);
} catch (err) {
error = err;
Copy link

@maeldonn maeldonn Jan 29, 2026

Choose a reason for hiding this comment

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

The goal of this variable is to get cors headesr before throwing the error but it adds complexity. You could call collectCorsHeaders in the catch to avoid this.

}

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

if (error) {
log.debug('error processing request', { error });
monitoring.promMetrics('GET', bucketName, error.code, 'listBucket');
error.additionalResHeaders = corsHeaders;
Copy link
Contributor

@francoisferrand francoisferrand Jan 26, 2026

Choose a reason for hiding this comment

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

this was not done before: as we kind of "abused" the continuation callback semantics with an 3rd parameter on error...
→ so I wonder if this is a good exemple? or it is on purpose, to show how to handle such tricks as well?
→ to minimize risk, should we first do a refactor to stop doing this (without migrating to async/await); then proceed with a more streamlined migration to async/await?

I don't see how this is handled in "caller" code though (i.e. code "receiving" these CORS/additional headers on error), did I miss it do we already support such use of the additionalResHeaders field ?

Copy link
Contributor Author

@DarkIsDude DarkIsDude Jan 29, 2026

Choose a reason for hiding this comment

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

🤦 thanks for the spot. I forgot to add that in the callback 🙏.

I'm not sure we should do a first refactor, it'll add delay and it's the goal of a refactor IMO ? But I guess I can add that in our docs on Confluence ? So yes the goal is to make a "good" example.

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];
}

let list;
try {
Copy link

@maeldonn maeldonn Jan 29, 2026

Choose a reason for hiding this comment

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

Not a big fan of multiple try/catch in a single function, could it be simplified ?

list = await promisify(services.getObjectListing)(bucketName, listParams, log);
} catch (err) {
log.debug('error processing request', { error: err });
monitoring.promMetrics('GET', bucketName, err.code, 'listBucket');

err.additionalResHeaders = corsHeaders;
throw err;
}

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

module.exports = {
processVersions,
processMasterVersions,
bucketGet,
bucketGet: (...args) => {
Copy link
Contributor

@francoisferrand francoisferrand Jan 26, 2026

Choose a reason for hiding this comment

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

should we go that road?

  • "rewiring" functions in imports/exports makes the code difficult to read IMHO, as the function is not called the same way it is "defined"
  • may not be an issue for this specific function, but this approach does not allow progressively migrating calls: even if the body of the function was updated already, it is still exported only as async or as continuation callback...
  • the slicing is dangerous, as it is based on the actual arguments of the call, not the parameters of the function. So if some code calls without callbacK (esp. likely while migrating...) we end up dropping the last (non-callback) parameter

Copy link
Contributor Author

@DarkIsDude DarkIsDude Jan 29, 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 point. So what is your suggestion ? Should I create a new wrapper bucketGet and rename the function bucketGetAsync ? Or integrate that directly in the function with a if (callback) { bucketGet(...).then().catch() } ? I'm not a big fan of this if as we introduce callback in an async/await code (but can be acceptable during migration I guess?). Or any other idea?

The idea of the slice was to avoid repeating the same arguments in the async and in the callback function but indeed the tradeoff is what you mention. Anyway if some code calls without callback, we'll have an issue in any case.

const callback = args.at(-1);
const argsWithoutCallback = args.slice(0, -1);

bucketGet(...argsWithoutCallback)
.then(result => callback(null, ...result))
.catch(err => callback(err, null, err.additionalResHeaders));
},
};

Loading
Loading