search

sourcenetwork / defradb

DefraDB is a Peer-to-Peer Edge Database. It's the core data storage system for the Source Network Ecosystem, built with IPLD, LibP2P, CRDTs, and Semantic open web properties.
446 stars 44 forks source link

Change detector tests do not assert that priority is consistent across db versions #1036

Open AndrewSisley opened 1 year ago

AndrewSisley commented 1 year ago

The change detector recently accidentally caught a breaking change in https://github.com/sourcenetwork/defradb/pull/1031

The way priority was calculated on set of an initial record changed, causing initial priorities of fields on new objects to change from 2=>1. This was only caught due to the presence of another bug.

Changing the way priority is calculated and stored could be a breaking change (it was in this case I believe). However it is not currently explicitly tested for.

We should add tests that would deliberately catch this kind of issue.

Alternatively look at https://github.com/sourcenetwork/defradb/issues/1038 and make it impossible for this to be a problem.

jsimnz commented 1 year ago

We should add tests that explicitly test priority. I think this can/should be done by adding commit tests that fetch the priority/height field (also an existing test gap).

There are current tests that check the priority (referred to as height in the query tests) within the integration suite. Part of the problem was that we were storing priority in three different places.

Once in the blockstore as delta-state encoded blocks, again in the data store, and again in the headstore. The planner reports the number from the blockstore, while the CRDT Merge functions are based on the datastore/headstore. The problem Fred uncovered was that the system wasn't accounting for the +1 difference between the priority in the blockstore vs datastore vs headstore.

This is certainly a breaking change, but not caught as you noted (except because of another bug). But as you asked, simply adding priority to the integration tests isn't sufficient (since we do have this).

What I propose instead is to add a consistency check to ensure that all the priority values for a doc/field between all 3 stores is consistent (equal) with itself (all report a single value, and these value is again consistent after updates/mutations, both from the collection API and query API).

Thanks for surfacing this!

AndrewSisley commented 1 year ago

three

:sweat_smile:

AndrewSisley commented 1 year ago

related: https://github.com/sourcenetwork/defradb/issues/1038

AndrewSisley commented 1 year ago

What I propose instead is to add a consistency check to ensure that all the priority values for a doc/field between all 3 stores is consistent (equal) with itself (all report a single value, and these value is again consistent after updates/mutations, both from the collection API and query API).

Cheers - Fred just let me know this and I created https://github.com/sourcenetwork/defradb/issues/1038 as a result. I really don't think we should store this val in multiple locations - this issue will just happen again and we can catch it well before any tests get run by just not storing it in 3 places (no need to check they are all in sync either).