Contacts API: replace raw image data with url #25081

Merged
merged 6 commits into from Jun 21, 2016

Projects

None yet

7 participants

@georgehrke
Contributor
georgehrke commented Jun 13, 2016 edited

fixes #23345

  • adjust tests
@georgehrke georgehrke added this to the 9.1-current milestone Jun 13, 2016
@mention-bot

By analyzing the blame information on this pull request, we identified @schiessle, @DeepDiver1975 and @nickvergessen to be potential reviewers

@DeepDiver1975 DeepDiver1975 and 1 other commented on an outdated diff Jun 14, 2016
apps/dav/appinfo/routes.php
+ * it under the terms of the GNU Affero General Public License, version 3,
+ * as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program. If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+return [
+ 'routes' => [
+ ['name' => 'photos#get', 'url' => '/contactsphoto/{addressbookId}/{contactUri}', 'verb' => 'GET'],
@DeepDiver1975
DeepDiver1975 Jun 14, 2016 Member

hmmm ... I would have built this as part of the webdav routes

please note there are only 2 public http based protocols we support: webdav and ocs api.
routes like above are considered private and should not be exposed to 3rdparty - the mail and contacts apps are treated as 3rdparty

@georgehrke
georgehrke Jun 14, 2016 Contributor

I'm not so sure about the webdav route, because this change only affects the PHP based Contacts API. Adding this to the WebDAV routes would require apps like the Mail app to implement a dav client just to fetch contact images πŸ˜•

@DeepDiver1975
DeepDiver1975 Jun 14, 2016 Member

Adding this to the WebDAV routes would require apps like the Mail app to implement a dav client just to fetch contact images πŸ˜•

A GET is a GET is a GET .... πŸ™Š

@georgehrke
georgehrke Jun 14, 2016 Contributor

A GET is a GET is a GET ....

Well, there is some truth to this πŸ™ˆ

@georgehrke
georgehrke Jun 15, 2016 Contributor

Is there some "easy" straight forward way to register routes or do I need to write a plugin like in https://github.com/owncloud/core/blob/master/apps/dav/lib/Comments/CommentsPlugin.php#L86?

@DeepDiver1975
DeepDiver1975 Jun 15, 2016 Member

here we go ... #25121

@DeepDiver1975 DeepDiver1975 and 1 other commented on an outdated diff Jun 16, 2016
apps/dav/lib/Server.php
@@ -103,6 +104,7 @@ public function __construct(IRequest $request, $baseUri) {
// addressbook plugins
$this->server->addPlugin(new \OCA\DAV\CardDAV\Plugin());
$this->server->addPlugin(new VCFExportPlugin());
+ $this->server->addPlugin(new ImageExportPlugin());
@DeepDiver1975
DeepDiver1975 Jun 16, 2016 Member

Must be added to the serverfactory as well for v1 routes

@georgehrke
georgehrke Jun 17, 2016 Contributor

done

@georgehrke georgehrke commented on the diff Jun 16, 2016
apps/dav/lib/CardDAV/AddressBookImpl.php
* @param VCard $vCard
* @return array
*/
- protected function vCard2Array(VCard $vCard) {
- $result = [];
+ protected function vCard2Array($uri, VCard $vCard) {
+ $result = [
@georgehrke
georgehrke Jun 16, 2016 Contributor

@DeepDiver Is there some helper method to generate the full carddav url here?
I have access to the principal uris, but no direct access to addressbooks/users/admin/contacts/

@DeepDiver1975
DeepDiver1975 Jun 16, 2016 Member

no helper method at hand - the addressbook name is in addressBookInfo or in the addrebook instance.

No idea if this helps. THX

@georgehrke
Contributor

please review @DeepDiver1975 @ChristophWurst @PVince81

@ChristophWurst
Contributor

πŸ‘ nice, works!

@DeepDiver1975 DeepDiver1975 commented on an outdated diff Jun 20, 2016
apps/dav/lib/CardDAV/ImageExportPlugin.php
+ * You should have received a copy of the GNU Affero General Public License, version 3,
+ * along with this program. If not, see <http://www.gnu.org/licenses/>
+ *
+ */
+
+namespace OCA\DAV\CardDAV;
+
+use Sabre\CardDAV\Card;
+use Sabre\DAV\Server;
+use Sabre\DAV\ServerPlugin;
+use Sabre\HTTP\RequestInterface;
+use Sabre\HTTP\ResponseInterface;
+use Sabre\VObject\Parameter;
+use Sabre\VObject\Reader;
+
+class ImageExportPlugin extends ServerPlugin {
@DeepDiver1975
DeepDiver1975 Jun 20, 2016 Member

this plugin deserves some unit tests as well πŸ™ˆ

@DeepDiver1975 DeepDiver1975 commented on an outdated diff Jun 20, 2016
apps/dav/lib/CardDAV/ImageExportPlugin.php
+ $vObject = $this->readCard($node->get());
+ if (!$vObject->PHOTO) {
+ return false;
+ }
+
+ $photo = $vObject->PHOTO;
+ try {
+ /** @var Parameter $type */
+ $type = $photo->parameters()['TYPE'];
+ $val = $photo->getValue();
+ return [
+ 'Content-Type' => $type->getValue(),
+ 'body' => $val
+ ];
+ } catch(\Exception $ex) {
+// $this->logger->debug($ex->getMessage());
@DeepDiver1975
DeepDiver1975 Jun 20, 2016 Member

remove or add logger

@DeepDiver1975
Member

@ChristophWurst @Gomez did you test this with the mail app? does it work? THX

@Gomez
Member
Gomez commented Jun 20, 2016

Yeah works in Mail! Nice @georgehrke

georgehrke and others added some commits Jun 13, 2016
@georgehrke @DeepDiver1975 georgehrke add uri to AddressBookImpl array 61c704a
@DeepDiver1975 DeepDiver1975 Introduce ImageExportPlugin for CardDav 067ec21
@georgehrke @DeepDiver1975 georgehrke add plugin to v1 routes a890571
@georgehrke @DeepDiver1975 georgehrke replace binary contact photo with link 543e119
@georgehrke @DeepDiver1975 georgehrke update tests 9c7689e
@DeepDiver1975 DeepDiver1975 and 1 other commented on an outdated diff Jun 20, 2016
apps/dav/tests/unit/CardDAV/ImageExportPluginTest.php
+ $this->plugin = new ImageExportPlugin($this->logger);
+ $this->plugin->initialize($this->server);
+
+ $result = $this->plugin->getPhoto($card);
+ $this->assertEquals($expected, $result);
+ }
+
+ public function providesPhotoData() {
+ return [
+ 'empty vcard' => [false, ''],
+ 'vcard without PHOTO' => [false, "BEGIN:VCARD\r\nVERSION:3.0\r\nPRODID:-//Sabre//Sabre VObject 3.5.0//EN\r\nUID:12345\r\nFN:12345\r\nN:12345;;;;\r\nEND:VCARD\r\n"],
+ 'vcard 3 with PHOTO' => [['Content-Type' => 'image/jpeg', 'body' => '12345'], "BEGIN:VCARD\r\nVERSION:3.0\r\nPRODID:-//Sabre//Sabre VObject 3.5.0//EN\r\nUID:12345\r\nFN:12345\r\nN:12345;;;;\r\nPHOTO;ENCODING=b;TYPE=JPEG:MTIzNDU=\r\nEND:VCARD\r\n"],
+ //
+ // TODO: these three below are not working - needs debugging
+ //
+// 'vcard 3 with PHOTO URL' => [['Content-Type' => 'image/jpeg', 'body' => '12345'], "BEGIN:VCARD\r\nVERSION:3.0\r\nPRODID:-//Sabre//Sabre VObject 3.5.0//EN\r\nUID:12345\r\nFN:12345\r\nN:12345;;;;\r\nPHOTO;TYPE=JPEG:http://example.org/photo.jpg\r\nEND:VCARD\r\n"],
@DeepDiver1975
DeepDiver1975 Jun 20, 2016 Member

@georgehrke mind debugging these 3 cases and adjust the code accordingly?

@georgehrke
georgehrke Jun 20, 2016 Contributor

Sure, I can take a look tomorrow

@georgehrke
georgehrke Jun 21, 2016 Contributor

How is http://example.org/photo.jpg supposed to return '12345' in the first place?

@georgehrke
georgehrke Jun 21, 2016 Contributor

anyway, 'vcard 3 with PHOTO URL' seems to be a bug in Sabre/Vobject because getValueType() returns BINARY

@georgehrke
georgehrke Jun 21, 2016 Contributor

'vcard 4 with PHOTO' seems to be a bug in Sabre/Vobject as well, getValueType() returns URI although it should be BINARY

@georgehrke
georgehrke Jun 21, 2016 Contributor

'vcard 4 with PHOTO URL' is fixed now

@DeepDiver1975
DeepDiver1975 Jun 21, 2016 Member

How is http://example.org/photo.jpg supposed to return '12345' in the first place?

it's not πŸ™ˆ

@DeepDiver1975
DeepDiver1975 Jun 21, 2016 Member

please file issues upstream - thx

@DeepDiver1975 @georgehrke DeepDiver1975 Adding unit tests
836def3
@DeepDiver1975
Member

πŸ‘

@DeepDiver1975 DeepDiver1975 merged commit 1452b74 into master Jun 21, 2016

20 checks passed

cla-bot-core Build #4937 succeeded in 45 sec
Details
continuous-integration/php-5.4 Build #5147 succeeded in 7 min 10 sec
Details
core-ci-linux-jsunit/database=sqlite,label=SLAVE Build #63028 succeeded in 1 min 36 sec
Details
core-ci-linux/database=mysql,label=SLAVE Build #31941 succeeded in 11 min
Details
core-ci-linux/database=oci,label=SLAVE Build #31941 succeeded in 35 min
Details
core-ci-linux/database=pgsql,label=SLAVE Build #31941 succeeded in 11 min
Details
core-ci-linux/database=sqlite,label=SLAVE Build #31941 succeeded in 5 min 52 sec
Details
ocs-api-integration-tests-ci Build #11761 succeeded in 45 min
Details
server-master-linux-externals-ci/database=sqlite,external=smb-silvershell,label=SLAVE Build #11266 succeeded in 1 min 51 sec
Details
server-master-linux-externals-ci/database=sqlite,external=swift-ceph,label=SLAVE Build #11266 succeeded in 5 min 54 sec
Details
server-master-linux-externals-ci/database=sqlite,external=webdav-ownCloud,label=SLAVE Build #11266 succeeded in 7 min 41 sec
Details
server-master-linux-externals-smb-windows-ext-ci/database=sqlite,external=smb-windows,label=master Build #22482 succeeded in 4 min 9 sec
Details
server-master-linux-php7-ci/database=sqlite,label=SLAVE Build #40259 succeeded in 4 min 37 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=litmus,mirallBranch=v2.0.2,slave=SMASH Build #15580 succeeded in 6 min 43 sec
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@0,mirallBranch=v2.0.2,slave=SMASH Build #15580 succeeded in 47 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_basicSync@1,mirallBranch=v2.0.2,slave=SMASH Build #15580 succeeded in 44 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_shareLink,mirallBranch=v2.0.2,slave=SMASH Build #15580 succeeded in 16 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePermissions,mirallBranch=v2.0.2,slave=SMASH Build #15580 succeeded in 40 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationGroups,mirallBranch=v2.0.2,slave=SMASH Build #15580 succeeded in 19 min
Details
smashbox-on-docker-ci/DOCKER_IMAGE=ubuntu_oc_lamp-git,TEST_NAME=test_sharePropagationInsideGroups,mirallBranch=v2.0.2,slave=SMASH Build #15580 succeeded in 24 min
Details
@DeepDiver1975 DeepDiver1975 deleted the dav_contacts_image_uri branch Jun 21, 2016
@jancborchardt
Member

Oooh this is awesome! :) Great work!

Will it be backported as well?

@DeepDiver1975
Member

@georgehrke please prepare backport pr - we want to make oc9 users happy as well πŸ™ˆ

@georgehrke
Contributor

sure

@PVince81
Collaborator

stable9: #25219

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment