fix: Consistent version query semantics #3477
Conversation
jsimnz
added
bug
jsimnz
force-pushed
the
jsimnz/fix/consistent-version-query
branch
from
296b841 to
e6baea5
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #3477 +/- ##
===========================================
- Coverage 78.48% 78.47% -0.01%
===========================================
Files 397 397
Lines 37574 37599 +25
===========================================
+ Hits 29489 29504 +15
- Misses 6394 6402 +8
- Partials 1691 1693 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 16 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
| cid: "bafybeica4js2abwqjjrz7dcialbortbz32uxp7ufxu7yljbwvmhjqqxzny" | ||
| cid: "bafyreiale6qsjc7qewod3c6h2odwamfwcf7vt4zlqtw7ldcm57xdkgxja4" |
There was a problem hiding this comment.
just highlighting this for reviewers. This isnt required by the new changes to the commit query semantics. Its actually fixing a unconsequential bug which is using the incorrect CID for the goal of the test. The test wanted to test field IDs, but the original CID was a composite CID.
| @@ -20,7 +20,7 @@ import ( | |||
| // desired behaviour (should return all latest commits). | |||
| func TestQueryLatestCommits(t *testing.T) { | |||
| test := testUtils.TestCase{ | |||
| Description: "Simple latest commits query", | |||
| Description: "Simple latest commits queries", | |||
There was a problem hiding this comment.
question: why is this plural now? I see only one query request in the test. Also then test name itself has only Query in it.
There was a problem hiding this comment.
honestly, fantastic question 🙃. I just saw fred suggestion and went with it. Im thinking now that the s was there accidentally from me trying to press ctrl+s to save at some point and not registering the ctrl
There was a problem hiding this comment.
I assumed John was meaning to make it plural because there were multiple queries in the test (Git change was not showing the full test and just took it for granted)
The assumption cascade is strong with this one... lol
There was a problem hiding this comment.
Lol 😆 I looked at it three times, thinking am I missing something or tripping.
jsimnz
force-pushed
the
jsimnz/fix/consistent-version-query
branch
from
e078848 to
b63d4da
Compare
| @@ -0,0 +1,2 @@ | |||
| # Consistent version query semantics | |||
| Updated semantics for commit/version/time-travel queries when using both DocID and CID. | |||
There was a problem hiding this comment.
todo: There is no data breaking change in this PR, if the change detector failed it is because the integration test definitions were changed, not the data on disk. Please change the wording to reflect this.
Relevant issue(s)
Resolves #3476
Description
Fixes the consistency between regular queries and time-travel queries with respect to the
_versionsubquery. Before, the regular queries would return all commits from the start to the current, but time-travel queries only used a "latestCommit" query at the target version, which meant only returning a single commit entry.This PR implements some semantics changes to the overall commits API, but maintains all necessary backwards compatibility (however, it does introduce new errors when using invalid cid/docid combinations).
New semantics:
Unchanged semantics:
Technically, there is also an optimization where we no longer use the
HeadFetcherin the commit queries if aCIDis specified.Tasks
How has this been tested?
(replace) Describe the tests performed to verify the changes. Provide instructions to reproduce them.
Specify the platform(s) on which this was tested: