Repository: solid/community-server (ts)

Issue:
Support `*/*` Accept header for RDF data
The `BasicResponseWriter` errors when it receives non-binary data, so we should make sure this never happens. Currently it can still happen with an `*/*` accept header if the backend returns a stream of Quads.
Have conditional data conversion
Related to https://github.com/solid/community-server/issues/137#issuecomment-721574716 and https://github.com/solid/community-server/issues/276#issuecomment-721571262.

In both cases mentioned above we want to convert `internal/quads` to turtle, but not touch any other data (unless specifically requested).

Hopefully it is possible to have 1 clean solution that covers both cases, hence the generic issue name. One solution would be to have the converting store to also take as input (optional) preferences/types and only check conversion for data that matches those.

Perhaps something could be done with configuring the `inPreferences` of the store to have `internal/quads` with weight 0, but the the actual converters would have to change to convert to a default format (turtle) if the preferences don't match the data but no alternative is provided.

Pull request:
Make internal/quads unacceptable as output format
This closes #306 and closes #276 . It does not yet resolve the issue of making sure the patching store does not send quads back to the file store, but that should just be a config change after #297 is resolved.

I like how we can now reject content-types using weight 0, not a big fan of how there is now just a line always adding this preference in `AcceptPreferenceParser`, but it seemed like the best place to add it. Could also add an input parameter taking forbidden content-types into account but that seems a bit overkill.

Patch:
diff --git a/src/ldp/representation/RepresentationPreference.ts b/src/ldp/representation/RepresentationPreference.ts
index 7ca5306c0..880899745 100644
--- a/src/ldp/representation/RepresentationPreference.ts
+++ b/src/ldp/representation/RepresentationPreference.ts
@@ -7,7 +7,12 @@ export interface RepresentationPreference {
    */
   value: string;
   /**
-   * How important this preference is in a value going from 0 to 1.
+   * How preferred this value is in a number going from 0 to 1.
+   * Follows the quality values rule from RFC 7231:
+   *
+   * "The weight is normalized to a real number in the range 0 through 1,
+   * where 0.001 is the least preferred and 1 is the most preferred; a
+   * value of 0 means "not acceptable"."
    */
   weight: number;
 }
diff --git a/src/storage/RepresentationConvertingStore.ts b/src/storage/RepresentationConvertingStore.ts
index 8fd746b7f..7cb26710c 100644
--- a/src/storage/RepresentationConvertingStore.ts
+++ b/src/storage/RepresentationConvertingStore.ts
@@ -2,9 +2,10 @@ import type { Representation } from '../ldp/representation/Representation';
 import type { RepresentationPreferences } from '../ldp/representation/RepresentationPreferences';
 import type { ResourceIdentifier } from '../ldp/representation/ResourceIdentifier';
 import { getLoggerFor } from '../logging/LogUtil';
-import { matchingMediaType } from '../util/Util';
+import { InternalServerError } from '../util/errors/InternalServerError';
 import type { Conditions } from './Conditions';
-import type { RepresentationConverter } from './conversion/RepresentationConverter';
+import { matchingTypes } from './conversion/ConversionUtil';
+import type { RepresentationConverter, RepresentationConverterArgs } from './conversion/RepresentationConverter';
 import { PassthroughStore } from './PassthroughStore';
 import type { ResourceStore } from './ResourceStore';
 
@@ -48,11 +49,7 @@ export class RepresentationConvertingStore<T extends ResourceStore = ResourceSto
   public async getRepresentation(identifier: ResourceIdentifier, preferences: RepresentationPreferences,
     conditions?: Conditions): Promise<Representation> {
     const representation = await super.getRepresentation(identifier, preferences, conditions);
-    if (!this.outConverter || this.matchesPreferences(representation, preferences)) {
-      return representation;
-    }
-    this.logger.info(`Convert ${identifier.path} from ${representation.metadata.contentType} to ${preferences.type}`);
-    return this.outConverter.handleSafe({ identifier, representation, preferences });
+    return this.convertRepresentation({ identifier, representation, preferences }, this.outConverter);
   }
 
   public async addResource(container: ResourceIdentifier, representation: Representation,
@@ -69,28 +66,46 @@ export class RepresentationConvertingStore<T extends ResourceStore = ResourceSto
     return this.source.setRepresentation(identifier, representation, conditions);
   }
 
+  /**
+   * Helper function that checks if the given representation matches the given preferences.
+   */
   private matchesPreferences(representation: Representation, preferences: RepresentationPreferences): boolean {
-    if (!preferences.type) {
-      return true;
-    }
     const { contentType } = representation.metadata;
-    return Boolean(
-      contentType &&
-      preferences.type.some((type): boolean =>
-        type.weight > 0 &&
-        matchingMediaType(type.value, contentType)),
-    );
+
+    if (!contentType) {
+      throw new InternalServerError('Content-Type is required for data conversion.');
+    }
+
+    // Check if there is a result if we try to map the preferences to the content-type
+    return matchingTypes(preferences, [ contentType ]).length > 0;
   }
 
+  /**
+   * Helper function that converts a Representation using the given args and converter,
+   * if the conversion is necessary and there is a converter.
+   */
+  private async convertRepresentation(args: RepresentationConverterArgs, converter?: RepresentationConverter):
+  Promise<Representation> {
+    if (!converter || !args.preferences.type || this.matchesPreferences(args.representation, args.preferences)) {
+      return args.representation;
+    }
+
+    const typeStr = args.preferences.type.map((pref): string => `${pref.value};q=${pref.weight}`).join(', ');
+    this.logger.info(`Convert ${args.identifier.path} from ${args.representation.metadata.contentType} to ${typeStr}`);
+
+    return converter.handleSafe(args);
+  }
+
+  /**
+   * Helper function that converts an incoming representation if necessary.
+   */
   private async convertInRepresentation(identifier: ResourceIdentifier, representation: Representation):
   Promise<Representation> {
     if (!this.inType) {
       return representation;
     }
-    const inPreferences: RepresentationPreferences = { type: [{ value: this.inType, weight: 1 }]};
-    if (!inPreferences || !this.inConverter || this.matchesPreferences(representation, inPreferences)) {
-      return representation;
-    }
-    return this.inConverter.handleSafe({ identifier, representation, preferences: inPreferences });
+    const preferences: RepresentationPreferences = { type: [{ value: this.inType, weight: 1 }]};
+
+    return this.convertRepresentation({ identifier, representation, preferences }, this.inConverter);
   }
 }
diff --git a/src/storage/conversion/ConversionUtil.ts b/src/storage/conversion/ConversionUtil.ts
index 72a5ec8c6..b13d2fc0d 100644
--- a/src/storage/conversion/ConversionUtil.ts
+++ b/src/storage/conversion/ConversionUtil.ts
@@ -1,26 +1,60 @@
 import type { RepresentationPreference } from '../../ldp/representation/RepresentationPreference';
 import type { RepresentationPreferences } from '../../ldp/representation/RepresentationPreferences';
+import { INTERNAL_ALL } from '../../util/ContentTypes';
+import { InternalServerError } from '../../util/errors/InternalServerError';
 import { UnsupportedHttpError } from '../../util/errors/UnsupportedHttpError';
 import { matchingMediaType } from '../../util/Util';
 import type { RepresentationConverterArgs } from './RepresentationConverter';
 
 /**
- * Filters out the media types from the preferred types that correspond to one of the supported types.
+ * Filters media types based on the given preferences.
+ * Based on RFC 7231 - Content negotiation.
+ * Will add a default `internal/*;q=0` to the preferences to prevent accidental use of internal types.
+ * 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 supported - Types supported by the parser.
+ * @param types - Media types to compare to the preferences.
  *
  * @throws UnsupportedHttpError
- * If the type preferences are undefined.
+ * If the type preferences are undefined or if there are duplicate preferences.
  *
- * @returns The filtered list of preferences.
+ * @returns The weighted and filtered list of matching types.
  */
