✨ CLDSRV-812: ListObjectsV2 create x-amz-optional-attributes header #6033

DarkIsDude · 2025-12-17T15:48:19Z

Pull request template

Description

Support a new header x-amz-object-optional-attributes to be able to retrieve in the ListObjectsV2 user metadata.

Motivation and context

https://github.com/scality/citadel/pull/301/changes

https://scality.atlassian.net/browse/CLDSRV-812

Related issues

scality/Arsenal#2581 is needed as the bump of Arsenal in Vault.

A new MR will be opened to update the response of ListObjectsV2 and return asked fields.

✨ ARSN-544: Add new custom permission for Optional Object Attributes Arsenal#2581 bump Arsenal

Issue: CLDSRV-812

bert-e · 2025-12-17T15:48:22Z

Hello darkisdude,

My role is to assist you with the merge of this
pull request. Please type @bert-e help to get information
on this process, or consult the user documentation.

Available options

name	description	privileged	authored
`/after_pull_request`	Wait for the given pull request id to be merged before continuing with the current one.
`/bypass_author_approval`	Bypass the pull request author's approval	⭐
`/bypass_build_status`	Bypass the build and test status	⭐
`/bypass_commit_size`	Bypass the check on the size of the changeset `TBA`	⭐
`/bypass_incompatible_branch`	Bypass the check on the source branch prefix	⭐
`/bypass_jira_check`	Bypass the Jira issue check	⭐
`/bypass_peer_approval`	Bypass the pull request peers' approval	⭐
`/bypass_leader_approval`	Bypass the pull request leaders' approval	⭐
`/approve`	Instruct Bert-E that the author has approved the pull request.		✍️
`/create_pull_requests`	Allow the creation of integration pull requests.
`/create_integration_branches`	Allow the creation of integration branches.
`/no_octopus`	Prevent Wall-E from doing any octopus merge and use multiple consecutive merge instead
`/unanimity`	Change review acceptance criteria from `one reviewer at least` to `all reviewers`
`/wait`	Instruct Bert-E not to run until further notice.

Available commands

name	description	privileged
`/help`	Print Bert-E's manual in the pull request.
`/status`	Print Bert-E's current status in the pull request `TBA`
`/clear`	Remove all comments from Bert-E from the history `TBA`
`/retry`	Re-start a fresh build `TBA`
`/build`	Re-start a fresh build `TBA`
`/force_reset`	Delete integration branches & pull requests, and restart merge process from the beginning.
`/reset`	Try to remove integration branches unless there are commits on them which do not appear on the source branch.

Status report is not available.

bert-e · 2025-12-17T15:48:33Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-812 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.11

Please check the Fix Version/s of CLDSRV-812, or the target
branch of this pull request.

codecov · 2025-12-17T15:51:23Z

Codecov Report

❌ Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 84.36%. Comparing base (c8fe018) to head (535b360).
⚠️ Report is 2 commits behind head on development/9.2.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
...i/apiUtils/authorization/prepareRequestContexts.js	83.33%	1 Missing ⚠️
lib/api/bucketGet.js	85.71%	1 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
...i/apiUtils/authorization/prepareRequestContexts.js	`94.57% <83.33%> (-0.55%)`	⬇️
lib/api/bucketGet.js	`96.24% <85.71%> (+0.08%)`	⬆️

... and 1 file with indirect coverage changes

@@                 Coverage Diff                 @@
##           development/9.2    #6033      +/-   ##
===================================================
+ Coverage            84.34%   84.36%   +0.01%     
===================================================
  Files                  204      204              
  Lines                12926    12935       +9     
===================================================
+ Hits                 10903    10912       +9     
  Misses                2023     2023

Flag	Coverage Δ
file-ft-tests	`67.44% <76.92%> (+<0.01%)`	⬆️
kmip-ft-tests	`28.13% <0.00%> (-0.02%)`	⬇️
mongo-v0-ft-tests	`68.67% <76.92%> (-0.02%)`	⬇️
mongo-v1-ft-tests	`68.70% <76.92%> (+0.02%)`	⬆️
multiple-backend	`35.35% <69.23%> (+0.02%)`	⬆️
sur-tests	`35.70% <69.23%> (+0.06%)`	⬆️
sur-tests-inflights	`37.49% <69.23%> (-0.01%)`	⬇️
unit	`69.93% <84.61%> (+0.01%)`	⬆️
utapi-v2-tests	`34.37% <69.23%> (+0.03%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Issue: CLDSRV-812

bert-e · 2025-12-18T13:13:00Z

Incorrect fix version

The Fix Version/s in issue CLDSRV-812 contains:

None

Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:

9.2.12

Please check the Fix Version/s of CLDSRV-812, or the target
branch of this pull request.

bert-e · 2025-12-18T16:45:00Z

Waiting for approval

The following approvals are needed before I can proceed with the merge:

the author
2 peers

DarkIsDude · 2025-12-19T09:10:31Z

/wait

francoisferrand · 2026-01-09T17:39:42Z

tests/unit/api/bucketGet.js

+                url: baseUrl,
+            }, baseGetRequest);
+            testGetRequest.headers['x-amz-optional-object-attributes'] = 'x-amz-meta-department';
+


nit: multiple empty lines

francoisferrand · 2026-01-09T17:41:06Z

tests/unit/api/bucketGet.js

+            async.waterfall(
+                [
+                    next => bucketPut(authInfo, testPutBucketRequest, log, next),
+                    (_, next) => bucketGet(authInfo, testGetRequest, log, next),
+                    (result, _, next) => parseString(result, next),
+                ],
+                (err, result) => {
+                    assert.strictEqual(err, null);
+                    assert.strictEqual(result.ListBucketResult.$.xmlns, 'http://s3.amazonaws.com/doc/2006-03-01/');
+                    done();
+                },


nit: inconsistent formatting of async.xxx

Suggested change

async.waterfall(

[

next => bucketPut(authInfo, testPutBucketRequest, log, next),

(_, next) => bucketGet(authInfo, testGetRequest, log, next),

(result, _, next) => parseString(result, next),

],

(err, result) => {

assert.strictEqual(err, null);

assert.strictEqual(result.ListBucketResult.$.xmlns, 'http://s3.amazonaws.com/doc/2006-03-01/');

done();

},

async.waterfall([

next => bucketPut(authInfo, testPutBucketRequest, log, next),

(_, next) => bucketGet(authInfo, testGetRequest, log, next),

(result, _, next) => parseString(result, next),

], (err, result) => {

assert.strictEqual(err, null);

assert.strictEqual(result.ListBucketResult.$.xmlns, 'http://s3.amazonaws.com/doc/2006-03-01/');

done();

},

francoisferrand · 2026-01-09T17:41:57Z

tests/unit/api/bucketGet.js

+            });
+        });
+
+        it('should ignore the missing permission if the header contains only RestoreStatus', done => {


nit: it does not really "ignore" the missing permission, it should not even try to check/require it...

Suggested change

it('should ignore the missing permission if the header contains only RestoreStatus', done => {

it('should return RestoreStatus without extra permission', done => {

francoisferrand · 2026-01-09T17:45:42Z

tests/unit/api/bucketGet.js

+        });
+
+        it('should ignore the missing permission if the header contains only RestoreStatus', done => {
+            const testGetRequest = Object.assign({


while we should have a test with this purpose, looking at the code I think it does not really test anything: permission-wise, we have the exact same test as should return valid xml if the user have the permission (the only difference between the tests is the attribute we ask for, but no difference in "auth" setup/mock or assertion on the requested permissions)

francoisferrand · 2026-01-09T17:46:48Z

tests/unit/api/bucketGet.js

+        it('should return an error if the user does not have all permissions for multiple attributes', done => {
+            const authInfoNoPerm = makeAuthInfo('accessKey2');


here as well, same auth setup as should return an error if the user does not have the permission : making this test redundant...

francoisferrand · 2026-01-09T17:49:40Z

tests/unit/api/bucketGet.js

+                url: baseUrl,
+            }, baseGetRequest);
+            testGetRequest.headers['x-amz-optional-object-attributes'] = 'RestoreStatus,InvalidAttribute';
+


completely unrelated, but I wonder if we need to be case-insensitive when matching attributes (esp. RestoreStatus which is capitalized) : we should check/test AWS behavior (and verify if user metadata "labels" are case sensitive or not), and handle case-senstitiveness accordingly

May be done in a followup ticket however

francoisferrand · 2026-01-09T17:52:18Z

tests/unit/api/bucketGet.js

+                query: {},
+                url: baseUrl,
+            }, baseGetRequest);
+            testGetRequest.headers['x-amz-optional-object-attributes'] = 'x-amz-meta-foo';


should have leading/trailing whitespaces

Suggested change

testGetRequest.headers['x-amz-optional-object-attributes'] = 'x-amz-meta-foo';

testGetRequest.headers['x-amz-optional-object-attributes'] = ' x-amz-meta-foo ';

NOTE: may be worth testing as well with RestoreStatus, since we will not pick the data form the same place...

francoisferrand · 2026-01-09T17:55:28Z

tests/unit/api/bucketGet.js

+            (err, result) => {
+                assert.strictEqual(err, null);
+                assert.strictEqual(result.ListBucketResult.$.xmlns, 'http://s3.amazonaws.com/doc/2006-03-01/');
+                done();


as such, this test (and the next) do not test much : since we don't have a way to check that "every" requested field is returned

I guess this will be fixed in next PR, but may be best to add a //TODO for now to ensure we don't forget to update the test in next PR

DarkIsDude added 3 commits December 17, 2025 09:11

📝 fix the RELEASE documentation

19ce8d6

🔧 add pino-prettier to the dev script

ef5c47a

✨ manage the new header x-amz-optional-attributes

3573352

Issue: CLDSRV-812

DarkIsDude self-assigned this Dec 17, 2025

✨ validate the format of the header

acf4175

Issue: CLDSRV-812

DarkIsDude requested review from a team, SylvainSenechal and delthas December 18, 2025 16:39

🚨 lint markdown RELEASE

535b360

DarkIsDude marked this pull request as ready for review December 19, 2025 09:09

francoisferrand reviewed Jan 9, 2026

View reviewed changes

francoisferrand approved these changes Jan 9, 2026

View reviewed changes

	it('should ignore the missing permission if the header contains only RestoreStatus', done => {
	it('should return RestoreStatus without extra permission', done => {

		it('should return an error if the user does not have all permissions for multiple attributes', done => {
		const authInfoNoPerm = makeAuthInfo('accessKey2');

	testGetRequest.headers['x-amz-optional-object-attributes'] = 'x-amz-meta-foo';
	testGetRequest.headers['x-amz-optional-object-attributes'] = ' x-amz-meta-foo ';

✨ CLDSRV-812: ListObjectsV2 create x-amz-optional-attributes header #6033

Are you sure you want to change the base?

✨ CLDSRV-812: ListObjectsV2 create x-amz-optional-attributes header #6033

Conversation

DarkIsDude commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull request template

Description

Motivation and context

Related issues

Uh oh!

bert-e commented Dec 17, 2025

Hello darkisdude,

Uh oh!

bert-e commented Dec 17, 2025

Incorrect fix version

Uh oh!

codecov bot commented Dec 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bert-e commented Dec 18, 2025

Incorrect fix version

Uh oh!

bert-e commented Dec 18, 2025

Waiting for approval

Uh oh!

DarkIsDude commented Dec 19, 2025

Uh oh!

francoisferrand Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

francoisferrand Jan 9, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

DarkIsDude commented Dec 17, 2025 •

edited

Loading

codecov bot commented Dec 17, 2025 •

edited

Loading

francoisferrand Jan 9, 2026 •

edited

Loading

francoisferrand Jan 9, 2026 •

edited

Loading