More sensible default formats for image auto conversion (#18615)

* Prevent auto conversion of png to jpg images

* Create red-swans-march.md

* Update transformation tests

* Update blackbox test

* Fix image allocation

* Fix test again :-)

* Convert formats with transparency support to png (if no accept header)

* Update tests & add final fallback

* Update changeset

* Update blackbox test

---------

Co-authored-by: rijkvanzanten <rijkvanzanten@me.com>

api/src/controllers/assets.ts

@@ -11,7 +11,7 @@ import logger from '../logger.js';
 import useCollection from '../middleware/use-collection.js';
 import { AssetsService } from '../services/assets.js';
 import { PayloadService } from '../services/payload.js';
-import type { TransformationParams } from '../types/assets.js';
+import type { TransformationFormat, TransformationParams } from '../types/assets.js';
 import { TransformationMethods } from '../types/assets.js';
 import asyncHandler from '../utils/async-handler.js';
 import { getCacheControlHeader } from '../utils/get-cache-headers.js';
@@ -144,24 +144,21 @@ router.get(
 
 		const vary = ['Origin', 'Cache-Control'];
 
-		const transformation: TransformationParams = res.locals['transformation'].key
+		const transformationParams: TransformationParams = res.locals['transformation'].key
 			? (res.locals['shortcuts'] as TransformationParams[]).find(
 					(transformation) => transformation['key'] === res.locals['transformation'].key
 			  )
 			: res.locals['transformation'];
 
-		if (transformation.format === 'auto') {
-			let format: Exclude<TransformationParams['format'], 'auto'>;
+		let acceptFormat: TransformationFormat | undefined;
 
+		if (transformationParams.format === 'auto') {
 			if (req.headers.accept?.includes('image/avif')) {
-				format = 'avif';
+				acceptFormat = 'avif';
 			} else if (req.headers.accept?.includes('image/webp')) {
-				format = 'webp';
-			} else {
-				format = 'jpg';
+				acceptFormat = 'webp';
 			}
 
-			transformation.format = format;
 			vary.push('Accept');
 		}
 
@@ -185,7 +182,7 @@ router.get(
 			}
 		}
 
-		const { stream, file, stat } = await service.getAsset(id, transformation, range);
+		const { stream, file, stat } = await service.getAsset(id, { transformationParams, acceptFormat }, range);
 
 		const filename = req.params['filename'] ?? file.filename_download;
 		res.attachment(filename);

api/src/services/assets.ts

@@ -17,7 +17,7 @@ import { RangeNotSatisfiableException } from '../exceptions/range-not-satisfiabl
 import { ServiceUnavailableException } from '../exceptions/service-unavailable.js';
 import logger from '../logger.js';
 import { getStorage } from '../storage/index.js';
-import type { AbstractServiceOptions, File, Transformation, TransformationParams } from '../types/index.js';
+import type { AbstractServiceOptions, File, Transformation, TransformationSet } from '../types/index.js';
 import { getMilliseconds } from '../utils/get-milliseconds.js';
 import * as TransformationUtils from '../utils/transformations.js';
 import { AuthorizationService } from './authorization.js';
@@ -35,7 +35,7 @@ export class AssetsService {
 
 	async getAsset(
 		id: string,
-		transformation: TransformationParams,
+		transformation: TransformationSet,
 		range?: Range
 	): Promise<{ stream: Readable; file: any; stat: Stat }> {
 		const storage = await getStorage();

api/src/types/assets.ts

@@ -67,9 +67,16 @@ export type Transformation = TransformationMap[keyof TransformationMap];
 
 export type TransformationResize = Pick<ResizeOptions, 'width' | 'height' | 'fit' | 'withoutEnlargement'>;
 
+export type TransformationFormat = 'jpg' | 'jpeg' | 'png' | 'webp' | 'tiff' | 'avif';
+
 export type TransformationParams = {
 	key?: string;
 	transforms?: Transformation[];
-	format?: 'auto' | 'jpg' | 'jpeg' | 'png' | 'webp' | 'tiff' | 'avif';
+	format?: TransformationFormat | 'auto';
 	quality?: number;
 } & TransformationResize;
+
+export type TransformationSet = {
+	transformationParams: TransformationParams;
+	acceptFormat?: TransformationFormat | undefined;
+};

api/src/utils/transformations.test.ts

@@ -27,7 +27,7 @@ const inputFile: File = {
 
 describe('resolvePreset', () => {
 	test('Prevent input mutation #18301', () => {
-		const inputData: TransformationParams = {
+		const transformationParams: TransformationParams = {
 			key: 'system-small-cover',
 			format: 'jpg',
 			transforms: [
@@ -42,9 +42,9 @@ describe('resolvePreset', () => {
 			],
 		};
 
-		resolvePreset(inputData, inputFile);
+		resolvePreset({ transformationParams }, inputFile);
 
-		expect(inputData.transforms).toStrictEqual([
+		expect(transformationParams.transforms).toStrictEqual([
 			[
 				'resize',
 				{
@@ -57,7 +57,7 @@ describe('resolvePreset', () => {
 	});
 
 	test('Add toFormat transformation', () => {
-		const inputData: TransformationParams = {
+		const transformationParams: TransformationParams = {
 			key: 'system-small-cover',
 			format: 'jpg',
 			quality: 80,
@@ -73,7 +73,7 @@ describe('resolvePreset', () => {
 			],
 		};
 
-		const output = resolvePreset(inputData, inputFile);
+		const output = resolvePreset({ transformationParams }, inputFile);
 
 		expect(output).toStrictEqual([
 			[
@@ -89,14 +89,14 @@ describe('resolvePreset', () => {
 	});
 
 	test('Add resize transformation', () => {
-		const inputData: TransformationParams = {
+		const transformationParams: TransformationParams = {
 			key: 'system-small-cover',
 			width: 64,
 			height: 64,
 			fit: 'cover',
 		};
 
-		const output = resolvePreset(inputData, inputFile);
+		const output = resolvePreset({ transformationParams }, inputFile);
 
 		expect(output).toStrictEqual([
 			[
@@ -110,6 +110,50 @@ describe('resolvePreset', () => {
 			],
 		]);
 	});
+
+	test('Resolve auto format (fallback)', () => {
+		const transformationParams: TransformationParams = {
+			format: 'auto',
+		};
+
+		const output = resolvePreset({ transformationParams }, inputFile);
+
+		expect(output).toStrictEqual([['toFormat', 'jpg', { quality: undefined }]]);
+	});
+
+	test('Resolve auto format (with accept header)', () => {
+		const transformationParams: TransformationParams = {
+			format: 'auto',
+		};
+
+		const output = resolvePreset({ transformationParams, acceptFormat: 'avif' }, inputFile);
+
+		expect(output).toStrictEqual([['toFormat', 'avif', { quality: undefined }]]);
+	});
+
+	test('Resolve auto format (format with transparency support)', () => {
+		const transformationParams: TransformationParams = {
+			format: 'auto',
+		};
+
+		const inputFileAvif = { ...inputFile, type: 'image/avif' };
+
+		const output = resolvePreset({ transformationParams }, inputFileAvif);
+
+		expect(output).toStrictEqual([['toFormat', 'png', { quality: undefined }]]);
+	});
+
+	test('Resolve auto format (original type)', () => {
+		const transformationParams: TransformationParams = {
+			format: 'auto',
+		};
+
+		const inputFilePng = { ...inputFile, type: 'image/png' };
+
+		const output = resolvePreset({ transformationParams }, inputFilePng);
+
+		expect(output).toStrictEqual([['toFormat', 'png', { quality: undefined }]]);
+	});
 });
 
 describe('maybeExtractFormat', () => {

api/src/utils/transformations.ts

@@ -1,26 +1,28 @@
-import type { File, Transformation, TransformationParams } from '../types/index.js';
+import type { File, Transformation, TransformationFormat, TransformationSet } from '../types/index.js';
 
-export function resolvePreset(input: TransformationParams, file: File): Transformation[] {
-	const transforms = input.transforms ? [...input.transforms] : [];
+export function resolvePreset({ transformationParams, acceptFormat }: TransformationSet, file: File): Transformation[] {
+	const transforms = transformationParams.transforms ? [...transformationParams.transforms] : [];
 
-	if (input.format || input.quality) {
+	if (transformationParams.format || transformationParams.quality) {
 		transforms.push([
 			'toFormat',
-			input.format || (file.type!.split('/')[1] as any),
+			getFormat(file, transformationParams.format, acceptFormat),
 			{
-				quality: input.quality ? Number(input.quality) : undefined,
+				quality: transformationParams.quality ? Number(transformationParams.quality) : undefined,
 			},
 		]);
 	}
 
-	if (input.width || input.height) {
+	if (transformationParams.width || transformationParams.height) {
 		transforms.push([
 			'resize',
 			{
-				width: input.width ? Number(input.width) : undefined,
-				height: input.height ? Number(input.height) : undefined,
-				fit: input.fit,
-				withoutEnlargement: input.withoutEnlargement ? Boolean(input.withoutEnlargement) : undefined,
+				width: transformationParams.width ? Number(transformationParams.width) : undefined,
+				height: transformationParams.height ? Number(transformationParams.height) : undefined,
+				fit: transformationParams.fit,
+				withoutEnlargement: transformationParams.withoutEnlargement
+					? Boolean(transformationParams.withoutEnlargement)
+					: undefined,
 			},
 		]);
 	}
@@ -28,6 +30,30 @@ export function resolvePreset(input: TransformationParams, file: File): Transfor
 	return transforms;
 }
 
+function getFormat(
+	file: File,
+	format: TransformationSet['transformationParams']['format'],
+	acceptFormat: TransformationSet['acceptFormat']
+): TransformationFormat {
+	const fileType = file.type?.split('/')[1] as TransformationFormat | undefined;
+
+	if (format) {
+		if (format !== 'auto') {
+			return format;
+		}
+
+		if (acceptFormat) {
+			return acceptFormat;
+		}
+
+		if (fileType && ['avif', 'webp', 'tiff'].includes(fileType)) {
+			return 'png';
+		}
+	}
+
+	return fileType || 'jpg';
+}
+
 /**
  * Try to extract a file format from an array of `Transformation`'s.
  */