Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: file size in cache is set back to zero if empty content is writt… #39016

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

fix: file size in cache is set back to zero if empty content is writt… #39016

wants to merge 2 commits into from
+33 −4

Conversation

DeepDiver1975
Copy link
Member

…en to a non-empty file

Description

When writing zero length content to a non-empty file the file size in the file cache was not updated.

How Has This Been Tested?

see provided test

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Database schema changes (next release will require increase of minor version instead of patch)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Technical debt
  • Tests only (no source changes)

Checklist:

  • Code changes
  • Unit tests added
  • Acceptance tests added
  • Documentation ticket raised:
  • Changelog item, see TEMPLATE

@update-docs
Copy link

update-docs bot commented Jul 16, 2021

Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes.

@DeepDiver1975 DeepDiver1975 self-assigned this Jul 16, 2021
@DeepDiver1975 DeepDiver1975 force-pushed the fix/update-cache-size-when-emptying-a-file branch from 7634ce8 to 53196ce Compare July 16, 2021 10:47
@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/31293/2/7
make test-php-style fails.

@DeepDiver1975 DeepDiver1975 force-pushed the fix/update-cache-size-when-emptying-a-file branch from 53196ce to aeca425 Compare July 16, 2021 11:19
@phil-davis
Copy link
Contributor

Only the PHP unit tests on Oracle failed:
https://drone.owncloud.com/owncloud/core/31304/19/9

There were 1508 errors:

1) Test\Cache\FileCacheTest::testGarbageCollectOldKeys
Doctrine\DBAL\Exception\DriverException: An exception occurred while executing 'UPDATE "oc_filecache" SET "mtime" = NULL, "storage_mtime" = NULL WHERE ("mtime" IS NOT NULL OR "storage_mtime" IS NOT NULL) AND "fileid" = ?' with params [4]:

ORA-01407: cannot update ("AUTOTEST"."oc_filecache"."mtime") to NULL

/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php:69
/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:182
/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:159
/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:2212
/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1302
/drone/src/lib/private/DB/Connection.php:191
/drone/src/lib/private/Files/Cache/Cache.php:388
/drone/src/lib/private/Files/Cache/Updater.php:247
/drone/src/lib/private/Files/Cache/Updater.php:160
/drone/src/lib/private/Files/View.php:342
/drone/src/lib/private/Files/View.php:1239
/drone/src/lib/private/Files/View.php:776
/drone/src/lib/public/Events/EventEmitterTrait.php:50
/drone/src/lib/private/Files/View.php:786
/drone/src/lib/private/Cache/File.php:160
/drone/src/tests/lib/Cache/TestCache.php:19
/drone/src/tests/lib/Cache/FileCacheTest.php:91

Caused by
Doctrine\DBAL\Driver\OCI8\OCI8Exception: ORA-01407: cannot update ("AUTOTEST"."oc_filecache"."mtime") to NULL

/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Exception.php:25
/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Driver/OCI8/OCI8Statement.php:409
/drone/src/lib/composer/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:1290
/drone/src/lib/private/DB/Connection.php:191
/drone/src/lib/private/Files/Cache/Cache.php:388
/drone/src/lib/private/Files/Cache/Updater.php:247
/drone/src/lib/private/Files/Cache/Updater.php:160
/drone/src/lib/private/Files/View.php:342
/drone/src/lib/private/Files/View.php:1239
/drone/src/lib/private/Files/View.php:776
/drone/src/lib/public/Events/EventEmitterTrait.php:50
/drone/src/lib/private/Files/View.php:786
/drone/src/lib/private/Cache/File.php:160
/drone/src/tests/lib/Cache/TestCache.php:19
/drone/src/tests/lib/Cache/FileCacheTest.php:91

2) Test\Cache\FileCacheTest::testGarbageCollectLeaveRecentKeys
Exception: Missing tearDown call in Test\Cache\FileCacheTest

/drone/src/tests/lib/Traits/UserTrait.php:52
/drone/src/tests/lib/TestCase.php:127
/drone/src/tests/lib/Cache/FileCacheTest.php:55

3) Test\Cache\FileCacheTest::testGarbageCollectIgnoreLockedKeys with data set #0 (OCP\Lock\LockedException Object (...))
Exception: Missing tearDown call in Test\Cache\FileCacheTest

/drone/src/tests/lib/Traits/UserTrait.php:52
/drone/src/tests/lib/TestCase.php:127
/drone/src/tests/lib/Cache/FileCacheTest.php:55
...

The first fail might be real?

But then it complains many times about Exception: Missing tearDown call in Test\Cache\FileCacheTest

I restarted CI so we can see if the fail is "variable".

@phil-davis
Copy link
Contributor

phil-davis commented Jul 16, 2021 •
edited
Loading

@DeepDiver1975 I used current master and tried overwriting a file with an empty file, then downloading it. The download comes back empty, which is correct. So this problem does not seem to have an externally-observable bad behavior.

Is that correct? This just fixes an internal cache problem?

@DeepDiver1975
Copy link
Member Author

This just fixes and internal cache problem?

This is a pure PHP API fix from my understanding - it was discovered while working on some other app which used these functions that way ....

@AlexAndBear AlexAndBear self-requested a review July 19, 2021 08:04
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline apiFederationToRoot1-git-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/31469/107/1

@DeepDiver1975 DeepDiver1975 force-pushed the fix/update-cache-size-when-emptying-a-file branch from aeca425 to 99801e4 Compare July 26, 2021 09:00
@sonarcloud
Copy link

sonarcloud bot commented Jul 26, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@phil-davis
Copy link
Contributor

https://drone.owncloud.com/owncloud/core/31515/36/9
"something happened" (tm) in unit tests with Oracle.

@DeepDiver1975
Copy link
Member Author

https://drone.owncloud.com/owncloud/core/31515/36/9
"something happened" (tm) in unit tests with Oracle.

🤷

@DeepDiver1975
Copy link
Member Author

Screenshot from 2021-07-26 13-03-40

@DeepDiver1975
Copy link
Member Author

looks like I need to fire up oracle locally one more time ....

@DeepDiver1975 DeepDiver1975 force-pushed the fix/update-cache-size-when-emptying-a-file branch from 99801e4 to 5fb41b0 Compare September 15, 2021 08:30
@ownclouders
Copy link
Contributor

💥 Acceptance tests pipeline webUISharingPublic1-chrome-mariadb10.2-php7.4 failed. The build has been cancelled.

https://drone.owncloud.com/owncloud/core/32396/144/1

@DeepDiver1975 DeepDiver1975 force-pushed the fix/update-cache-size-when-emptying-a-file branch from 5d74544 to d293089 Compare September 15, 2021 09:59
@sonarcloud
Copy link

sonarcloud bot commented Sep 15, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

50.0% 50.0% Coverage
0.0% 0.0% Duplication

@butonic
Copy link
Member

butonic commented Jul 26, 2022

@DeepDiver1975 merga master or rebase to kick ci again?

@DeepDiver1975 DeepDiver1975 force-pushed the fix/update-cache-size-when-emptying-a-file branch from d293089 to a269c4a Compare November 8, 2023 11:59
@DeepDiver1975
Copy link
Member Author

🙈

There were 5 failures:

1) Test\Files\Cache\UpdaterTest::testMove
Failed asserting that 0 matches expected 1699446390.

/drone/src/tests/lib/Files/Cache/UpdaterTest.php:147
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:106

2) Test\Files\Cache\UpdaterTest::testMoveNonExistingOverwrite
Failed asserting that 0 matches expected 1699446390.

/drone/src/tests/lib/Files/Cache/UpdaterTest.php:165
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:106

3) Test\Files\Cache\UpdaterTest::testUpdateStorageMTime
file mtime preserved
Failed asserting that 0 matches expected 1699446390.

/drone/src/tests/lib/Files/Cache/UpdaterTest.php:200
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:106

4) Test\Files\Cache\UpdaterTest::testMoveCrossStorage
Failed asserting that 0 matches expected 1699446390.

/drone/src/tests/lib/Files/Cache/UpdaterTest.php:246
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:106

5) Test\Files\Cache\UpdaterTest::testMoveFolderCrossStorage
Failed asserting that 0 matches expected 1699446390.

/drone/src/tests/lib/Files/Cache/UpdaterTest.php:300
phpvfscomposer:///drone/src/lib/composer/phpunit/phpunit/phpunit:106

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexAndBear AlexAndBear AlexAndBear approved these changes

@jvillafanez jvillafanez jvillafanez approved these changes

Assignees

@DeepDiver1975 DeepDiver1975

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@DeepDiver1975 @phil-davis @ownclouders @butonic @jvillafanez @AlexAndBear