Skip to content

Conversation

@jvillafanez
Copy link
Member

Description

When a mount point isn't set to check for changes on access, a 503 HTTP code could be thrown if that file has been removed. With the PR, it will throw a 404 HTTP code instead.

Related Issue

No public issue

Motivation and Context

Throw the right error instead of a wrong one.

How Has This Been Tested?

Manually with SFTP: upload a file through ownCloud to a SFTP mount configured with "check for changes = never", remove the file in the backend, try to download the file.

A 404 HTTP code will be thrown, although the page show to the user will be the same

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)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

@PVince81
Copy link
Contributor

PVince81 commented Aug 7, 2017

We should cover this with integration tests. There's a "local" external storage on which we could remotely delete the file to test this.

@jvillafanez
Copy link
Member Author

I'd rather wait for @owncloud/qa for the integration tests. I'm not sure how it is planned or what is being covered for external storages.

@PVince81
Copy link
Contributor

PVince81 commented Aug 7, 2017

@owncloud/qa can you help @jvillafanez write the tests?

@PVince81 PVince81 added the p2-high Escalation, on top of current planning, release blocker label Aug 7, 2017
@PVince81
Copy link
Contributor

PVince81 commented Aug 7, 2017

CI restarted. Since this at least has unit tests I think we can merge first.

@phil-davis
Copy link
Contributor

https://travis-ci.org/owncloud/core/jobs/261783308 got some wacky error when testing setting invalid quotas. I copied the error output in case I see it again, then restarted that job - hopefully Travis will be happy in 20 minutes or so.

@PVince81 PVince81 merged commit 9949930 into master Aug 8, 2017
@PVince81 PVince81 deleted the no_changes_not_found branch August 8, 2017 07:15
@PVince81
Copy link
Contributor

PVince81 commented Aug 8, 2017

@jvillafanez please backport to stable10

@jvillafanez jvillafanez mentioned this pull request Aug 8, 2017
9 tasks
@jvillafanez
Copy link
Member Author

Backport to stable10: #28613

@PVince81
Copy link
Contributor

PVince81 commented Aug 8, 2017

integration test here: #28614

@lock
Copy link

lock bot commented Aug 2, 2019

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.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

3 - To Review app:dav app:files_external p2-high Escalation, on top of current planning, release blocker status/STALE

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants