AppFramework StreamResponse #13616

BernhardPosselt · 2015-01-22T19:47:07Z

First stab at the StreamResponse, see #12988

The idea is to use an interface ICallbackResponse (I'm not 100% happy with the name yet, suggestions?) that allow the response to output things in its own way, for instance stream the file using readfile

Unittests are atm lacking, plan is to

check if a mock of ICallbackResponse will be used by calling its callback (also unhappy with this name) method

Usage is:

$response = new StreamResponse('path/to/file');

@DeepDiver1975 @MorrisJobke @LukasReschke @PVince81 @oparoz

DeepDiver1975 · 2015-01-22T19:49:55Z

nice - THX @Raydiation

LukasReschke · 2015-01-22T22:33:54Z

lib/public/appframework/http/streamresponse.php

+     * Streams the file using readfile
+     */
+    public function callback () {
+        @readfile($this->filepath);


I'm somewhat wondering whether it would make sense to throw an exception in case an error happened (i.e. readfile returned false)

We might also want to evaluate XSendFile support for this one

Good question, I actually don't know because there is nothing that catches the exception. ATM it will just silently fail if the content is not available which is not a good idea i think. Just throw any exception?

Ideally we'd have a 404 if the file does not exist, the question is just where and how we should throw the exception. Maybe it would be best to just leave this up to the developer, e.g.:

public function controllerMethod($fileId) { try { $filePath = $this->service->findPath($fileId); return new StreamResponse($filePath); } catch (DoesNotExistException $e) { return new Response(Http::STATUS_NOT_FOUND); } }

otherwise we'd need custom middleware that checks the filepaths of all StreamResponses and replaces it with 404 responses which is akward I think.

As for xsendfile: a good idea but how do we handle php streams? How would we return a file that is on an FTP? It is fast but probably has a different use case because of the imposed limits and should go into a separate response that falls back to readfile if xsendfile does not exist

+1 for the exception which has to be catched by the single dev.

It is fast but probably has a different use case because of the imposed limits and should go into a separate response that falls back to readfile if xsendfile does not exist

Agreed as well.

Scrutinizer has found a typo
https://scrutinizer-ci.com/g/owncloud/core/inspections/633fd8eb-b449-4ca0-b04e-49036d0e7d11/code-structure/operation/OCP%5CAppFramework%5CHttp%5CStreamResponse%3A%3Acallback

Yep should be upper-case p

BernhardPosselt · 2015-01-23T18:06:50Z

Tests are added by using a very simple wrapper around print/header/setcookie

BernhardPosselt · 2015-01-23T18:07:57Z

The only thing im still unhappy about is the naming

LukasReschke · 2015-02-12T17:58:37Z

lib/public/appframework/http/streamresponse.php

+    }
+
+
+}


Newline at EoF

LukasReschke · 2015-02-16T14:19:28Z

I fixed the code style issues as well as the issue with the wrong variable and squashed all commits.

BernhardPosselt · 2015-02-16T15:41:27Z

@LukasReschke thanks!

LukasReschke · 2015-02-24T14:34:10Z

Makes sense - works 👍

@MorrisJobke @nickvergessen Second review?

LukasReschke · 2015-02-24T14:52:14Z

By the way, I'm wondering if it would make sense to forbid directory traversals (e.g. ../) and throw an exception instead?

What do you think, @Raydiation?

BernhardPosselt · 2015-02-24T14:59:48Z

It makes sense to require a realpath (as in no ../../ in it) but then people will probably just work around that using realpath().

It may be easier to restrict it to certain paths. Maybe data and apps/appid/data? We can always change that by passing additional constructor parameters to change the behavior, as in whitelist more directories

LukasReschke · 2015-02-24T15:00:55Z

It may be easier to restrict it to certain paths. Maybe data and apps/appid/data? We can always change that by passing additional constructor parameters to change the behavior, as in whitelist more directories

That would probably be a good idea. I'd say it's enough to go for data/ as the other thing can anyways served by the Webserver without any PHP processing overhead.

nickvergessen · 2015-02-24T15:18:22Z

lib/private/appframework/http/io.php

+/**
+ * Very thin wrapper class to make output testable
+ */
+class IO {


OC IO, kappa 🙊
I'd prefer meaningful names, although IO might be known, InputOutput doesn't hurt either?

IO is very common, no need to fully write it, InputOutput on the other hand sounds weird.

Seems the class is only doing output, so I'm going to rename it to Output

nickvergessen · 2015-02-24T15:22:10Z

Didn't test, but looks good to me

oparoz · 2015-02-24T20:02:28Z

Does this work with all supported resources? Encrypted files, S3, SMB, NFS, external cloud, etc. ?
It seems to me that this class will always need to be sub-classed to define various headers (content-type, attachment, mimetype, etc.). Am I correct?

As a concrete example, I would use the exact same class, but extend StreamResponse, modify the ctor and pass in a path instead of a string.
https://github.com/interfasys/galleryplus/blob/master/http/imageresponse.php

BernhardPosselt · 2015-02-24T20:36:11Z

@oparoz if it supports streams, yes it will work. You can simply inherit from StreamResponse and set the headers in the constructor and pass the filepath to the parent constructor. Depending on your needs you may want to use mod xsendfile though since serving the file with PHP is painfully slow.

You can do that by implementing ICallbackResponse and setting the appropriate headers and send a PR if you are interested.

oparoz · 2015-02-24T20:55:13Z

@Raydiation - Thanks. I'll give it a try once this is merged then. I probably won't use mod xsendfile though as pictures are not that big.

if it supports streams, yes it will work

Will it fail gracefully if the resource is of the wrong type? To me the AppFramework is an abstraction layer. I don't want to have to think about where the resource is located. I ask for a file, I'm given a path, I send it to the browser.
Am I wrong? This can be left to discuss elsewhere btw.

BernhardPosselt · 2015-02-24T21:23:57Z

Fail gracefully as in wont output anything ;)

oparoz · 2015-02-24T21:26:57Z

Hmmm ;)
How about an error code in case of a failure or a timeout? I'm guessing it's the responsibility of the implemented call back?
I need to find time to play with this with a concrete example...

BernhardPosselt · 2015-02-27T09:30:17Z

Ok, updated the PR:

IO is now called Output
The callback receives an instance of IOutput so the response becomes testable
StreamResponse does not stream the file if the status is not modified
StreamResponse handles the case that the file does not exist (sends a 404) or if the streaming fails (400). I've also tried to get 403 in but it seems there is no way to tell if a file is readable because is_readable($path) does not work on streams

MorrisJobke · 2015-02-27T09:35:40Z

lib/private/appframework/http/output.php

+	}
+
+	/**
+     * @param string $path


Intendation is a bit shifted

MorrisJobke · 2015-02-27T09:36:38Z

lib/public/appframework/http/streamresponse.php

+		// handle caching
+		if ($output->getHttpResponseCode() !== Http::STATUS_NOT_MODIFIED) {
+			if (!file_exists($this->filePath)) {
+				$output->setHttpResponseCode(HTTP::STATUS_NOT_FOUND);


UPPERCASE vs Capitalcase?

MorrisJobke · 2015-02-27T09:37:18Z

Looks good beside my comments

MorrisJobke · 2015-02-27T09:56:49Z

tests/lib/appframework/AppTest.php

@@ -24,6 +24,10 @@

 namespace OC\AppFramework;

+use OCP\AppFramework\Http\Response;
+use OCP\AppFramework\Http\IO;


Is this really needed? And then it's misspelled;)

ghost · 2015-02-27T12:00:39Z

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/10051/
Test PASSed.

LukasReschke · 2015-02-27T12:09:11Z

Squash? 🙈 – 6 commits that say "Fix indention" are not really helpful 🙈

BernhardPosselt · 2015-02-27T12:45:29Z

Will squash once there are two thumbs up ;)

MorrisJobke · 2015-02-27T14:39:24Z

Works 👍

First stab at the StreamResponse, see #12988 The idea is to use an interface ICallbackResponse (I'm not 100% happy with the name yet, suggestions?) that allow the response to output things in its own way, for instance stream the file using readfile Unittests are atm lacking, plan is to check if a mock of ICallbackResponse will be used by calling its callback (also unhappy with this name) method Usage is: $response = new StreamResponse('path/to/file'); rename io to output, add additional methods and handle error and not modified cases when using StreamResponse fix indention and uppercasing, also handle forbidden cases fix indention fix indention no forbidden, figuring out if a file is really readable is too complicated to get to work across OSes and streams remove useless import remove useless import fix intendation

AppFramework StreamResponse

scrutinizer-notifier · 2015-02-27T15:24:12Z

The inspection completed: 1 new issues, 18 updated code elements

scrutinizer-notifier · 2015-02-27T15:24:12Z

The inspection completed: 1 new issues, 18 updated code elements

BernhardPosselt added this to the 8.1-next milestone Jan 22, 2015

BernhardPosselt added the enhancement label Jan 22, 2015

LukasReschke reviewed Jan 22, 2015
View reviewed changes

oparoz mentioned this pull request Jan 23, 2015

Fixing preview caching #13609

Closed

11 tasks

oparoz added the 2 - Developing label Jan 23, 2015

BernhardPosselt added 3 - To Review and removed 2 - Developing labels Jan 26, 2015

LukasReschke reviewed Feb 12, 2015
View reviewed changes

lib/public/appframework/http/streamresponse.php

}

}

Copy link

Member

LukasReschke Feb 12, 2015

Choose a reason for hiding this comment

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

Newline at EoF

LukasReschke force-pushed the streamresponse branch from ab7b845 to b716c3b Compare February 16, 2015 14:19

nickvergessen reviewed Feb 24, 2015
View reviewed changes

MorrisJobke reviewed Feb 27, 2015
View reviewed changes

lib/private/appframework/http/output.php

}

/**

* @param string $path

Copy link

Contributor

MorrisJobke Feb 27, 2015

Choose a reason for hiding this comment

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

Intendation is a bit shifted

MorrisJobke reviewed Feb 27, 2015
View reviewed changes

BernhardPosselt force-pushed the streamresponse branch from a355128 to d0852d7 Compare February 27, 2015 09:52

MorrisJobke reviewed Feb 27, 2015
View reviewed changes

BernhardPosselt force-pushed the streamresponse branch from cc60db1 to 95239ad Compare February 27, 2015 14:42

BernhardPosselt pushed a commit that referenced this pull request Feb 27, 2015

Merge pull request #13616 from owncloud/streamresponse

970b14d

AppFramework StreamResponse

BernhardPosselt merged commit 970b14d into master Feb 27, 2015

BernhardPosselt deleted the streamresponse branch February 27, 2015 14:43

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

AppFramework StreamResponse #13616

AppFramework StreamResponse #13616

Conversation

BernhardPosselt commented Jan 22, 2015

DeepDiver1975 commented Jan 22, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BernhardPosselt commented Jan 23, 2015

BernhardPosselt commented Jan 23, 2015

Choose a reason for hiding this comment

LukasReschke commented Feb 16, 2015

BernhardPosselt commented Feb 16, 2015

LukasReschke commented Feb 24, 2015

LukasReschke commented Feb 24, 2015

BernhardPosselt commented Feb 24, 2015

LukasReschke commented Feb 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nickvergessen commented Feb 24, 2015

oparoz commented Feb 24, 2015

BernhardPosselt commented Feb 24, 2015

oparoz commented Feb 24, 2015

BernhardPosselt commented Feb 24, 2015

oparoz commented Feb 24, 2015

BernhardPosselt commented Feb 27, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MorrisJobke commented Feb 27, 2015

Choose a reason for hiding this comment

ghost commented Feb 27, 2015

LukasReschke commented Feb 27, 2015

BernhardPosselt commented Feb 27, 2015

MorrisJobke commented Feb 27, 2015

scrutinizer-notifier commented Feb 27, 2015

scrutinizer-notifier commented Feb 27, 2015