X Tutup
Skip to content

Apply proper error handling for unsupported file types and errors dur…#37173

Merged
phil-davis merged 1 commit intomasterfrom
bugfix/37164-and-37165
Mar 27, 2020
Merged

Apply proper error handling for unsupported file types and errors dur…#37173
phil-davis merged 1 commit intomasterfrom
bugfix/37164-and-37165

Conversation

@DeepDiver1975
Copy link
Member

@DeepDiver1975 DeepDiver1975 commented Mar 26, 2020 •
edited
Loading

…ing preview generation

Description

Returning now proper http status codes in error cases

Related Issue

How Has This Been Tested?

  • ✋ send curl requests as per issue description

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:
  • Changelog item, see TEMPLATE

@DeepDiver1975 DeepDiver1975 self-assigned this Mar 26, 2020
@DeepDiver1975 DeepDiver1975 force-pushed the bugfix/37164-and-37165 branch from d10e904 to fb66bbb Compare March 26, 2020 19:10
@codecov
Copy link

codecov bot commented Mar 26, 2020 •
edited
Loading

Codecov Report

Merging #37173 into master will increase coverage by 0.00%.
The diff coverage is 84.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master   #37173   +/-   ##
=========================================
  Coverage     64.87%   64.87%           
- Complexity    19144    19146    +2     
=========================================
  Files          1268     1268           
  Lines         74940    74944    +4     
  Branches       1331     1331           
=========================================
+ Hits          48619    48623    +4     
  Misses        25929    25929           
  Partials        392      392
Flag Coverage Δ Complexity Δ
#javascript 54.14% <ø> (ø) 0.00 <ø> (ø)
#phpunit 66.07% <84.61%> (+<0.01%) 19146.00 <0.00> (+2.00)
Impacted Files Coverage Δ Complexity Δ
apps/dav/lib/Files/PreviewPlugin.php 92.15% <84.61%> (+0.66%) 18.00 <0.00> (+2.00)

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 92d7a49...e92a218. Read the comment docs.

@phil-davis
Copy link
Contributor

phil-davis commented Mar 27, 2020

@individual-it you have PR #37166 in progress that adds acceptance tests that demonstrate this.
We need to sort out the process for that. Maybe we get that finished and merged, then rebase this bugfix PR and add the necessary test scenario fixes to here?

@phil-davis
Copy link
Contributor

phil-davis commented Mar 27, 2020

Before the change:

curl '172.17.0.1:8080/remote.php/dav/files/admin/abc.txt?x=0&y=32&forceIcon=1&preview=1' -u admin:admin -v
...
< HTTP/1.1 500 Internal Server Error
...
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Exception</s:exception>
  <s:message>Cannot set width of 0 or smaller!</s:message>
</d:error>

With this fix:

curl '172.17.0.1:8080/remote.php/dav/files/admin/abc.txt?x=0&y=32&forceIcon=1&preview=1' -u admin:admin -v
...
< HTTP/1.1 400 Bad Request
...
<?xml version="1.0" encoding="utf-8"?>
<d:error xmlns:d="DAV:" xmlns:s="http://sabredav.org/ns">
  <s:exception>Sabre\DAV\Exception\BadRequest</s:exception>
  <s:message>Cannot set width of 0 or smaller!</s:message>
</d:error>

And similar for setting x or y to 0, -1, abc, false, true...

LGTM

phil-davis
phil-davis reviewed Mar 27, 2020
View reviewed changes
Copy link
Contributor

@phil-davis phil-davis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add some unit test cases that trigger the BadRequest exception>
That will also help codecov to be happy.

@DeepDiver1975 DeepDiver1975 force-pushed the bugfix/37164-and-37165 branch from fb66bbb to e92a218 Compare March 27, 2020 08:25
@DeepDiver1975
Copy link
Member Author

DeepDiver1975 commented Mar 27, 2020

@phil-davis unit test added

@phil-davis phil-davis merged commit 1fdf412 into master Mar 27, 2020
@delete-merged-branch delete-merged-branch bot deleted the bugfix/37164-and-37165 branch March 27, 2020 09:47
@individual-it
Copy link
Member

individual-it commented Mar 27, 2020

As this PR is merged now, we will simply adjust the tests in #37166

@phil-davis phil-davis mentioned this pull request Mar 27, 2020
Merged
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@phil-davis phil-davis phil-davis approved these changes

@individual-it individual-it Awaiting requested review from individual-it

Assignees

@DeepDiver1975 DeepDiver1975

Labels

3 - To Review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

requesting a file preview with an ivalid size results in HTTP error 500 requesting preview of folder results in error 500

3 participants

@DeepDiver1975 @phil-davis @individual-it
X Tutup