From 8cd3f7d2e5266a1fb376aba8cb852cbe09d9bc6c Mon Sep 17 00:00:00 2001 From: Ruben Verborgh Date: Tue, 5 Jan 2021 00:12:26 +0100 Subject: [PATCH] feat: Incorporate server-side representation quality. Closes https://github.com/solid/community-server/issues/467 --- src/storage/RepresentationConvertingStore.ts | 2 +- src/storage/conversion/ChainedConverter.ts | 8 +-- src/storage/conversion/ConversionUtil.ts | 49 +++++++------- src/storage/conversion/QuadToRdfConverter.ts | 4 +- .../TypedRepresentationConverter.ts | 2 +- .../RepresentationConvertingStore.test.ts | 4 +- .../conversion/ChainedConverter.test.ts | 7 +- .../storage/conversion/ConversionUtil.test.ts | 67 +++++++++++-------- 8 files changed, 70 insertions(+), 73 deletions(-) diff --git a/src/storage/RepresentationConvertingStore.ts b/src/storage/RepresentationConvertingStore.ts index cb3ec2b61..b912ae38e 100644 --- a/src/storage/RepresentationConvertingStore.ts +++ b/src/storage/RepresentationConvertingStore.ts @@ -77,7 +77,7 @@ export class RepresentationConvertingStore 0; + return matchingMediaTypes(preferences.type, { [contentType]: 1 }).length > 0; } /** diff --git a/src/storage/conversion/ChainedConverter.ts b/src/storage/conversion/ChainedConverter.ts index aa3f02ef0..10fdecbc1 100644 --- a/src/storage/conversion/ChainedConverter.ts +++ b/src/storage/conversion/ChainedConverter.ts @@ -46,13 +46,7 @@ export class ChainedConverter extends TypedRepresentationConverter { public async canHandle(input: RepresentationConverterArgs): Promise { // We assume a chain can be constructed, otherwise there would be a configuration issue // So we only check if the input can be parsed and the preferred type can be written - const inTypes = this.getAcceptableTypes(await this.first.getInputTypes()); - const outTypes = this.getAcceptableTypes(await this.last.getOutputTypes()); - supportsConversion(input, inTypes, outTypes); - } - - private getAcceptableTypes(preferences: ValuePreferences): string[] { - return Object.keys(preferences).filter((name): boolean => preferences[name] > 0); + supportsConversion(input, await this.first.getInputTypes(), await this.last.getOutputTypes()); } public async handle(input: RepresentationConverterArgs): Promise { diff --git a/src/storage/conversion/ConversionUtil.ts b/src/storage/conversion/ConversionUtil.ts index 6927276e6..68017da4e 100644 --- a/src/storage/conversion/ConversionUtil.ts +++ b/src/storage/conversion/ConversionUtil.ts @@ -1,4 +1,4 @@ -import type { RepresentationPreferences } from '../../ldp/representation/RepresentationPreferences'; +import type { ValuePreferences } from '../../ldp/representation/RepresentationPreferences'; import { INTERNAL_ALL } from '../../util/ContentTypes'; import { BadRequestHttpError } from '../../util/errors/BadRequestHttpError'; import { InternalServerError } from '../../util/errors/InternalServerError'; @@ -12,43 +12,42 @@ import type { RepresentationConverterArgs } from './RepresentationConverter'; * Since more specific media ranges override less specific ones, * this will be ignored if there is a specific internal type preference. * - * @param preferences - Preferences for output type. - * @param types - Media types to compare to the preferences. + * @param preferredTypes - Preferences for output type. + * @param availableTypes - Media types to compare to the preferences. * * @throws BadRequestHttpError * If the type preferences are undefined or if there are duplicate preferences. * * @returns The weighted and filtered list of matching types. */ -export const matchingMediaTypes = (preferences: RepresentationPreferences, available: string[]): +export const matchingMediaTypes = (preferredTypes: ValuePreferences = {}, availableTypes: ValuePreferences = {}): string[] => { - const preferredTypes = preferences.type; - if (!preferredTypes || Object.keys(preferredTypes).length === 0) { - throw new BadRequestHttpError('Output type required for conversion.'); - } - + // No preference means anything is acceptable + const preferred = { ...preferredTypes }; + if (Object.keys(preferredTypes).length === 0) { + preferred['*/*'] = 1; // Prevent accidental use of internal types - if (!preferredTypes[INTERNAL_ALL]) { - preferredTypes[INTERNAL_ALL] = 0; + } else if (!(INTERNAL_ALL in preferred)) { + preferred[INTERNAL_ALL] = 0; } // RFC 7231 // Media ranges can be overridden by more specific media ranges or // specific media types. If more than one media range applies to a // given type, the most specific reference has precedence. - const weightedSupported = available.map((type): [string, number] => { + const weightedSupported = Object.entries(availableTypes).map(([ type, quality ]): [string, number] => { const match = /^([^/]+)\/([^\s;]+)/u.exec(type); if (!match) { throw new InternalServerError(`Unexpected type preference: ${type}`); } const [ , main, sub ] = match; const weight = - preferredTypes[type] ?? - preferredTypes[`${main}/${sub}`] ?? - preferredTypes[`${main}/*`] ?? - preferredTypes['*/*'] ?? + preferred[type] ?? + preferred[`${main}/${sub}`] ?? + preferred[`${main}/*`] ?? + preferred['*/*'] ?? 0; - return [ type, weight ]; + return [ type, weight * quality ]; }); // Return all non-zero preferences in descending order of weight @@ -96,16 +95,16 @@ export const matchesMediaType = (mediaA: string, mediaB: string): boolean => { * @param supportedIn - Media types that can be parsed by the converter. * @param supportedOut - Media types that can be produced by the converter. */ -export const supportsConversion = (request: RepresentationConverterArgs, supportedIn: string[], - supportedOut: string[]): void => { +export const supportsConversion = (request: RepresentationConverterArgs, supportedIn: ValuePreferences, + supportedOut: ValuePreferences): void => { const inType = request.representation.metadata.contentType; if (!inType) { - throw new BadRequestHttpError('Input type required for conversion.'); + throw new BadRequestHttpError('No content type indicated on request.'); } - if (!supportedIn.some((type): boolean => matchesMediaType(inType, type))) { - throw new NotImplementedHttpError(`Can only convert from ${supportedIn} to ${supportedOut}.`); - } - if (matchingMediaTypes(request.preferences, supportedOut).length <= 0) { - throw new NotImplementedHttpError(`Can only convert from ${supportedIn} to ${supportedOut}.`); + if (!Object.keys(supportedIn).some((type): boolean => matchesMediaType(inType, type)) || + matchingMediaTypes(request.preferences.type, supportedOut).length === 0) { + throw new NotImplementedHttpError( + `Can only convert from ${Object.keys(supportedIn)} to ${Object.keys(supportedOut)}.`, + ); } }; diff --git a/src/storage/conversion/QuadToRdfConverter.ts b/src/storage/conversion/QuadToRdfConverter.ts index 0f9a8ffa9..78a22f943 100644 --- a/src/storage/conversion/QuadToRdfConverter.ts +++ b/src/storage/conversion/QuadToRdfConverter.ts @@ -29,8 +29,8 @@ export class QuadToRdfConverter extends TypedRepresentationConverter { return this.quadsToRdf(input.representation, input.preferences); } - private async quadsToRdf(quads: Representation, preferences: RepresentationPreferences): Promise { - const contentType = matchingMediaTypes(preferences, await rdfSerializer.getContentTypes())[0]; + private async quadsToRdf(quads: Representation, { type }: RepresentationPreferences): Promise { + const contentType = matchingMediaTypes(type, await this.getOutputTypes())[0]; const metadata = new RepresentationMetadata(quads.metadata, { [CONTENT_TYPE]: contentType }); return { binary: true, diff --git a/src/storage/conversion/TypedRepresentationConverter.ts b/src/storage/conversion/TypedRepresentationConverter.ts index 600782ec8..b142d7428 100644 --- a/src/storage/conversion/TypedRepresentationConverter.ts +++ b/src/storage/conversion/TypedRepresentationConverter.ts @@ -23,6 +23,6 @@ export abstract class TypedRepresentationConverter extends RepresentationConvert public async canHandle(args: RepresentationConverterArgs): Promise { const types = [ this.getInputTypes(), this.getOutputTypes() ]; const [ inputTypes, outputTypes ] = await Promise.all(types); - supportsConversion(args, Object.keys(inputTypes), Object.keys(outputTypes)); + supportsConversion(args, inputTypes, outputTypes); } } diff --git a/test/unit/storage/RepresentationConvertingStore.test.ts b/test/unit/storage/RepresentationConvertingStore.test.ts index 93667ef0a..f1b55c0ea 100644 --- a/test/unit/storage/RepresentationConvertingStore.test.ts +++ b/test/unit/storage/RepresentationConvertingStore.test.ts @@ -42,7 +42,7 @@ describe('A RepresentationConvertingStore', (): void => { expect(source.getRepresentation).toHaveBeenCalledTimes(1); expect(source.getRepresentation).toHaveBeenLastCalledWith( { path: 'path' }, - { type: { 'application/*': 0, 'text/turtle': 1, 'internal/*': 0 }}, + { type: { 'application/*': 0, 'text/turtle': 1 }}, undefined, ); expect(outConverter.handleSafe).toHaveBeenCalledTimes(0); @@ -70,7 +70,7 @@ describe('A RepresentationConvertingStore', (): void => { expect(outConverter.handleSafe).toHaveBeenLastCalledWith({ identifier: { path: 'path' }, representation: { data: 'data', metadata }, - preferences: { type: { 'text/plain': 1, 'text/turtle': 0, 'internal/*': 0 }}, + preferences: { type: { 'text/plain': 1, 'text/turtle': 0 }}, }); }); diff --git a/test/unit/storage/conversion/ChainedConverter.test.ts b/test/unit/storage/conversion/ChainedConverter.test.ts index daf502183..f86c10a7c 100644 --- a/test/unit/storage/conversion/ChainedConverter.test.ts +++ b/test/unit/storage/conversion/ChainedConverter.test.ts @@ -5,7 +5,6 @@ import type { RepresentationPreferences, } from '../../../../src/ldp/representation/RepresentationPreferences'; import { ChainedConverter } from '../../../../src/storage/conversion/ChainedConverter'; -import { supportsConversion } from '../../../../src/storage/conversion/ConversionUtil'; import type { RepresentationConverterArgs } from '../../../../src/storage/conversion/RepresentationConverter'; import { TypedRepresentationConverter } from '../../../../src/storage/conversion/TypedRepresentationConverter'; import { CONTENT_TYPE } from '../../../../src/util/Vocabularies'; @@ -28,10 +27,6 @@ class DummyConverter extends TypedRepresentationConverter { return this.outTypes; } - public async canHandle(input: RepresentationConverterArgs): Promise { - supportsConversion(input, Object.keys(this.inTypes), Object.keys(this.outTypes)); - } - public async handle(input: RepresentationConverterArgs): Promise { const metadata = new RepresentationMetadata(input.representation.metadata, { [CONTENT_TYPE]: Object.keys(input.preferences.type!)[0] }); @@ -85,7 +80,7 @@ describe('A ChainedConverter', (): void => { }); it('errors if the end of the chain does not support the preferences.', async(): Promise => { - delete preferences.type; + preferences.type = { 'abc/def': 1 }; await expect(converter.canHandle(args)).rejects.toThrow(); }); diff --git a/test/unit/storage/conversion/ConversionUtil.test.ts b/test/unit/storage/conversion/ConversionUtil.test.ts index 3399c1898..b3451901c 100644 --- a/test/unit/storage/conversion/ConversionUtil.test.ts +++ b/test/unit/storage/conversion/ConversionUtil.test.ts @@ -1,6 +1,9 @@ import type { Representation } from '../../../../src/ldp/representation/Representation'; import { RepresentationMetadata } from '../../../../src/ldp/representation/RepresentationMetadata'; -import type { RepresentationPreferences } from '../../../../src/ldp/representation/RepresentationPreferences'; +import type { + ValuePreferences, + RepresentationPreferences, +} from '../../../../src/ldp/representation/RepresentationPreferences'; import type { ResourceIdentifier } from '../../../../src/ldp/representation/ResourceIdentifier'; import { matchesMediaType, @@ -22,15 +25,19 @@ describe('ConversionUtil', (): void => { describe('#supportsConversion', (): void => { it('requires an input type.', async(): Promise => { const preferences: RepresentationPreferences = {}; - expect((): any => supportsConversion({ identifier, representation, preferences }, [ 'a/x' ], [ 'a/x' ])) - .toThrow('Input type required for conversion.'); + expect((): any => supportsConversion({ identifier, representation, preferences }, + { 'a/x': 1 }, + { 'a/x': 1 })) + .toThrow('No content type indicated on request.'); }); it('requires a matching input type.', async(): Promise => { metadata.contentType = 'a/x'; const preferences: RepresentationPreferences = { type: { 'b/x': 1 }}; - expect((): any => supportsConversion({ identifier, representation, preferences }, [ 'c/x' ], [ 'a/x' ])) + expect((): any => supportsConversion({ identifier, representation, preferences }, + { 'c/x': 1 }, + { 'a/x': 1 })) .toThrow('Can only convert from c/x to a/x.'); }); @@ -38,7 +45,9 @@ describe('ConversionUtil', (): void => { metadata.contentType = 'a/x'; const preferences: RepresentationPreferences = { type: { 'b/x': 1 }}; - expect((): any => supportsConversion({ identifier, representation, preferences }, [ 'a/x' ], [ 'c/x' ])) + expect((): any => supportsConversion({ identifier, representation, preferences }, + { 'a/x': 1 }, + { 'c/x': 1 })) .toThrow('Can only convert from a/x to c/x.'); }); @@ -46,58 +55,58 @@ describe('ConversionUtil', (): void => { metadata.contentType = 'a/x'; const preferences: RepresentationPreferences = { type: { 'b/x': 1 }}; - expect(supportsConversion({ identifier, representation, preferences }, [ 'a/x' ], [ 'b/x' ])) + expect(supportsConversion({ identifier, representation, preferences }, + { 'a/x': 1 }, + { 'b/x': 1 })) .toBeUndefined(); }); }); describe('#matchingMediaTypes', (): void => { - it('requires type preferences.', async(): Promise => { - const preferences: RepresentationPreferences = - {}; - expect((): any => matchingMediaTypes(preferences, [ 'a/b' ])) - .toThrow('Output type required for conversion.'); + it('returns the empty array if no preferences specified.', async(): Promise => { + expect(matchingMediaTypes()) + .toEqual([]); }); it('returns matching types if weight > 0.', async(): Promise => { - const preferences: RepresentationPreferences = - { type: { 'a/x': 1, 'b/x': 0.5, 'c/x': 0 }}; - expect(matchingMediaTypes(preferences, [ 'b/x', 'c/x' ])) + const preferences: ValuePreferences = { 'a/x': 1, 'b/x': 0.5, 'c/x': 0 }; + expect(matchingMediaTypes(preferences, { 'b/x': 1, 'c/x': 1 })) .toEqual([ 'b/x' ]); }); it('sorts by descending weight.', async(): Promise => { - const preferences: RepresentationPreferences = - { type: { 'a/x': 1, 'b/x': 0.5, 'c/x': 0.8 }}; - expect(matchingMediaTypes(preferences, [ 'a/x', 'b/x', 'c/x' ])) + const preferences: ValuePreferences = { 'a/x': 1, 'b/x': 0.5, 'c/x': 0.8 }; + expect(matchingMediaTypes(preferences, { 'a/x': 1, 'b/x': 1, 'c/x': 1 })) .toEqual([ 'a/x', 'c/x', 'b/x' ]); }); + it('incorporates representation qualities when calculating weight.', async(): Promise => { + const preferences: ValuePreferences = { 'a/x': 1, 'b/x': 0.5, 'c/x': 0.8 }; + expect(matchingMediaTypes(preferences, { 'a/x': 0.1, 'b/x': 1, 'c/x': 0.6 })) + .toEqual([ 'b/x', 'c/x', 'a/x' ]); + }); + it('errors if there invalid types.', async(): Promise => { - const preferences: RepresentationPreferences = - { type: { 'b/x': 1 }}; - expect((): any => matchingMediaTypes(preferences, [ 'noType' ])) + const preferences: ValuePreferences = { 'b/x': 1 }; + expect((): any => matchingMediaTypes(preferences, { noType: 1 })) .toThrow(new InternalServerError(`Unexpected type preference: noType`)); }); it('filters out internal types.', async(): Promise => { - const preferences: RepresentationPreferences = - { type: { '*/*': 1 }}; - expect(matchingMediaTypes(preferences, [ 'a/x', 'internal/quads' ])) + const preferences: ValuePreferences = { '*/*': 1 }; + expect(matchingMediaTypes(preferences, { 'a/x': 1, 'internal/quads': 1 })) .toEqual([ 'a/x' ]); }); it('keeps internal types that are specifically requested.', async(): Promise => { - const preferences: RepresentationPreferences = - { type: { '*/*': 1, 'internal/*': 0.5 }}; - expect(matchingMediaTypes(preferences, [ 'a/x', 'internal/quads' ])) + const preferences: ValuePreferences = { '*/*': 1, 'internal/*': 0.5 }; + expect(matchingMediaTypes(preferences, { 'a/x': 1, 'internal/quads': 1 })) .toEqual([ 'a/x', 'internal/quads' ]); }); it('takes the most relevant weight for a type.', async(): Promise => { - const preferences: RepresentationPreferences = - { type: { '*/*': 1, 'internal/quads': 0.5 }}; - expect(matchingMediaTypes(preferences, [ 'a/x', 'internal/quads' ])) + const preferences: ValuePreferences = { '*/*': 1, 'internal/quads': 0.5 }; + expect(matchingMediaTypes(preferences, { 'a/x': 1, 'internal/quads': 1 })) .toEqual([ 'a/x', 'internal/quads' ]); }); });