Skip to content
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.

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

syncObject: delete 'coms-id' tag on new file if id already in use #289

Merged
merged 1 commit into from
Dec 5, 2024

Conversation

norrisng-bc
Copy link
Contributor

Description

When syncing, if a object not currently in COMS has a coms-id S3 tag with a value already in use inside COMS, delete the S3 tag and re-generate a new object ID before adding it to COMS.

This resolves a bug with renaming a file and syncing the folder it's in; COMS will mark the file as soft-deleted but fail to pick up the name change (thus resulting in a de-sync between COMS/S3). In BCBox, it will appear as if renaming the file and syncing it removes it.

https://apps.nrs.gov.bc.ca/int/jira/browse/SHOWCASE-3782

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read the CONTRIBUTING doc
  • I have checked that unit tests pass locally with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

Motivation

Please see Jira ticket for a more detailed explanation

S3 doesn't support file renaming; any attempt to do so (e.g. in GeoDrive or any external S3 tool/browser) is actually 2 operations:

  1. Create a copy of the existing file with the new name
  2. Delete the existing file

In (1), the new copy will include the same S3 tags and metadata as the original.

In (2), if the bucket has file versioning enabled, then it is a soft deletion. For COMS, this means that the old file still fully exists in its database.

When syncing, COMS will detect (2) as a new file and will try to add it to COMS, using the same coms-id it sees in its S3 tags. Attempting to re-use the same id will result in database errors, since it's the primary key for a COMS object.

Consequences on file version history

Unfortunately, the "new" file with the updated name will lose its version history, since it's held by the "old" copy. However, since it's only soft-deleted in a versioned bucket, the file history technically still exists; it's just that it's not associated with the renamed "active" file.

While it's probably technically possible (on some level) to have COMS "link" the renamed file to the old soft-deleted one, such that you'd get one continuous file history, this is probably overly complex for limited gain - the 2 files are completely separate entities in S3, so you'd have to hack together some novel non-S3 concept to have them linked.

Copy link

github-actions bot commented Nov 23, 2024

Coverage Report

Totals Coverage
Statements: 61.07% ( 3059 / 5009 )
Methods: 51.14% ( 337 / 659 )
Lines: 68.16% ( 1837 / 2695 )
Branches: 53.47% ( 885 / 1655 )

app/src/services/sync.js Outdated Show resolved Hide resolved
app/src/services/sync.js Outdated Show resolved Hide resolved
app/src/services/sync.js Outdated Show resolved Hide resolved
@norrisng-bc norrisng-bc force-pushed the bug/coms-id-tag-conflict branch 3 times, most recently from 78be18f to 8acd305 Compare November 29, 2024 01:57
if (TagSet.length < 10) { // putObjectTagging replaces S3 tags so new TagSet must contain existing values
} else {
// remove `coms-id` tag since it conflicts with an existing COMS object
TagSet.filter(x => x.Key != 'coms-id');
Copy link
Contributor

Choose a reason for hiding this comment

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

i think TagSet.filter() creates a copy but wont change the value of TagSet. you need to re-assign it. (eg: const filtered = Tagse..) and then use that const in the code below.
or you could change TagSet with: TagSet.splice(TagSet.findIndex(e => e.Key=== "coms-id"),1);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch - I've tweaked theTagSet declaration a little further up (it's now a variable instead of a constant) and made this line assign the filtered results (instead of just filter()'ing it and calling it a day).

@norrisng-bc norrisng-bc force-pushed the bug/coms-id-tag-conflict branch from 8acd305 to 4c0e413 Compare December 4, 2024 00:29
@norrisng-bc norrisng-bc force-pushed the bug/coms-id-tag-conflict branch from 4c0e413 to a7ef751 Compare December 4, 2024 23:55
@TimCsaky TimCsaky merged commit 30833d7 into master Dec 5, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants