Override an unversioned object originOp when moving it to a version #6036
Conversation
This prevents triggering a possible s3:ObjectCreated:Put bucket notification when moving this object to a version, when deleting it (when creating a deletion marker). Issue: CLDSRV-816 Signed-off-by: Thomas Flament <[email protected]>
Hello delthas,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
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 |
There was a problem hiding this comment.
Pull request overview
This PR prevents triggering unintended s3:ObjectCreated:Put bucket notifications when moving an unversioned object to a versioned state during deletion (when creating a deletion marker). The change sets a specific originOp value to distinguish this internal operation from user-initiated PUT operations.
- Sets
originOpto's3:ObjectNullVersionStoredAsNewVersion'when storing null version metadata
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files
@@ Coverage Diff @@
## development/9.0 #6036 +/- ##
================================================
Coverage 83.38% 83.38%
================================================
Files 189 189
Lines 12199 12200 +1
================================================
+ Hits 10172 10173 +1
Misses 2027 2027
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
|
Tested locally and with the full workflow on an ARTESCA, working for both manual deletion and expiration workflows. |
| }); | ||
| }); | ||
|
|
||
| it('should set originOp when moving null version', done => { |
There was a problem hiding this comment.
I think it's worth it to document more either this test or the one in objectDelete.js.
Here you added a comment "M was moved to V", I understand it because I read the Jira ticket where you give a lot of context, but someone else probably wouldn't understand.
This new StoreNullVersion doesn't look very documented either, so this test could be a an opportunity to write a few line about the bug that happened before and why we added StoreNullVersion to fix it
There was a problem hiding this comment.
This new StoreNullVersion doesn't look very documented either, so this test could be a an opportunity to write a few line about the bug that happened before and why we added StoreNullVersion to fix it
this is not a bug, this is a "known" corner-case when moving from the "master-only bucket format" (non-versioned) to the "versioned bucket format". And this is documented, in docs/ARCHITECTURE.rst, in section "Preserving Existing Null Versions in Versioning-Enabled Bucket".
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
| }); | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: remove extra empty line
| url: `/${bucketName}/${objectKey}`, | ||
| }); | ||
|
|
||
| it('should store modified originOp when moving object null version', done => { |
There was a problem hiding this comment.
maybe better describe the scenario, to explain this happens when putting a delete marker on top of a "master-only null version"
| it('should store modified originOp when moving object null version', done => { | |
| it('should store modified originOp when storing null object due to object marker', done => { |
...or we could make this more general, and consider this test actually tests this overall process of "upgrading" the null-version object (in case an object marker is put on top of it) : i.e. we do not just want to verify that the object has the proper originOp, but also that it has all proper metadata and "null" version id (which is mostly what you do here, actually)
| isNull: true, | ||
| originOp: 's3:StoreNullVersion', | ||
| }), any, any, any); | ||
| }, |
There was a problem hiding this comment.
just like the other test, I guess we should also check that the second call created a delete marker with the expected originOp (s3:ObjectRemoved:DeleteMarkerCreated)
| }), any, any, any); | ||
| }, | ||
| async () => { | ||
| // New V was created with the right originOp. |
There was a problem hiding this comment.
| // New V was created with the right originOp. | |
| // New version document was created with the right originOp. |
| next => bucketPutVersioning(authInfo, enableVersioningRequest, log, next), | ||
| next => objectPut(authInfo, testPutObjectRequest, undefined, log, next), | ||
| async () => { | ||
| // M was moved to V, with originOp overridden to prevent bucket notifications. |
There was a problem hiding this comment.
| // M was moved to V, with originOp overridden to prevent bucket notifications. | |
| // Old master-only document was moved to a proper version, with originOp overridden to prevent unexpected bucket notifications. |
This prevents triggering a possible s3:ObjectCreated:Put bucket notification when moving this object to a version, when deleting it (when creating a deletion marker).
Issue: CLDSRV-816