Fix persistent lock retrieval by path #33888

PVince81 · 2018-12-13T16:29:04Z

Description

Fix query to make it strictly search in the given storage.
Fix query to prevent LIKE bleeding into unrelated folders (trailing slash issue).
Added enforcement of Depth 0 or -1 as per RFC 4918 Section 9.10.3.
Added many more unit tests to test the query from different angles, making sure that locks from parent and child paths are returned when the correct conditions are met and also not returned when not met.

Related Issue

Fixes #33885

Motivation and Context

How Has This Been Tested?

Unit test
Reenable acceptance tests after Feature/webdav locking frontend #32250 is merged

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)
Technical debt
Tests

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)
actually fix the issue

PVince81 · 2018-12-14T16:46:16Z

@DeepDiver1975 please carefully review. I added a lot of comments in the code to clarify the tested scenarios and the SQL query. Seems there were a few more bugs in there which I fixed (see PR description for detail)

PVince81 · 2018-12-14T16:47:17Z

lib/private/Lock/Persistent/Lock.php

@@ -39,6 +39,9 @@
 */
 class Lock extends Entity implements ILock {

+	const DEPTH_ZERO = 0;


ok seems I missed that this is already on ILock

will change after the first review pass

PVince81 · 2018-12-14T16:48:04Z

lib/private/Lock/Persistent/LockMapper.php

 				)
 			);
 		}

-		return $this->findEntities($query->getSQL(), $query->getParameters());
+		$query->andWhere($pathMatchClauses);


I should probably also add "ORDER BY" as some databases will mess up the order and break the tests...

Order by path ?

ok, fix the unit test instead

PVince81 · 2018-12-14T16:49:27Z

add sort order by path in query
fix Lock constants

PVince81 · 2018-12-20T11:59:30Z

check how our endpoint behaves with depth = 1, 500 or 400 ?

DeepDiver1975

Looks good - THX .. adjust remaining topics as discussed .... 👍

PVince81 · 2018-12-20T13:44:08Z

Adjusted. Now hoping Oracle will pass...

codecov · 2018-12-20T13:44:25Z

Codecov Report

Merging #33888 into master will decrease coverage by 0.28%.
The diff coverage is 96.96%.

@@             Coverage Diff              @@
##             master   #33888      +/-   ##
============================================
- Coverage     64.76%   64.47%   -0.29%     
+ Complexity    18336    18334       -2     
============================================
  Files          1198     1198              
  Lines         69401    69397       -4     
  Branches       1276     1276              
============================================
- Hits          44948    44746     -202     
- Misses        24084    24279     +195     
- Partials        369      372       +3

Flag	Coverage Δ	Complexity Δ
#javascript	`52.98% <ø> (-0.12%)`	`0 <ø> (ø)`
#phpunit	`65.8% <96.96%> (-0.31%)`	`18334 <9> (-2)`

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Lock/Persistent/Lock.php	`100% <ø> (ø)`	`14 <0> (ø)`	⬇️
lib/private/Lock/Persistent/LockMapper.php	`97.22% <96.96%> (-1.03%)`	`18 <9> (+4)`
lib/private/DB/AdapterOCI8.php	`0% <0%> (-86.67%)`	`4% <0%> (ø)`
...Builder/ExpressionBuilder/OCIExpressionBuilder.php	`0% <0%> (-85.19%)`	`18% <0%> (ø)`
lib/private/DB/OracleMigrator.php	`0% <0%> (-80.71%)`	`29% <0%> (ø)`
lib/private/DB/OracleConnection.php	`0% <0%> (-66.67%)`	`12% <0%> (ø)`
lib/private/DB/Adapter.php	`82.71% <0%> (-6.18%)`	`25% <0%> (ø)`
core/js/files/fileinfo.js	`85% <0%> (-5%)`	`0% <0%> (ø)`
lib/private/DB/ConnectionFactory.php	`85.5% <0%> (-4.35%)`	`20% <0%> (ø)`
lib/private/Repair/RepairMismatchFileCachePath.php	`93.38% <0%> (-3.51%)`	`51% <0%> (ø)`
... and 12 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 182a125...8daf011. Read the comment docs.

codecov · 2018-12-20T13:44:27Z

Codecov Report

Merging #33888 into master will increase coverage by <.01%.
The diff coverage is 96.96%.

@@             Coverage Diff              @@
##             master   #33888      +/-   ##
============================================
+ Coverage     64.76%   64.77%   +<.01%     
- Complexity    18336    18340       +4     
============================================
  Files          1198     1198              
  Lines         69403    69418      +15     
  Branches       1276     1276              
============================================
+ Hits          44950    44964      +14     
- Misses        24084    24085       +1     
  Partials        369      369

Flag	Coverage Δ	Complexity Δ
#javascript	`53.09% <ø> (ø)`	`0 <ø> (ø)`	⬇️
#phpunit	`66.12% <96.96%> (ø)`	`18340 <9> (+4)`	⬆️

Impacted Files	Coverage Δ	Complexity Δ
lib/private/Lock/Persistent/LockMapper.php	`97.22% <96.96%> (-1.03%)`	`18 <9> (+4)`

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 4454737...0f87cf0. Read the comment docs.

PVince81 · 2018-12-20T15:29:39Z

Oracle fail:

1) Test\Lock\Persistent\LockMapperTest::testGetLocksByPathWithoutChildren
Failed asserting that 6 matches expected 5.

/drone/src/tests/lib/Lock/Persistent/LockMapperTest.php:316
/drone/src/tests/lib/Lock/Persistent/LockMapperTest.php:251

2) Test\Lock\Persistent\LockMapperTest::testGetLocksByPathMultipleWithChildren
Failed asserting that 9 matches expected 8.

/drone/src/tests/lib/Lock/Persistent/LockMapperTest.php:316
/drone/src/tests/lib/Lock/Persistent/LockMapperTest.php:281

PVince81 · 2018-12-20T15:31:00Z

acceptance test, something weird:

Updating .htaccess for proper rewrites
Could not set mail_domain on https://server/ocs/v2.php/apps/testing/api/v1/occ. Result:
'<?xml version="1.0"?>
<ocs>
 <meta>
  <status>failure</status>
  <statuscode>500</statuscode>
  <message>Invalid query, please check the syntax. API specifications are here: http://www.freedesktop.org/wiki/Specifications/open-collaboration-services.</message>
 </meta>
 <data/>
</ocs>'

PVince81 · 2018-12-20T15:42:23Z

seems the id with Oracle is shifted. it almost looks like the insert id that was returned does not match the actual one returned by subsequent select

PVince81 · 2018-12-20T16:05:17Z

now sorting by id instead of fileid, let's see...

PVince81 · 2018-12-20T16:09:06Z

maybe the "testing" app did not enable for some reason?? @phil-davis @individual-it

phil-davis · 2018-12-20T17:11:31Z

After PR #33956 has been merged to master then rebase this and the acceptance test issue will be fixed

Fix query to make it strictly search in the given storage.. Fix query to prevent LIKE bleeding into unrelated folders (trailing slash issue). Added enforcement of Depth 0 or -1 as per RFC 4918 Section 9.10.3. Added many more unit tests to test the query from different angles, making sure that locks from parent and child paths are returned when the correct conditions are met and also not returned when not met. Reenable matching acceptance test

PVince81 · 2018-12-20T19:22:35Z

Rebased. Fixed Oracle by properly sorting the expected output before asserting...

PVince81 · 2018-12-20T19:24:34Z

stable10: #33957

phil-davis · 2019-01-08T05:05:39Z

stable10: #33957

Removed backport-request label because backport is done.

PVince81 added 2 - Developing backport-request labels Dec 13, 2018

PVince81 added this to the development milestone Dec 13, 2018

PVince81 self-assigned this Dec 13, 2018

PVince81 mentioned this pull request Dec 13, 2018

Don't apply persistent lock on received declined shares #33885

Closed

PVince81 force-pushed the lock-leak-fix branch from 72ee8e5 to 8d8734a Compare December 14, 2018 16:44

PVince81 changed the title ~~Add tests for lock leak issue~~ Fix persistent lock retrieval by path Dec 14, 2018

PVince81 added 3 - To Review and removed 2 - Developing labels Dec 14, 2018

PVince81 requested a review from DeepDiver1975 December 14, 2018 16:45

PVince81 commented Dec 14, 2018

View reviewed changes

PVince81 added 2 - Developing and removed 3 - To Review labels Dec 14, 2018

DeepDiver1975 approved these changes Dec 20, 2018

View reviewed changes

PVince81 force-pushed the lock-leak-fix branch from 8d8734a to 8daf011 Compare December 20, 2018 13:44

PVince81 force-pushed the lock-leak-fix branch from 8daf011 to 030233b Compare December 20, 2018 16:01

PVince81 mentioned this pull request Dec 20, 2018

[stable10] Webdav locking frontend #33951

Merged

PVince81 force-pushed the lock-leak-fix branch from 030233b to 0f87cf0 Compare December 20, 2018 19:19

PVince81 mentioned this pull request Dec 20, 2018

[stable10] Fix persistent lock retrieval by path #33957

Merged

PVince81 added 4 - To release and removed 2 - Developing labels Dec 20, 2018

PVince81 merged commit 91b662d into master Dec 20, 2018

delete-merged-branch bot deleted the lock-leak-fix branch December 20, 2018 19:58

phil-davis removed the backport-request label Jan 8, 2019

ownclouders mentioned this pull request Jan 24, 2019

make acceptance tests for webdav locking issues that are not catched by litmus #34239

Closed

3 tasks

lock bot locked as resolved and limited conversation to collaborators Jan 8, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix persistent lock retrieval by path #33888

Fix persistent lock retrieval by path #33888

PVince81 commented Dec 13, 2018 •

edited

Loading

PVince81 commented Dec 14, 2018

PVince81 Dec 14, 2018

PVince81 Dec 14, 2018

PVince81 Dec 20, 2018

PVince81 commented Dec 14, 2018 •

edited

Loading

PVince81 commented Dec 20, 2018

DeepDiver1975 left a comment

PVince81 commented Dec 20, 2018

codecov bot commented Dec 20, 2018

codecov bot commented Dec 20, 2018 •

edited

Loading

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

phil-davis commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

phil-davis commented Jan 8, 2019

Fix persistent lock retrieval by path #33888

Fix persistent lock retrieval by path #33888

Conversation

PVince81 commented Dec 13, 2018 • edited Loading

Description

Related Issue

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

Open tasks:

PVince81 commented Dec 14, 2018

PVince81 Dec 14, 2018

Choose a reason for hiding this comment

PVince81 Dec 14, 2018

Choose a reason for hiding this comment

PVince81 Dec 20, 2018

Choose a reason for hiding this comment

PVince81 commented Dec 14, 2018 • edited Loading

PVince81 commented Dec 20, 2018

DeepDiver1975 left a comment

Choose a reason for hiding this comment

PVince81 commented Dec 20, 2018

codecov bot commented Dec 20, 2018

Codecov Report

codecov bot commented Dec 20, 2018 • edited Loading

Codecov Report

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

phil-davis commented Dec 20, 2018

PVince81 commented Dec 20, 2018

PVince81 commented Dec 20, 2018

phil-davis commented Jan 8, 2019

PVince81 commented Dec 13, 2018 •

edited

Loading

PVince81 commented Dec 14, 2018 •

edited

Loading

codecov bot commented Dec 20, 2018 •

edited

Loading