Repair step to remove bogus expiration dates from non-link shares #19132

PVince81 · 2015-09-17T14:03:45Z

To clean up the mess by the bug that existed pre-8.2: #11396
The new share dialog doesn't set the expiration date for user shares any more, so cleaning that up should be permanent (hence the version check)

@MorrisJobke @nickvergessen @schiesbn @DeepDiver1975

(8.2 only)

karlitschek · 2015-09-17T16:12:55Z

👍

nickvergessen · 2015-09-18T07:15:46Z

lib/repair/repairinvalidshares.php

+	 * though it is not supported. This functions removes the expiration date from such entries.
+	 */
+	private function removeExpirationDateFromNonLinkShares() {
+		$sql = 'UPDATE `*PREFIX*share` SET `expiration`=null WHERE `expiration` IS NOT null AND `share_type` != ?';


SQL syntax for unequal is <> not !=
Most DBs understand != but you never know how long stares at oracle

Why not use the query builder?

Right... I copy-pasted from the orphaned shares code that also didn't use the query builder

@rullzer because it's broken: #19314

nickvergessen · 2015-09-18T07:16:10Z

Looks good apart from the comment

PVince81 · 2015-09-18T08:30:51Z

Right, I wanted to run the OCI test locally

DeepDiver1975 · 2015-09-23T12:42:00Z

@PVince81 oracle doesn't like that:

Test\Repair\RepairInvalidShares::testRemoveExpirationDateForNonLinkShares
Doctrine\DBAL\Exception\InvalidFieldNameException: An exception occurred while executing 'INSERT INTO "oc_share" (share_type, share_with, uid_owner, item_type, item_source, item_target,file_source, file_target, permissions, stime, expiration, token) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' with params [0, "recipientuser1", "user1", "folder", 123, "\/123", 123, "\/test", 1, 1443004205, "2015-09-25 00:00:00", null]:

ORA-00904: "TOKEN": invalid identifier

/var/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php:48
/var/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:116
/var/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:996
/var/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/lib/private/db/connection.php:203
/var/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/tests/lib/repair/repairinvalidshares.php:60

PVince81 · 2015-09-23T12:49:25Z

Yeah... I'll use the query builder instead and the proper operator...

PVince81 · 2015-09-23T15:31:04Z

I fixed the comparator, hoping it will work now with OCI.

DeepDiver1975 · 2015-09-23T21:41:06Z

still issues with oracle:

Test\Repair\RepairInvalidShares::testRemoveExpirationDateForNonLinkShares
Doctrine\DBAL\Exception\InvalidFieldNameException: An exception occurred while executing 'INSERT INTO "oc_share" (share_type, share_with, uid_owner, item_type, item_source, item_target,file_source, file_target, permissions, stime, expiration, token) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?, ?)' with params [0, "recipientuser1", "user1", "folder", 123, "\/123", 123, "\/test", 1, 1443025757, "2015-09-25 00:00:00", null]:

ORA-00904: "TOKEN": invalid identifier

/var/jenkins/workspace/core-ci-linux/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Driver/AbstractOracleDriver.php:48
/var/jenkins/workspace/core-ci-linux/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/DBALException.php:116
/var/jenkins/workspace/core-ci-linux/database/oci/label/SLAVE/3rdparty/doctrine/dbal/lib/Doctrine/DBAL/Connection.php:996
/var/jenkins/workspace/core-ci-linux/database/oci/label/SLAVE/lib/private/db/connection.php:203
/var/jenkins/workspace/core-ci-linux/database/oci/label/SLAVE/tests/lib/repair/repairinvalidshares.php:60

nickvergessen · 2015-09-24T07:00:20Z

tests/lib/repair/repairinvalidshares.php

+	public function testRemoveExpirationDateForNonLinkShares() {
+		$sql =
+			'INSERT INTO `*PREFIX*share` ' .
+			'(share_type, share_with, uid_owner, item_type, item_source, item_target,' .


Fails on oracle, no quotes in sight 😞

Gargglllllllbrlrlrlrlrl okay

We all work in the service of the mighty Oracle 🔮

PVince81 · 2015-09-24T08:52:40Z

Backticks added and squashed

nickvergessen · 2015-09-24T08:54:47Z

lib/repair/repairinvalidshares.php

+	 * though it is not supported. This functions removes the expiration date from such entries.
+	 */
+	private function removeExpirationDateFromNonLinkShares() {
+		$sql = 'UPDATE `*PREFIX*share` SET `expiration`=null WHERE `expiration` IS NOT null AND `share_type` <> ?';


Now you reverted it back? No more query builder?

I never pushed the query builder here because it didn't work.
Now that you fixed it I can move the query builder version to this branch.

PVince81 · 2015-09-24T10:30:35Z

Let me move this to the query builder

PVince81 · 2015-09-24T10:47:16Z

Ok, I query buildered all the things, including the SQLs from the test case

MorrisJobke · 2015-09-24T12:49:23Z

tests/lib/repair/repairinvalidshares.php

+
+		$userShare = $results[0];
+		$linkShare = $results[1];
+		$this->assertEquals($bogusShareId, $userShare['id'], 'sanity check');


13:08:16 1) Test\Repair\RepairInvalidShares::testRemoveExpirationDateForNonLinkShares 13:08:16 sanity check 13:08:16 Failed asserting that '1' matches expected false. 13:08:16 13:08:16 /var/jenkins/workspace/core-ci-linux@2/database/oci/label/SLAVE/tests/lib/repair/repairinvalidshares.php:100

$bogusShareId = $this->connection->lastInsertId();

This seems to return false. @nickvergessen didn't you mention that this is tricky on oracle?

I knew it, found the code:

core/lib/private/share/share.php

Lines 2303 to 2317 in 1911472

$id = \OC::$server->getDatabaseConnection()->lastInsertId();

// Fallback, if lastInterId() doesn't work we need to perform a select

// to get the ID (seems to happen sometimes on Oracle)

if (!$id) {

$getId = \OC_DB::prepare('

SELECT `id`

FROM`*PREFIX*share`

WHERE `uid_owner` = ? AND `item_target` = ? AND `item_source` = ? AND `stime` = ?

');

$r = $getId->execute(array($shareData['uidOwner'], $shareData['itemTarget'], $shareData['itemSource'], $shareData['shareTime']));

if ($r) {

$row = $r->fetchRow();

$id = $row['id'];

}

}

🙈

Thanks... ok I'll rewrite the test to not rely on the id

PVince81 · 2015-09-28T10:42:29Z

Replaced 'lastInsertId` with a select to get the id.

PVince81 · 2015-09-28T13:06:21Z

@nickvergessen @MorrisJobke tests are green now 😄

nickvergessen · 2015-09-28T15:18:17Z

tests/lib/repair/repairinvalidshares.php

+			->execute()
+			->fetchAll();
+
+		$this->assertCount(2, $results);


Should delete all existing shares in setUp(), to avoid problems when a previous test one day creates a share

MorrisJobke · 2015-09-29T08:13:30Z

Tested with a group share:

doesn't remove the share date on master
remove the share date on this branch

Beside my comment above 👍

nickvergessen · 2015-09-29T12:35:27Z

#19132 (comment) is still open @MorrisJobke

icewind1991 · 2015-09-30T15:03:59Z

Code looks good, agree with @nickvergessen about cleaning up before starting the test

PVince81 · 2015-09-30T15:18:13Z

Feel free to change it yourself. Busy fixing IE8 (unless you want to swap with me)

nickvergessen · 2015-10-01T06:32:54Z

Done and rebased

PVince81 · 2015-10-01T08:28:20Z

@nickvergessen thanks

PVince81 · 2015-10-01T08:38:18Z

👍 for @nickvergessen's changes, merging

…date Repair step to remove bogus expiration dates from non-link shares

PVince81 added the 3 - To Review label Sep 17, 2015

PVince81 added this to the 8.2-current milestone Sep 17, 2015

PVince81 mentioned this pull request Sep 17, 2015

Can't reshare link with default expiration date #11396

Closed

nickvergessen reviewed Sep 18, 2015
View reviewed changes

PVince81 mentioned this pull request Sep 18, 2015

Allow setting expire on user / group shares via API #15459

Closed

DeepDiver1975 force-pushed the sharing-removebogusexpirationdate branch from 171ce28 to 0e45fab Compare September 23, 2015 10:12

PVince81 mentioned this pull request Sep 23, 2015

Cannot set NULL values using query builder #19314

Closed

PVince81 force-pushed the sharing-removebogusexpirationdate branch from 0e45fab to b6929a9 Compare September 23, 2015 15:30

PVince81 force-pushed the sharing-removebogusexpirationdate branch from b6929a9 to 5e6453f Compare September 23, 2015 16:06

nickvergessen reviewed Sep 24, 2015
View reviewed changes

PVince81 force-pushed the sharing-removebogusexpirationdate branch from 5e6453f to 884bc2f Compare September 24, 2015 08:52

nickvergessen reviewed Sep 24, 2015
View reviewed changes

PVince81 force-pushed the sharing-removebogusexpirationdate branch from 884bc2f to 32f30e2 Compare September 24, 2015 10:46

MorrisJobke reviewed Sep 24, 2015
View reviewed changes

PVince81 force-pushed the sharing-removebogusexpirationdate branch from 32f30e2 to 1bd7d97 Compare September 28, 2015 10:42

nickvergessen reviewed Sep 28, 2015
View reviewed changes

MorrisJobke force-pushed the sharing-removebogusexpirationdate branch from 1bd7d97 to 6aed476 Compare September 29, 2015 08:28

Vincent Petry and others added 3 commits October 1, 2015 08:27

Repair step to remove bogus expiration dates from non-link shares

6e104bc

add more precise version to also repair git master instances

cc08123

Also delete all shares before the test, so the count matches

8f2600a

nickvergessen force-pushed the sharing-removebogusexpirationdate branch from 6aed476 to 8f2600a Compare October 1, 2015 06:32

PVince81 pushed a commit that referenced this pull request Oct 1, 2015

Merge pull request #19132 from owncloud/sharing-removebogusexpiration…

64994fa

…date Repair step to remove bogus expiration dates from non-link shares

PVince81 merged commit 64994fa into master Oct 1, 2015

PVince81 deleted the sharing-removebogusexpirationdate branch October 1, 2015 08:38

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Repair step to remove bogus expiration dates from non-link shares #19132

Repair step to remove bogus expiration dates from non-link shares #19132

PVince81 commented Sep 17, 2015

karlitschek commented Sep 17, 2015

nickvergessen Sep 18, 2015

rullzer Sep 21, 2015

PVince81 Sep 22, 2015

PVince81 Sep 23, 2015

nickvergessen commented Sep 18, 2015

PVince81 commented Sep 18, 2015

DeepDiver1975 commented Sep 23, 2015

PVince81 commented Sep 23, 2015

PVince81 commented Sep 23, 2015

DeepDiver1975 commented Sep 23, 2015

nickvergessen Sep 24, 2015

PVince81 Sep 24, 2015

PVince81 Sep 24, 2015

PVince81 commented Sep 24, 2015

nickvergessen Sep 24, 2015

PVince81 Sep 24, 2015

PVince81 commented Sep 24, 2015

PVince81 commented Sep 24, 2015

MorrisJobke Sep 24, 2015

MorrisJobke Sep 24, 2015

nickvergessen Sep 24, 2015

PVince81 Sep 24, 2015

PVince81 commented Sep 28, 2015

PVince81 commented Sep 28, 2015

nickvergessen Sep 28, 2015

MorrisJobke commented Sep 29, 2015

nickvergessen commented Sep 29, 2015

icewind1991 commented Sep 30, 2015

PVince81 commented Sep 30, 2015

nickvergessen commented Oct 1, 2015

PVince81 commented Oct 1, 2015

PVince81 commented Oct 1, 2015

	$id = \OC::$server->getDatabaseConnection()->lastInsertId();
	// Fallback, if lastInterId() doesn't work we need to perform a select
	// to get the ID (seems to happen sometimes on Oracle)
	if (!$id) {
	$getId = \OC_DB::prepare('
	SELECT `id`
	FROM`PREFIXshare`
	WHERE `uid_owner` = ? AND `item_target` = ? AND `item_source` = ? AND `stime` = ?
	');
	$r = $getId->execute(array($shareData['uidOwner'], $shareData['itemTarget'], $shareData['itemSource'], $shareData['shareTime']));
	if ($r) {
	$row = $r->fetchRow();
	$id = $row['id'];
	}
	}

Repair step to remove bogus expiration dates from non-link shares #19132

Repair step to remove bogus expiration dates from non-link shares #19132

Conversation

PVince81 commented Sep 17, 2015

karlitschek commented Sep 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Sep 18, 2015

PVince81 commented Sep 18, 2015

DeepDiver1975 commented Sep 23, 2015

PVince81 commented Sep 23, 2015

PVince81 commented Sep 23, 2015

DeepDiver1975 commented Sep 23, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Sep 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Sep 24, 2015

PVince81 commented Sep 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PVince81 commented Sep 28, 2015

PVince81 commented Sep 28, 2015

Choose a reason for hiding this comment

MorrisJobke commented Sep 29, 2015

nickvergessen commented Sep 29, 2015

icewind1991 commented Sep 30, 2015

PVince81 commented Sep 30, 2015

nickvergessen commented Oct 1, 2015

PVince81 commented Oct 1, 2015

PVince81 commented Oct 1, 2015