-export const matchingTypes = (preferences: RepresentationPreferences, supported: string[]):
+export const matchingTypes = (preferences: RepresentationPreferences, types: string[]):
 RepresentationPreference[] => {
   if (!Array.isArray(preferences.type)) {
     throw new UnsupportedHttpError('Output type required for conversion.');
   }
-  return preferences.type.filter(({ value, weight }): boolean => weight > 0 &&
-    supported.some((type): boolean => matchingMediaType(value, type)));
+
+  const prefMap = preferences.type.reduce((map: Record<string, number>, pref): Record<string, number> => {
+    if (map[pref.value]) {
+      throw new UnsupportedHttpError(`Duplicate type preference found: ${pref.value}`);
+    }
+    map[pref.value] = pref.weight;
+    return map;
+  }, {});
+
+  // Prevent accidental use of internal types
+  if (!prefMap[INTERNAL_ALL]) {
+    prefMap[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 = types.map((type): RepresentationPreference => {
+    const match = /^([^/]+)\/([^\s;]+)/u.exec(type);
+    if (!match) {
+      throw new InternalServerError(`Unexpected type preference: ${type}`);
+    }
+    const [ , main, sub ] = match;
+    const weight = prefMap[type] ?? prefMap[`${main}/${sub}`] ?? prefMap[`${main}/*`] ?? prefMap['*/*'] ?? 0;
+    return { value: type, weight };
+  });
+
+  return weightedSupported.filter((preference): boolean => preference.weight !== 0);
 };
 
 /**
diff --git a/src/util/ContentTypes.ts b/src/util/ContentTypes.ts
index 3c22a6e75..f4fdf0f48 100644
--- a/src/util/ContentTypes.ts
+++ b/src/util/ContentTypes.ts
@@ -4,4 +4,5 @@ export const APPLICATION_OCTET_STREAM = 'application/octet-stream';
 export const APPLICATION_SPARQL_UPDATE = 'application/sparql-update';
 
 // Internal (non-exposed) content types
+export const INTERNAL_ALL = 'internal/*';
 export const INTERNAL_QUADS = 'internal/quads';


Test patch:
diff --git a/test/unit/storage/RepresentationConvertingStore.test.ts b/test/unit/storage/RepresentationConvertingStore.test.ts
index 7920acf7c..26a8bf9c9 100644
--- a/test/unit/storage/RepresentationConvertingStore.test.ts
+++ b/test/unit/storage/RepresentationConvertingStore.test.ts
@@ -3,6 +3,7 @@ import { RepresentationMetadata } from '../../../src/ldp/representation/Represen
 import type { RepresentationConverter } from '../../../src/storage/conversion/RepresentationConverter';
 import { RepresentationConvertingStore } from '../../../src/storage/RepresentationConvertingStore';
 import type { ResourceStore } from '../../../src/storage/ResourceStore';
+import { InternalServerError } from '../../../src/util/errors/InternalServerError';
 import { CONTENT_TYPE } from '../../../src/util/UriConstants';
 
 describe('A RepresentationConvertingStore', (): void => {
@@ -30,7 +31,7 @@ describe('A RepresentationConvertingStore', (): void => {
 
   it('returns the Representation from the source if no changes are required.', async(): Promise<void> => {
     const result = await store.getRepresentation({ path: 'path' }, { type: [
-      { value: 'text/*', weight: 0 }, { value: 'text/turtle', weight: 1 },
+      { value: 'application/*', weight: 0 }, { value: 'text/turtle', weight: 1 },
     ]});
     expect(result).toEqual({
       data: 'data',
@@ -39,7 +40,9 @@ describe('A RepresentationConvertingStore', (): void => {
     expect(result.metadata.contentType).toEqual('text/turtle');
     expect(source.getRepresentation).toHaveBeenCalledTimes(1);
     expect(source.getRepresentation).toHaveBeenLastCalledWith(
-      { path: 'path' }, { type: [{ value: 'text/*', weight: 0 }, { value: 'text/turtle', weight: 1 }]}, undefined,
+      { path: 'path' },
+      { type: [{ value: 'application/*', weight: 0 }, { value: 'text/turtle', weight: 1 }]},
+      undefined,
     );
     expect(outConverter.handleSafe).toHaveBeenCalledTimes(0);
   });
@@ -98,4 +101,11 @@ describe('A RepresentationConvertingStore', (): void => {
     expect(inConverter.handleSafe).toHaveBeenCalledTimes(2);
     expect(source.setRepresentation).toHaveBeenLastCalledWith(id, 'inConvert', 'conditions');
   });
+
+  it('throws an error if no content-type is provided.', async(): Promise<void> => {
+    metadata.removeAll(CONTENT_TYPE);
+    const id = { path: 'identifier' };
+
+    await expect(store.addResource(id, representation, 'conditions' as any)).rejects.toThrow(InternalServerError);
+  });
 });
diff --git a/test/unit/storage/conversion/ConversionUtil.test.ts b/test/unit/storage/conversion/ConversionUtil.test.ts
index f9ca0aaef..d2007f033 100644
--- a/test/unit/storage/conversion/ConversionUtil.test.ts
+++ b/test/unit/storage/conversion/ConversionUtil.test.ts
@@ -3,6 +3,8 @@ import { RepresentationMetadata } from '../../../../src/ldp/representation/Repre
 import type { RepresentationPreferences } from '../../../../src/ldp/representation/RepresentationPreferences';
 import type { ResourceIdentifier } from '../../../../src/ldp/representation/ResourceIdentifier';
 import { checkRequest, matchingTypes } from '../../../../src/storage/conversion/ConversionUtil';
+import { InternalServerError } from '../../../../src/util/errors/InternalServerError';
+import { UnsupportedHttpError } from '../../../../src/util/errors/UnsupportedHttpError';
 
 describe('A ConversionUtil', (): void => {
   const identifier: ResourceIdentifier = { path: 'path' };
@@ -17,28 +19,28 @@ describe('A ConversionUtil', (): void => {
   describe('#checkRequest', (): void => {
     it('requires an input type.', async(): Promise<void> => {
       const preferences: RepresentationPreferences = {};
-      expect((): any => checkRequest({ identifier, representation, preferences }, [ '*/*' ], [ '*/*' ]))
+      expect((): any => checkRequest({ identifier, representation, preferences }, [ 'a/x' ], [ 'a/x' ]))
         .toThrow('Input type required for conversion.');
     });
 
     it('requires a matching input type.', async(): Promise<void> => {
       metadata.contentType = 'a/x';
       const preferences: RepresentationPreferences = { type: [{ value: 'b/x', weight: 1 }]};
-      expect((): any => checkRequest({ identifier, representation, preferences }, [ 'c/x' ], [ '*/*' ]))
-        .toThrow('Can only convert from c/x to */*.');
+      expect((): any => checkRequest({ identifier, representation, preferences }, [ 'c/x' ], [ 'a/x' ]))
+        .toThrow('Can only convert from c/x to a/x.');
     });
 
     it('requires a matching output type.', async(): Promise<void> => {
       metadata.contentType = 'a/x';
       const preferences: RepresentationPreferences = { type: [{ value: 'b/x', weight: 1 }]};
-      expect((): any => checkRequest({ identifier, representation, preferences }, [ '*/*' ], [ 'c/x' ]))
-        .toThrow('Can only convert from */* to c/x.');
+      expect((): any => checkRequest({ identifier, representation, preferences }, [ 'a/x' ], [ 'c/x' ]))
+        .toThrow('Can only convert from a/x to c/x.');
     });
 
     it('succeeds with a valid input and output type.', async(): Promise<void> => {
       metadata.contentType = 'a/x';
       const preferences: RepresentationPreferences = { type: [{ value: 'b/x', weight: 1 }]};
-      expect(checkRequest({ identifier, representation, preferences }, [ '*/*' ], [ '*/*' ]))
+      expect(checkRequest({ identifier, representation, preferences }, [ 'a/x' ], [ 'b/x' ]))
         .toBeUndefined();
     });
   });
@@ -46,7 +48,7 @@ describe('A ConversionUtil', (): void => {
   describe('#matchingTypes', (): void => {
     it('requires type preferences.', async(): Promise<void> => {
       const preferences: RepresentationPreferences = {};
-      expect((): any => matchingTypes(preferences, [ '*/*' ]))
+      expect((): any => matchingTypes(preferences, [ 'a/b' ]))
         .toThrow('Output type required for conversion.');
     });
 
@@ -55,5 +57,38 @@ describe('A ConversionUtil', (): void => {
           [{ value: 'a/x', weight: 1 }, { value: 'b/x', weight: 0.5 }, { value: 'c/x', weight: 0 }]};
       expect(matchingTypes(preferences, [ 'b/x', 'c/x' ])).toEqual([{ value: 'b/x', weight: 0.5 }]);
     });
+
+    it('errors if there are duplicate preferences.', async(): Promise<void> => {
+      const preferences: RepresentationPreferences =
+        { type: [{ value: 'b/x', weight: 1 }, { value: 'b/x', weight: 0 }]};
+      expect((): any => matchingTypes(preferences, [ 'b/x' ]))
+        .toThrow(new UnsupportedHttpError(`Duplicate type preference found: b/x`));
+    });
+
+    it('errors if there invalid types.', async(): Promise<void> => {
+      const preferences: RepresentationPreferences =
+        { type: [{ value: 'b/x', weight: 1 }]};
+      expect((): any => matchingTypes(preferences, [ 'noType' ]))
+        .toThrow(new InternalServerError(`Unexpected type preference: noType`));
+    });
+
+    it('filters out internal types.', async(): Promise<void> => {
+      const preferences: RepresentationPreferences = { type: [{ value: '*/*', weight: 1 }]};
+      expect(matchingTypes(preferences, [ 'a/x', 'internal/quads' ])).toEqual([{ value: 'a/x', weight: 1 }]);
+    });
+
+    it('keeps internal types that are specifically requested.', async(): Promise<void> => {
+      const preferences: RepresentationPreferences =
+        { type: [{ value: '*/*', weight: 1 }, { value: 'internal/*', weight: 0.5 }]};
+      expect(matchingTypes(preferences, [ 'a/x', 'internal/quads' ]))
+        .toEqual([{ value: 'a/x', weight: 1 }, { value: 'internal/quads', weight: 0.5 }]);
+    });
+
+    it('takes the most relevant weight for a type.', async(): Promise<void> => {
+      const preferences: RepresentationPreferences =
+        { type: [{ value: '*/*', weight: 1 }, { value: 'internal/quads', weight: 0.5 }]};
+      expect(matchingTypes(preferences, [ 'a/x', 'internal/quads' ]))
+        .toEqual([{ value: 'a/x', weight: 1 }, { value: 'internal/quads', weight: 0.5 }]);
+    });
   });
 });