Fix cleanup of orphaned properties #35407

skazi0 · 2019-06-03T08:08:13Z

Description

It was not working correctly when some property already had NULL fileid.
In such cases, fileid was cast to 0 and then used in a ... WHERE fileid in (0)
query which was not matching NULLs as expected.
Example of such property is {http://owncloud.org/ns}calendar-enabled.

Related Issue

Fixes Cron.php runs forever #35292

Motivation and Context

Fix for hanging cron.php

How Has This Been Tested?

Tested on a live installation.

Screenshots (if appropriate):

n/a

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:

Open tasks:

Backport (if applicable set "backport-request" label and remove when the backport was done)

CLAassistant · 2019-06-03T08:08:22Z

All committers have signed the CLA.

codecov · 2019-06-13T08:00:50Z

Codecov Report

Merging #35407 into master will decrease coverage by 34.18%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #35407       +/-   ##
=============================================
- Coverage     65.68%    31.5%   -34.19%     
=============================================
  Files          1221       48     -1173     
  Lines         70793     3282    -67511     
  Branches       1288        0     -1288     
=============================================
- Hits          46503     1034    -45469     
+ Misses        23913     2248    -21665     
+ Partials        377        0      -377

Flag	Coverage Δ	Complexity Δ
#javascript	`?`	`?`
#phpunit	`31.5% <ø> (-35.56%)`	`0 <ø> (-18735)`

Impacted Files	Coverage Δ	Complexity Δ
apps/files_external/lib/Lib/Storage/Swift.php	`0% <0%> (-66.17%)`	`0% <0%> (ø)`
lib/private/Files/Storage/DAV.php	`59.45% <0%> (-21.64%)`	`0% <0%> (ø)`
apps/files_external/lib/config.php	`31.57% <0%> (-3.43%)`	`0% <0%> (ø)`
apps/files_external/lib/Command/ListCommand.php	`67.66% <0%> (-0.94%)`	`0% <0%> (ø)`
apps/updatenotification/templates/admin.php
lib/private/Encryption/Keys/Storage.php
lib/private/App/CodeChecker/NodeVisitor.php
lib/private/RedisFactory.php
apps/dav/lib/Avatars/AvatarNode.php
...s/dav/appinfo/Migrations/Version20170202213905.php
... and 1166 more

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 ab8d8e9...66aadeb. Read the comment docs.

codecov · 2019-06-13T08:00:50Z

Codecov Report

Merging #35407 into master will decrease coverage by 0.61%.
The diff coverage is 79.54%.

@@             Coverage Diff              @@
##             master   #35407      +/-   ##
============================================
- Coverage     66.43%   65.81%   -0.62%     
+ Complexity    20183    18818    -1365     
============================================
  Files          1233     1228       -5     
  Lines         68965    70982    +2017     
  Branches          0     1289    +1289     
============================================
+ Hits          45814    46716     +902     
- Misses        23151    23888     +737     
- Partials          0      378     +378

Flag	Coverage Δ	Complexity Δ
#javascript	`53.7% <ø> (?)`	`0 <ø> (?)`
#phpunit	`67.19% <79.54%> (+0.76%)`	`18818 <1003> (-1365)`	⬇️

Impacted Files	Coverage Δ	Complexity Δ
apps/dav/lib/Upload/AssemblyStreamZsync.php	`88.37% <ø> (ø)`	`33 <0> (?)`
...s/dav/appinfo/Migrations/Version20170427182800.php	`0% <ø> (ø)`	`2 <0> (ø)`	⬇️
apps/dav/lib/Upload/ChunkLocationProvider.php	`100% <ø> (ø)`	`4 <0> (ø)`	⬇️
apps/dav/lib/Upload/FutureFile.php	`88.57% <ø> (ø)`	`16 <0> (ø)`	⬇️
apps/dav/lib/CardDAV/SyncJob.php	`0% <ø> (ø)`	`2 <0> (ø)`	⬇️
apps/dav/lib/Upload/FutureFileZsync.php	`83.33% <ø> (ø)`	`11 <0> (?)`
apps/dav/lib/CalDAV/Schedule/Plugin.php	`0% <ø> (ø)`	`2 <0> (ø)`	⬇️
...nnector/Sabre/Exception/PasswordLoginForbidden.php	`0% <ø> (ø)`	`2 <0> (ø)`	⬇️
...ib/SystemTag/SystemTagsObjectMappingCollection.php	`95.77% <ø> (ø)`	`28 <0> (ø)`	⬇️
...v/lib/SystemTag/SystemTagsObjectTypeCollection.php	`91.66% <ø> (ø)`	`12 <0> (ø)`	⬇️
... and 518 more

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 851bd2e...e684cc0. Read the comment docs.

micbar · 2019-07-09T19:01:00Z

@skazi0 What do you want to achieve?

We try to remove properties where no files exist in the filecache.
If you change the query, it would require a fileId.

skazi0 · 2019-07-09T20:32:36Z

@micbar I want to fix the looping problem (see linked issue). Probably my change breaks the functionality of this "orphan cleaner" feature while fixing the problem. I blindly assumed that while the code and comment don't match, the comment is the right version.
After a second look I think that the both queries are wrong.
In the problematic case current version does select * from oc_properties where fileid in (0) while fileid==NULL. MySQL can't compare NULL to 0 like that.
I think that the real solution is to exclude oc_properties with NULL fileid as they will always pop up in the search for NULLs on a left-joined query.
Give me a sec and I will update the PR.

DeepDiver1975 · 2019-07-30T07:25:45Z

As discussed in #35777 the master branch will from now on hold the ownCloud 10 codebase.

This PR targetted ownCloud 11 which is postponed to a far distant future.

Because of that I'm closing this PR and kindly ask you to re-submit this PR in a few days.

Thanks a lot for your patience

phil-davis · 2019-08-05T03:22:50Z

@skazi0 can you recreate this please?
GitHub reports "silly" conflicts because of the "new" master change.
Probably make a new branch and cherry-pick your commit then push and submit a new PR.

It was not working correctly when some property already had NULL fileid. In such cases, `fileid` was cast to 0 and then used in a `... WHERE fileid in (0)` query which was not matching NULLs as expected. Example of such property is `{http://owncloud.org/ns}calendar-enabled`.

skazi0 · 2019-08-05T06:07:01Z

@phil-davis @DeepDiver1975 yeah, I struggled a bit with rebasing but cherry-pick on top of "new" master seems to be correct.

sharidas · 2019-08-16T12:11:12Z

If the fileid is null then I am not sure how the properties will be used. This PR does not clean up the entry which has fileid null.

skazi0 · 2019-08-16T13:31:00Z

@sharidas This is what I have in my oc_properties:

MariaDB [owncloud]> select * from oc_properties;
+----+------------------------------------------+---------------+--------+
| id | propertyname                             | propertyvalue | fileid |
+----+------------------------------------------+---------------+--------+
|  2 | {http://owncloud.org/ns}calendar-enabled | 0             |   NULL |
|  3 | {http://owncloud.org/ns}calendar-enabled | 1             |   NULL |
|  4 | {http://owncloud.org/ns}calendar-enabled | 0             |   NULL |
|  5 | {http://owncloud.org/ns}calendar-enabled | 1             |   NULL |
|  6 | {http://owncloud.org/ns}calendar-enabled | 0             |   NULL |
|  7 | {http://owncloud.org/ns}calendar-enabled | 1             |   NULL |
+----+------------------------------------------+---------------+--------+
6 rows in set (0.001 sec)

I don't know what these are for and how they work without fileid but I know that they cause the property cleanup code to loop forever. My fix just skips them which solves the looping problem. Maybe there is another cleanup needed for these or they should be left alone but I can't afford multiple processes running busy loops forever.

sharidas · 2019-08-19T08:13:04Z

@skazi0 I appreciate your feedback.

Now I wonder if the fileid points to NULL, which means there is no file which is present in the cache. This also means, we would need to clean them up. Looking into the same.

sharidas · 2019-08-20T11:44:50Z

@skazi0 I would need your helping hand here in reproducing this issue. May I know if you have used curl or any other client to populate the table? If so, could you help us know roughly what is the data provided so that I can reproduce this issue.
Also can you also let me know roughly when you started noticing this? Meaning any activity that you might have done in the server which you could recollect.

Thanks.

skazi0 · 2019-08-23T16:29:34Z

@sharidas Sorry for late response but I was offline for a week. I have not done any manual changes or API calls to get to this problematic state. As #35292 was reported by another user, we are sure that I'm not the only one having this issue. My installation is quite old (my oldest backups are from 2015) and the oldest db snapshot I found with those entries is from 2017-07-01. I didn't have time for a deeper look into the calendar app so I have no idea if these are some old entries or if this is still reproducible with current version.

sharidas · 2019-09-03T11:40:00Z

@skazi0 No worries. There is a pull request which is under review #36084.

skazi0 · 2019-09-03T14:37:25Z

@sharidas have you found the root cause of these entries?

sharidas · 2019-09-03T15:09:48Z

@skazi0 Unfortunately I couldn't reproduce the issue. All I could confirm is that there was an issue in the table which was allowing null entries for fileid. For whatever reason, that shouldn't happen. My PR addresses the issue in the table. So after applying this change, no longer null entries would creep into the column fileid in the table.

sharidas · 2019-09-09T06:14:50Z

@skazi0 There is a PR which we have merged #36084. This does fix the problem of having null values to the fileid column. So when we release the new version of core(i.e, 10.3), you should be able to get this fixed. Hence imho, this PR can be safely closed. Hope you agree with me.

skazi0 · 2019-09-09T06:33:19Z

closed in favor of #36084

skazi0 force-pushed the property-cleanup-fix branch from ba9ba13 to 66aadeb Compare June 13, 2019 08:00

mmattel requested a review from sharidas June 18, 2019 21:08

DeepDiver1975 added 3 - To Review backport-request labels Jul 2, 2019

skazi0 force-pushed the property-cleanup-fix branch from 66aadeb to e5d758a Compare July 9, 2019 20:41

DeepDiver1975 closed this Jul 30, 2019

phil-davis reopened this Aug 5, 2019

phil-davis removed the backport-request label Aug 5, 2019

skazi0 force-pushed the property-cleanup-fix branch from e5d758a to e684cc0 Compare August 5, 2019 06:05

phil-davis requested a review from DeepDiver1975 August 5, 2019 06:21

micbar assigned sharidas Aug 9, 2019

micbar added this to the development milestone Aug 15, 2019

DeepDiver1975 mentioned this pull request Aug 16, 2019

ETag has to be surrounded by double quotes #36053

Merged

11 tasks

sharidas mentioned this pull request Aug 20, 2019

Remove null fileid from the properties #36062

Closed

11 tasks

sharidas mentioned this pull request Aug 21, 2019

Cron.php runs forever #35292

Closed

skazi0 closed this Sep 9, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix cleanup of orphaned properties #35407

Fix cleanup of orphaned properties #35407

skazi0 commented Jun 3, 2019 •

edited

CLAassistant commented Jun 3, 2019 •

edited

codecov bot commented Jun 13, 2019

codecov bot commented Jun 13, 2019 •

edited

micbar commented Jul 9, 2019

skazi0 commented Jul 9, 2019

DeepDiver1975 commented Jul 30, 2019

phil-davis commented Aug 5, 2019 •

edited

skazi0 commented Aug 5, 2019

sharidas commented Aug 16, 2019

skazi0 commented Aug 16, 2019

sharidas commented Aug 19, 2019

sharidas commented Aug 20, 2019

skazi0 commented Aug 23, 2019

sharidas commented Sep 3, 2019

skazi0 commented Sep 3, 2019

sharidas commented Sep 3, 2019

sharidas commented Sep 9, 2019

skazi0 commented Sep 9, 2019

Fix cleanup of orphaned properties #35407

Fix cleanup of orphaned properties #35407

Conversation

skazi0 commented Jun 3, 2019 • edited

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

CLAassistant commented Jun 3, 2019 • edited

codecov bot commented Jun 13, 2019

Codecov Report

codecov bot commented Jun 13, 2019 • edited

Codecov Report

micbar commented Jul 9, 2019

skazi0 commented Jul 9, 2019

DeepDiver1975 commented Jul 30, 2019

phil-davis commented Aug 5, 2019 • edited

skazi0 commented Aug 5, 2019

sharidas commented Aug 16, 2019

skazi0 commented Aug 16, 2019

sharidas commented Aug 19, 2019

sharidas commented Aug 20, 2019

skazi0 commented Aug 23, 2019

sharidas commented Sep 3, 2019

skazi0 commented Sep 3, 2019

sharidas commented Sep 3, 2019

sharidas commented Sep 9, 2019

skazi0 commented Sep 9, 2019

skazi0 commented Jun 3, 2019 •

edited

CLAassistant commented Jun 3, 2019 •

edited

codecov bot commented Jun 13, 2019 •

edited

phil-davis commented Aug 5, 2019 •

edited