Return user metadata in GetObjectAttributes API

[maeldonn]

Issue: CLDSRV-817

[codecov]

❌ 4 Tests Failed:

Tests completed	Failed	Passed	Skipped
8221	4	8217	0

View the full list of 4 ❄️ flaky test(s)

"after each" hook for "should put an object and set the acl via query param"::PUT object With v4 signature "after each" hook for "should put an object and set the acl via query param"

Flake rate in main: 100.00% (Passed 0 times, Failed 18 times)

Stack Traces | 0.013s run time

Expected values to be strictly equal:
+ actual - expected

+ '404 NOT FOUND'
- '200 OK'

"after each" hook for "should put an object and set the acl via query param"::PUT object With v4 signature "after each" hook for "should put an object and set the acl via query param"

Flake rate in main: 100.00% (Passed 0 times, Failed 18 times)

Stack Traces | 0.013s run time

done() called multiple times in hook <PUT object With v4 signature "after each" hook for "should put an object and set the acl via query param"> of file .../test/object/put.js

"after each" hook for "should put object with valid object lock retention date and mode when object lock is enabled on the bucket"::PUT object with object lock With default signature "after each" hook for "should put object with valid object lock retention date and mode when object lock is enabled on the bucket"

Flake rate in main: 100.00% (Passed 0 times, Failed 18 times)

Stack Traces | 0.005s run time

The specified bucket does not exist.

"before each" hook for "should put object with valid object lock retention date and mode when object lock is enabled on the bucket"::PUT object with object lock With default signature "before each" hook for "should put object with valid object lock retention date and mode when object lock is enabled on the bucket"

Flake rate in main: 100.00% (Passed 0 times, Failed 18 times)

Stack Traces | 0.006s run time

The requested bucket name is not available. The bucket namespace is shared by all users of the system. Please select a different name and try again.

To view more test analytics, go to the Test Analytics Dashboard
_{📋 Got 3 mins? Take this short survey to help us improve Test Analytics.}

[SylvainSenechal]

wait so it means even if the attribute is not in supportedAttribute, it will be returned as lower cased ?
Shouldn't this be a filter instead ?

I mean, ok I guess the overall function works because later there is the attributes.some() and has() thing, but could it be clearer

[SylvainSenechal]

Anyways, maybe the funciton is good enough as it is, but if you're able to make it a bit clearer it's nice, cause it's doing multiple thing that aren't completely obvious from the function name, it looks like it's gonna do some generic parsing, then ends up doing lowercasing and having a special case for x-amz-meta. Maybe renaming function name and intermediates variables help

[maeldonn]

If it's not a supported attribute:

1. It's user metadata so it's ok
2. It's something else -> will throw an exception

[francoisferrand]

this seems uselessly innefficient: we traverse the list 3 times to do the same thing...

• trim() can be done in the same iteration, no need to the extra strings/list allocation
• isn't user metadata already mandated (by spec) to be lower case? do we do the same lower case conversion in other APIs? if not, not need to be more accommodating here
• if an attribute is in the supportedAttributes, there is no point making another check for the prefix (and rechecking later it is a supported attribute, if there is no prefix)

Suggested change

	.map(attr => (supportedAttributes.has(attr) ? attr : attr.toLowerCase())) ?? [];
	const attributes =
	headers[headerName]?.split(',').map(attr => {
	const res = attr.trim())
	if (!supportedAttributes.has(res) && !res.startsWith('x-amz-meta-')) {
	throw errorInstances.InvalidArgument.customizeDescription('Invalid attribute name specified.');
	}
	return res;
	});

[SylvainSenechal]

some comments to address, 2 tests to add

[francoisferrand]

this seems uselessly innefficient: we traverse the list 3 times to do the same thing...

• trim() can be done in the same iteration, no need to the extra strings/list allocation
• isn't user metadata already mandated (by spec) to be lower case? do we do the same lower case conversion in other APIs? if not, not need to be more accommodating here
• if an attribute is in the supportedAttributes, there is no point making another check for the prefix (and rechecking later it is a supported attribute, if there is no prefix)

Suggested change

	.map(attr => (supportedAttributes.has(attr) ? attr : attr.toLowerCase())) ?? [];
	const attributes =
	headers[headerName]?.split(',').map(attr => {
	const res = attr.trim())
	if (!supportedAttributes.has(res) && !res.startsWith('x-amz-meta-')) {
	throw errorInstances.InvalidArgument.customizeDescription('Invalid attribute name specified.');
	}
	return res;
	});

[benzekrimaha]

couple comments but nothing blocking for me , approving

[francoisferrand]

the previous code was done purposely for efficiency to avoid repeatedly iterating through every attribute : not really an improvement here...

esp. ideally we want to reuse the whole logic (i.e. same single pass iteration), not just extractUserMetadata. Especially we don't really care if extra attributes are supported in this "generation" stage, as the "supported" attributes will be validated earlier when receiving the API (some the optionalAttributes should only contain the allowed parameters.

(but this requires following the same pattern as all APIs, and generate XML manually instead of using xml2js.Builder)

[maeldonn]

Reverted to the listObjectV2 implementation and created an utility function for the two APIs.