feat: Enable downgrading of documents via Lens inverses #1721
Conversation
AndrewSisley
added
feature
Codecov ReportPatch coverage:
@@ Coverage Diff @@
## develop #1721 +/- ##
===========================================
+ Coverage 75.36% 75.47% +0.11%
===========================================
Files 208 208
Lines 21765 21801 +36
===========================================
+ Hits 16402 16454 +52
+ Misses 4217 4202 -15
+ Partials 1146 1145 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 4 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
lens/lens.go
Outdated
| @@ -128,7 +130,18 @@ func (l *lens) Next() (bool, error) { | |||
| // up to the output via any intermediary pipes. | |||
| inputPipe = p | |||
| } else { | |||
| historyLocation := l.schemaVersionHistory[doc.SchemaVersionID] | |||
| historyLocation, ok := l.schemaVersionHistory[doc.SchemaVersionID] | |||
| if !ok { | |||
There was a problem hiding this comment.
question: In what situation would we have an unknown version (Maybe a test to demonstrate it would help visualise it)? And if there is such a situation, should we disallow it?
There was a problem hiding this comment.
This is already tested, for example by TestP2PPeerUpdateWithNewFieldSyncsDocsToOlderSchemaVersionMultistep. The commit message went into why this is changing now.
If the schema does not exist locally, and no migrations have been registered for the version, and a document of that version is synced via P2P, then ok will be false
There was a problem hiding this comment.
The commit message didn't help me understand this sorry.
If the result of TestP2PPeerUpdateWithNewFieldSyncsDocsToOlderSchemaVersionMultistep is the same with and without this change, then it's hard to know that this is being tested and that it's even needed (I understand why it's needed conceptually).
There was a problem hiding this comment.
The commit message didn't help me understand this sorry.
Ah okay - I'll try add more next time :P It is a non-obvious change, essentially pre-empting the next commit breaking things by exposing this issue
AndrewSisley
force-pushed
the
1719-lens-downgrade
branch
from
625ddb9 to
6891713
Compare
Lol, the Rust code isn't really important to review, is just an example/test lens. Is worth looking at the func signature though, as it show-cases that the inverse is optional, and lens devs don't actually need to declare them |
…k#1721) ## Relevant issue(s) Resolves sourcenetwork#1719 ## Description Enables downgrading of documents via Lens inverses. Inverse migrations are optional.
Relevant issue(s)
Resolves #1719
Description
Enables downgrading of documents via Lens inverses. Inverse migrations are optional.
This PR is based off of #1720 - please review only the last 2 commits here.
There are so far fewer tests than I thought there would be, but I am a bit tired atm, and I may add more whilst waiting on review. Please suggest any where you think there may be gaps.
This IMO breaches the code freeze, but it could be very nice to get it in the release if everyone is happy.Discussed in standup, this should not be merged before the release.