Change file/image field serializers to return a dict with additional metadata #115

lukasgraf · 2016-05-26T17:15:40Z

The Archetypes part of this needs a thorough review, I was struggling slightly with the different File / Image field type's APIs.

This changes the serialization format for file and image fields from returning a simple string to returning dictionaries that contain the download URL in a download key, according to #9 and #10.

There's also a change in behavior regarding empty file / image fields: Previously, those were still serialized to an URL that would have returned a 404 (or 500, not exactly sure). Now they'll be serialized to None.

RelationValue to a NamedBlobFile field

lukasgraf · 2016-05-26T17:19:09Z

src/plone/restapi/tests/test_atfield_serializer.py

+
+        url = u'http://nohost/plone/doc1/@@download/testFileField'
+        self.assertEqual(
+            {u'filename': '',


This part confused me - I wasn't able to set a filename for an atapi.FileField for the life of me. Does this field type actually support filenames? I wasn't able to find a clear reference for or against that. @buchi you probably know this.

To set the filename, you need to do (also works for AT blob fields):

file.getField('file').get(file).setFilename('newname.txt')

@erral thank you, that works. Though after looking at it again, it also seems to work for that field type by passing a keyword argument to the mutator - which I somehow couldn't get to work at first for that field type.

We've been hit several times by the filenames (specially when running large imports), I think it's a bit obscure 😉

tisto · 2016-05-26T17:46:47Z

@lukasgraf Awesome!!! I will make a release as soon as this pr has been merged!

buchi · 2016-05-27T08:12:20Z

I strongly recommend to include the image dimensions in the output. Without the dimensions it's not possible to render "good" image tags.

See also Google Page Speed Guide: "If no dimensions are specified in the containing document, or if the dimensions specified don't match those of the actual images, the browser will require a reflow and repaint once the images are downloaded. To prevent reflows, specify the width and height of all images, either in the HTML tag, or in CSS."

buchi · 2016-05-27T08:15:13Z

docs/source/_json/image.json

-    "thumb": "http://localhost:55001/plone/image/@@images/image/thumb", 
-    "tile": "http://localhost:55001/plone/image/@@images/image/tile"
+    "content-type": "image/png", 
+    "download": "http://localhost:55001/plone/image/@@images/image", 


I would only include either the "download" or the "original" link, because both point to the same url.

Ok, I'll drop the original scale.

lukasgraf · 2016-05-27T08:16:11Z

Ok, I'll update the PR to also include image dimensions with the scales.

tisto · 2016-05-27T08:35:52Z

@buchi +1 for including the image dimensions. I had the same feeling, but did not want to complain before looking into it. :)

As soon as we have framing in place we can minimize the output if we need to...

lukasgraf · 2016-05-27T09:19:43Z

@buchi @tisto updated:

Also got setting filename for atapi.FileField via mutator kwargs to work, simplified mutator call (always pass kwargs) (squashed)
Dropped original scale from list of scales (squashed)
Included dimensions in image scales, updated docs & JSON dumps accordingly
Factored out the scale / imaging related helper functions into plone.restapi.imaging, since duplication got out of hand

buchi · 2016-05-27T12:59:39Z

src/plone/restapi/serializer/atfields.py

+        result = {
+            'filename': self.field.getFilename(self.context),
+            'content-type': self.field.getContentType(self.context),
+            'size': self.field .get_size(self.context),


unnecessary space after self.field

buchi · 2016-05-27T13:04:54Z

The returned image dimensions should be the real dimensions of the scaled image and not the scales defined in image scaling. E.g. when scaling an image of size 500 x 400 to an image scale of 200 x 200 the resulting image will have the size 200 x 160, not 200 x 200 because the scaling is maintaining aspect ratio.

lukasgraf · 2016-05-27T13:14:05Z

@buchi good point, I'll look into this.

instead of just the download URL.

instead of just the download URLs to image scales.

lukasgraf · 2016-05-27T21:44:16Z

@buchi @tisto updated:

Included the original image's width and height as top-level attributes
Return the actual dimensions of the scales (maintaining aspect ratio)
Updated docs and JSON dumps
Used a test image closer to real life in tests to demonstrate aspect ratio handling
Fixed unnecessary spaces

Using the @@images ImageScaling implementations for getting at the actual scale
dimensions would in theory be possible, but has some downsides:

Accessing a scale might trigger on-demand rendering, killing performance and possibly causing writes on GET.

I haven't had the time to definitely confirm this, but depending on what ImageScaling implementation is being used, accessing a scale might not be free of side-effects.
Different ImageScale implementations seem to have slightly different APIs, and while a first attempt at using it seemed to work, getting a clean, abstracted solution for the different field types, Archetypes vs. Dexterity and Plone 4 vs 5 turned out to be non-trivial.

I briefly discussed this with @buchi, and I went with a solution that basically just applies the aspect ratio of the original image to the scale's (usually square) bounding box, which should result in the final scaled image's actual dimensions.

There's a couple boundary conditions to handle, but nothing too major:

How to deal with fractional pixel dimensions - truncate or round. Plone seems to truncate, so I did the same.
Don't produce dimensions with zero pixels on one side as a result of integer truncation
How to deal with scales that exceed the original image's dimensions:
For the default named image scales, Plone doesn't upscale images, it just returns the image in it's original dimensions. We do the same.

buchi · 2016-05-29T08:42:13Z

docs/source/serialization.rst

+      "..." : {}
+    }
+
+(For non-square images, those will be the actual scaled images' dimensions,


I would simply drop this remark because maintaining aspect ratio is not related to square or non-square images. One could also define non-square image sizes (e.g. 600 x 400).

lukasgraf · 2016-05-29T12:12:13Z

@buchi updated

tisto · 2016-05-29T15:58:53Z

Wohoo! I will go over the docs one last time and do some final checks and then make a first alpha release!

Fix strange test that for some reason assigned a

f949e51

RelationValue to a NamedBlobFile field

tisto added the in progress label May 26, 2016

lukasgraf reviewed May 26, 2016
View reviewed changes

lukasgraf mentioned this pull request May 26, 2016

First alpha release #111

Closed

3 tasks

lukasgraf added the 32 needs: review label May 26, 2016

lukasgraf force-pushed the file-image-download branch from aaeeec0 to 6e93384 Compare May 27, 2016 06:54

buchi reviewed May 27, 2016
View reviewed changes

lukasgraf force-pushed the file-image-download branch from 6e93384 to abf9ab4 Compare May 27, 2016 09:13

lukasgraf force-pushed the file-image-download branch from abf9ab4 to f043db0 Compare May 27, 2016 09:25

buchi reviewed May 27, 2016
View reviewed changes

lukasgraf added 5 commits May 27, 2016 15:16

Change file field serialization to return a dict with metadata

ff79949

instead of just the download URL.

Change image field serialization to return a dict with metadata

d38ff2c

instead of just the download URLs to image scales.

Factor out imaging related helper functions.

bcbd0bc

Image field serialization: Include actual dimensions for scales.

8824f89

Image field serialization: Include original image's width and height.

31fad72

lukasgraf force-pushed the file-image-download branch from f043db0 to d9d83e6 Compare May 27, 2016 21:21

buchi reviewed May 29, 2016
View reviewed changes

Update docs about file/image field serialization.

c2d011e

lukasgraf force-pushed the file-image-download branch from d9d83e6 to c2d011e Compare May 29, 2016 12:01

buchi merged commit fb77e42 into master May 29, 2016

buchi deleted the file-image-download branch May 29, 2016 13:22

tisto removed the in progress label May 29, 2016

This was referenced May 29, 2016

Implement File API #9

Closed

Implement Image API #10

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change file/image field serializers to return a dict with additional metadata #115

Change file/image field serializers to return a dict with additional metadata #115

lukasgraf commented May 26, 2016 •

edited

lukasgraf May 26, 2016

erral May 27, 2016

lukasgraf May 27, 2016

erral May 27, 2016

tisto commented May 26, 2016

buchi commented May 27, 2016

buchi May 27, 2016

lukasgraf May 27, 2016

lukasgraf commented May 27, 2016

tisto commented May 27, 2016

lukasgraf commented May 27, 2016

buchi May 27, 2016

buchi commented May 27, 2016

lukasgraf commented May 27, 2016

lukasgraf commented May 27, 2016

buchi May 29, 2016

lukasgraf commented May 29, 2016

tisto commented May 29, 2016

Change file/image field serializers to return a dict with additional metadata #115

Change file/image field serializers to return a dict with additional metadata #115

Conversation

lukasgraf commented May 26, 2016 • edited

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

tisto commented May 26, 2016

buchi commented May 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasgraf commented May 27, 2016

tisto commented May 27, 2016

lukasgraf commented May 27, 2016

Choose a reason for hiding this comment

buchi commented May 27, 2016

lukasgraf commented May 27, 2016

lukasgraf commented May 27, 2016

Choose a reason for hiding this comment

lukasgraf commented May 29, 2016

tisto commented May 29, 2016

lukasgraf commented May 26, 2016 •

edited