Skip to content

Update functional tests to use cloudserverclient extended commands #6103

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
maeldonn wants to merge 5 commits into
base: development/9.3
Choose a base branch
from
+1,425 −796
Open

Update functional tests to use cloudserverclient extended commands #6103

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
f659ecb
Add @scality/cloudserverclient dev dependency
maeldonn Mar 13, 2026
8cf8811
Resolve fast-xml-parser CVE-2026-26278 via yarn resolution
maeldonn Mar 13, 2026
8c4bd96
Refactor GetObjectAttributes functional tests to use extended command
maeldonn Mar 12, 2026
32201f4
Refactor ListObjectsV2 functional tests to use extended command
maeldonn Mar 12, 2026
59372f0
Add GetObjectAttributes functional test for non-existent user metadata
maeldonn Mar 13, 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
7 changes: 5 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
"homepage": "https://github.com/scality/S3#readme",
"dependencies": {
"@aws-sdk/client-iam": "^3.930.0",
"@aws-sdk/client-s3": "^3.908.0",
"@aws-sdk/client-s3": "^3.1012.0",
"@aws-sdk/client-sts": "^3.930.0",
"@aws-sdk/credential-providers": "^3.864.0",
"@aws-sdk/middleware-retry": "^3.374.0",
Expand Down Expand Up @@ -62,6 +62,7 @@
},
"devDependencies": {
"@eslint/compat": "^1.2.2",
"@scality/cloudserverclient": "1.0.7",
Copy link

@claude claude bot Mar 18, 2026

Choose a reason for hiding this comment

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

Adding @scality/cloudserverclient (which depends on @aws-sdk/client-s3@^3.1009.0) as a devDependency causes yarn to deduplicate and resolve the production @aws-sdk/client-s3 from 3.917.0 to 3.1009.0. This is a much larger jump than the ^3.908.0 to ^3.931.0 bump in package.json suggests. Ensure this is intentional and that the 3.1009.0 SDK has been validated against the production server code (not just functional tests).

— Claude Code

"@scality/eslint-config-scality": "scality/Guidelines#8.3.0",
"eslint": "^9.14.0",
"eslint-plugin-import": "^2.31.0",
Expand All @@ -83,7 +84,9 @@
},
"resolutions": {
"jsonwebtoken": "^9.0.0",
"nan": "v2.22.0"
"nan": "v2.22.0",
"fast-xml-parser": ">=5.5.6",
"@aws-sdk/xml-builder": ">=3.972.13"
Copy link

@claude claude bot Mar 19, 2026

Choose a reason for hiding this comment

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

The >= operator is very loose — it allows any future major version, which could pull in breaking changes. Use ^ (caret) to stay within compatible versions.

```suggestion
"fast-xml-parser": "^5.5.6",
"@aws-sdk/xml-builder": "^3.972.13"

},
"mocha": {
"recursive": true,
Expand Down
94 changes: 10 additions & 84 deletions tests/functional/aws-node-sdk/test/bucket/get.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ const {
ListObjectsV2Command,
S3Client,
} = require('@aws-sdk/client-s3');
const { streamCollector } = require('@smithy/node-http-handler');
const { ListObjectsV2ExtendedCommand } = require('@scality/cloudserverclient');
const {
IAMClient,
CreatePolicyCommand,
Expand All @@ -17,8 +17,6 @@ const {
DeletePolicyCommand,
DeleteUserCommand,
} = require('@aws-sdk/client-iam');
const { parseStringPromise } = require('xml2js');

const withV4 = require('../support/withV4');
const BucketUtility = require('../../lib/utility/bucket-util');
const bucketSchema = require('../../schema/bucket');
Expand Down Expand Up @@ -609,75 +607,6 @@ describe('GET Bucket - AWS.S3.listObjects', () => {
await iamClient.send(new DeleteUserCommand({ UserName: userWithoutPermission.UserName }));
});

const listObjectsV2WithOptionalAttributes = async (s3, bucket, headerValue) => {
const localS3 = s3;
let rawXml = '';

const addHeaderMiddleware = next => async args => {
const localArgs = args;
localArgs.request.headers['x-amz-optional-object-attributes'] = headerValue;
return next(localArgs);
};

const originalHandler = s3.config.requestHandler;
const wrappedHandler = {
async handle(request, options) {
const { response } = await originalHandler.handle(request, options);

if (response && response.body) {
const collected = await streamCollector(response.body);
const buffer = Buffer.from(collected);
rawXml = buffer.toString('utf-8');

const { Readable } = require('stream');
response.body = Readable.from([buffer]);
}

return { response };
},
destroy() {
if (originalHandler.destroy) {
originalHandler.destroy();
}
}
};

localS3.config.requestHandler = wrappedHandler;
localS3.middlewareStack.add(addHeaderMiddleware, {
step: 'build',
name: 'addOptionalAttributesHeader',
});

try {
const result = await s3.send(new ListObjectsV2Command({ Bucket: bucket }));

if (!rawXml) {
return result;
}

const parsedXml = await parseStringPromise(rawXml);
const contents = result.Contents;
const parsedContents = parsedXml?.ListBucketResult?.Contents;

if (!contents || !parsedContents) {
return result;
}

if (parsedContents[0]?.['x-amz-meta-department']) {
contents[0]['x-amz-meta-department'] = parsedContents[0]['x-amz-meta-department'][0];
}

if (parsedContents[0]?.['x-amz-meta-hr']) {
contents[0]['x-amz-meta-hr'] = parsedContents[0]['x-amz-meta-hr'][0];
}

return result;
} finally {
localS3.config.requestHandler = originalHandler;
localS3.middlewareStack.remove('addOptionalAttributesHeader');
}
};

it('should return an XML if the header is set', async () => {
const s3 = bucketUtil.s3;
const Bucket = bucketName;
Expand All @@ -690,11 +619,10 @@ describe('GET Bucket - AWS.S3.listObjects', () => {
hr: 'true',
},
}));
const result = await listObjectsV2WithOptionalAttributes(
s3,
const result = await s3.send(new ListObjectsV2ExtendedCommand({
Bucket,
'x-amz-meta-*,RestoreStatus,x-amz-meta-department',
);
ObjectAttributes: ['x-amz-meta-*', 'RestoreStatus', 'x-amz-meta-department'],
}));

assert.strictEqual(result.Contents.length, 1);
assert.strictEqual(result.Contents[0].Key, 'super-power-object');
Expand All @@ -716,11 +644,10 @@ describe('GET Bucket - AWS.S3.listObjects', () => {
}));

try {
await listObjectsV2WithOptionalAttributes(
s3ClientWithoutPermission,
await s3ClientWithoutPermission.send(new ListObjectsV2ExtendedCommand({
Bucket,
'x-amz-meta-*,RestoreStatus,x-amz-meta-department',
);
ObjectAttributes: ['x-amz-meta-*', 'RestoreStatus', 'x-amz-meta-department'],
}));
throw new Error('Request should have been rejected');
} catch (err) {
if (err.message === 'Request should have been rejected') {
Expand All @@ -743,11 +670,10 @@ describe('GET Bucket - AWS.S3.listObjects', () => {
hr: 'true',
},
}));
const result = await listObjectsV2WithOptionalAttributes(
s3ClientWithoutPermission,
const result = await s3ClientWithoutPermission.send(new ListObjectsV2ExtendedCommand({
Bucket,
'RestoreStatus',
);
ObjectAttributes: ['RestoreStatus'],
}));

assert.strictEqual(result.Contents.length, 1);
assert.strictEqual(result.Contents[0].Key, 'super-power-object');
Expand Down
3 changes: 2 additions & 1 deletion tests/functional/aws-node-sdk/test/bucket/head.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,8 @@ describe('HEAD bucket', () => {
await s3.send(new HeadBucketCommand({ Bucket: '' }));
assert.fail('Expected failure but got success');
} catch (err) {
assert.strictEqual(err.message, 'Empty value provided for input HTTP label: Bucket.');
assert.strictEqual(err.$metadata.httpStatusCode, 405);
assert.strictEqual(err.name, 'Unknown');
Copy link

@claude claude bot Mar 18, 2026

Choose a reason for hiding this comment

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

The assertion on err.name === 'Unknown' is fragile. If the server returns a proper error code in the future, this test will pass silently even if behavior changes. Consider adding a comment explaining why 'Unknown' is the expected error name here (likely because HEAD responses have no body for error details).

— Claude Code

Copy link

@claude claude bot Mar 18, 2026

Choose a reason for hiding this comment

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

This test previously validated SDK client-side input checking (err.message). The newer SDK no longer validates empty bucket names client-side and sends the request to the server, returning 405. Asserting on err.name === 'Unknown' is fragile — if CloudServer starts returning a named error for this case, the test breaks silently. Consider also asserting on a more descriptive property if available.

— Claude Code

}
});
});
Expand Down
5 changes: 1 addition & 4 deletions tests/functional/aws-node-sdk/test/multipleBackend/unknownEndpoint.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,7 @@ let s3;
describe('Requests to ip endpoint not in config', () => {
withV4(sigCfg => {
before(() => {
bucketUtil = new BucketUtility('default', sigCfg);
// change endpoint to endpoint with ip address
// not in config
bucketUtil.s3.config.endpoint = specifiedEndpoint;
bucketUtil = new BucketUtility('default', { ...sigCfg, endpoint: specifiedEndpoint });
s3 = bucketUtil.s3;
});

Expand Down
2 changes: 1 addition & 1 deletion tests/functional/aws-node-sdk/test/object/deleteObject.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ describe('DELETE object', () => {
const bucketName = 'testdeleteobjectlockbucket';
let versionIdOne;
let versionIdTwo;
const retainDate = moment().add(10, 'days');
const retainDate = moment().add(10, 'days').toDate();

before(async () => {
try {
Expand Down
14 changes: 1 addition & 13 deletions tests/functional/aws-node-sdk/test/object/get.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -224,18 +224,6 @@ describe('GET object', () => {
await s3.send(new DeleteBucketCommand({ Bucket: bucketName }));
});

Copy link

@claude claude bot Mar 16, 2026

Choose a reason for hiding this comment

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

The test for empty bucket name was removed, but the equivalent test in head.js was kept and changed to assert a 405 HTTP status. Was this removal intentional? If the newer SDK now sends the request to the server (instead of rejecting client-side), this test should be updated like head.js rather than deleted, to retain coverage of the server's behavior on empty bucket names.

— Claude Code


it('should return an error to get request without a valid ' +
'bucket name',
done => {
s3.send(new GetObjectCommand({ Bucket: '', Key: 'somekey' })).then(() => {
assert.fail('Expected failure but got success');
}).catch(err => {
assert.strictEqual(err.message, 'Empty value provided for input HTTP label: Bucket.');
return done();
});
});

it('should return NoSuchKey error when no such object',
Copy link

@claude claude bot Mar 18, 2026

Choose a reason for hiding this comment

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

The test for GET object with empty bucket name was removed entirely. Unlike the HEAD bucket test (head.js) which was updated to check the new server-side error behavior, this test was just deleted. If the newer SDK now sends the request to the server instead of rejecting client-side, consider adding a replacement test that asserts the expected server response (as was done for HEAD).

— Claude Code

done => {
s3.send(new GetObjectCommand({ Bucket: bucketName, Key: 'nope' })).then(() => {
Expand Down Expand Up @@ -1073,7 +1061,7 @@ describeSkipIfCeph('GET object with object lock', () => {
const params = {
Bucket: bucket,
Key: key,
ObjectLockRetainUntilDate: mockDate,
ObjectLockRetainUntilDate: mockDate.toDate(),
ObjectLockMode: mockMode,
ObjectLockLegalHoldStatus: 'ON',
};
Expand Down
Loading
Loading