Skip to content

[forward_port] Verify checksums command#31082

Merged
PVince81 merged 3 commits into
masterfrom
verify_checksums_command_forward_port
Apr 11, 2018
Merged

[forward_port] Verify checksums command#31082
PVince81 merged 3 commits into
masterfrom
verify_checksums_command_forward_port

Conversation

@IljaN

@IljaN IljaN commented Apr 11, 2018

Copy link
Copy Markdown
Contributor

Forward port of #31008

@IljaN IljaN requested review from DeepDiver1975 and PVince81 April 11, 2018 10:47
@IljaN IljaN changed the title Verify checksums command forward port [forward_port] Verify checksums command Apr 11, 2018
@IljaN IljaN self-assigned this Apr 11, 2018
@codecov

codecov Bot commented Apr 11, 2018

Copy link
Copy Markdown

Codecov Report

Merging #31082 into master will increase coverage by 0.03%.
The diff coverage is 85.05%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #31082      +/-   ##
============================================
+ Coverage     62.47%    62.5%   +0.03%     
- Complexity    18237    18268      +31     
============================================
  Files          1145     1146       +1     
  Lines         68313    68397      +84     
  Branches       1234     1234              
============================================
+ Hits          42678    42753      +75     
- Misses        25274    25283       +9     
  Partials        361      361
Flag Coverage Δ Complexity Δ
#javascript 52.03% <ø> (ø) 0 <ø> (ø) ⬇️
#phpunit 63.7% <85.05%> (+0.03%) 18268 <26> (+31) ⬆️
Impacted Files Coverage Δ Complexity Δ
lib/private/Files/Storage/Wrapper/Checksum.php 100% <100%> (ø) 23 <0> (ø) ⬇️
lib/private/Files/Cache/Scanner.php 86.47% <100%> (+0.19%) 114 <0> (+3) ⬆️
lib/private/Files/ObjectStore/NoopScanner.php 61.53% <50%> (-5.13%) 5 <0> (+2)
apps/files/lib/Command/VerifyChecksums.php 85.71% <85.71%> (ø) 26 <26> (?)
lib/private/Files/Node/Node.php 86.76% <0%> (+1.47%) 62% <0%> (ø) ⬇️
lib/private/Files/Node/File.php 95.65% <0%> (+2.89%) 32% <0%> (ø) ⬇️

Continue to review full report at Codecov.

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

@PVince81 PVince81 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

@PVince81 PVince81 merged commit 2897421 into master Apr 11, 2018
@PVince81 PVince81 deleted the verify_checksums_command_forward_port branch April 11, 2018 12:52
return rtrim($checksumString);
}

public function testFilePutContentsClearsChecksum() {

@mrow4a mrow4a May 6, 2018

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hey @IljaN @PVince81 you removed that unit test, but did you cover that logic with another unit test? Where do you unit test condition to clear the checksum?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The checksum must not be cleared anymore, the checksum wrapper recalculates the new checksum on file_put_contents. The above tests is from the old checksum impl. where the client just sent some checksum and it got written to the db without any checks.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@lock

lock Bot commented Jul 31, 2019

Copy link
Copy Markdown

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock Bot locked as resolved and limited conversation to collaborators Jul 31, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants