Commit 6eba2490 authored by Diegodlh's avatar Diegodlh
Browse files

Fix T305267

* domain configuration revision cache now stores content revisions instead of (parsed) configuration revisions
* parsing of revision content now occurs later, at revision loading, rather than at revision fetching
* template, field and procedure constructors now have a "strict" option which if set to false ignores individual invalid elements (fields, procedures and steps, respectively)
* TemplateConfiguration implementation of the "parse" method uses the actual constructors to validate definitions
parent 1edc51f7
......@@ -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) {
......
......@@ -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");
......@@ -264,3 +266,98 @@ 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();
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,
},
],
},
],
},
],
},
]);
});
});
......@@ -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,12 +132,15 @@ 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, {
strict: false,
});
templateDefinitions.push(template.toJSON());
} catch {
let info = "Ignoring misformatted template";
if ("path" in definition) {
info = info + ` for path "${definition.path}"`;
......
......@@ -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
......@@ -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;
},
[]
);
}
......
......@@ -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,42 @@ 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();
});
......@@ -18,7 +18,13 @@ 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);
......@@ -28,16 +34,31 @@ export abstract class BaseTranslationTemplate {
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 +120,15 @@ export class TranslationTemplate extends BaseTranslationTemplate {
constructor(
domain: string,
template: TemplateDefinition,
forceRequiredFields: Array<FieldName> = []
{
forceRequiredFields = [],
strict = true,
}: {
forceRequiredFields?: Array<FieldName>;
strict?: boolean;
} = {}
) {
super(domain, template, forceRequiredFields);
super(domain, template, { forceRequiredFields, strict });
const url = "http://" + this.domain + template.path;
try {
this.template = new Webpage(url);
......@@ -136,7 +163,7 @@ export class FallbackTemplate extends BaseTranslationTemplate {
if ("path" in template) {
throw new Error("Fallback template should not have template path");
}
super(domain, template, forceRequiredFields);
super(domain, template, { forceRequiredFields });
}
get path() {
return undefined;
......
......@@ -2,6 +2,8 @@ import { Webpage } from "../webpage/webpage";
import { TemplateField } from "./templateField";
import * as nodeFetch from "node-fetch";
import { pages } from "../webpage/samplePages";
import log from "loglevel";
import { TemplateFieldDefinition } from "../types";
const mockNodeFetch = nodeFetch as typeof import("../../__mocks__/node-fetch");
......@@ -16,8 +18,9 @@ beforeEach(() => {
});
describe("Use default procedures", () => {
const loadDefaults = true;
test("itemType template field", () => {
const field = new TemplateField("itemType", true);
const field = new TemplateField("itemType", { loadDefaults });
return field.translate(target).then((output) => {
expect(output.output).toEqual(["webpage"]);
expect(output.valid).toBe(true);
......@@ -25,7 +28,7 @@ describe("Use default procedures", () => {
});
});
test("title template field", () => {
const field = new TemplateField("title", true);
const field = new TemplateField("title", { loadDefaults });
return field.translate(target).then((output) => {
expect(output.output).toEqual(["Sample article"]);
expect(output.valid).toBe(true);
......@@ -33,7 +36,7 @@ describe("Use default procedures", () => {
});
});
test("authorFirst template field", () => {
const field = new TemplateField("authorFirst", true);
const field = new TemplateField("authorFirst", { loadDefaults });
return field.translate(target).then((output) => {
expect(output.output).toEqual(["John", "Jane", ""]);
expect(output.valid).toBe(true);
......@@ -41,7 +44,7 @@ describe("Use default procedures", () => {
});
});
test("authorLast template field", () => {
const field = new TemplateField("authorLast", true);
const field = new TemplateField("authorLast", { loadDefaults });
return field.translate(target).then((output) => {
expect(output.output).toEqual([
"Smith",
......@@ -53,7 +56,7 @@ describe("Use default procedures", () => {
});
});
test("date template field", () => {
const field = new TemplateField("date", true);
const field = new TemplateField("date", { loadDefaults });
return field.translate(target).then((output) => {
expect(output.output).toEqual(["2022-02-04"]);
expect(output.valid).toBe(true);
......@@ -61,7 +64,7 @@ describe("Use default procedures", () => {
});
});
test("publishedIn template field", () => {
const field = new TemplateField("publishedIn", true);
const field = new TemplateField("publishedIn", { loadDefaults });
return field.translate(target).then((output) => {
expect(output.output).toEqual(["Journal title"]);
expect(output.valid).toBe(true);
......@@ -69,7 +72,7 @@ describe("Use default procedures", () => {
});
});
test("publishedby template field", () => {
const field = new TemplateField("publishedBy", true);
const field = new TemplateField("publishedBy", { loadDefaults });
return field.translate(target).then((output) => {