Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: type aggregation validation #164

Merged
merged 5 commits into from
Jun 11, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/language-server/src/xml-view-diagnostics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ function validationIssuesToLspDiagnostics(
case "UnknownAttributeKey":
case "UnknownTagName":
case "InvalidAggregationCardinality":
case "InvalidAggregationType":
return {
...commonDiagnosticPros,
};
Expand Down
8 changes: 8 additions & 0 deletions packages/logic-utils/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,6 +75,14 @@ export function findClassesMatchingType({
model: UI5SemanticModel;
}): UI5Class[];

/**
* Check if a UI5 Class is type of the give `type`.
*/
export function classIsOfType(
clazz: UI5Class,
type: UI5Class | UI5Interface
): boolean;

/**
* Check if a UI5 node is a root symbol. A root symbol is a symbol that exists in one of the model symbol maps.
*/
Expand Down
5 changes: 4 additions & 1 deletion packages/logic-utils/src/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,10 @@ export {
flattenProperties,
flattenAssociations,
} from "./utils/flatten-members";
export { findClassesMatchingType } from "./utils/find-classes-matching-type";
export {
findClassesMatchingType,
classIsOfType,
} from "./utils/find-classes-matching-type";
export { isRootSymbol, getRootSymbolParent } from "./utils/root-symbols";
export { typeToString } from "./utils/type-to-string";
export {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export function findClassesMatchingType({
return values(matchingClasses);
}

function classIsOfType(
export function classIsOfType(
clazz: UI5Class,
type: UI5Class | UI5Interface
): boolean {
Expand Down
31 changes: 30 additions & 1 deletion packages/logic-utils/test/utils/find-classes-matching-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,11 @@ import { map, forEach } from "lodash";
import { expect } from "chai";
import { UI5SemanticModel } from "@ui5-language-assistant/semantic-model-types";
import { generateModel } from "@ui5-language-assistant/test-utils";
import { findClassesMatchingType, ui5NodeToFQN } from "../../src/api";
import {
findClassesMatchingType,
ui5NodeToFQN,
classIsOfType,
} from "../../src/api";

describe("The @ui5-language-assistant/logic-utils <findClassesMatchingType> function", () => {
let ui5Model: UI5SemanticModel;
Expand Down Expand Up @@ -62,3 +66,28 @@ describe("The @ui5-language-assistant/logic-utils <findClassesMatchingType> func
]);
});
});

describe("The @ui5-language-assistant/logic-utils <classIsOfType> function", () => {
let ui5Model: UI5SemanticModel;
before(async () => {
ui5Model = await generateModel({ version: "1.74.0" });
});

it("can tell if the class matching a UI5Interface type", () => {
bd82 marked this conversation as resolved.
Show resolved Hide resolved
const targetInterface = ui5Model.interfaces["sap.m.IconTab"];
const ui5Class = ui5Model.classes["sap.m.IconTabFilter"];
expect(classIsOfType(ui5Class, targetInterface)).to.be.true;
});

it("can tell if the class matching a UI5Class type", () => {
const targetClassType = ui5Model.classes["sap.m.ListBase"];
const ui5Class = ui5Model.classes["sap.m.Table"];
expect(classIsOfType(ui5Class, targetClassType)).to.be.true;
});

it("negative case - can tell if the class matching a UI5Interface type", () => {
const targetInterface = ui5Model.interfaces["sap.m.IBar"];
const ui5Class = ui5Model.classes["sap.m.IconTabFilter"];
expect(classIsOfType(ui5Class, targetInterface)).to.be.false;
});
});
7 changes: 6 additions & 1 deletion packages/xml-views-validation/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,8 @@ export type UI5XMLViewIssue =
| NonUniqueIDIssue
| UnknownAttributeKeyIssue
| UnknownTagNameIssue
| InvalidAggregationCardinalityIssue;
| InvalidAggregationCardinalityIssue
| InvalidAggregationTypeIssue;

// A sub-interface per issue type may seem redundant, but this allows
// a sub-issue type to have additional properties (if needed) in the future.
Expand All @@ -45,6 +46,10 @@ export interface InvalidAggregationCardinalityIssue
kind: "InvalidAggregationCardinality";
}

export interface InvalidAggregationTypeIssue extends BaseUI5XMLViewIssue {
kind: "InvalidAggregationType";
}

export interface UnknownNamespaceInXmlnsAttributeValueIssue
extends BaseUI5XMLViewIssue {
kind: "UnknownNamespaceInXmlnsAttributeValue";
Expand Down
1 change: 1 addition & 0 deletions packages/xml-views-validation/src/utils/messages.ts
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ export const UNKNOWN_TAG_NAME_IN_NS = `The "{0}" name is neither a class name in
export const UNKNOWN_TAG_NAME_NO_NS = `The "{0}" name is neither a class name nor an aggregation of its parent tag, please specify a namespace`;

export const INVALID_AGGREGATION_CARDINALITY = `The aggregation "{0}" has cardinality of 0..1 and may only contain one element`;
export const INVALID_AGGREGATION_TYPE = `The class "{0}" is under the aggregation "{1}" and must match the type "{2}"`;
export const NON_UNIQUE_ID = `Duplicate ID: "{0}" found.`;

export function getMessage(template: string, ...params: string[]): string {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,123 @@
import { XMLElement } from "@xml-tools/ast";
import {
UI5SemanticModel,
UI5Interface,
UI5Class,
UI5Aggregation,
} from "@ui5-language-assistant/semantic-model-types";
import {
getUI5ClassByXMLElement,
getUI5AggregationByXMLElement,
classIsOfType,
} from "@ui5-language-assistant/logic-utils";
import { InvalidAggregationTypeIssue } from "../../../api";
import { INVALID_AGGREGATION_TYPE, getMessage } from "../../utils/messages";

export function validateAggregationType(
xmlElement: XMLElement,
model: UI5SemanticModel
): InvalidAggregationTypeIssue[] {
// Root element can never be inside aggregation
if (xmlElement.parent.type === "XMLDocument") {
bd82 marked this conversation as resolved.
Show resolved Hide resolved
return [];
}

const ui5class = getUI5ClassByXMLElement(xmlElement, model);
const parentAggregation = getAggregationByXMLElement(
xmlElement.parent,
model
);

if (ui5class === undefined || parentAggregation === undefined) {
return [];
}

const allowedTypeInAggregation = getValidAggregationTypeForSubNodes(
parentAggregation
);

if (allowedTypeInAggregation === undefined) {
return [];
}

const invalidAggregationTypeIssue = getInvalidAggregationTypeIssue({
xmlElement,
ui5Class: ui5class,
aggregationName: parentAggregation.name,
aggregationType: allowedTypeInAggregation,
});

return invalidAggregationTypeIssue;
}

function getInvalidAggregationTypeIssue({
xmlElement,
ui5Class,
aggregationName,
aggregationType,
}: {
xmlElement: XMLElement;
ui5Class: UI5Class;
aggregationName: string;
aggregationType: UI5Class | UI5Interface;
}): InvalidAggregationTypeIssue[] {
const isTypeOf = classIsOfType(ui5Class, aggregationType);
if (!isTypeOf && xmlElement.syntax.openName !== undefined) {
const invalidAggregationTypeIssue: InvalidAggregationTypeIssue = {
kind: "InvalidAggregationType",
message: getMessage(
INVALID_AGGREGATION_TYPE,
ui5Class.name,
aggregationName,
aggregationType.name
),
severity: "error",
offsetRange: {
start: xmlElement.syntax.openName.startOffset,
end: xmlElement.syntax.openName.endOffset,
},
};
return [invalidAggregationTypeIssue];
}

return [];
}

function getValidAggregationTypeForSubNodes(
aggregation: UI5Aggregation
): UI5Class | UI5Interface | undefined {
const aggregationType = aggregation.type;
if (aggregationType === undefined) {
return undefined;
}

switch (aggregationType.kind) {
case "UI5Class":
return aggregationType as UI5Class;
case "UI5Interface":
return aggregationType as UI5Interface;
/* istanbul ignore next - we can only recieve aggregation type of 'UI5Class' or 'UI5Interface' */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its probably true that the model does not contain any aggregation with an invalid type.
But that does not mean we could never receive one, just that we are unlikely to.

So perhaps we should create a test for this (with deepClone of the model).

But maybe the best solution would be to narrow down the type signature of UI5Aggregation?
@tal-sapan WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If its not clear I meant that the UI5Aggregation.type would be narrowed down and be more specific UI5Class | UI5Interface and that the semanticModelBuilder would create a warn/error if that assumption is not met.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No. The type might be unresolved (e.g. if there is a typo) or there might be other errors in the api.json (and cases which I'm not even sure are errors, like primitive types, though I think they would be specified in altTypes instead). It should be handled in the code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Representing unresolved in the model seems fine.
  • But assuming an aggregation may not be of enum/primitive types, why should it even be represented in the model?

The model should not represent the API.json verbatim, rather it should represent the UI5 Semantics.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will investigate this outside the scope of this PR

default:
return undefined;
}
}

function getAggregationByXMLElement(
xmlElement: XMLElement,
model: UI5SemanticModel
): UI5Aggregation | undefined {
const aggregation = getUI5AggregationByXMLElement(xmlElement, model);

// Case of explicit aggregation
if (aggregation !== undefined) {
return aggregation;
}

// Case of possible default aggregation
const ui5class = getUI5ClassByXMLElement(xmlElement, model);
if (ui5class !== undefined && ui5class.defaultAggregation !== undefined) {
return ui5class.defaultAggregation;
}

return undefined;
}
2 changes: 2 additions & 0 deletions packages/xml-views-validation/src/validators/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,15 @@ import { validateNonUniqueID } from "./document/non-unique-id";
import { validateUnknownAttributeKey } from "./attributes/unknown-attribute-key";
import { validateUnknownTagName } from "./elements/unknown-tag-name";
import { validateExplicitAggregationCardinality } from "./elements/cardinality-of-aggregation";
import { validateAggregationType } from "./elements/type-of-aggregation";

export const allValidators: UI5Validators = {
document: [validateNonUniqueID],
element: [
validateUseOfDeprecatedClass,
validateUnknownTagName,
validateExplicitAggregationCardinality,
validateAggregationType,
],
attribute: [
validateUnknownEnumValue,
Expand Down
Loading