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

Ensure coms-id tags buried in history are used when soft-deleted #198

Merged
merged 3 commits into from
Aug 21, 2023

Conversation

jujaga
Copy link
Member

@jujaga jujaga commented Aug 18, 2023

Description

This PR focuses on making sure that when objects are soft deleted that their previous coms-id values are still used during synchronization where appropriate.

  • 50f62e4 Implement _deriveObjectId utility synchronize method
  • 58cfed0 Ensure that isAtPath utility recognizes files at root level as valid
  • aff9799 Drop partial implementation of all queryparam from syncBucket controller

SHOWCASE-3317

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)

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

@jujaga jujaga added bug Something isn't working enhancement New feature or request labels Aug 18, 2023
@jujaga jujaga requested a review from wilwong89 August 18, 2023 00:14
@jujaga jujaga self-assigned this Aug 18, 2023
@jujaga jujaga changed the base branch from master to release/sync August 18, 2023 00:14
app/src/services/sync.js Show resolved Hide resolved
app/src/services/sync.js Show resolved Hide resolved
@codeclimate
Copy link

codeclimate bot commented Aug 18, 2023

Code Climate has analyzed commit a12fb5b and detected 54 issues on this pull request.

Here's the issue category breakdown:

Category Count
Complexity 19
Duplication 35

The test coverage on the diff in this pull request is 22.1% (50% is the threshold).

This pull request will bring the total coverage in the repository to 53.0% (-5.5% change).

View more on Code Climate.

@github-actions
Copy link

Coverage Report

Totals Coverage
Statements: 46.42% ( 2133 / 4595 )
Methods: 37.74% ( 234 / 620 )
Lines: 53.05% ( 1338 / 2522 )
Branches: 38.61% ( 561 / 1453 )

The concept of requesting the entire COMS instance to update everything
all at once would require a full inspection of all objects and buckets.
This will be deferred for now.

Signed-off-by: Jeremy Ho <jujaga@gmail.com>
Root level files and folders behave differently than standard "folders" as
they are technically in the path when there are no preceeding slashes. This
should resolve the side issue where listAllObjectVersions was not usable.

Signed-off-by: Jeremy Ho <jujaga@gmail.com>
This method focuses on attempting to acquire a previously known 'coms-id'
tag. If it can procedurally find one in the version history, it will yield
that uuid; otherwise a new one will be generated. This is expected to only
be used when creating new COMS Object DB records. A 'coms-id' will be
written back to S3 in the event this is newly assigned.

Signed-off-by: Jeremy Ho <jujaga@gmail.com>
@github-actions
Copy link

Copy link
Contributor

@TimCsaky TimCsaky left a comment

Choose a reason for hiding this comment

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

some good optimizations here

@@ -81,7 +127,8 @@ const service = {
response[0].versions.find(v => v.id === version.id).metadata = metadata;
}
}
log.verbose(`Finished syncing ${path} in bucket ${bucketId}`, { function: 'syncJob', result: response });

log.verbose(`Finished syncing ${path} in bucket ${bucketId}`, { function: 'syncJob', result: response });
return response;
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't actually expose this response to the api user.. but it seems like a nice thing to have (a nested array of object>version?meta/tags) for logging.

* @returns {object} synced objects that exist in COMS and S3 eg:
* <ObjectModel> (synced object)
* or `undefined` (when object was pruned from COMS db after sync)
* @returns {Promise<Array<object | undefined>>} Either an array of synced objects or undefined
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 this syncObject method returns just one js object
@returns {Promise<object | undefined>}

let versions = [];
// IF this is a new (or existing) object in COMS DB (from step 1)
// OR doing 'full' sync AND object wasn't deleted
Copy link
Contributor

Choose a reason for hiding this comment

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

some kind of comments here explaining the logic are useful. we could add them later.
I think the current general behaviour is that without full mode, we only sync versions and meta/tags for new objects

@TimCsaky TimCsaky merged commit 9097ef0 into release/sync Aug 21, 2023
8 checks passed
@jujaga jujaga deleted the feature/softcomsid branch August 23, 2023 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants