Repository: solid/community-server (ts)

Issue:
Pod template folder can only contain text files
The `TemplatedResourcesGenerator` tries to interpret every file in the template folder as a string and feed it into the templating engine: https://github.com/solid/community-server/blob/e43b579ae7a14a0b7be5c89545922346c7fb3833/src/pods/generate/TemplatedResourcesGenerator.ts#L153

This goes wrong if the file is a non-text file such as an image. The templating engine should only be called on text-based files, or perhaps even only on files that are indicated to be a template.

We could just catch the error there and return the raw file in case of error. Or we could check the content-type, but then we need a list of supported types. Or we could use a specific extension for files that need te be templated so they can easily be recognized, such as `card.ttl.hbs`.

Pull request description:
feat: Differentiate between templates and non-templates for pods
Closes #821 

Patch:
diff --git a/src/pods/generate/TemplatedResourcesGenerator.ts b/src/pods/generate/TemplatedResourcesGenerator.ts
index 130bb6536..9c4bdca40 100644
--- a/src/pods/generate/TemplatedResourcesGenerator.ts
+++ b/src/pods/generate/TemplatedResourcesGenerator.ts
@@ -1,4 +1,5 @@
-import { promises as fsPromises } from 'fs';
+import { createReadStream, promises as fsPromises } from 'fs';
+import type { Readable } from 'stream';
 import { Parser } from 'n3';
 import { BasicRepresentation } from '../../ldp/representation/BasicRepresentation';
 import { RepresentationMetadata } from '../../ldp/representation/RepresentationMetadata';
@@ -8,11 +9,18 @@ import type {
   FileIdentifierMapperFactory,
   ResourceLink,
 } from '../../storage/mapping/FileIdentifierMapper';
+import { guardStream } from '../../util/GuardedStream';
+import type { Guarded } from '../../util/GuardedStream';
 import { joinFilePath, isContainerIdentifier, resolveAssetPath } from '../../util/PathUtil';
+import { guardedStreamFrom, readableToString } from '../../util/StreamUtil';
 import type { Resource, ResourcesGenerator } from './ResourcesGenerator';
 import type { TemplateEngine } from './TemplateEngine';
 import Dict = NodeJS.Dict;
 
+interface TemplateResourceLink extends ResourceLink {
+  isTemplate: boolean;
+}
+
 /**
  * Generates resources by making use of a template engine.
  * The template folder structure will be kept.
@@ -26,6 +34,7 @@ export class TemplatedResourcesGenerator implements ResourcesGenerator {
   private readonly templateFolder: string;
   private readonly factory: FileIdentifierMapperFactory;
   private readonly engine: TemplateEngine;
+  private readonly templateExtension: string;
 
   /**
    * A mapper is needed to convert the template file paths to identifiers relative to the given base identifier.
@@ -33,26 +42,30 @@ export class TemplatedResourcesGenerator implements ResourcesGenerator {
    * @param templateFolder - Folder where the templates are located.
    * @param factory - Factory used to generate mapper relative to the base identifier.
    * @param engine - Template engine for generating the resources.
+   * @param templateExtension - The extension of files that need to be interpreted as templates.
+   *                            Will be removed to generate the identifier.
    */
