From e7ff134b258f984f241969d4ba18dfe0525d5f6a Mon Sep 17 00:00:00 2001 From: Joachim Van Herwegen Date: Fri, 2 Jul 2021 15:23:00 +0200 Subject: [PATCH] fix: Always find the best path with ChainedConverter --- src/storage/conversion/ChainedConverter.ts | 45 +++++++++++++++++-- .../conversion/ChainedConverter.test.ts | 41 ++++++++++++++++- 2 files changed, 80 insertions(+), 6 deletions(-) diff --git a/src/storage/conversion/ChainedConverter.ts b/src/storage/conversion/ChainedConverter.ts index 7394c052dd..b99b1c94d5 100644 --- a/src/storage/conversion/ChainedConverter.ts +++ b/src/storage/conversion/ChainedConverter.ts @@ -96,7 +96,7 @@ class LruPathCache { * A meta converter that takes an array of other converters as input. * It chains these converters by finding a path of converters * that can go from the given content-type to the given type preferences. - * In case there are multiple paths, the shortest one with the highest weight gets found. + * In case there are multiple paths, the one with the highest weight gets found. * Will error in case no path can be found. * * Generated paths get stored in an internal cache for later re-use on similar requests. @@ -194,7 +194,7 @@ export class ChainedConverter extends RepresentationConverter { } /** - * Tries to generate the optimal and shortest `ConversionPath` that supports the given parameters, + * Tries to generate the optimal `ConversionPath` that supports the given parameters, * which will then be used to instantiate a specific `MatchedPath` for those parameters. * * Errors if such a path does not exist. @@ -215,13 +215,21 @@ export class ChainedConverter extends RepresentationConverter { return matches; }, Promise.resolve([])); + // 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(inType, outPreferences, paths); + paths = this.removeBadPaths(paths, maxWeight, inType, bestPath); // This will always stop at some point since paths can't have the same converter twice - while (!bestPath && paths.length > 0) { + 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)); paths = (await Promise.all(promises)).flat(); - bestPath = this.findBest(inType, outPreferences, paths); + const newBest = this.findBest(inType, outPreferences, paths); + if (newBest && (!bestPath || newBest.weight > bestPath.weight)) { + bestPath = newBest; + } + paths = this.removeBadPaths(paths, maxWeight, inType, bestPath); } if (!bestPath) { @@ -251,6 +259,35 @@ export class ChainedConverter extends RepresentationConverter { }, null) ?? undefined; } + /** + * Filter out paths that can no longer be better than the current best solution. + * This depends on a valid path already being found, if not all the input paths will be returned. + * + * @param paths - Paths to filter. + * @param maxWeight - The maximum weight in the output preferences. + * @param inType - The input type. + * @param bestMatch - The current best path. + */ + private removeBadPaths(paths: ConversionPath[], maxWeight: number, inType: string, bestMatch?: MatchedPath): + ConversionPath[] { + // All paths are still good if there is no best match yet + if (!bestMatch) { + return paths; + } + // Do not improve if the maximum weight has been achieved (accounting for floating point errors) + if (bestMatch.weight >= maxWeight - 0.01) { + return []; + } + + // Only return paths that can potentially improve upon bestPath + return paths.filter((path): boolean => { + const optimisticWeight = getTypeWeight(inType, path.inTypes) * + Math.max(...Object.values(path.outTypes)) * + maxWeight; + return optimisticWeight > bestMatch.weight; + }); + } + /** * 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. diff --git a/test/unit/storage/conversion/ChainedConverter.test.ts b/test/unit/storage/conversion/ChainedConverter.test.ts index f194ce9ed9..e4879fb013 100644 --- a/test/unit/storage/conversion/ChainedConverter.test.ts +++ b/test/unit/storage/conversion/ChainedConverter.test.ts @@ -126,7 +126,7 @@ describe('A ChainedConverter', (): void => { expect(result.metadata.contentType).toBe('x/x'); }); - it('will use the best path among the shortest found.', async(): Promise => { + it('will use the shortest path among the best found.', async(): Promise => { // Valid paths: 0 -> 1 -> 2, 3 -> 2, 4 -> 2, 5 -> 2, *6 -> 2* const converters = [ new DummyConverter({ 'a/a': 1 }, { 'b/b': 1 }), @@ -135,7 +135,7 @@ describe('A ChainedConverter', (): void => { new DummyConverter({ '*/*': 0.5 }, { 'c/c': 1 }), new DummyConverter({ 'a/a': 0.8 }, { 'c/c': 1 }), new DummyConverter({ 'a/*': 1 }, { 'c/c': 0.5 }), - new DummyConverter({ 'a/a': 1 }, { 'c/c': 0.9 }), + new DummyConverter({ 'a/a': 1 }, { 'c/c': 1 }), ]; const converter = new ChainedConverter(converters); @@ -172,6 +172,43 @@ describe('A ChainedConverter', (): void => { expect(metadata.contentType).toBe('d/d'); }); + it('will continue if an even better path can be found by adding a converter.', async(): Promise => { + // Path: a/a -> x/a -> x/x + const converters = [ + new DummyConverter({ 'a/a': 1 }, { 'x/a': 0.9 }), + new DummyConverter({ 'x/a': 1 }, { 'x/x': 1 }), + ]; + const converter = new ChainedConverter(converters); + + const result = await converter.handle(args); + expect(result.metadata.contentType).toBe('x/x'); + }); + + it('will continue if an even better path can be found through another path.', async(): Promise => { + // Path: a/a -> b/b -> x/x + const converters = [ + new DummyConverter({ 'a/a': 1 }, { 'x/a': 0.5 }), + new DummyConverter({ 'a/a': 1 }, { 'b/b': 1 }), + new DummyConverter({ 'b/b': 1 }, { 'x/x': 0.6 }), + ]; + const converter = new ChainedConverter(converters); + + const result = await converter.handle(args); + expect(result.metadata.contentType).toBe('x/x'); + }); + + it('will stop if all future paths are worse.', async(): Promise => { + // Path: a/a -> x/a + const converters = [ + new DummyConverter({ 'a/a': 1 }, { 'x/a': 1 }), + new DummyConverter({ 'x/a': 1 }, { 'x/x': 0.1 }), + ]; + const converter = new ChainedConverter(converters); + + const result = await converter.handle(args); + expect(result.metadata.contentType).toBe('x/a'); + }); + it('calls handle when calling handleSafe.', async(): Promise => { const converters = [ new DummyConverter({ 'a/a': 1 }, { 'x/x': 1 }) ]; const converter = new ChainedConverter(converters);