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

patternProperties: skip keys with undefined values #1176

Closed

Conversation

YellowKirby
Copy link

What issue does this pull request resolve?

Fixes #1173

When iterating over object keys that match patternProperties, ignore keys whose data values that are undefined. This makes it consistent with how regular properties are handled.

What changes did you make?

In properties.jst, when iterating over properties in an object, check both that the property key matches the given RegExp (this check is not new) and that the data value is not undefined (this part is new).

Is there anything that requires more attention while reviewing?

First time making changes to this codebase. Not sure if there's anything I missed.

When iterating over object keys that match patternProperties, ignore
keys whose data values that are `undefined`. This makes it consistent
with how regular properties are handled.
@epoberezkin
Copy link
Member

@YellowKirby thanks for the PR and sorry for the delay.

I am a bit reluctant to fix it without major version change and even more reluctant doing it piece by piece.

There are quite a few cases where undefined properties would be used, depending on ownProperties option - check out other places where iterateProperties is used and also noPropertyInData - so probably easier to fix there. Both are defined in definitions.def. Also there is required keyword - not sure what is the current behaviour, it would depend on some other options...

To do it comprehensively, in the current major version, some option (e.g. validateUndefined) should be added:

  • without option: keep current behaviour for all keywords, accounting for ownProperties option.
  • false: ignore undefined properties for all keywords
  • true: take undefined properties into account for all keywords

It will result in a much bigger test case.

@YellowKirby
Copy link
Author

Thank you for the feedback! I think adding a new option makes a lot of sense. I can work on adding that as well as checking more edge cases/combinations with other options when I get some free time.

@epoberezkin
Copy link
Member

closing it, the code moved to TS

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

Successfully merging this pull request may close these issues.

schema for objects: patternProperties and properties treat undefined values differently
2 participants