fix: Only update updated fields via update requests

[AndrewSisley]

Relevant issue(s)

Resolves #1811

Description

Only updates updated fields via update requests by returning a clean document from Decode.

How has this been tested?

I have tested this via #1816 (https://github.com/sourcenetwork/defradb/compare/develop...AndrewSisley:1816-mutation-equivilency?expand=1) however this is a very simple and quite important fix deserving of its own commit/change-log entry in develop, and the test changes are complex and may bog down the merging of the fix.

[fredcarle]

Can you please add an integration test that shows that this will result in the expected DAG state.

[AndrewSisley]

Can you please add an integration test that shows that this will result in the expected DAG state.

We have them, in the branch linked in the PR description.

[fredcarle]

We have them, in the branch linked in the PR description.

When I click on it I see no new tests. Nonetheless, we should test the changes that we make in the same PR as much as possible. Otherwise reviewers have to manually test the changed behaviour.

[AndrewSisley]

We have them, in the branch linked in the PR description.

When I click on it I see no new tests. Nonetheless, we should test the changes that we make in the same PR as much as possible. Otherwise reviewers have to manually test the changed behaviour.

It doesnt add any new tests. It allows all our existing tests to test this for us.

Otherwise reviewers have to manually test the changed behaviour.

That depends on how much reviewers trust the author to make a one line change, with the existing test gap provably closed in another branch.

[fredcarle]

That depends on how much reviewers trust the author to make a one line change, with the existing test gap provably closed in another branch.

The one line change doesn't obviously demonstrate the behaviour change. If I can't easily see what is happening in the code change, I have to have another way to confirm that the behaviour is as expected. In this case I had to manually run the queries.

[AndrewSisley]

That depends on how much reviewers trust the author to make a one line change, with the existing test gap provably closed in another branch.

The one line change doesn't obviously demonstrate the behaviour change. If I can't easily see what is happening in the code change, I have to have another way to confirm that the behaviour is as expected. In this case I had to manually run the queries.

So read the description of the change in the PR header, or read the wip branch linked in it if you have too little faith in the author to trust the description.

[codecov]

Codecov Report

Patch coverage: 100.00% and project coverage change: -0.03% ⚠️

Comparison is base (5240086) 75.79% compared to head (23a9f7e) 75.76%.

@@             Coverage Diff             @@
##           develop    #1817      +/-   ##
===========================================
- Coverage    75.79%   75.76%   -0.03%     
===========================================
  Files          209      209              
  Lines        22263    22268       +5     
===========================================
- Hits         16874    16871       -3     
- Misses        4225     4231       +6     
- Partials      1164     1166       +2     

Flag	Coverage Δ
all-tests	75.76% <100.00%> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed	Coverage Δ
db/fetcher/encoded_doc.go	77.22% <100.00%> (+1.54%)	⬆️

... and 3 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

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

[fredcarle]

Next time please consider adding the tests with the PR.

[jsimnz]

@AndrewSisley

We have them, in the branch linked in the PR description.

We actually don't really have them. I understand the benefit/goals of the linked branch running all mutations via both collection and GQL APIs, which I think is a nice and efficient way to ensure parity between the two APIs.

However, the linked branch doesn't add any tests that further gurantee the behavior of the original issue. It relies on the existing set of tests and runs those tests with the GQL mutation API. Unfortuently, the original tests only contain a single small test to check the issue, and its not even necessarily the core focus of the test (TestQueryCommitsWithDockeyAndUpdateAndLinks).

Based on how you've split the work, im very much OK to have the tests in the followup PR, but the tests in that branch need to be expanded to further cement coverage of the issue.

cc: @fredcarle

[AndrewSisley]

However, the linked branch doesn't add any tests that further gurantee the behavior of the original issue. It relies on the existing set of tests and runs those tests with the GQL mutation API. Unfortuently, the original tests only contain a single small test to check the issue, and its not even necessarily the core focus of the test (TestQueryCommitsWithDockeyAndUpdateAndLinks).

I believe you are mistaken here. 5 of our existing tests failed because of this bug, it is not just visible via links, but the field level commits.