✨ CLDSRV-812: ListObjectsV2 create x-amz-optional-attributes header#6033
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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files
@@ Coverage Diff @@
## development/9.1 #6033 +/- ##
===================================================
+ Coverage 83.80% 83.81% +0.01%
===================================================
Files 191 191
Lines 12319 12328 +9
===================================================
+ Hits 10324 10333 +9
Misses 1995 1995
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
|
tests/unit/api/bucketGet.js
Outdated
| }); | ||
|
|
||
| it('should ignore the missing permission if the header contains only RestoreStatus', done => { | ||
| const testGetRequest = Object.assign({ |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
The idea of tests is to tests each "path" of the code. Here we have 2 differents cases (coming from product) that I'm trying to reflect in the code. If you have the permission, then you can ask for user metadata, if not, you should be able to request only RestoreStatus. So even the result is the same, the code path is not the same and we test this condition if (requestedAttributes.filter(attr => attr != 'RestoreStatus').length > 0) {
There was a problem hiding this comment.
Testing the different code path is fine (i.e. RestoreStatus vs x-amz-meta-department)
My issue is that the test mentions permissions, which is not tested at all in this test : i.e. we do not verify that the right permissions/actions were requested, use some mocks, [...] to ensure that the custom permission is not required in case only RestoreStatus is present...
| (err, result) => { | ||
| assert.strictEqual(err, null); | ||
| assert.strictEqual(result.ListBucketResult.$.xmlns, 'http://s3.amazonaws.com/doc/2006-03-01/'); | ||
| done(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
No, in next PR I create new test as the logic tested is different and a test should test only one thing. Even if the result is the same, the logic tested here is to make sure that with several attributes, the GET is working and nothing break
There was a problem hiding this comment.
it does not test GET is working (only that it does not crash), as it does not validate which permissions are requested
There was a problem hiding this comment.
I'll add some spy to be sure then 👍
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 optionalAttributesHeader = request.headers['x-amz-optional-object-attributes']; | ||
| const requestedAttributes = optionalAttributesHeader ? | ||
| optionalAttributesHeader.split(',').map( | ||
| header => header !== 'RestoreStatus' ? header.toLowerCase() : header |
There was a problem hiding this comment.
If I understand correctly, the reason why RestoreStatus is not lower-cased is because of AWS standard ? why not leaving the other params the same ? Because we store them lower-cased ?
There was a problem hiding this comment.
for what I understand, you added this since user metadata are expected to be lower case in AWS (did not check, but I think I saw you mention this). In that case,
- what happens (on AWS) if an upper case metadata is used? It is rejected, just "converted" to lower case?
- how does our API behave? If
- Does this apply to the prefix (
x-amz-meta-) as well? e.g. if we only consider lower case prefix, no need to convert to lower case for filtering user-metadata properties...
(overall, trying to say that we don't need to be too defensive here: if spec & API both only support lower user-metadata, no need to spend cycle trying to accomodate, we can just be strict and expect user to behave correctly)
There was a problem hiding this comment.
Indeed, user metadata are lowercase. AWS will convert a non lowercase usermetadata to a lowercase (we are doing the same). And yes, this apply to the prefix also.
lib/api/bucketGet.js
Outdated
| request.headers['x-amz-optional-object-attributes']?.split(',').map(attr => attr.trim()) ?? []; | ||
| if (optionalAttributes.some(attr => !attr.startsWith('x-amz-meta-') && attr != 'RestoreStatus')) { | ||
| return callback( | ||
| errorInstances.InvalidArgument.customizeDescription('Invalid header x-amz-optional-object-attributes') |
There was a problem hiding this comment.
Is this the official aws error ? If so we can leave it, but if its a custom one that you choose, why not add the value of the problematic value
There was a problem hiding this comment.
Good catch, the error returned by AWS is Invalid attribute name specified so I guess I must do the same 👏
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 |
Issue: CLDSRV-813
DarkIsDude
force-pushed
the
feature/CLDSRV-812/list-objects-v2-optional-permissions
branch
from
9872996 to
ccaf400
Compare
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 |
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.2/feature/CLDSRV-812/list-objects-v2-optional-permissions origin/development/9.2
git merge origin/feature/CLDSRV-812/list-objects-v2-optional-permissions
# <intense conflict resolution>
git commit
git push -u origin w/9.2/feature/CLDSRV-812/list-objects-v2-optional-permissionsThe following options are set: approve |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.3/feature/CLDSRV-812/list-objects-v2-optional-permissions origin/development/9.3
git merge origin/w/9.2/feature/CLDSRV-812/list-objects-v2-optional-permissions
# <intense conflict resolution>
git commit
git push -u origin w/9.3/feature/CLDSRV-812/list-objects-v2-optional-permissionsThe following options are set: approve |
|
/create_integration_branches |
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.3/feature/CLDSRV-812/list-objects-v2-optional-permissions origin/development/9.3
git merge origin/w/9.2/feature/CLDSRV-812/list-objects-v2-optional-permissions
# <intense conflict resolution>
git commit
git push -u origin w/9.3/feature/CLDSRV-812/list-objects-v2-optional-permissionsThe following options are set: approve, create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_integration_branches |
|
/create_pull_requests |
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
Follow integration pull requests if you would like to be notified of The following options are set: create_pull_requests, create_integration_branches |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
The following options are set: create_pull_requests, create_integration_branches |
|
/approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue CLDSRV-812. Goodbye darkisdude. The following options are set: approve, create_pull_requests, create_integration_branches |
bert-e
deleted the
feature/CLDSRV-812/list-objects-v2-optional-permissions
branch
4 participants
Pull request template
Description
Support a new header
x-amz-object-optional-attributesto 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
ListObjectsV2and return asked fields.