Keep null in getMetaData in Checksum storage wrapper #29739

PVince81 · 2017-12-01T10:18:55Z

Description

Keep null in getMetaData in Checksum storage wrapper.

Not funny is that in PHP if you add an item into null as if it was an array, it suddenly becomes one...

Related Issue

None raised. Discovered while debugging background scanner with size=-1 and I noticed that getMetaData() on a non-existing file returns not null but a partial array.

Motivation and Context

How Has This Been Tested?

Not tested yet.
Why is there no unit test class for the Checksum storage wrapper ?!! @IljaN

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
All new and existing tests passed.

TODO

write unit test (well, write the whole class as it's missing)
think about what was broken before this fix
think about what could break after this fix

mrow4a · 2017-12-05T13:23:35Z

Sounds like missing unit test somewhere :> Nice catch.

PVince81 · 2017-12-11T15:50:24Z

@IljaN can you take over and add the missing tests ? Somehow it seems we missed checking for tests back in the checksum PR.

There are already some example for storage wrapper unit tests.

IljaN · 2017-12-13T14:21:12Z

Checksums are covered by acceptance tests. But a UnitTest won`t hurt :)

mmattel · 2017-12-14T11:37:28Z

For the record, tested and fixes the referenced issue #29806 (comment)

Seems that tests need a restart so we can go for a green and merge...

IljaN · 2017-12-14T11:40:53Z

@mmattel Thanks! I am in the process of adding some unit-tests. So no manual restart needed.

IljaN · 2017-12-14T17:39:51Z

We are running in to this exception in a versioning test with this fix:

core/lib/private/Files/Cache/Scanner.php

Lines 117 to 120 in 639986a

    
           if ($this->storage->instanceOfStorage('\OCP\Files\IHomeStorage') && !$this->storage->instanceOfStorage('OCA\Files_Sharing\ISharedStorage') && ($path === '' || $path === 'files')) { 
        
           	\OCP\Util::writeLog('OC\Files\Cache\Scanner', 'Missing important folder "' . $path . '" in home storage!!! - ' . $this->storageId, \OCP\Util::ERROR); 
        
           	throw new \OCP\Files\StorageNotAvailableException('Missing important folder "' . $path . '" in home storage - ' . $this->storageId); 
        
           }

1) OCA\Files_Versions\Tests\VersioningTest::testRenameInSharedFolder
OCP\Files\StorageNotAvailableException: Missing important folder "files" in home storage - home::test-versions-user2

/home/ilja/code/core2/lib/private/Files/Cache/Scanner.php:119
/home/ilja/code/core2/lib/private/Files/Cache/Scanner.php:150
/home/ilja/code/core2/lib/private/Files/Cache/Watcher.php:106
/home/ilja/code/core2/lib/private/Files/View.php:1299
/home/ilja/code/core2/lib/private/Files/View.php:1335
/home/ilja/code/core2/lib/private/Files/Node/Root.php:188
/home/ilja/code/core2/lib/private/Files/Node/Folder.php:129
/home/ilja/code/core2/lib/private/Files/Node/Root.php:367
/home/ilja/code/core2/lib/private/Server.php:970
/home/ilja/code/core2/apps/files_versions/tests/VersioningTest.php:725
/home/ilja/code/core2/apps/files_versions/tests/VersioningTest.php:330

PVince81 · 2017-12-15T08:30:25Z

@IljaN if you're testing on actual FS, make sure that the "data/$userId/files" folder of the user exists as if the user had logged in before. Maybe some of previous running tests delete this and this test here expected it to exist without proper initialization. (happened before...)

IljaN · 2017-12-15T11:16:39Z

Yeah this test is actually quite brittle, runs just once locally and then throws user already exists error, will try to fix it

IljaN · 2017-12-15T15:17:40Z

@PVince81 you mentioned that there are some filesystem conditions which can cause null meta-data, who is in the know?

If null can happen we either must refactor so that getMetaData never returns null and all codepaths (Scanner?) can work with an emtpy array. There are ~16 Usages of get MetaData

Or we need to change the docs for the storage interface to allow null:

core/lib/private/Files/Storage/Storage.php

Lines 92 to 95 in f10d105

    
           	 * @param string $path 
        
           	 * @return array 
        
           	 */ 
        
           	public function getMetaData($path);

Should we create a follow up issue?

PVince81 · 2017-12-15T15:20:12Z

@IljaN the only way I found currently to reproduce this issue was having a NFD-formatted file name on an external storage and running occ files:scan. Not sure about code paths, maybe something to look into separately, considering that this PR here already fixes this issue.

IljaN · 2017-12-15T16:01:44Z

@PVince81 Not sure If I was clear enough: return null here fixes the issue:

core/lib/private/Files/Storage/Wrapper/Checksum.php

Line 193 in fdd04d6

return null;

...but violates storage interface getMetaData (should return array).

PVince81 · 2017-12-15T16:04:16Z

Hmm... but it seems the underlying storage does return null, or is it returning an empty array ? Maybe just return whatever the underlying storage gave you...

IljaN · 2017-12-15T16:10:36Z

Yeah there are actually a few other sites which handle null from getMetaData calls, but I consider this a bug/incorrect implementation because the interface states that it should return array unless we change the doc.

IljaN · 2017-12-15T16:11:15Z

Example from above:

core/lib/private/Files/Cache/Scanner.php

Lines 117 to 120 in 639986a

    
           if ($this->storage->instanceOfStorage('\OCP\Files\IHomeStorage') && !$this->storage->instanceOfStorage('OCA\Files_Sharing\ISharedStorage') && ($path === '' || $path === 'files')) { 
        
           	\OCP\Util::writeLog('OC\Files\Cache\Scanner', 'Missing important folder "' . $path . '" in home storage!!! - ' . $this->storageId, \OCP\Util::ERROR); 
        
           	throw new \OCP\Files\StorageNotAvailableException('Missing important folder "' . $path . '" in home storage - ' . $this->storageId); 
        
           }

IljaN · 2017-12-15T16:13:55Z

Maybe some deeper layer returns null and this is propagated up so I would assume that the deeper layer should return an array, or the first wrapper which encounters null should pass an empty array up to the next wrapper.

PVince81 · 2017-12-15T16:23:01Z

I suggest we look into this separately... maybe adjust the interfaces for null then. I think null is a valid value for not found metadata. Even better of course would be exceptions, but that would likely break a lot of existing code. We'll likely rework all this mess as part of storage API rework.

codecov · 2017-12-16T00:30:56Z

Codecov Report

Merging #29739 into master will increase coverage by <.01%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master   #29739      +/-   ##
============================================
+ Coverage     60.52%   60.52%   +<.01%     
- Complexity    18380    18382       +2     
============================================
  Files          1090     1090              
  Lines         60949    60953       +4     
============================================
+ Hits          36888    36892       +4     
  Misses        24061    24061

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Files/Storage/Wrapper/Checksum.php	`100% <100%> (ø)`	`23 <0> (+1)`	⬆️
lib/private/legacy/app.php	`59.25% <0%> (-0.24%)`	`198% <0%> (+1%)`
lib/private/Files/Cache/Propagator.php	`98.7% <0%> (+1.29%)`	`16% <0%> (ø)`	⬇️
apps/files_sharing/lib/Scanner.php	`90% <0%> (+5%)`	`7% <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 25ba394...760a938. Read the comment docs.

PVince81 · 2017-12-18T09:09:34Z

👍 for the unit tests.

PVince81 · 2017-12-18T09:34:09Z

backport conflict with versions stuff, might be better to wait for backport of https://github.com/owncloud/core/pull/29257/files#diff-5ad1bbf2fd6f06e61ebecc59cd61a983L130

cc @DeepDiver1975

PVince81 · 2018-01-08T14:47:54Z

@DeepDiver1975 ping on this blocker, we can't backport without this.

Or well we could backport only half of this and the rest later. Let me know.

DeepDiver1975 · 2018-01-08T14:49:51Z

Lets chat in about two days again - I should have an understanding of the situation by then ;-)

PVince81 · 2018-01-29T09:22:14Z

I made a backport of only the fix without the unit tests as the latter are blocked (see previous comments).

PR here #30302

PVince81 · 2018-01-29T09:22:28Z

backport unit tests when ready

PVince81 · 2018-02-09T13:17:39Z

Now #30302 is a full backport of this + adjustments to make it work with stable10

lock · 2019-08-01T04:02:35Z

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.

PVince81 added 2 - Developing backport-request labels Dec 1, 2017

PVince81 added this to the development milestone Dec 1, 2017

PVince81 self-assigned this Dec 1, 2017

PVince81 requested review from IljaN and mrow4a December 1, 2017 10:18

PVince81 assigned IljaN and unassigned PVince81 Dec 11, 2017

PVince81 mentioned this pull request Dec 12, 2017

Error running files:scan --all --repair #29806

Closed

This was referenced Dec 14, 2017

External file scanning error #28181

Closed

Bring back NFD encoding warning when scanning #29845

Closed

Vincent Petry and others added 3 commits December 15, 2017 16:01

Keep null in getMetaData in Checksum storage wrapper

fdd04d6

unit-test checksum wrapper

ac24816

Improve test isolation by generating random user names for each test

760a938

IljaN force-pushed the keep-null-metadata branch from 4cfd6a8 to 760a938 Compare December 15, 2017 15:01

