torchbox / wagtail-grapple

A Wagtail app that makes building GraphQL endpoints a breeze!
https://wagtail-grapple.readthedocs.io/en/latest/
Other
151 stars 57 forks source link

Rendition queries using `preserveSvg` should default to `original` renditions for SVGs #389

Closed jams2 closed 2 months ago

jams2 commented 2 months ago

If I have an image field that may refer to both raster or SVG images, when querying that field I may pass the preserveSvg argument, so as to handle the fact that I might have SVGs in the set, and want to return them without error.

Example query:

query Image {
  images(offset: 2, limit: 2) {
    title
    id
    rendition(format: "webp", preserveSvg: true) {
      url     
    }
  }
}

Example response:

{
  "data": {
    "images": [
      {
        "id": "78",
        "rendition": null,
        "title": "tiger"
      },
      {
        "id": "77",
        "rendition": {
          "url": "https://my-bucket.s3.eu-west-1.amazonaws.com/images/Tsunami_by_hokusai_19th_century.format-webp.webp"
        },
        "title": "Tsunami_by_hokusai_19th_century"
      }
    ]
  },
  "errors": [
    {
      "locations": [
        {
          "column": 5,
          "line": 5
        }
      ],
      "message": "No valid filter specs for SVG. See https://docs.wagtail.org/en/stable/topics/images.html#svg-images for details.",
      "path": [
        "images",
        0,
        "rendition"
      ]
    }
  ]
}

As the source image for "tiger" is an SVG, it cannot be converted to webp. This causes an error. In this scenario my expectation would be that I receive an SVG rendition having its original dimensions.

Another scenario is querying with a mix of SVG safe and unsafe operations, e.g. rendition(width: 100, format: "webp", preserveSvg: true). In this case I would expect to receive any SVGs as width 100 renditions, alongside the other webp renditions.

Perhaps an excludeSvg flag would be useful too - to exclude any SVG images from the returned set.

zerolab commented 2 months ago

🤔 I thought we covered this use case with

https://github.com/torchbox/wagtail-grapple/blob/460060e6057981851d16addb299e4a1b95d8637f/grapple/types/images.py#L153-L161

unless I am missing a nuance

jams2 commented 2 months ago

We don't support an argument for requesting the "original" rendition — the expectation might be that by passing no width/height/fill/etc. args, you are requesting the original rendition, which is the behaviour for raster images. However, to_svg_safe_spec might return the empty string, causing the error to be thrown — the issue is the inconsistency between handling of SVG and raster images I think.

(it would also be good to provide an original boolean arg)

zerolab commented 2 months ago

ah, d'oh! you're right. I am torn between checking that filter_specs is empty and setting it to original, and adding the original boolean arg because the latter opens a can of worms.. and then we need to support variations etc etc. Happy to be convinved otherwise

jams2 commented 2 months ago

The former makes sense to me, and is in line with the current behaviour of raster image renditions which I think is fairly intuitive.

zerolab commented 2 months ago

https://github.com/torchbox/wagtail-grapple/releases/tag/v0.25.1