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.

Sign up for GitHub

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

Jump to bottom

feat: Sync skip deletes <do not merge> #2131

Open
wants to merge 13 commits into
base: trunk
Choose a base branch
from
Open

feat: Sync skip deletes <do not merge> #2131

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
e577b31
feat:skipDeletes server changes
murali-shris Oct 8, 2024
2187883
Merge branch 'trunk' of https://github.com/atsign-foundation/at_serve…
murali-shris Oct 17, 2024
1a05096
fix: pubspec add at_commons dependency overrides
murali-shris Oct 17, 2024
5714380
fix: replace skipDeletes check with strategy pattern
murali-shris Oct 17, 2024
e9a6356
fix: added dart docs
murali-shris Oct 22, 2024
6d706a7
Merge branch 'trunk' into sync_skip_deletes
murali-shris Oct 22, 2024
0c53be2
fix: skip delete until server changes and unit tests
murali-shris Nov 5, 2024
832f142
feat: add skip delete unit test
murali-shris Nov 5, 2024
b5bac00
Merge branch 'sync_skip_deletes' of https://github.com/atsign-foundat…
murali-shris Nov 5, 2024
38b11da
fix: added regex scenario in sync unit test
murali-shris Nov 5, 2024
e949f00
feat: added unit tests in sync for skip deletes
murali-shris Nov 7, 2024
2b9285b
fix: rename skipDeletes to skipDeletes until
murali-shris Nov 7, 2024
8e3f568
Merge branch 'trunk' into sync_skip_deletes
murali-shris Nov 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions packages/at_persistence_secondary_server/lib/src/log/commitlog/at_commit_log.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,12 +127,13 @@ class AtCommitLog extends BaseAtCommitLog {
/// Returns the Iterator of [_commitLogCacheMap] from the commitId specified.
@server
Iterator<MapEntry<String, CommitEntry>> getEntries(int commitId,
{String? regex, int limit = 25}) {
{String? regex, int limit = 25, bool skipDeletes = false}) {
Copy link
Member

@VJag VJag Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can make this a strategy.. where FetchAllKeysStrategy is to get all and SkipDeletesStrategy is to skip deletes, instead of if-else code might look more readable.

// If regex is null or isEmpty set regex to match all keys
if (regex == null || regex.isEmpty) {
regex = '.*';
}
return _commitLogKeyStore.getEntries(commitId, regex: regex, limit: limit);
return _commitLogKeyStore.getEntries(commitId,
regex: regex, limit: limit, skipDeletes: skipDeletes);
}

Future<void> _publishChangeEvent(CommitEntry commitEntry) async {
Expand Down
5 changes: 3 additions & 2 deletions packages/at_persistence_secondary_server/lib/src/log/commitlog/commit_log_keystore.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -238,13 +238,14 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {

/// Returns the Iterator of entries as Key value pairs after the given the [commitId] for the keys that matches the [regex]
Iterator<MapEntry<String, CommitEntry>> getEntries(int commitId,
{String regex = '.*', int limit = 25}) {
{String regex = '.*', int limit = 25, bool skipDeletes = false}) {
Iterable<MapEntry<String, CommitEntry>> commitEntriesIterable =
commitLogCache
.entriesList()
.where((element) =>
element.value.commitId! >= commitId &&
_shouldIncludeKeyInSyncResponse(element.value.atKey!, regex))
_shouldIncludeKeyInSyncResponse(element.value.atKey!, regex) &&
(!skipDeletes || element.value.operation != CommitOp.DELETE))
.take(limit);
return commitEntriesIterable.iterator;
}
Expand Down
27 changes: 27 additions & 0 deletions packages/at_persistence_secondary_server/test/commit_log_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -840,6 +840,33 @@ void main() async {
expect(commitEntriesMap.containsKey('public:phone.wavi@alice'), false);
expect(commitEntriesMap.containsKey('public:location@alice'), true);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add another test which both uses a regex and sets skipDeletes: true

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

test(
'A test to verify delete commit entries are NOT returned when skipDeletes is true',
() async {
var commitLogInstance =
await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
var commitLogKeystore = commitLogInstance!.commitLogKeyStore;
await commitLogKeystore.add(CommitEntry(
'test_key_true_1@alice', CommitOp.UPDATE, DateTime.now()));
await commitLogKeystore.add(CommitEntry(
'test_key_true_2@alice', CommitOp.DELETE, DateTime.now()));
await commitLogKeystore.add(CommitEntry(
'test_key_true_3@alice', CommitOp.DELETE, DateTime.now()));
await commitLogKeystore.add(CommitEntry(
'test_key_true_4@alice', CommitOp.UPDATE, DateTime.now()));
Iterator<MapEntry<String, CommitEntry>>? changes = commitLogInstance
.commitLogKeyStore
.getEntries(-1, skipDeletes: true);
Map<String?, CommitEntry> commitEntriesMap = {};
while (changes.moveNext()) {
var commitEntry = changes.current.value;
commitEntriesMap[commitEntry.atKey] = commitEntry;
}
expect(commitEntriesMap.containsKey('test_key_true_1@alice'), true);
expect(commitEntriesMap.containsKey('test_key_true_2@alice'), false);
expect(commitEntriesMap.containsKey('test_key_true_3@alice'), false);
expect(commitEntriesMap.containsKey('test_key_true_4@alice'), true);
});
});
tearDown(() async => await tearDownFunc());
});
Expand Down
3 changes: 2 additions & 1 deletion packages/at_secondary_server/lib/src/verb/handler/sync_progressive_verb_handler.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ class SyncProgressiveVerbHandler extends AbstractVerbHandler {
// Get entries to sync
var commitEntryIterator = atCommitLog!.getEntries(
int.parse(verbParams[AtConstants.fromCommitSequence]!) + 1,
regex: verbParams['regex']);
regex: verbParams['regex'],
skipDeletes: verbParams['skipDeletes'] == 'true');
Copy link
Member

