Merge branch 'dev' into domain

src/domain/domainConfiguration.ts

@@ -27,8 +27,7 @@ export abstract class DomainConfiguration<
   revisions: Promise<RevisionMetadata[]> | undefined;
   revisionCache: Map<
     RevisionMetadata["revid"],
-    | Promise<ConfigurationRevision<ConfigurationDefinitionType> | undefined>
-    | undefined
+    Promise<ContentRevision | undefined> | undefined
   > = new Map();
   currentRevid: RevisionMetadata["revid"] | undefined;
 
@@ -92,7 +91,7 @@ export abstract class DomainConfiguration<
 
   private async fetchRevision(
     revid?: RevisionMetadata["revid"]
-  ): Promise<ConfigurationRevision<ConfigurationDefinitionType> | undefined> {
+  ): Promise<ContentRevision | undefined> {
     const api = new RevisionsApi(this.mediawiki.instance);
     const revisions = await api.fetchRevisions(this.title, true, revid, 1);
     const revision = revisions[0];
@@ -101,31 +100,9 @@ export abstract class DomainConfiguration<
       let info = `No revision found for page "${this.title}"`;
       if (revid !== undefined) info = info + ` and revid ${revid}`;
       log.info(info);
-      return undefined;
     }
 
-    if (revision.content === undefined) {
-      return Promise.reject(`Unexpected undefined revision content`);
-    }
-
-    const strippedContent = revision.content
-      .replace('<syntaxhighlight lang="json">', "")
-      .replace("</syntaxhighlight>", "");
-
-    let configuration: ConfigurationDefinitionType[];
-    try {
-      configuration = this.parse(strippedContent);
-    } catch (e) {
-      let message = "Could not parse revision content";
-      if (e instanceof Error) message = message + `: ${e.message}`;
-      throw new ContentRevisionError(message, revision);
-    }
-
-    return {
-      revid: revision.revid,
-      timestamp: revision.timestamp,
-      configuration: configuration,
-    };
+    return revision;
   }
 
   getRevisionIds(refresh = false): Promise<RevisionMetadata[]> {
@@ -138,7 +115,7 @@ export abstract class DomainConfiguration<
   getRevision(
     revid: RevisionMetadata["revid"],
     refresh = false
-  ): Promise<ConfigurationRevision<ConfigurationDefinitionType> | undefined> {
+  ): Promise<ContentRevision | undefined> {
     let revisionPromise = this.revisionCache.get(revid);
     if (revisionPromise === undefined || refresh) {
       revisionPromise = this.fetchRevision(revid);
@@ -147,9 +124,7 @@ export abstract class DomainConfiguration<
     return revisionPromise;
   }
 
-  getLatestRevision(): Promise<
-    ConfigurationRevision<ConfigurationDefinitionType> | undefined
-  > {
+  getLatestRevision(): Promise<ContentRevision | undefined> {
     return this.fetchRevision().then((revision) => {
       if (revision !== undefined) {
         const revisionPromise = Promise.resolve(revision);
@@ -159,10 +134,28 @@ export abstract class DomainConfiguration<
     });
   }
 
-  loadRevision(
-    revision: ConfigurationRevision<ConfigurationDefinitionType>
-  ): void {
-    this.loadConfiguration(revision.configuration);
+  loadRevision(revision: ContentRevision): void {
+    if (revision.content === undefined) {
+      throw new ContentRevisionError(
+        `Unexpected undefined revision content`,
+        revision
+      );
+    }
+
+    const strippedContent = revision.content
+      .replace('<syntaxhighlight lang="json">', "")
+      .replace("</syntaxhighlight>", "");
+
+    let configuration: ConfigurationDefinitionType[];
+    try {
+      configuration = this.parse(strippedContent);
+    } catch (e) {
+      let message = "Could not parse revision content";
+      if (e instanceof Error) message = message + `: ${e.message}`;
+      throw new ContentRevisionError(message, revision);
+    }
+
+    this.loadConfiguration(configuration);
     this.currentRevid = revision.revid;
   }
 
@@ -185,10 +178,6 @@ ${this.toJSON()}
   }
 }
 
-interface ConfigurationRevision<T> extends RevisionMetadata {
-  configuration: T[];
-}
-
 class ContentRevisionError extends Error {
   revision: ContentRevision;
   constructor(message: string, revision: ContentRevision) {

src/domain/templateConfiguration.test.ts

@@ -7,6 +7,8 @@ import {
 import { Webpage } from "../webpage/webpage";
 import * as nodeFetch from "node-fetch";
 import { pages } from "../webpage/samplePages";
+import log from "loglevel";
+import { ContentRevision } from "../mediawiki/revisions";
 
 const mockNodeFetch = nodeFetch as typeof import("../../__mocks__/node-fetch");
 
@@ -273,3 +275,128 @@ it("multiple templates for the same path are silently ignored", () => {
   );
   expect(configuration.get().length).toBe(1);
 });
+
+describe("Configuration revisions", () => {
+  const warnSpy = jest.spyOn(log, "warn").mockImplementation();
+  beforeEach(() => {
+    jest.clearAllMocks();
+  });
+  it("skips misformatted elements individually", () => {
+    const content = JSON.stringify([
+      {
+        path: "/",
+        fields: [
+          {
+            fieldname: "itemType",
+            required: true,
+            procedures: [
+              {
+                selections: [
+                  {
+                    type: "citoid",
+                    config: "itemType",
+                  },
+                  {
+                    type: "invalidType",
+                    config: "itemType",
+                  },
+                ],
+                transformations: [
+                  {
+                    type: "range",
+                    config: "0",
+                    itemwise: true,
+                  },
+                  {
+                    type: "range",
+                    config: "invalidConfig",
+                    itemwise: true,
+                  },
+                ],
+              },
+              {
+                selections: [],
+              },
+              {
+                transformations: [],
+              },
+            ],
+          },
+          {
+            fieldname: "invalidField",
+            required: true,
+            procedures: [],
+          },
+        ],
+      },
+    ]);
+    const revision: ContentRevision = {
+      revid: 0,
+      timestamp: "",
+      content,
+    };
+    const configuration = new TemplateConfiguration(
+      "example.com",
+      [],
+      undefined
+    );
+    configuration.loadRevision(revision);
+    expect(warnSpy).toHaveBeenCalledTimes(5);
+    expect(configuration.get().map((template) => template.toJSON())).toEqual([
+      {
+        path: "/",
+        label: "",
+        fields: [
+          {
+            fieldname: "itemType",
+            required: true,
+            procedures: [
+              {
+                selections: [
+                  {
+                    type: "citoid",
+                    config: "itemType",
+                  },
+                ],
+                transformations: [
+                  {
+                    type: "range",
+                    config: "0",
+                    itemwise: true,
+                  },
+                ],
+              },
+            ],
+          },
+        ],
+      },
+    ]);
+  });
+  it("skips templates with missing mandatory fields", () => {
+    const definitions: TemplateDefinition[] = [
+      {
+        path: "/",
+        fields: [
+          {
+            fieldname: "itemType",
+            required: true,
+            procedures: [],
+          },
+        ],
+      },
+    ];
+    const revision: ContentRevision = {
+      revid: 0,
+      timestamp: "",
+      content: JSON.stringify(definitions),
+    };
+    const configuration = new TemplateConfiguration(
+      "example.com",
+      ["itemType", "title"],
+      undefined
+    );
+    configuration.loadRevision(revision);
+    expect(warnSpy).toHaveBeenCalledTimes(1);
+    expect(configuration.get().length).toBe(0);
+  });
+});

src/domain/templateConfiguration.ts

@@ -9,7 +9,6 @@ import log from "loglevel";
 import { Webpage } from "../webpage/webpage";
 import {
   FallbackTemplateDefinition,
-  isTemplateDefinition,
   TemplateDefinition,
   TemplateOutput,
 } from "../types";
@@ -80,11 +79,9 @@ export class TemplateConfiguration extends DomainConfiguration<
   add(definition: TemplateDefinition, index?: number): TranslationTemplate {
     // create template instance before checking if path already exists
     // because the template constructor may make changes to the path
-    const newTemplate = new TranslationTemplate(
-      this.domain,
-      definition,
-      this.mandatoryFields
-    );
+    const newTemplate = new TranslationTemplate(this.domain, definition, {
+      forceRequiredFields: this.mandatoryFields,
+    });
     if (this.templates.some((template) => template.path === newTemplate.path)) {
       throw new DuplicateTemplatePathError(definition.path);
     } else {
@@ -135,19 +132,23 @@ export class TemplateConfiguration extends DomainConfiguration<
     }
     const templateDefinitions = definitions.reduce(
       (templateDefinitions: TemplateDefinition[], definition, index) => {
-        // fixme: instead of ignoring templates
-        // with unsupported fields or selection/transformation steps
-        // simply ignore the unsupported objects
-        if (isTemplateDefinition(definition)) {
-          templateDefinitions.push(definition);
-        } else {
+        // to address T305267, instead of using isTemplateDefinition,
+        // create template from definition, skipping individual invalid elements
+        // and convert back to json
+        try {
+          const template = new TranslationTemplate(this.domain, definition, {
+            forceRequiredFields: this.mandatoryFields,
+            strict: false,
+          });
+          templateDefinitions.push(template.toJSON());
+        } catch (error) {
           let info = "Ignoring misformatted template";
           if ("path" in definition) {
             info = info + ` for path "${definition.path}"`;
           } else {
             info = info + ` at index ${index}`;
           }
-          log.info(info);
+          log.warn(info + `: ${error}`);
         }
         return templateDefinitions;
       },
@@ -157,15 +158,9 @@ export class TemplateConfiguration extends DomainConfiguration<
   }
 
   loadConfiguration(templates: TemplateDefinition[]): void {
-    if (this.mandatoryFields === undefined) {
-      throw new Error(
-        "Mandatory template fields must be defined before loading any template configuration"
-      );
-    }
     // fixme?: wiping previous translation templates erases template caches
     // see T302239
     this.templates = [];
-    // silently ignore duplicate
     templates.forEach((definition) => {
       try {
         this.add(definition);

src/templates/procedure.test.ts

@@ -4,6 +4,8 @@ import { CitoidSelection } from "./selection";
 import { JoinTransformation, RangeTransformation } from "./transformation";
 import * as nodeFetch from "node-fetch";
 import { pages } from "../webpage/samplePages";
+import log from "loglevel";
+import { ProcedureDefinition } from "../types";
 
 const mockNodeFetch = nodeFetch as typeof import("../../__mocks__/node-fetch");
 
@@ -66,4 +68,61 @@ it("does not return selection output if transformation output is an empty array"
   });
 });
 
+it("constructor optionally skips invalid translation step definitions", () => {
+  const warnSpy = jest.spyOn(log, "warn").mockImplementation();
+  const definition: ProcedureDefinition = {
+    selections: [
+      {
+        type: "citoid",
+        config: "itemType",
+      },
+      {
+        type: "citoid",
+        config: "invalidConfig",
+      },
+      {
+        type: "invalidType",
+        config: "itemType",
+      },
+    ],
+    transformations: [
+      {
+        type: "range",
+        config: "0",
+        itemwise: true,
+      },
+      {
+        type: "invalidType",
+        config: "0",
+        itemwise: true,
+      },
+      {
+        type: "range",
+        config: "invalidConfig",
+        itemwise: true,
+      },
+    ],
+  };
+  const procedure = new TranslationProcedure(definition, { strict: false });
+  expect(warnSpy).toHaveBeenCalledTimes(4);
+  expect(procedure.toJSON()).toEqual({
+    selections: [
+      {
+        type: "citoid",
+        config: "itemType",
+      },
+    ],
+    transformations: [
+      {
+        type: "range",
+        config: "0",
+        itemwise: true,
+      },
+    ],
+  });
+  expect(() => {
+    new TranslationProcedure(definition);
+  }).toThrow();
+});
+
 // empty selection output should give empty transformation output

src/templates/procedure.ts

@@ -3,6 +3,7 @@ import { Transformation } from "./transformation";
 import { Webpage } from "../webpage/webpage";
 import { StepOutput } from "../types";
 import { ProcedureDefinition, ProcedureOutput } from "../types";
+import log from "loglevel";
 
 export class TranslationProcedure {
   selections: Array<Selection>;
@@ -12,13 +13,48 @@ export class TranslationProcedure {
     procedure: ProcedureDefinition = {
       selections: [],
       transformations: [],
-    }
+    },
+    {
+      strict = true,
+    }: {
+      strict?: boolean;
+    } = {}
   ) {
-    this.selections = procedure.selections.map((selection) =>
-      Selection.create(selection)
+    this.selections = procedure.selections.reduce(
+      (selections: Selection[], selection) => {
+        try {
+          selections.push(Selection.create(selection));
+        } catch (e) {
+          if (!strict) {
+            const type = selection.type ?? "untitled";
+            log.warn(
+              `Failed to parse "${type}" selection step definition: ${e}`
+            );
+          } else {
+            throw e;
+          }
+        }
+        return selections;
+      },
+      []
     );
-    this.transformations = procedure.transformations.map((transformation) =>
-      Transformation.create(transformation)
+    this.transformations = procedure.transformations.reduce(
+      (transformations: Transformation[], transformation) => {
+        try {
+          transformations.push(Transformation.create(transformation));
+        } catch (e) {
+          if (!strict) {
+            const type = transformation.type ?? "untitled";
+            log.warn(
+              `Failed to parse "${type}" transformation step definition: ${e}`
+            );
+          } else {
+            throw e;
+          }
+        }
+        return transformations;
+      },
+      []
     );
   }
 

src/templates/template.test.ts

@@ -4,6 +4,7 @@ import { Webpage } from "../webpage/webpage";
 import * as nodeFetch from "node-fetch";
 import { pages } from "../webpage/samplePages";
 import { TemplateFieldDefinition, TemplateDefinition } from "../types";
+import log from "loglevel";
 
 const mockNodeFetch = nodeFetch as typeof import("../../__mocks__/node-fetch");
 
@@ -90,7 +91,7 @@ it("outputs a JSON template definition", () => {
     fields: [],
   });
   template.label = "sample label";
-  template.addField(new TemplateField("itemType", true));
+  template.addField(new TemplateField("itemType", { loadDefaults: true }));
   expect(template.toJSON()).toEqual<TemplateDefinition>({
     path: "/article1",
     label: "sample label",
@@ -113,3 +114,60 @@ it("outputs a JSON template definition", () => {
     ],
   });
 });
+
+it("constructor optionally skips invalid field definitions", () => {
+  const warnSpy = jest.spyOn(log, "warn").mockImplementation();
+  const definition: unknown = {
+    path: "/",
+    fields: [
+      {
+        fieldname: "itemType",
+        required: true,
+        procedures: [],
+      },
+      {
+        fieldname: "invalidField",
+        required: true,
+        procedures: [],
+      },
+    ],
+  };
+  const template = new TranslationTemplate(
+    "example.com",
+    definition as TemplateDefinition,
+    { strict: false }
+  );
+  expect(warnSpy).toHaveBeenCalledTimes(1);
+  expect(template.toJSON()).toEqual({
+    path: "/",
+    label: "",
+    fields: [
+      {
+        fieldname: "itemType",
+        required: true,
+        procedures: [],
+      },
+    ],
+  });
+  expect(() => {
+    new TranslationTemplate("example.com", definition as TemplateDefinition);
+  }).toThrow();
+});
+
+it("rejects creation if a mandatory field is missing", () => {
+  const definition: TemplateDefinition = {
+    path: "/",
+    fields: [
+      {
+        fieldname: "title",
+        required: true,
+        procedures: [],
+      },
+    ],
+  };
+  expect(() => {
+    new TranslationTemplate("example.com", definition, {
+      forceRequiredFields: ["itemType", "title"],
+    });
+  }).toThrow('Mandatory field "itemType" missing from template definition');
+});

src/templates/template.ts

@@ -18,26 +18,58 @@ export abstract class BaseTranslationTemplate {
   protected constructor(
     domain: string,
     template: TemplateDefinition | FallbackTemplateDefinition,
-    forceRequiredFields: Array<FieldName> = []
+    {
+      forceRequiredFields = [],
+      strict = true,
+    }: {
+      forceRequiredFields?: Array<FieldName>;
+      strict?: boolean;
+    } = {}
   ) {
     if (!isDomainName(domain)) {
       throw new DomainNameError(domain);
     }
+
+    // reject template creation if any mandatory field is missing
+    if ("fields" in template) {
+      const fieldnames = template.fields.map((field) => field.fieldname);
+      for (const field of forceRequiredFields) {
+        if (!fieldnames.includes(field)) {
+          throw new MissingFieldError(field);
+        }
+      }
+    }
+
     this.domain = domain;
     this.forceRequiredFields = forceRequiredFields;
     this.label = template.label ?? "";
     if (template.fields) {
       template.fields.forEach((definition) => {
-        const field = new TemplateField(definition);
+        let field;
         try {
-          this.addField(field);
+          field = new TemplateField(definition, { strict });
         } catch (e) {
-          if (e instanceof DuplicateFieldError) {
-            log.info(`Skipping duplicate field "${field.name}"`);
+          if (!strict) {
+            const fieldname = definition.fieldname ?? "untitled";
+            log.warn(
+              `Failed to parse "${fieldname}" template field definition: ${e}`
+            );
           } else {
             throw e;
           }
         }
+        if (field !== undefined) {
+          try {
+            this.addField(field);
+          } catch (e) {
+            if (e instanceof DuplicateFieldError) {
+              log.info(`Skipping duplicate field "${field.name}"`);
+            } else {
+              log.info(``);
+              throw e;
+            }
+          }
+        }
       });
     }
   }
@@ -99,9 +131,15 @@ export class TranslationTemplate extends BaseTranslationTemplate {
   constructor(
     domain: string,
     template: TemplateDefinition,
-    forceRequiredFields: Array<FieldName> = []
+    {
+      forceRequiredFields = [],
+      strict = true,
+    }: {
+      forceRequiredFields?: Array<FieldName>;
+      strict?: boolean;
+    } = {}
   ) {