-  public constructor(templateFolder: string, factory: FileIdentifierMapperFactory, engine: TemplateEngine) {
+  public constructor(templateFolder: string, factory: FileIdentifierMapperFactory, engine: TemplateEngine,
+    templateExtension = '.hbs') {
     this.templateFolder = resolveAssetPath(templateFolder);
     this.factory = factory;
     this.engine = engine;
+    this.templateExtension = templateExtension;
   }
 
   public async* generate(location: ResourceIdentifier, options: Dict<string>): AsyncIterable<Resource> {
     const mapper = await this.factory.create(location.path, this.templateFolder);
-    const folderLink = await mapper.mapFilePathToUrl(this.templateFolder, true);
+    const folderLink = await this.toTemplateLink(this.templateFolder, mapper);
     yield* this.parseFolder(folderLink, mapper, options);
   }
 
   /**
    * Generates results for all entries in the given folder, including the folder itself.
    */
-  private async* parseFolder(folderLink: ResourceLink, mapper: FileIdentifierMapper, options: Dict<string>):
+  private async* parseFolder(folderLink: TemplateResourceLink, mapper: FileIdentifierMapper, options: Dict<string>):
   AsyncIterable<Resource> {
     // Group resource links with their corresponding metadata links
-    const links = await this.groupLinks(this.generateLinks(folderLink.filePath, mapper));
+    const links = await this.groupLinks(folderLink.filePath, mapper);
 
     // Remove root metadata if it exists
     const metaLink = links[folderLink.identifier.path]?.meta;
@@ -71,30 +84,37 @@ export class TemplatedResourcesGenerator implements ResourcesGenerator {
   }
 
   /**
-   * Generates ResourceLinks for each entry in the given folder.
+   * Creates a TemplateResourceLink for the given filePath.
+   * The identifier will be based on the file path stripped from the template extension,
+   * but the filePath parameter will still point to the original file.
    */
-  private async* generateLinks(folderPath: string, mapper: FileIdentifierMapper): AsyncIterable<ResourceLink> {
-    const files = await fsPromises.readdir(folderPath);
-    for (const name of files) {
-      const filePath = joinFilePath(folderPath, name);
-      const stats = await fsPromises.lstat(filePath);
-      yield mapper.mapFilePathToUrl(filePath, stats.isDirectory());
-    }
+  private async toTemplateLink(filePath: string, mapper: FileIdentifierMapper): Promise<TemplateResourceLink> {
+    const stats = await fsPromises.lstat(filePath);
+
+    // Slice the template extension from the filepath for correct identifier generation
+    const isTemplate = filePath.endsWith(this.templateExtension);
+    const slicedPath = isTemplate ? filePath.slice(0, -this.templateExtension.length) : filePath;
+    const link = await mapper.mapFilePathToUrl(slicedPath, stats.isDirectory());
+    // We still need the original file path for disk reading though
+    return {
+      ...link,
+      filePath,
+      isTemplate,
+    };
   }
 
   /**
-   * Parses a group of ResourceLinks so resources and their metadata are grouped together.
+   * Generates TemplateResourceLinks for each entry in the given folder
+   * and combines the results so resources and their metadata are grouped together.
    */
-  private async groupLinks(linkGen: AsyncIterable<ResourceLink>):
-  Promise<Record<string, { link: ResourceLink; meta?: ResourceLink }>> {
-    const links: Record<string, { link: ResourceLink; meta?: ResourceLink }> = { };
-    for await (const link of linkGen) {
+  private async groupLinks(folderPath: string, mapper: FileIdentifierMapper):
+  Promise<Record<string, { link: TemplateResourceLink; meta?: TemplateResourceLink }>> {
+    const files = await fsPromises.readdir(folderPath);
+    const links: Record<string, { link: TemplateResourceLink; meta?: TemplateResourceLink }> = { };
+    for (const name of files) {
+      const link = await this.toTemplateLink(joinFilePath(folderPath, name), mapper);
       const { path } = link.identifier;
-      if (link.isMetadata) {
-        links[path] = Object.assign(links[path] || {}, { meta: link });
-      } else {
-        links[path] = Object.assign(links[path] || {}, { link });
-      }
+      links[path] = Object.assign(links[path] || {}, link.isMetadata ? { meta: link } : { link });
     }
     return links;
   }
@@ -104,15 +124,14 @@ export class TemplatedResourcesGenerator implements ResourcesGenerator {
    * In the case of documents the corresponding template will be used.
    * If a ResourceLink of metadata is provided the corresponding data will be added as metadata.
    */
-  private async generateResource(link: ResourceLink, options: Dict<string>, metaLink?: ResourceLink):
+  private async generateResource(link: TemplateResourceLink, options: Dict<string>, metaLink?: TemplateResourceLink):
   Promise<Resource> {
-    const data: string[] = [];
+    let data: Guarded<Readable> | undefined;
     const metadata = new RepresentationMetadata(link.identifier);
 
     // Read file if it is not a container
     if (!isContainerIdentifier(link.identifier)) {
-      const compiled = await this.parseTemplate(link.filePath, options);
-      data.push(compiled);
+      data = await this.parseFile(link, options);
       metadata.contentType = link.contentType;
     }
 
@@ -124,30 +143,34 @@ export class TemplatedResourcesGenerator implements ResourcesGenerator {
 
     return {
       identifier: link.identifier,
-      representation: new BasicRepresentation(data, metadata),
+      representation: new BasicRepresentation(data ?? [], metadata),
     };
   }
 
   /**
    * Generates a RepresentationMetadata using the given template.
    */
-  private async generateMetadata(metaLink: ResourceLink, options: Dict<string>):
+  private async generateMetadata(metaLink: TemplateResourceLink, options: Dict<string>):
   Promise<RepresentationMetadata> {
     const metadata = new RepresentationMetadata(metaLink.identifier);
 
-    const data = await this.parseTemplate(metaLink.filePath, options);
+    const data = await this.parseFile(metaLink, options);
     const parser = new Parser({ format: metaLink.contentType, baseIRI: metaLink.identifier.path });
-    const quads = parser.parse(data);
+    const quads = parser.parse(await readableToString(data));
     metadata.addQuads(quads);
 
     return metadata;
   }
 
   /**
-   * Applies the given options to the template found at the given path.
+   * Creates a read stream from the file and applies the template if necessary.
    */
-  private async parseTemplate(filePath: string, options: Dict<string>): Promise<string> {
-    const raw = await fsPromises.readFile(filePath, 'utf8');
-    return this.engine.apply(raw, options);
+  private async parseFile(link: TemplateResourceLink, options: Dict<string>): Promise<Guarded<Readable>> {
+    if (link.isTemplate) {
+      const raw = await fsPromises.readFile(link.filePath, 'utf8');
+      const result = this.engine.apply(raw, options);
+      return guardedStreamFrom(result);
+    }
+    return guardStream(createReadStream(link.filePath));
   }
 }


Test patch:
diff --git a/templates/pod/.acl b/templates/pod/.acl.hbs
similarity index 100%
rename from templates/pod/.acl
rename to templates/pod/.acl.hbs
diff --git a/templates/pod/profile/card$.ttl b/templates/pod/profile/card$.ttl.hbs
similarity index 100%
rename from templates/pod/profile/card$.ttl
rename to templates/pod/profile/card$.ttl.hbs
diff --git a/templates/pod/profile/card.acl b/templates/pod/profile/card.acl.hbs
similarity index 100%
rename from templates/pod/profile/card.acl
rename to templates/pod/profile/card.acl.hbs
diff --git a/test/unit/pods/generate/TemplatedResourcesGenerator.test.ts b/test/unit/pods/generate/TemplatedResourcesGenerator.test.ts
index 91cef2f31..c5cdb0544 100644
--- a/test/unit/pods/generate/TemplatedResourcesGenerator.test.ts
+++ b/test/unit/pods/generate/TemplatedResourcesGenerator.test.ts
@@ -56,7 +56,7 @@ describe('A TemplatedResourcesGenerator', (): void => {
   });
 
   it('fills in a template with the given options.', async(): Promise<void> => {
-    cache.data = { template };
+    cache.data = { 'template.hbs': template };
     const result = await genToArray(generator.generate(location, { webId }));
     const identifiers = result.map((res): ResourceIdentifier => res.identifier);
     const id = { path: `${location.path}template` };
@@ -70,7 +70,7 @@ describe('A TemplatedResourcesGenerator', (): void => {
   });
 
   it('creates the necessary containers.', async(): Promise<void> => {
-    cache.data = { container: { container: { template }}};
+    cache.data = { container: { container: { 'template.hbs': template }}};
     const result = await genToArray(generator.generate(location, { webId }));
     const identifiers = result.map((res): ResourceIdentifier => res.identifier);
     const id = { path: `${location.path}container/container/template` };
@@ -86,6 +86,19 @@ describe('A TemplatedResourcesGenerator', (): void => {
       .toEqual(`<${webId}> a <http://xmlns.com/foaf/0.1/Person>.`);
   });
 
+  it('copies the file stream directly if no template extension is found.', async(): Promise<void> => {
+    cache.data = { noTemplate: template };
+    const result = await genToArray(generator.generate(location, { webId }));
+    const identifiers = result.map((res): ResourceIdentifier => res.identifier);
+    const id = { path: `${location.path}noTemplate` };
+    expect(identifiers).toEqual([ location, id ]);
+
+    const { representation } = result[1];
+    expect(representation.binary).toBe(true);
+    expect(representation.metadata.contentType).toBe('text/turtle');
+    await expect(readableToString(representation.data)).resolves.toEqual(template);
+  });
+
   it('adds metadata from .meta files.', async(): Promise<void> => {
     const meta = '<> <pre:has> "metadata".';
     cache.data = { '.meta': meta, container: { 'template.meta': meta, template }};