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

Implement new validation spec #204

Merged
merged 127 commits into from
Aug 6, 2022
Merged

Implement new validation spec #204

merged 127 commits into from
Aug 6, 2022

Conversation

sandreae
Copy link
Member

@sandreae sandreae commented Jul 14, 2022

Implement validation flows for publish() and next_args() as specified here: https://whimsical.com/api-validation-4X53TdGvPLFUtmuBFvriW5

NOTES:

  • I am making sure publish and next_args only depend on having access to LogStore, EntryStore and OperationStore. This results in some slightly less efficient queries, but in removing a previous dependency on DocumentStore it means we no longer require documents to be materialised before being able to process new entries or providing next args. I think this is a good trade-off for now. It means these methods don't assume a certain materialisation flow and helps in separating concerns in the testing environment.
  • after this PR we can deprecate publish_entry and next_entry_args currently on StorageProvider in p2panda-rs )
  • the above means we can remove the request and response structs which are currently required by StorageProvider as well
  • nice side-effect of creating these validation and domain methods is that StorageProvider becomes very straightforward only containing simple insert and get actions, no complex logic.
  • apologies for for the 2000 odd lines of code... maybe 800 of those are just splitting the test utils into smaller modules....

THIS PR:

  • introduce validation module which contains modular validation methods (but not for operations / schema yet)
  • introduce domain module which contains publish() and next_args() high level API methods
  • write many unit tests 🧟
    • just need some tests for for different log states i think
  • split db/stores/test_utils.rs into smaller modules as it was getting massive
  • make validation, domain & db/store/test_utils/* module methods generic over storage provider implementation. This is sweeeeet. Everything works with SqlStorage or MemoryStore (from p2panda-rs). The immediate benefit is that we can use MemoryStore for tests, which is much easier to work with (read mess with than the SQL dbs). I'll try and separate this into it's own PR before completing this one.

NEXT UP:

  • also validate operations against their claimed schema
  • figure out best way to inject expected schema into the test db (as now we validate operations against their claimed schema 😱)
  • remove many deprecated methods from StorageProvider traits
  • move validation and domain to p2panda-rs (we need to do this when we deprecate publish_entry and next_entry_args on storage provider).

closes: p2panda/p2panda#400 p2panda/p2panda#301 #159

requires: p2panda/p2panda#413 & p2panda/p2panda#408

📋 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

@codecov
Copy link

codecov bot commented Jul 15, 2022

Codecov Report

Merging #204 (0eaf75c) into main (bbc55d5) will increase coverage by 0.61%.
The diff coverage is 94.83%.

❗ Current head 0eaf75c differs from pull request most recent head fd8a302. Consider uploading reports for the commit fd8a302 to get more accurate results

@@            Coverage Diff             @@
##             main     #204      +/-   ##
==========================================
+ Coverage   93.70%   94.32%   +0.61%     
==========================================
  Files          42       46       +4     
  Lines        3179     4086     +907     
==========================================
+ Hits         2979     3854     +875     
- Misses        200      232      +32     
Impacted Files Coverage Δ
aquadoggo/src/db/mod.rs 83.33% <ø> (ø)
aquadoggo/src/manager.rs 96.20% <ø> (ø)
aquadoggo/src/node.rs 0.00% <ø> (ø)
aquadoggo/src/materializer/worker.rs 86.04% <44.82%> (-4.41%) ⬇️
aquadoggo/src/materializer/tasks/schema.rs 87.95% <87.95%> (+87.95%) ⬆️
aquadoggo/src/test_helpers.rs 88.67% <88.23%> (-0.21%) ⬇️
aquadoggo/src/db/stores/test_utils/store.rs 91.30% <91.30%> (ø)
aquadoggo/src/materializer/tasks/reduce.rs 95.58% <91.30%> (+5.69%) ⬆️
aquadoggo/src/db/stores/log.rs 96.26% <93.33%> (-1.10%) ⬇️
aquadoggo/src/db/stores/test_utils/runner.rs 93.33% <93.33%> (ø)
... and 27 more

Help us with your feedback. Take ten seconds to tell us how you rate us.

@cafca cafca mentioned this pull request Jul 31, 2022
7 tasks
Copy link
Member

@adzialocha adzialocha left a comment

Choose a reason for hiding this comment

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

Super good! I've added only some minor change request, otherwise it's good to go from my side 👍

aquadoggo/Cargo.toml Outdated Show resolved Hide resolved
aquadoggo/src/graphql/client/mutation.rs Outdated Show resolved Hide resolved
aquadoggo/src/test_helpers.rs Show resolved Hide resolved
cafca and others added 5 commits August 6, 2022 18:53
* Refactor `get_expected_skiplink()`

Matches current behavior of `skiplink_seq_num()` method

* Remove import

* Remove comment

* Fix tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment