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

Merged
merged 3 commits into from
Jul 29, 2022

Conversation

jcsackett
Copy link
Contributor

No description provided.

@jcsackett jcsackett force-pushed the feat/complex-types-in-arrays branch from 52ed2aa to cde08f5 Compare July 28, 2022 21:34
@jcsackett jcsackett marked this pull request as ready for review July 28, 2022 21:37
@jcsackett jcsackett requested a review from a team as a code owner July 28, 2022 21:37
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.

Comment on lines 212 to 219
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.

@jcsackett jcsackett merged commit bfe7042 into main Jul 29, 2022
@jcsackett jcsackett deleted the feat/complex-types-in-arrays branch July 29, 2022 19:20
@snyksec
Copy link

snyksec commented Jul 29, 2022

🎉 This PR is included in version 1.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants