Return 403 instead of 503 to resume syncing of desktop client #30059
Conversation
| @@ -226,6 +226,9 @@ function ($path) use ($storage) { | |||
| $this->assertInstanceOf($expectedException, $caughtException); | |||
| if ($checkPreviousClass) { | |||
| $this->assertInstanceOf(get_class($thrownException), $caughtException->getPrevious()); | |||
| if (strpos(get_class($thrownException), "GenericEncryptionException") !== false) { | |||
There was a problem hiding this comment.
why not assertInstanceOf with GenericEncryptionException::class ?
There was a problem hiding this comment.
The reason why assertInstanceOf not used is because assert would fail if $thrownException is not of instance GenericEncryptionException . The @dataprovider https://github.com/owncloud/core/pull/30059/files/a650cd8f7c132a833072b98adb7846f1c092b81a#diff-9e251176dc475a2f3b02337f5288d2eaR119 provides other instances.
There was a problem hiding this comment.
why not just provide the instance through the data provider then ?
I don't understand how or whether this line is even called.
There was a problem hiding this comment.
@PVince81 Updated the change by passing message also via data provider.
Codecov Report
@@ Coverage Diff @@
## master #30059 +/- ##
============================================
+ Coverage 58.25% 58.34% +0.09%
- Complexity 18520 18596 +76
============================================
Files 1093 1093
Lines 63712 63980 +268
============================================
+ Hits 37113 37329 +216
- Misses 26599 26651 +52
Continue to review full report at Codecov.
|
sharidas
force-pushed
the
return403-encryption-for-client
branch
from
a650cd8 to
cdb9e53
Compare
| @@ -154,7 +154,9 @@ public function fopenFailuresProvider() { | |||
| ], | |||
| [ | |||
| new GenericEncryptionException(), | |||
| 'Sabre\DAV\Exception\ServiceUnavailable' | |||
| 'Sabre\DAV\Exception\ServiceUnavailable', | |||
There was a problem hiding this comment.
wasn't this supposed to become a ForbiddenException ?
also in your previous changeset you had something like GenericEncryptionException ?
When 503 is returned the syncing is stopped by desktop client. Hence its found better to return 403. Signed-off-by: Sujith H <[email protected]>
sharidas
force-pushed
the
return403-encryption-for-client
branch
from
cdb9e53 to
2ddad25
Compare
| * @expectedException \Sabre\Dav\Exception\Forbidden | ||
| * @expectedExceptionMessage Encryption not ready | ||
| */ | ||
| public function testFopenForbiddenExceptionEncryption() { |
There was a problem hiding this comment.
@PVince81 Created a function to check the get() call making Forbidden exception. Earlier change set can be ignored.
|
Added backport request so that we can backport this change after codefreeze. Let me know know if its ok to backport. |
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
When 503 is returned the syncing is stopped
by desktop client. Hence its found better
to return 403.
Signed-off-by: Sujith H [email protected]
Description
Returning 503 was causing sync operation of desktop client to stop. Hence returning 403 makes sync operation not to stop.
Related Issue
#29794
Motivation and Context
When 503 is returned the sync operation is stopped by the desktop client. Hence 403 is being returned.
How Has This Been Tested?
Steps followed as mentioned in #29794.
The result captured from desktop client after applying the patch:
Screenshots (if appropriate):
Types of changes
Checklist: