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.

Sign up for GitHub

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: allows arrays with oneOf/anyOf/allOf definitions to pass lint #329

Merged
merged 3 commits into from
Jul 29, 2022
Merged

feat: allows arrays with oneOf/anyOf/allOf definitions to pass lint #329

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cde08f5
feat: allows arrays with oneOf/anyOf/allOf definitions to pass lint
jcsackett Jul 28, 2022
625b938
fix: updates type in tests to pass tsc build
jcsackett Jul 29, 2022
96675cf
feat: moves items validation into helper function
jcsackett Jul 29, 2022
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
201 changes: 201 additions & 0 deletions src/rulesets/rest/2022-05-25/__tests__/property-rules.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -867,6 +867,207 @@ describe("body properties", () => {
});
});

describe("array properties", () => {
test("fails if items have no type information", () => {
const ruleRunner = new RuleRunner([propertyRules]);
const afterSpec: OpenAPIV3.Document = {
...baseOpenAPI,
paths: {
"/example": {
get: {
responses: {
"200": {
description: "",
content: {
"application/json": {
schema: {
type: "object",
properties: {
things: {
type: "array",
items: {},
},
},
},
},
},
},
},
},
},
},
};
const ruleInputs = {
...TestHelpers.createRuleInputs(afterSpec, afterSpec),
context,
};
const results = ruleRunner.runRulesWithFacts(ruleInputs);
expect(results.every((result) => result.passed)).toBe(false);
expect(results.filter((result) => !result.passed)).toEqual(
expect.arrayContaining([
expect.objectContaining({
error: "type was not found array items",
}),
]),
);
});
test("succeeds if items have type", () => {
const ruleRunner = new RuleRunner([propertyRules]);
const afterSpec: OpenAPIV3.Document = {
...baseOpenAPI,
paths: {
"/example": {
get: {
responses: {
"200": {
description: "",
content: {
"application/json": {
schema: {
type: "object",
properties: {
things: {
type: "array",
items: {
type: string,
},
},
},
},
},
},
},
},
},
},
},
};
const ruleInputs = {
...TestHelpers.createRuleInputs(afterSpec, afterSpec),
context,
};
const results = ruleRunner.runRulesWithFacts(ruleInputs);
expect(results.every((result) => result.passed)).toBe(true);
});

test("succeeds if items have oneOf schema", () => {
const ruleRunner = new RuleRunner([propertyRules]);
const afterSpec: OpenAPIV3.Document = {
...baseOpenAPI,
paths: {
"/example": {
get: {
responses: {
"200": {
description: "",
content: {
"application/json": {
schema: {
type: "object",
properties: {
things: {
type: "array",
items: {
oneOf: [],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we pass if it's an empty composite type though? I think we should require at least one concrete type in here.

I suppose the worst pathological case would be nested composite types with nothing at the bottom -- so to be really comprehensive we should do a recursive descent to make sure all "leaf-node" type-paths are defined (not empty composites). This would be the most formally-correct solution.

For example, this type contains a whole lot of choices, all leading to "no type defined":

items:
  oneOf:
    - oneOf: []
    - {}
    - anyOf:
        - allOf: []
        - {}

A helper function isCompletelyDefinedType could probably tackle this.

A band-aid quick-fix solution might be to check that "all composites must contain one or more simple non-empty type" and deal with the formal solution in a followup. AFAIK no one's trying to nest composites at the moment (but it may happen unintentionally later!)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was under the impression that there were other checks for composite types having concrete types--if not, wouldn't it be better to have those checks across all schemas rather than making that part of this check?

Copy link
Contributor

Choose a reason for hiding this comment

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

There might be... I'll have to look (it's been a little while)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Did a quick check and there aren't; I'm going to file an issue for validating that composites are completely typed, and go with a band-aid helper in this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#330

},
},
},
},
},
},
},
},
},
},
},
};
const ruleInputs = {
...TestHelpers.createRuleInputs(afterSpec, afterSpec),
context,
};
const results = ruleRunner.runRulesWithFacts(ruleInputs);
expect(results.every((result) => result.passed)).toBe(true);
});

test("succeeds if items have allOf schema", () => {
const ruleRunner = new RuleRunner([propertyRules]);
const afterSpec: OpenAPIV3.Document = {
...baseOpenAPI,
paths: {
"/example": {
get: {
responses: {
"200": {
description: "",
content: {
"application/json": {
schema: {
type: "object",
properties: {
things: {
type: "array",
items: {
allOf: [],
},
},
},
},
},
},
},
},
},
},
},
};
const ruleInputs = {
...TestHelpers.createRuleInputs(afterSpec, afterSpec),
context,
};
const results = ruleRunner.runRulesWithFacts(ruleInputs);
expect(results.every((result) => result.passed)).toBe(true);
});

test("succeeds if items have anyOf schema", () => {
const ruleRunner = new RuleRunner([propertyRules]);
const afterSpec: OpenAPIV3.Document = {
...baseOpenAPI,
paths: {
"/example": {
get: {
responses: {
"200": {
description: "",
content: {
"application/json": {
schema: {
type: "object",
properties: {
things: {
type: "array",
items: {
anyOf: [],
},
},
},
},
},
},
},
},
},
},
},
};
const ruleInputs = {
...TestHelpers.createRuleInputs(afterSpec, afterSpec),
context,
};
const results = ruleRunner.runRulesWithFacts(ruleInputs);
expect(results.every((result) => result.passed)).toBe(true);
});
});

describe("breaking changes", () => {
test("fails if a property is removed", () => {
const ruleRunner = new RuleRunner([propertyRules]);
Expand Down
22 changes: 16 additions & 6 deletions src/rulesets/rest/2022-05-25/property-rules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,9 +209,14 @@ const arrayWithItemsInRequest = new RequestRule({
(property) => {
if (property.raw.type === "array") {
if (!property.raw.items || !("type" in property.raw.items)) {
throw new RuleError({
message: "type was not found array items",
});
const oneOf = property.raw.items["oneOf"];
const allOf = property.raw.items["allOf"];
const anyOf = property.raw.items["anyOf"];
if (!oneOf && !allOf && !anyOf) {
throw new RuleError({
message: "type was not found array items",
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Recommend turning this into a helper, for incremental improvement as described above. Asserting a non-empty type seems like a generally useful thing we may want for property types as well as array item types.

}
}
},
Expand All @@ -227,9 +232,14 @@ const arrayWithItemsInResponse = new ResponseBodyRule({
(property) => {
if (property.raw.type === "array") {
if (!property.raw.items || !("type" in property.raw.items)) {
throw new RuleError({
message: "type was not found array items",
});
const oneOf = property.raw.items["oneOf"];
const allOf = property.raw.items["allOf"];
const anyOf = property.raw.items["anyOf"];
if (!oneOf && !allOf && !anyOf) {
throw new RuleError({
message: "type was not found array items",
});
}
}
}
},
Expand Down