filter anyOf and oneOf alternative#180
filter anyOf and oneOf alternative#180animeshsahoo1 wants to merge 2 commits intohyperjump-io:mainfrom
Conversation
|
hi @jdesrosiers I have implemented what we discussed about, I tried to not add anything unnecessary but if there is need of changes let me know, I made sure to run the linting and tests as well. |
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
Please add tests. They need to cover all the edge cases we discussed and more if you can think of them.
|
I did run it on tests we talk about but I was unsure if I had to add the code for that, I will add it soon. |
|
I have added the test case for matchcount=0 for oneOf and anyOf test cases that were discussed, I tried to think of a failing case but didn't really find any let me know if I could add any other test case |
|
@jdesrosiers tagging you in case you didn't get notification since I didn't tag last time |
|
You don't need to tag me. I'll get the notification. In fact, bumping the issue will likely make it take longer before I get to it. I generally work on the most stale issue on my plate first. So, if you bump the issue, it effectively moves your issue to the back of the queue. |
jdesrosiers
left a comment
jdesrosiers
left a comment
There was a problem hiding this comment.
Please try to match the code style used in the rest of the project. Please turn off Prettier or at least give it a much more reasonable line length limit.
I think this works! But, getting it to work is just the first step. This needs some refactoring.
Every time you do a filter operation, you're looping over everything again and creating a new array. Creating a bunch of arrays and objects gets expensive at scale. See if you can refactor so that you can loop over the alternatives only one time.
Also try to avoid things like [...instanceProps]. Again, we're unnecessarily creating an array. Have a look at @hyperjump/pact. It allows you to use filter, map and bunch of other things that work on any iterator (not just arrays) and don't create unnecessary intermediate representations. It looks like it could come in handy in a few places.
| /** @type (alternative: NormalizedOutput, propLocation: string) => boolean */ | ||
| const propertyPasses = (alternative, propLocation) => { | ||
| const propOutput = alternative[propLocation]; | ||
| if (!propOutput || Object.keys(propOutput).length === 0) return false; | ||
| return Object.values(propOutput).every((keywordResults) => | ||
| Object.values(keywordResults).every((v) => v === true) | ||
| ); | ||
| }; | ||
|
|
There was a problem hiding this comment.
I prefer to put helper functions at the bottom.
|
|
||
| /** @type (alternative: NormalizedOutput, propLocation: string) => boolean */ | ||
| const propertyPasses = (alternative, propLocation) => { | ||
| const propOutput = alternative[propLocation]; |
There was a problem hiding this comment.
alernatative and propLocation aren't used individually. Should we just pass propOutput directly instead?
| filtered = filtered.filter((alternative) => { | ||
| const declaredProps = Object.keys(alternative) | ||
| .filter((loc) => loc.startsWith(prefix)) | ||
| .map((loc) => loc.slice(prefix.length)); |
There was a problem hiding this comment.
This isn't going to to always work. Consider the case where the property is an object. You could get declared properties that look like a/b/c.
Also, consider the case where the pointer is escaped like ~0a. You'll need to unescape it to get the actual property name.
You can solve both problems with @hyperjump/pact and @hyperjump/json-pointer to get the unescaped first segment.
const pointer = location.slice(prefix.length - 1);
return Pact.head(JsonPointer.pointerSegments(pointer));| } else { | ||
| filtered = filtered.filter((alternative) => { | ||
| const typeResults | ||
| = alternative[instanceLocation]?.[ |
| const match = !typeErrors || Object.values(typeErrors).every((isValid) => isValid); | ||
| filtered = filtered.filter((alternative) => | ||
| [...instanceProps].some((prop) => | ||
| propertyPasses(alternative, `${instanceLocation}/${prop}`) |
There was a problem hiding this comment.
You can't construct pointers using simple string concatenation because some characters need to be escaped. Use JsonPointer.append from @hyperjump/json-pointer.
| if (filtered.length === 0) { | ||
| filtered = anyOf; | ||
| } |
There was a problem hiding this comment.
This is duplicated in both branches. It can be moved out.
| filtered = filtered.filter((alternative) => { | ||
| const typeResults | ||
| = alternative[instanceLocation]?.[ | ||
| "https://json-schema.org/keyword/type" | ||
| ]; | ||
| return ( | ||
| !typeResults || Object.values(typeResults).every((isValid) => isValid) | ||
| ); | ||
| }); | ||
|
|
||
| if (alternatives.length === 0) { | ||
| for (const alternative of anyOf) { | ||
| alternatives.push(await getErrors(alternative, instance, localization)); | ||
| if (filtered.length === 0) { | ||
| filtered = anyOf; | ||
| } |
There was a problem hiding this comment.
Is it right for this to be in an else? It seems to me that we should be doing this check always and doing it first. It's the most generic check and the best chance to filter out potential cases early.
2 participants
The anyof and oneof error handlers now filter alternatives based on the instance's properties. For object instances, an alternative is excluded from the error report if none of its declared properties exist on the instance (Rule 1), or if none of the instance's properties pass validation in that alternative (Rule 2). For non-object instances, the original type-keyword filtering is done.
fixes #175