IljaN mentioned this pull request Dec 15, 2017

Gracefully ignore files with wrong permissions during filescan #28960

Closed

PVince81 merged commit 79bb290 into master Dec 18, 2017

PVince81 deleted the keep-null-metadata branch December 18, 2017 09:09

mmattel mentioned this pull request Dec 28, 2017

Accented characters in filenames cause weird errors during scan #29985

Closed

PVince81 mentioned this pull request Jan 29, 2018

[stable10] Keep null in getMetaData in Checksum storage wrapper #30302

Merged

PVince81 removed the backport-request label Feb 9, 2018

lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Keep null in getMetaData in Checksum storage wrapper #29739

Keep null in getMetaData in Checksum storage wrapper #29739

PVince81 commented Dec 1, 2017 •

edited by IljaN

Loading

mrow4a commented Dec 5, 2017 •

edited

Loading

PVince81 commented Dec 11, 2017

IljaN commented Dec 13, 2017

mmattel commented Dec 14, 2017

IljaN commented Dec 14, 2017

IljaN commented Dec 14, 2017 •

edited

Loading

PVince81 commented Dec 15, 2017

IljaN commented Dec 15, 2017

IljaN commented Dec 15, 2017 •

edited

Loading

PVince81 commented Dec 15, 2017

IljaN commented Dec 15, 2017 •

edited

Loading

PVince81 commented Dec 15, 2017

IljaN commented Dec 15, 2017 •

edited

Loading

IljaN commented Dec 15, 2017

IljaN commented Dec 15, 2017

PVince81 commented Dec 15, 2017

codecov bot commented Dec 16, 2017 •

edited

Loading

PVince81 commented Dec 18, 2017

PVince81 commented Dec 18, 2017

PVince81 commented Jan 8, 2018

DeepDiver1975 commented Jan 8, 2018

PVince81 commented Jan 29, 2018

PVince81 commented Jan 29, 2018 •

edited

Loading

PVince81 commented Feb 9, 2018

lock bot commented Aug 1, 2019

Keep null in getMetaData in Checksum storage wrapper #29739

Keep null in getMetaData in Checksum storage wrapper #29739

Conversation

PVince81 commented Dec 1, 2017 • edited by IljaN Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

TODO

mrow4a commented Dec 5, 2017 • edited Loading

PVince81 commented Dec 11, 2017

IljaN commented Dec 13, 2017

mmattel commented Dec 14, 2017

IljaN commented Dec 14, 2017

IljaN commented Dec 14, 2017 • edited Loading

PVince81 commented Dec 15, 2017

IljaN commented Dec 15, 2017

IljaN commented Dec 15, 2017 • edited Loading

PVince81 commented Dec 15, 2017

IljaN commented Dec 15, 2017 • edited Loading

PVince81 commented Dec 15, 2017

IljaN commented Dec 15, 2017 • edited Loading

IljaN commented Dec 15, 2017

IljaN commented Dec 15, 2017

PVince81 commented Dec 15, 2017

codecov bot commented Dec 16, 2017 • edited Loading

Codecov Report

PVince81 commented Dec 18, 2017

PVince81 commented Dec 18, 2017

PVince81 commented Jan 8, 2018

DeepDiver1975 commented Jan 8, 2018

PVince81 commented Jan 29, 2018

PVince81 commented Jan 29, 2018 • edited Loading

PVince81 commented Feb 9, 2018

lock bot commented Aug 1, 2019

PVince81 commented Dec 1, 2017 •

edited by IljaN

Loading

mrow4a commented Dec 5, 2017 •

edited

Loading

IljaN commented Dec 14, 2017 •

edited

Loading

IljaN commented Dec 15, 2017 •

edited

Loading

IljaN commented Dec 15, 2017 •

edited

Loading

IljaN commented Dec 15, 2017 •

edited

Loading

codecov bot commented Dec 16, 2017 •

edited

Loading

PVince81 commented Jan 29, 2018 •

edited

Loading