Streaming download from Swift in files_external

[tjdett]

Speeds up downloads as they no longer need to buffer completely on the ownCloud server before being sent to the client.

A downside of this implementation is that the file can only be traversed once. ownCloud never actually attempts to do this, and r+ offers this behaviour if required.

[scrutinizer-notifier]

A new inspection was created.

[karlitschek]

@butonic one more question for you.

[RobinMcCorkell]

cc @icewind1991

[LukasReschke]

@owncloud-bot This is okay to test.

@DeepDiver1975 CLA bot? ;-)

[icewind1991]

Code looks good from what I know about swift, haven't got the chance to test atm

[icewind1991]

Do we have swift covered by jenkins?

[RobinMcCorkell]

@icewind1991 #14077

[butonic]

cool 👍 wait for #14077, then rebase please

[PVince81]

I ran the tests locally and beside the expected fails, this branch has an additional fail:

2) Test\Files\Storage\Swift::testFOpen
Failed asserting that Resource id #5992 is false.

/var/www/html/owncloud/tests/lib/files/storage/storage.php:349

To run the unit tests, I did this:

• edit "tests/enable_all.php" and add a line enableApp('files_external');
• edit "apps/files_external/tests/config.php" and adjust the swift config to your test server settings
• run ./autotest.sh sqlite apps/files_external/tests/backends/swift.php

[icewind1991]

Sounds like it needs a check if the file exists and return false if it doesnt

[DanielTosello]

Hey all, i'm new. I'm not used to git yet so please be patient with me.

It's a stream, so actually needs is_resource() or to test response header values.

[PVince81]

Looks like it's still happening:

1) Test\Files\Storage\Swift::testFOpen
Failed asserting that NULL is false.

/srv/www/htdocs/owncloud/tests/lib/files/storage/storage.php:349

@DanielTosello to run the unit tests you will need docker, and then run:

./autotest-external.sh sqlite swift-ceph 

[PVince81]

seems that you need to return false instead of NULL if fopen failed 😄

[DanielTosello]

@PVince81 Thanks for your help. I'm currently wrestling with docker's default config under ubuntu. Using loopback devices seems to crash xenopathic's image on boot.

EDIT: I thought i'd gotten it working under fedora but i'm currently getting 503 on every test. Assume that's a docker config issue on my end... (RESOLVED: firewalld; perm levels)

I'm also seeing examples of this: #7897 (comment) when running the tests against a swift instance and they make very little sense to me. I don't suppose you remember how you resolved that?

[DanielTosello]

This was more complex than expected. Guzzle doesn't close streams that 404, so they're still valid resources even if they're empty. It was necessary to extract response text from response metadata explicitly.

[PVince81]

Unit tests pass locally now.
I pushed the commits to a local branch #21729 to wake up CI.

Code looks good from my POV 👍
Could possibly be reused for other storage types.

@butonic @icewind1991 second review ?

[PVince81]

@DanielTosello got curious about your solution and had a look how it was done for other storages types.
It looks like it might be even easier, see how it's done for DAV: https://github.com/owncloud/core/pull/18653/files#diff-782207b41e0a420a99054e41c7e7946dR344
Note that HttpClientService itself is just a Guzzle instance. Looks like setting stream to true would be enough.

Did you consider that approach too or did not know about it ? (I also just found out today 😄)

[DanielTosello]

@PVince81 I didn't know, & the Guzzle docs i found are not too good with regards to how to handle streams... i learned quite a few new things in doing this (including fun docker problems), and some of them are going to be really useful going forward.

As you can probably see from my addition here, the biggest problem i had was actually locating the client response in the stream metadata package, so if there's a solution that mitigates the need for that i'm all for it.

[DeepDiver1975]

@DanielTosello @tjdett did you sign the contribution agreement already?

[PVince81]

Yes, let me find the statement from the other PR

[PVince81]

MIT license statement: #21008 (comment)

[PVince81]

@DeepDiver1975 CI passed in the CI ticket: #21729

Merge ?