fix update to remove shares where file doesn't exist on postgres

[MorrisJobke]

This is just a cherry pick of the changes from #6016 to be able to add unit tests to this PR.

credit for the fix goes to @jmcclelland

[karlitschek]

makes sense 👍

[MorrisJobke]

I added the unit test. Ready to review and merge. cc @PVince81 @schiesbn

[schiessle]

code looks good, I also executed the test successfully on my local sqlite... Let's wait for Jenkins tests with the other databases... otherwise 👍

[MorrisJobke]

@PVince81 It's broken, because previous tests doesn't clean up the database :(

@DeepDiver1975 Should I just clear the tables in "setup" or should I try to find the cause of the data?

[DeepDiver1975]

Should I just clear the tables in "setup"

yes - that's acceptable - thx

[MorrisJobke]

Test_Files_Sharing_Updater::testRemoveBrokenShares
But this is the name of that column. @butonic How does Oracle handle column names in a way that this is a problem?

Column definition: https://github.com/owncloud/core/blob/master/db_structure.xml#L840

Doctrine\DBAL\DBALException: An exception occurred while executing 'INSERT INTO "oc_share" (file_source, id, item_type, uid_owner) VALUES (?, ?, 'file', 1)':

ORA-00904: "UID_OWNER": invalid identifier

[PVince81]

Maybe the field backticks are missing. I'll see if I can fix it, I can run the test against the Stuttgart Oracle instance.

[PVince81]

@MorrisJobke I've fixed the backticks, the tests can now run on Oracle (they did locally).
Nevertheless, one of the tests is failing:

There was 1 failure:

1) Test_Files_Sharing_Updater::testRemoveBrokenShares
Failed asserting that '1' matches expected 0.

/home/vincent/workspace/owncloud/apps/files_sharing/tests/updater.php:82

[ghost]

💣 Test Failed. 💣
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3434/

[MorrisJobke]

@PVince81 Seems to be related to a misinterpreted SQL statement on oracle. Is anybody here who can try the actual SQL statement on oracle? I don't have an oracle DB here.

Steps to do:

1. Add a entry to oc_share with a reference to a not existant item in oc_filescache
2. Run the SQL statement of fixBrokenSharesOnAppUpdate
3. The entry added in 1) should be gone

cc @butonic

[PVince81]

The deletion worked but it seems to be the one with fileid=3 that was deleted, not 2.
Maybe Oracle generates the ids in a different order ?

[PVince81]

@MorrisJobke can you change the code and make it "id proof" ? Maybe put a name in "item_target" and select on the string instead of the id when checking whether it was deleted correctly.

[butonic]

@MorrisJobke the query runs fine:

SELECT "oc_share"."id"
FROM "oc_share" LEFT JOIN "oc_filecache" ON "file_source" = "oc_filecache"."fileid"
WHERE "oc_filecache"."fileid" IS NULL AND "oc_share"."item_type" IN ('file', 'folder');

    id
----------
    34

Instead of first querying the database for all shares on files that don't exist and then deleting every obsolete share individually, you should let the database handle all of it:

DELETE FROM "oc_share" WHERE "file_source" NOT IN (
    SELECT "fileid" FROM "oc_filecache" WHERE "item_type" IN ('file','folder')
);

Everything executed in one query to the DB.

[PVince81]

I think at some point @bantu mentionned that we can't use subqueries ? (might have been something else)

[schiessle]

also keep in mind that we are talking about a update script. This gets executed exactly once for every user. I don't think this need to be highly optimized, let's take the safe way.

[butonic]

@PVince81 Yes, on mysql there is a problem with modifying a table when you already used it in a subselect: http://stackoverflow.com/a/12969601. The workaround is tu use a virtual table as I did in search_lucene. Anyway, that workaround is not necessary in this case.

@schiesbn If you consider going through the php stack - with all it's typelessness - safer ... ;) Also, users with millions of files tend to complain when migration takes ages. Ask @felixboehm for details. Which is why I prefer letting the database handle it natively and if necessary take spacial care of mysql as in search_lucene.

[MorrisJobke]

@butonic I will give your subselect idea a try and jenkins will show us the problems, right?

[jmcclelland]

We've been down this road before with this very patch. See: #6016

Tests passed with the sub-query, but instead of committing the patch, a request was made to do it over again without the subquery.

[MorrisJobke]

@jmcclelland Sorry for that back and forth. I just checked the PR and the subquery there contains a needless JOIN and this one here is covered by an unit test, which hopefully covers the relevant cases. Maybe we can find together the correct solution. :)

[MorrisJobke]

@karlitschek Your opinion?

[karlitschek]

Let's go with the simple approach. Performance is not important in this case.

[DeepDiver1975]

@owncloud-bot retest this please

[PVince81]

@MorrisJobke looks like now all the shares were deleted ?

[MorrisJobke]

@PVince81 It's weird... will have another try tomorrow

[PVince81]

Any update ?

[PVince81]

@MorrisJobke did you have any luck with the VM ?
I tried downloading it, it took a few hours and in the end the file was corrupt. 😕

[MorrisJobke]

@PVince81 Not yet ... internet connection is sort of unstable and resume doesn't work 😕

[PVince81]

@MorrisJobke I've discovered that Oracle ignores the ids you give for primary keys, I had this in another PR. Maybe that's what's happening here.

[PVince81]

Better use \OC_DB::insertId('PREFIX'table') after the INSERT to retrieve whichever id Oracle decided to give instead of yours.

[PVince81]

Seems it's the mapping between filecache entries and share entries might be wrong because of that generated id issue.

I'll give it a try.

[PVince81]

Here you go, it should work now.

[scrutinizer-notifier]

A new inspection was created.

[PVince81]

@owncloud-bot retest this please

[PVince81]

Something was wrong with the tests and the wrong ids were used. I'm surprised that the tests still passed locally. "Fortunately" Oracle has spotted these...

Now let's hope that they pass on Jenkins!

[scrutinizer-notifier]

The inspection completed: 34 new issues, 33 updated code elements

[ghost]

🚀 Test Passed. 🚀
Refer to this link for build results: https://ci.owncloud.org/job/pull-request-analyser/3891/

[PVince81]

The code has been changed since the last review, so please re-review

[jancborchardt]

@MorrisJobke @jmcclelland @schiesbn can you please review @PVince81’s new changes?

[jancborchardt]

Also cc'ing @julienfastre for review because you commented on the previous pull request #6016

[MorrisJobke]

👍 Works

My steps (for SQLite): (123456 and mjob should be replaced with a corresponding value - first must NOT exists in oc_filecache - second is just a user id)

> select * from oc_appconfig where appid = 'files_sharing';
files_sharing|installed_version|0.3.5
files_sharing|types|filesystem
files_sharing|enabled|yes
> insert into oc_share (file_source, item_type, uid_owner) values (1234567, 'file', 'mjob');
// this inserts a broken share - which should be removed

Checkout this branch and load your ownCloud instance to update the app.

> select * from oc_appconfig where appid = 'files_sharing';
files_sharing|installed_version|0.3.5.6
files_sharing|types|filesystem
files_sharing|enabled|yes
> select * from oc_share where file_source = 123456;
// should result in nothing as this is removed by the script

[PVince81]

Ok, that should be enough, merging now.
Thanks everyone.

[MorrisJobke]

@PVince81 Awesome ...it gets into master \o/