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

fix: Commit log special key bug #2115

Merged
merged 16 commits into from
Oct 16, 2024
Merged

fix: Commit log special key bug #2115

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
3ea9883
fix: special key bug fix
murali-shris Oct 8, 2024
e42136a
fix: pubspec change to test persistence in server
murali-shris Oct 8, 2024
9ddbacb
fix: sync unit test assertion change
murali-shris Oct 8, 2024
1a09a6d
fix: changes to publish persistence
murali-shris Oct 9, 2024
ee4edd8
Merge branch 'trunk' into commit_log_special_key_bug
murali-shris Oct 9, 2024
719ab2b
fix: add at_commons dependency overrides
murali-shris Oct 15, 2024
494d497
Merge branch 'commit_log_special_key_bug' of https://github.com/atsig…
murali-shris Oct 15, 2024
011c9f0
fix: add at_commons dependency overrides
murali-shris Oct 15, 2024
17190dd
fix: added test and doc for _acceptKey
murali-shris Oct 15, 2024
0411139
Merge branch 'trunk' into commit_log_special_key_bug
murali-shris Oct 15, 2024
498c51c
fix: modified pubspec and change
murali-shris Oct 15, 2024
f1df357
Merge branch 'commit_log_special_key_bug' of https://github.com/atsig…
murali-shris Oct 15, 2024
69ec2c2
fix: added test for public key without namespace
murali-shris Oct 16, 2024
c22e236
Merge branch 'trunk' into commit_log_special_key_bug
murali-shris Oct 16, 2024
3ea497f
fix: rename methods
murali-shris Oct 16, 2024
e670ee8
Merge branch 'commit_log_special_key_bug' of https://github.com/atsig…
murali-shris Oct 16, 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
2 changes: 2 additions & 0 deletions packages/at_persistence_secondary_server/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
## 3.0.65
- fix: Modified check for public encryption key in _specialKey method in commit log keystore
## 3.0.64
- build[deps]: Upgraded the following packages:
- at_commons to v5.0.0
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 @@ -220,7 +220,7 @@ class CommitLogKeyStore extends BaseCommitLogKeyStore {

bool _isSpecialKey(String atKey) {
return atKey.contains(AtConstants.atEncryptionSharedKey) ||
Copy link
Contributor

@gkc gkc Oct 9, 2024 •
edited by murali-shris
Loading

Choose a reason for hiding this comment

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

this contains AtConstants.atEncryptionSharedKey check is also a bug. Why are we treating keys that contain "shared_key" as special?

And if we are treating them as special for good reason, we should specifically only match shared_key\..*@this_atservers_at_sign or @some_other_atsign:shared_key@this_servers_at_sign (precise regexes are in at_commons in Regexes._reservedKeysWithAtsignSuffix) ... currently this code would also match keys like

"@bob:key1.shared_keys.buzz@alice"

Copy link
Contributor

Choose a reason for hiding this comment

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

@murali-shris I don't see a good explanation for why we are not treating the two reserved shared_key variants as 'special' which (as I understand it) would result in those keys not being synced?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh - ignore that last comment. I understand it now. This ensures that those keys are synced, regardless of whether they match the enrollment namespace or not.

atKey.startsWith('public:') ||
atKey.startsWith(AtConstants.atEncryptionPublicKey) ||
atKey.contains(AtConstants.atPkamSignature) ||
atKey.contains(AtConstants.atSigningPrivateKey);
}
Expand Down Expand Up @@ -564,7 +564,8 @@ class CommitLogCache {
if (existingCommitId != null &&
commitEntry.commitId != null &&
existingCommitId > commitEntry.commitId!) {
_logger.shout('Ignoring commit entry update to cache. existingCommitId: $existingCommitId | toUpdateWithCommitId: ${commitEntry.commitId}');
_logger.shout(
'Ignoring commit entry update to cache. existingCommitId: $existingCommitId | toUpdateWithCommitId: ${commitEntry.commitId}');
return;
}
_updateCacheLog(key, commitEntry);
Expand Down
2 changes: 1 addition & 1 deletion packages/at_persistence_secondary_server/pubspec.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
name: at_persistence_secondary_server
description: A Dart library with the implementation classes for the persistence layer of the secondary server.
version: 3.0.64
version: 3.0.65
repository: https://github.com/atsign-foundation/at_server
homepage: https://docs.atsign.com/

Expand Down
36 changes: 36 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 @@ -250,6 +250,42 @@ void main() async {
expect(commitId, 0);
expect(commitLogInstance?.lastCommittedSequenceNumber(), 0);
});
test('test to verify last committed sequenceNumber with regex', () async {
var commitLogInstance =
await (AtCommitLogManagerImpl.getInstance().getCommitLog('@alice'));
await commitLogInstance?.commit(
'public:location_1.wavi@alice', CommitOp.UPDATE);
await commitLogInstance?.commit(
'public:phone.buzz@alice', CommitOp.UPDATE);
await commitLogInstance?.commit(
'public:location_2.wavi@alice', CommitOp.UPDATE);
await commitLogInstance?.commit(
'public:email.buzz@alice', CommitOp.UPDATE);
expect(
await commitLogInstance
?.lastCommittedSequenceNumberWithRegex('buzz'),
3);
expect(
await commitLogInstance
?.lastCommittedSequenceNumberWithRegex('wavi'),
2);
await commitLogInstance?.commit(
'public:location_1.wavi@alice', CommitOp.UPDATE);
await commitLogInstance?.commit(
'public:location_2.wavi@alice', CommitOp.DELETE);
await commitLogInstance?.commit(
'public:phone.buzz@alice', CommitOp.DELETE);
await commitLogInstance?.commit(
'public:email.buzz@alice', CommitOp.DELETE);
expect(
await commitLogInstance
?.lastCommittedSequenceNumberWithRegex('buzz'),
7);
expect(
await commitLogInstance
?.lastCommittedSequenceNumberWithRegex('wavi'),
5);
});
});
group('A group of commit log compaction tests', () {
setUp(() async => await setUpFunc(storageDir));
Expand Down
5 changes: 5 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,11 @@ dependencies:
yaml: 3.1.2
logging: 1.2.0

#TODO replace with published version
dependency_overrides:
at_persistence_secondary_server:
path: ../at_persistence_secondary_server

dev_dependencies:
build_runner: ^2.3.3
test: ^1.24.4
Expand Down
13 changes: 4 additions & 9 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 @@ -491,17 +491,12 @@ void main() {

List syncResponse = jsonDecode(response.data!);

expect(syncResponse.length, 2);
// As per design, all the public keys are not filtered when matching with regex.
expect(syncResponse[0]['atKey'], 'public:country.wavi@alice');
expect(syncResponse[0]['commitId'], 1);
expect(syncResponse.length, 1);

expect(syncResponse[0]['atKey'], 'firstname.buzz@alice');
expect(syncResponse[0]['commitId'], 3);
expect(syncResponse[0]['operation'], '+');
expect(syncResponse[0]['metadata']['version'], '0');

expect(syncResponse[1]['atKey'], 'firstname.buzz@alice');
expect(syncResponse[1]['commitId'], 3);
expect(syncResponse[1]['operation'], '+');
expect(syncResponse[1]['metadata']['version'], '0');
});
test('test to verify sync response does not exceed the buffer limit',
() async {
Expand Down