@VJag VJag Oct 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

VerbSyntax also needs this parameter, otherwise regex validation would fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. those changes are in at_commons
https://github.com/atsign-foundation/at_libraries/pull/696/files


List<KeyStoreEntry> syncResponse = [];
await prepareResponse(capacity, syncResponse, commitEntryIterator,
Expand Down
9 changes: 9 additions & 0 deletions packages/at_secondary_server/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,15 @@ dependencies:
yaml: 3.1.2
logging: 1.2.0

dependency_overrides:
at_commons:
git:
url: https://github.com/atsign-foundation/at_libraries.git
path: packages/at_commons
ref: sync_skip_delete
at_persistence_secondary_server:
path: ../at_persistence_secondary_server

dev_dependencies:
build_runner: ^2.3.3
test: ^1.24.4
Expand Down
33 changes: 33 additions & 0 deletions packages/at_secondary_server/test/sync_unit_test.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -469,6 +469,39 @@ void main() {
expect(syncResponse[3]['operation'], '*');
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Please add another test which both uses a regex and sets skipDeletes: true

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

test(
'test to verify delete commit entries are not sent when skipDeletes is true',
() async {
await secondaryPersistenceStore!
.getSecondaryKeyStore()
?.put('test_key_1@alice', AtData()..data = 'alice');
await secondaryPersistenceStore!
.getSecondaryKeyStore()
?.remove('test_key_1@alice');
await secondaryPersistenceStore!
.getSecondaryKeyStore()
?.put('test_key_2@alice', AtData()..data = 'alice');
await secondaryPersistenceStore!
.getSecondaryKeyStore()
?.remove('test_key_2@alice');
var syncProgressiveVerbHandler = SyncProgressiveVerbHandler(
secondaryPersistenceStore!.getSecondaryKeyStore()!);
var response = Response();
var inBoundSessionId = '_6665436c-29ff-481b-8dc6-129e89199718';
var atConnection = InboundConnectionImpl(mockSocket, inBoundSessionId);
atConnection.metaData.isAuthenticated = true;
var syncVerbParams = HashMap<String, String>();
syncVerbParams.putIfAbsent(AtConstants.fromCommitSequence, () => '-1');
syncVerbParams.putIfAbsent('skipDeletes', () => 'true');
await syncProgressiveVerbHandler.processVerb(
response, syncVerbParams, atConnection);
List syncResponse = jsonDecode(response.data!);
for (var entry in syncResponse) {
expect(entry['atKey'] != 'test_key_1@alice', true);
expect(entry['atKey'] != 'test_key_2@alice', true);
}
});

test(
'test to verify only entries matching the regex are added to sync response',
() async {
Expand Down