diff --git a/config/util/representation-conversion/default.json b/config/util/representation-conversion/default.json index 9cd2e64e4..3731ff557 100644 --- a/config/util/representation-conversion/default.json +++ b/config/util/representation-conversion/default.json @@ -15,12 +15,7 @@ "@id": "urn:solid-server:default:RepresentationConverter", "@type": "WaterfallHandler", "handlers": [ - { "@id": "urn:solid-server:default:MarkdownToHtmlConverter" }, { "@id": "urn:solid-server:default:DynamicJsonToTemplateConverter" }, - { - "@type": "IfNeededConverter", - "comment": "Only continue converting if the requester cannot accept the available content type" - }, { "comment": "Automatically finds a path through a set of converters from one type to another.", "@id": "urn:solid-server:default:ChainedConverter", diff --git a/src/index.ts b/src/index.ts index 1fe5047ce..5352a0bf6 100644 --- a/src/index.ts +++ b/src/index.ts @@ -261,7 +261,6 @@ export * from './storage/conversion/ErrorToJsonConverter'; export * from './storage/conversion/ErrorToQuadConverter'; export * from './storage/conversion/ErrorToTemplateConverter'; export * from './storage/conversion/FormToJsonConverter'; -export * from './storage/conversion/IfNeededConverter'; export * from './storage/conversion/MarkdownToHtmlConverter'; export * from './storage/conversion/PassthroughConverter'; export * from './storage/conversion/QuadToRdfConverter'; diff --git a/src/storage/conversion/ChainedConverter.ts b/src/storage/conversion/ChainedConverter.ts index 569727412..4704385d1 100644 --- a/src/storage/conversion/ChainedConverter.ts +++ b/src/storage/conversion/ChainedConverter.ts @@ -1,5 +1,7 @@ +import { BasicRepresentation } from '../../http/representation/BasicRepresentation'; import type { Representation } from '../../http/representation/Representation'; -import type { ValuePreference, ValuePreferences } from '../../http/representation/RepresentationPreferences'; +import { RepresentationMetadata } from '../../http/representation/RepresentationMetadata'; +import type { ValuePreferences } from '../../http/representation/RepresentationPreferences'; import { getLoggerFor } from '../../logging/LogUtil'; import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError'; import { NotImplementedHttpError } from '../../util/errors/NotImplementedHttpError'; @@ -14,24 +16,35 @@ type ConverterPreference = { outTypes: ValuePreferences; }; -/** - * A chain of converters that can go from `inTypes` to `outTypes`. - * `intermediateTypes` contains the exact types that have the highest weight when going from converter i to i+1. - */ type ConversionPath = { + /** + * List of converters used in the path. + */ converters: TypedRepresentationConverter[]; + /** + * The intermediate conversion types when going from converter i to i+1. + * Length is one less than the list of converters. + */ intermediateTypes: string[]; + /** + * The type on which this conversion path starts. + */ inType: string; + /** + * The types this path can generate. + * Weights indicate the quality of transforming to that specific type. + */ outTypes: ValuePreferences; -}; - -/** - * The result of choosing a specific output for a `ConversionPath`. - */ -type MatchedPath = { - path: ConversionPath; - outType: string; + /** + * The weight of the path matched against the output preferences. + * Will be 0 if the path does not match any of those preferences. + */ weight: number; + /** + * The output type for which this path has the highest weight. + * Value is irrelevant if weight is 0. + */ + outType: string; }; /** @@ -52,6 +65,9 @@ type MatchedPath = { * - The algorithm could start on both ends of a possible path and work towards the middle. * - When creating a path, store the list of unused converters instead of checking every step. * - Caching: https://github.com/solid/community-server/issues/832 + * - Making sure each intermediate type is only used once. + * - The TypedRepresentationConverter interface could potentially be updated + * so paths only differing in intermediate types can be combined. */ export class ChainedConverter extends RepresentationConverter { protected readonly logger = getLoggerFor(this); @@ -76,43 +92,25 @@ export class ChainedConverter extends RepresentationConverter { public async handle(input: RepresentationConverterArgs): Promise { const match = await this.findPath(input); - // No conversion needed - if (!this.isMatchedPath(match)) { - return input.representation; - } - - const { path, outType } = match; - this.logger.debug(`Converting ${path.inType} -> ${[ ...path.intermediateTypes, outType ].join(' -> ')}.`); + this.logger.debug(`Converting ${match.inType} -> ${[ ...match.intermediateTypes, match.outType ].join(' -> ')}.`); const args = { ...input }; - for (let i = 0; i < path.converters.length - 1; ++i) { - const type = path.intermediateTypes[i]; - args.preferences = { type: { [type]: 1 }}; - args.representation = await path.converters[i].handle(args); + const outTypes = [ ...match.intermediateTypes, match.outType ]; + for (let i = 0; i < match.converters.length; ++i) { + args.preferences = { type: { [outTypes[i]]: 1 }}; + args.representation = await match.converters[i].handle(args); } - // For the last converter we set the preferences to the best output type - args.preferences = { type: { [outType]: 1 }}; - return path.converters.slice(-1)[0].handle(args); - } - - private isMatchedPath(path: unknown): path is MatchedPath { - return typeof (path as MatchedPath).path === 'object'; + return args.representation; } /** * Finds a conversion path that can handle the given input. */ - private async findPath(input: RepresentationConverterArgs): Promise { + private async findPath(input: RepresentationConverterArgs): Promise { const type = input.representation.metadata.contentType!; const preferences = cleanPreferences(input.preferences.type); - const weight = getTypeWeight(type, preferences); - if (weight > 0) { - this.logger.debug(`No conversion required: ${type} already matches ${preferencesToString(preferences)}`); - return { value: type, weight }; - } - - return this.generatePath(type, preferences); + return this.generatePath(type, preferences, input.representation.metadata); } /** @@ -121,33 +119,33 @@ export class ChainedConverter extends RepresentationConverter { * * Errors if such a path does not exist. */ - private async generatePath(inType: string, outPreferences: ValuePreferences): Promise { - // Generate paths from all converters that match the input type - let paths = await this.converters.reduce(async(matches: Promise, converter): - Promise => { - const outTypes = await converter.getOutputTypes(inType); - if (Object.keys(outTypes).length > 0) { - (await matches).push({ - converters: [ converter ], - intermediateTypes: [], - inType, - outTypes, - }); - } - return matches; - }, Promise.resolve([])); + private async generatePath(inType: string, outPreferences: ValuePreferences, metadata: RepresentationMetadata): + Promise { + // + const weight = getTypeWeight(inType, outPreferences); + let paths: ConversionPath[] = [{ + converters: [], + intermediateTypes: [], + inType, + outTypes: { [inType]: 1 }, + weight, + outType: inType, + }]; // It's impossible for a path to have a higher weight than this value const maxWeight = Math.max(...Object.values(outPreferences)); - let bestPath = this.findBest(outPreferences, paths); - paths = this.removeBadPaths(paths, maxWeight, bestPath); + // This metadata will be used to simulate canHandle checks + const metadataPlaceholder = new RepresentationMetadata(metadata); + + let bestPath = this.findBest(paths); // This will always stop at some point since paths can't have the same converter twice while (paths.length > 0) { // For every path, find all the paths that can be made by adding 1 more converter - const promises = paths.map(async(path): Promise => this.takeStep(path)); + const promises = paths.map(async(path): Promise => this.takeStep(path, metadataPlaceholder)); paths = (await Promise.all(promises)).flat(); - const newBest = this.findBest(outPreferences, paths); + this.updatePathWeights(paths, outPreferences); + const newBest = this.findBest(paths); if (newBest && (!bestPath || newBest.weight > bestPath.weight)) { bestPath = newBest; } @@ -155,26 +153,37 @@ export class ChainedConverter extends RepresentationConverter { } if (!bestPath) { - this.logger.warn(`No conversion path could be made from ${inType} to ${Object.keys(outPreferences)}.`); + this.logger.warn(`No conversion path could be made from ${inType} to ${preferencesToString(outPreferences)}.`); throw new NotImplementedHttpError( - `No conversion path could be made from ${inType} to ${Object.keys(outPreferences)}.`, + `No conversion path could be made from ${inType} to ${preferencesToString(outPreferences)}.`, ); } return bestPath; } + /** + * Checks if a path can match the requested preferences and updates the type and weight if it can. + */ + private updatePathWeights(paths: ConversionPath[], outPreferences: ValuePreferences): void { + for (const path of paths) { + const outMatch = getBestPreference(path.outTypes, outPreferences); + if (outMatch && outMatch.weight > 0) { + path.weight = outMatch.weight; + path.outType = outMatch.value; + } + } + } + /** * Finds the path from the given list that can convert to the given preferences. * If there are multiple matches the one with the highest result weight gets chosen. * Will return undefined if there are no matches. */ - private findBest(preferences: ValuePreferences, paths: ConversionPath[]): MatchedPath | undefined { + private findBest(paths: ConversionPath[]): ConversionPath | undefined { // Need to use null instead of undefined so `reduce` doesn't take the first element of the array as `best` - return paths.reduce((best: MatchedPath | null, path): MatchedPath | null => { - const outMatch = getBestPreference(path.outTypes, preferences); - if (outMatch && !(best && best.weight >= outMatch.weight)) { - // Create new MatchedPath, using the output match above - return { path, outType: outMatch.value, weight: outMatch.weight }; + return paths.reduce((best: ConversionPath | null, path): ConversionPath | null => { + if (path.weight > 0 && !(best && best.weight >= path.weight)) { + return path; } return best; }, null) ?? undefined; @@ -188,7 +197,7 @@ export class ChainedConverter extends RepresentationConverter { * @param maxWeight - The maximum weight in the output preferences. * @param bestMatch - The current best path. */ - private removeBadPaths(paths: ConversionPath[], maxWeight: number, bestMatch?: MatchedPath): ConversionPath[] { + private removeBadPaths(paths: ConversionPath[], maxWeight: number, bestMatch?: ConversionPath): ConversionPath[] { // All paths are still good if there is no best match yet if (!bestMatch) { return paths; @@ -209,16 +218,19 @@ export class ChainedConverter extends RepresentationConverter { * Finds all converters that could take the output of the given path as input. * For each of these converters a new path gets created which is the input path appended by the converter. */ - private async takeStep(path: ConversionPath): Promise { + private async takeStep(path: ConversionPath, metadata: RepresentationMetadata): Promise { const unusedConverters = this.converters.filter((converter): boolean => !path.converters.includes(converter)); - const nextConverters = await this.supportedConverters(path.outTypes, unusedConverters); + const nextConverters = await this.supportedConverters(path.outTypes, metadata, unusedConverters); // Create a new path for every converter that can be appended return Promise.all(nextConverters.map(async(pref): Promise => ({ converters: [ ...path.converters, pref.converter ], - intermediateTypes: [ ...path.intermediateTypes, pref.inType ], + intermediateTypes: path.converters.length > 0 ? [ ...path.intermediateTypes, pref.inType ] : [], inType: path.inType, outTypes: pref.outTypes, + // These will be updated later + weight: 0, + outType: 'invalid', }))); } @@ -232,18 +244,43 @@ export class ChainedConverter extends RepresentationConverter { /** * Finds all converters in the given list that support taking any of the given types as input. + * Filters out converters that would produce an already seen type. */ - private async supportedConverters(types: ValuePreferences, converters: TypedRepresentationConverter[]): - Promise { + private async supportedConverters(types: ValuePreferences, metadata: RepresentationMetadata, + converters: TypedRepresentationConverter[]): Promise { const typeEntries = Object.entries(types); const results: ConverterPreference[] = []; for (const converter of converters) { for (const [ inType, weight ] of typeEntries) { - let outTypes = await converter.getOutputTypes(inType); - outTypes = this.modifyTypeWeights(weight, outTypes); - results.push({ converter, inType, outTypes }); + // This metadata object is only used internally so changing the content-type is fine + metadata.contentType = inType; + const preference = await this.findConverterPreference(inType, weight, metadata, converter); + if (preference) { + results.push(preference); + } } } return results; } + + /** + * Returns a ConverterPreference if the given converter supports the given type. + * All types that have already been used will be removed from the output types. + */ + private async findConverterPreference(inType: string, weight: number, metadata: RepresentationMetadata, + converter: TypedRepresentationConverter): Promise { + const representation = new BasicRepresentation([], metadata); + try { + const identifier = { path: representation.metadata.identifier.value }; + // Internal types get ignored when trying to match everything, so they need to be specified to also match. + await converter.canHandle({ representation, identifier, preferences: { type: { '*/*': 1, 'internal/*': 1 }}}); + } catch { + // Skip converters that fail the canHandle test + return; + } + + let outTypes = await converter.getOutputTypes(inType); + outTypes = this.modifyTypeWeights(weight, outTypes); + return { converter, inType, outTypes }; + } } diff --git a/src/storage/conversion/IfNeededConverter.ts b/src/storage/conversion/IfNeededConverter.ts deleted file mode 100644 index 547cfa67b..000000000 --- a/src/storage/conversion/IfNeededConverter.ts +++ /dev/null @@ -1,60 +0,0 @@ -import type { Representation } from '../../http/representation/Representation'; -import { getLoggerFor } from '../../logging/LogUtil'; -import { InternalServerError } from '../../util/errors/InternalServerError'; -import { UnsupportedAsyncHandler } from '../../util/handlers/UnsupportedAsyncHandler'; -import { matchesMediaPreferences } from './ConversionUtil'; -import { RepresentationConverter } from './RepresentationConverter'; -import type { RepresentationConverterArgs } from './RepresentationConverter'; - -const EMPTY_CONVERTER = new UnsupportedAsyncHandler('The content type does not match the preferences'); - -/** - * A {@link RepresentationConverter} that only converts representations - * that are not compatible with the preferences. - */ -export class IfNeededConverter extends RepresentationConverter { - private readonly converter: RepresentationConverter; - protected readonly logger = getLoggerFor(this); - - public constructor(converter: RepresentationConverter = EMPTY_CONVERTER) { - super(); - this.converter = converter; - } - - public async canHandle(args: RepresentationConverterArgs): Promise { - if (this.needsConversion(args)) { - await this.converter.canHandle(args); - } - } - - public async handle(args: RepresentationConverterArgs): Promise { - return !this.needsConversion(args) ? args.representation : this.convert(args, false); - } - - public async handleSafe(args: RepresentationConverterArgs): Promise { - return !this.needsConversion(args) ? args.representation : this.convert(args, true); - } - - protected needsConversion({ identifier, representation, preferences }: RepresentationConverterArgs): boolean { - // No conversion is needed if there are any matches for the provided content type - const { contentType } = representation.metadata; - if (!contentType) { - throw new InternalServerError('Content-Type is required for data conversion.'); - } - const noMatchingMediaType = !matchesMediaPreferences(contentType, preferences.type); - if (noMatchingMediaType) { - this.logger.debug(`Conversion needed for ${identifier - .path} from ${contentType} to satisfy ${!preferences.type ? - '""' : - Object.entries(preferences.type).map(([ value, weight ]): string => `${value};q=${weight}`).join(', ')}`); - } - return noMatchingMediaType; - } - - protected async convert(args: RepresentationConverterArgs, safely: boolean): Promise { - const converted = await (safely ? this.converter.handleSafe(args) : this.converter.handle(args)); - this.logger.info(`Converted representation for ${args.identifier - .path} from ${args.representation.metadata.contentType} to ${converted.metadata.contentType}`); - return converted; - } -} diff --git a/test/unit/storage/conversion/ChainedConverter.test.ts b/test/unit/storage/conversion/ChainedConverter.test.ts index d75c1a212..01d1dbf4e 100644 --- a/test/unit/storage/conversion/ChainedConverter.test.ts +++ b/test/unit/storage/conversion/ChainedConverter.test.ts @@ -70,19 +70,29 @@ describe('A ChainedConverter', (): void => { args.representation.metadata.contentType = 'b/b'; await expect(converter.handle(args)).rejects - .toThrow('No conversion path could be made from b/b to x/x,x/*,internal/*.'); + .toThrow('No conversion path could be made from b/b to x/x:1,x/*:0.8,internal/*:0.'); }); it('can handle situations where no conversion is required.', async(): Promise => { - const converters = [ new DummyConverter({ 'a/a': 1 }, { 'x/x': 1 }) ]; + const converters = [ new DummyConverter({ 'b/b': 1 }, { 'x/x': 1 }) ]; args.representation.metadata.contentType = 'b/b'; - args.preferences.type = { 'b/*': 0.5 }; + args.preferences.type = { 'b/*': 1, 'x/x': 0.5 }; const converter = new ChainedConverter(converters); const result = await converter.handle(args); expect(result.metadata.contentType).toBe('b/b'); }); + it('converts input matching the output preferences if a better output can be found.', async(): Promise => { + const converters = [ new DummyConverter({ 'b/b': 1 }, { 'x/x': 1 }) ]; + args.representation.metadata.contentType = 'b/b'; + args.preferences.type = { 'b/*': 0.5, 'x/x': 1 }; + const converter = new ChainedConverter(converters); + + const result = await converter.handle(args); + expect(result.metadata.contentType).toBe('x/x'); + }); + it('interprets no preferences as */*.', async(): Promise => { const converters = [ new DummyConverter({ 'a/a': 1 }, { 'x/x': 1 }) ]; const converter = new ChainedConverter(converters); @@ -210,4 +220,15 @@ describe('A ChainedConverter', (): void => { expect(converter.handle).toHaveBeenCalledTimes(1); expect(converter.handle).toHaveBeenLastCalledWith(args); }); + + it('does not get stuck in infinite conversion loops.', async(): Promise => { + const converters = [ + new DummyConverter({ 'a/a': 1 }, { 'b/b': 1 }), + new DummyConverter({ 'b/b': 1 }, { 'a/a': 1 }), + ]; + const converter = new ChainedConverter(converters); + + await expect(converter.handle(args)).rejects + .toThrow('No conversion path could be made from a/a to x/x:1,x/*:0.8,internal/*:0.'); + }); }); diff --git a/test/unit/storage/conversion/IfNeededConverter.test.ts b/test/unit/storage/conversion/IfNeededConverter.test.ts deleted file mode 100644 index af5642d79..000000000 --- a/test/unit/storage/conversion/IfNeededConverter.test.ts +++ /dev/null @@ -1,131 +0,0 @@ -import type { Representation } from '../../../../src/http/representation/Representation'; -import { IfNeededConverter } from '../../../../src/storage/conversion/IfNeededConverter'; -import type { - RepresentationConverter, -} from '../../../../src/storage/conversion/RepresentationConverter'; - -describe('An IfNeededConverter', (): void => { - const identifier = { path: 'identifier' }; - const representation: Representation = { - metadata: { contentType: 'text/turtle' }, - } as any; - const converted = { - metadata: { contentType: 'application/ld+json' }, - }; - - const innerConverter: jest.Mocked = { - canHandle: jest.fn().mockResolvedValue(true), - handle: jest.fn().mockResolvedValue(converted), - handleSafe: jest.fn().mockResolvedValue(converted), - } as any; - - const converter = new IfNeededConverter(innerConverter); - - afterEach((): void => { - jest.clearAllMocks(); - }); - - it('performs no conversion when there are no content type preferences.', async(): Promise => { - const preferences = {}; - const args = { identifier, representation, preferences }; - - await expect(converter.canHandle(args)).resolves.toBeUndefined(); - await expect(converter.handle(args)).resolves.toBe(representation); - await expect(converter.handleSafe(args)).resolves.toBe(representation); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(0); - expect(innerConverter.handle).toHaveBeenCalledTimes(0); - expect(innerConverter.handleSafe).toHaveBeenCalledTimes(0); - }); - - it('performs conversion when there are no preferences but the content-type is internal.', async(): Promise => { - const preferences = {}; - const internalRepresentation = { - metadata: { contentType: 'internal/quads' }, - } as any; - const args = { identifier, representation: internalRepresentation, preferences }; - - await expect(converter.handleSafe(args)).resolves.toBe(converted); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(0); - expect(innerConverter.handle).toHaveBeenCalledTimes(0); - expect(innerConverter.handleSafe).toHaveBeenCalledTimes(1); - expect(innerConverter.handleSafe).toHaveBeenCalledWith(args); - }); - - it('errors if no content type is specified on the representation.', async(): Promise => { - const preferences = { type: { 'text/turtle': 1 }}; - const args = { identifier, representation: { metadata: {}} as any, preferences }; - - await expect(converter.handleSafe(args)).rejects - .toThrow('Content-Type is required for data conversion.'); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(0); - expect(innerConverter.handle).toHaveBeenCalledTimes(0); - expect(innerConverter.handleSafe).toHaveBeenCalledTimes(0); - }); - - it('performs no conversion when the content type matches the preferences.', async(): Promise => { - const preferences = { type: { 'text/turtle': 1 }}; - const args = { identifier, representation, preferences }; - - await expect(converter.handleSafe(args)).resolves.toBe(representation); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(0); - expect(innerConverter.handle).toHaveBeenCalledTimes(0); - expect(innerConverter.handleSafe).toHaveBeenCalledTimes(0); - }); - - it('performs a conversion when the content type matches the preferences.', async(): Promise => { - const preferences = { type: { 'text/turtle': 0 }}; - const args = { identifier, representation, preferences }; - - await expect(converter.handleSafe(args)).resolves.toBe(converted); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(0); - expect(innerConverter.handle).toHaveBeenCalledTimes(0); - expect(innerConverter.handleSafe).toHaveBeenCalledTimes(1); - expect(innerConverter.handleSafe).toHaveBeenCalledWith(args); - }); - - it('does not support conversion when the inner converter does not support it.', async(): Promise => { - const preferences = { type: { 'text/turtle': 0 }}; - const args = { identifier, representation, preferences }; - const error = new Error('unsupported'); - innerConverter.canHandle.mockRejectedValueOnce(error); - - await expect(converter.canHandle(args)).rejects.toThrow(error); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(1); - expect(innerConverter.canHandle).toHaveBeenCalledWith(args); - }); - - it('supports conversion when the inner converter supports it.', async(): Promise => { - const preferences = { type: { 'text/turtle': 0 }}; - const args = { identifier, representation, preferences }; - - await expect(converter.canHandle(args)).resolves.toBeUndefined(); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(1); - expect(innerConverter.canHandle).toHaveBeenCalledWith(args); - - await expect(converter.handle(args)).resolves.toBe(converted); - - expect(innerConverter.canHandle).toHaveBeenCalledTimes(1); - expect(innerConverter.handle).toHaveBeenCalledTimes(1); - expect(innerConverter.handle).toHaveBeenCalledWith(args); - }); - - it('does not support conversion when there is no inner converter.', async(): Promise => { - const emptyConverter = new IfNeededConverter(); - const preferences = { type: { 'text/turtle': 0 }}; - const args = { identifier, representation, preferences }; - - await expect(emptyConverter.canHandle(args)).rejects - .toThrow('The content type does not match the preferences'); - await expect(emptyConverter.handle(args)).rejects - .toThrow('The content type does not match the preferences'); - await expect(emptyConverter.handleSafe(args)).rejects - .toThrow('The content type does not match the preferences'); - }); -});