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

Refactor replication API #184

Merged
merged 29 commits into from
Jul 1, 2022
Merged

Refactor replication API #184

merged 29 commits into from
Jul 1, 2022

Conversation

adzialocha
Copy link
Member

@adzialocha adzialocha commented Jun 30, 2022

This PR refactors the Replication API:

  • Removes concept of aliased authors: Its a good idea but at this point a too complex optimization. Also it probably needs a moment in the boiling hot pot of the p2panda specification process 🐈
  • Moves EntryArgsRequest and PublishEntryRequest into db module as it is more a struct used to query the database than an actual GraphQL thing
  • Consistent access of Context: ReplicationContext has been removed in favor of accessing already existing SqlStorage
  • Replaces graphql-client with simpler gql-client, removes need of external .graphql files, bin generator and auto-generated types
  • Removes last test using automock by replacing it with rstest mocking approach
  • Updates GraphQL field and params names according to the latest specification
  • Moves scalars out of replication as they should be used both by client and replication
  • Serde methods to represent LogId and SeqNum as strings are moved directly next to the regarding scalar implementations
  • Added some docstrings and GraphQL descriptions
  • Added some more tests
  • Updated dependencies and removed unused ones

Depends on #137

Closing #180 and #182 and #120

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@adzialocha adzialocha changed the base branch from main to graphql-replication-client June 30, 2022 22:10
@codecov
Copy link

codecov bot commented Jun 30, 2022

Codecov Report

Merging #184 (1da94dc) into main (c60dd2e) will increase coverage by 3.01%.
The diff coverage is 88.92%.

❗ Current head 1da94dc differs from pull request most recent head 464716f. Consider uploading reports for the commit 464716f to get more accurate results

@@            Coverage Diff             @@
##             main     #184      +/-   ##
==========================================
+ Coverage   90.58%   93.60%   +3.01%     
==========================================
  Files          43       42       -1     
  Lines        2986     3142     +156     
==========================================
+ Hits         2705     2941     +236     
+ Misses        281      201      -80     
Impacted Files Coverage Δ
aquadoggo/src/db/mod.rs 83.33% <ø> (ø)
aquadoggo/src/db/provider.rs 87.50% <ø> (ø)
aquadoggo/src/db/stores/operation.rs 91.15% <ø> (ø)
aquadoggo/src/node.rs 0.00% <ø> (ø)
aquadoggo_cli/src/main.rs 0.00% <0.00%> (ø)
aquadoggo/src/graphql/scalars/document_id.rs 14.28% <14.28%> (ø)
aquadoggo/src/graphql/scalars/encoded_entry.rs 71.42% <71.42%> (ø)
aquadoggo/src/graphql/scalars/public_key.rs 71.42% <71.42%> (ø)
aquadoggo/src/graphql/scalars/entry_hash.rs 81.81% <81.81%> (ø)
aquadoggo/src/replication/service.rs 82.60% <82.60%> (ø)
... and 27 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ec3ff36...464716f. Read the comment docs.

Copy link
Member

@sandreae sandreae left a comment

Choose a reason for hiding this comment

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

This looks great to me, nice refactoring 🌟

I like entry_encoded => entry and operation_encoded => operation a LOT!

I had a couple of comments, one of which I have fix with a commit. I'll raise issues for the other two.

.map(|entry_and_operation| {
// Unwrap as the entry was already validated before it entered the database
//
// @TODO: Is there a more efficient way than decoding the entry to access
Copy link
Member

Choose a reason for hiding this comment

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

Yes! You can use entry_and_operation.seq_num() ;-p

Right now, that actually just decodes the entry under-the-hood... but that is because we have a rather inefficient implementation of AsStorageEntry which we can improve later with a small amount of effort.

Comment on lines +254 to +258
// The test runner creates a test entry for us, we can retreive the public key from the
// author
let public_key: Author = db
.test_data
.key_pairs
.first()
.unwrap()
.public_key()
.to_owned()
.try_into()
.unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

This is so ugly... 🤣 I think we need some more conversion traits for Author.

@adzialocha adzialocha changed the base branch from graphql-replication-client to main July 1, 2022 13:26
@adzialocha adzialocha merged commit 05c23f0 into main Jul 1, 2022
@adzialocha adzialocha deleted the refactor-replication-api branch July 1, 2022 16:45
adzialocha added a commit that referenced this pull request Jul 1, 2022
* main:
  Update CHANGELOG.md
  Refactor replication API (#184)
  GraphQL replication client (#137)
  Update aquadoggo to v0.3.0 in aquadoggo_cli
  v0.3.0
  Prepare CHANGELOG for v0.3.0 release
  Commit Cargo.lock
  p2panda-rs v0.4.0
  Remove unused import
  Update naming in `publishEntry` query and `nextEntryArgs` response (#181)
  Fix bug when materialising documents containing (non-existent) pinned relations (#177)
  Logging in reduce task (#175)
  Refactor test db creation (#176)
adzialocha added a commit that referenced this pull request Jul 1, 2022
* schema-provider:
  Minor clean ups
  Move to unreleased section in CHANGELOG
  Update CHANGELOG.md
  Refactor replication API (#184)
  GraphQL replication client (#137)
  Update aquadoggo to v0.3.0 in aquadoggo_cli
  v0.3.0
  Prepare CHANGELOG for v0.3.0 release
  Commit Cargo.lock
  p2panda-rs v0.4.0
  Remove unused import
  Update naming in `publishEntry` query and `nextEntryArgs` response (#181)
  Fix bug when materialising documents containing (non-existent) pinned relations (#177)
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.

Remove mockall Use gql_client as GraphQL client crate Remove unused dependencies
2 participants