🎨 Async/Await migration on bucketGet and objectGetLegalHold #6045
Conversation
Hello darkisdude,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
DarkIsDude
changed the title
Codecov Report❌ Patch coverage is
Additional details and impacted files
@@ Coverage Diff @@
## development/9.2 #6045 +/- ##
===================================================
+ Coverage 84.49% 84.50% +0.01%
===================================================
Files 206 206
Lines 13142 13157 +15
===================================================
+ Hits 11104 11118 +14
- Misses 2038 2039 +1
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
DarkIsDude
force-pushed
the
feature/CLDSRV-823/async-await-start
branch
from
16402cf to
4e7d644
Compare
DarkIsDude
requested review from
a team,
anurag4DSB,
delthas and
maeldonn
and removed request for
a team
DarkIsDude
changed the title
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
| const val = p.tag !== 'NextVersionIdMarker' || p.value === 'null' ? | ||
| p.value : versionIdUtils.encode(p.value); | ||
| p.value : | ||
| versionIdUtils.encode(p.value); |
There was a problem hiding this comment.
Does not seem related to async/await: kind of dilutes the message here (ie show casing the migration)
There was a problem hiding this comment.
(significant part of this change seems to be in that case...)
There was a problem hiding this comment.
indeed, I re-read the whole file and try to simplify at the same time 🙏
There was a problem hiding this comment.
can we split the PR, to keep the focus?
There was a problem hiding this comment.
Does it make really sense ? +160 −171 the PR is pretty small already ?
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
DarkIsDude
force-pushed
the
feature/CLDSRV-823/async-await-start
branch
2 times, most recently
from
bafa722 to
08d8dc0
Compare
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
DarkIsDude
force-pushed
the
feature/CLDSRV-823/async-await-start
branch
from
08d8dc0 to
2196def
Compare
DarkIsDude
requested review from
SylvainSenechal,
delthas and
francoisferrand
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
lib/api/bucketGet.js
Outdated
| return res; | ||
| } | ||
|
|
||
| const validateBucket = (params, denials, log) => new Promise(resolve => { |
There was a problem hiding this comment.
- we should use promisify instead of manually recoding it?
- is it worth introducing a wrapper? Esp. in such case where it is called just once, I feel it just kind of "hides" the call to
standardMetadataValidateBucketwithout much value; and I fear such helper would end up being harder to remove eventually (once migration is complete), because it would require changes the code in multiple places when not needed anymore [vs just searching forstandardMetadataValidateBucketand updating all uses]
| const validateBucket = (params, denials, log) => new Promise(resolve => { | |
| const validateBucket = promisify(standardMetadataValidateBucket); |
I.e. overall, since this is a temporary state, I would rather not try to make the migrated function look "perfect", but rather embrace the fact this is a transition, and temporarily keep promisify calls in the middle of these...
There was a problem hiding this comment.
My goal was also to try to lighten the function as it's already a 100 lines function but I agree with you that it's maybe not the best choices 🙏.
| if (error) { | ||
| log.debug('error processing request', { error }); | ||
| monitoring.promMetrics('GET', bucketName, error.code, 'listBucket'); | ||
| error.additionalResHeaders = corsHeaders; |
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
🤦 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.
| processVersions, | ||
| processMasterVersions, | ||
| bucketGet, | ||
| bucketGet: (...args) => { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Branches have divergedThis pull request's source branch To avoid any integration risks, please re-synchronize them using one of the
Note: If you choose to rebase, you may have to ask me to rebuild |
DarkIsDude
force-pushed
the
feature/CLDSRV-823/async-await-start
branch
from
2196def to
250de6f
Compare
Request integration branchesWaiting for integration branch creation to be requested by the user. To request integration branches, please comment on this pull request with the following command: Alternatively, the |
DarkIsDude
force-pushed
the
feature/CLDSRV-823/async-await-start
branch
from
0ef0fb3 to
474bad8
Compare
DarkIsDude
force-pushed
the
feature/CLDSRV-823/async-await-start
branch
from
474bad8 to
7a14efc
Compare
|
|
||
| objectGetLegalHold(...argsWithoutCallback) | ||
| .then(result => callback(null, ...result)) | ||
| .catch(err => callback(err, null, err.additionalResHeaders)); |
There was a problem hiding this comment.
| .catch(err => callback(err, null, err.additionalResHeaders)); | |
| .catch(err => callback(err, null, err.additionalResHeaders ?? {})); |
It's just defensive, maybe useless...
There was a problem hiding this comment.
The same thing can be found in the other file if you need to modify it. 🤷♂️
|
|
||
| const { convertToXml } = s3middleware.objectLegalHold; | ||
|
|
||
| const standardMetadataValidateBucketAndObjPromised = (metadataValParams, actionImplicitDenies, log) => |
There was a problem hiding this comment.
Instead of doing one custom promisify for each file that is migrated, we can do the same thing in another way:
https://nodejs.org/docs/latest-v22.x/api/util.html#custom-promisified-functions
Doing it at function level will facilitate its migration and reduce impacts.
| * @param {object} log - Werelogs logger | ||
| * @param {function} callback - callback to server | ||
| * @return {undefined} | ||
| * @return {Promise<object>} - object containing xml and additionalResHeaders |
There was a problem hiding this comment.
Consider adding @throws documentation for better visibility on error handling.
| let bucket; | ||
|
|
||
| try { | ||
| const standardMetadataValidateBucketPromised = promisify(standardMetadataValidateBucket); |
There was a problem hiding this comment.
In a migration context, I think it's easier to keep all promisified functions at the top of the file.
| const standardMetadataValidateBucketPromised = promisify(standardMetadataValidateBucket); | ||
| bucket = await standardMetadataValidateBucketPromised(metadataValParams, request.actionImplicitDenies, log); | ||
| } catch (err) { | ||
| error = err; |
There was a problem hiding this comment.
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.
| @@ -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 | |||
There was a problem hiding this comment.
| * @param {function} callback - callback to respond to http request |
| }); | ||
| xmlParams.push({ tag: 'StartAfter', value: listParams.startAfter || '' }); | ||
| xmlParams.push({ tag: 'FetchOwner', value: `${listParams.fetchOwner}` }); | ||
| xmlParams.push({ tag: 'ContinuationToken', value: generateToken(listParams.continuationToken) || '', }); |
| throw errors.NoSuchObjectLockConfiguration; | ||
| } | ||
|
|
||
| const additionalResHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket); |
There was a problem hiding this comment.
To keep the initial behavior, this function should be call earlier to do the err.additionalResHeader trick.
| } | ||
|
|
||
| let list; | ||
| try { |
There was a problem hiding this comment.
Not a big fan of multiple try/catch in a single function, could it be simplified ?
Issue: CLDSRV-823
Description
Motivation and context
Opening a first PR to migrate from callback to async/await syntax. A TAD is opened.
https://scality.atlassian.net/wiki/spaces/OS/pages/3523346468/2025-10-30+-+Async+Await+migration
This is a first PR to start the migration. As mention in the TAD, the idea is to migrate to async/await when working on a specific part of our component. Maybe a common goal of one PR per squad per sprint is a good idea.
All idea or remarks are welcome 🙏