Fix #493 repeated item in JSON schema required #505
Conversation
src/refract/JSONSchemaVisitor.cc
Outdated
|
|
||
| for (const auto& member: members) { | ||
| assert(member); | ||
| if (!member) { |
There was a problem hiding this comment.
This conditional is never entered. Otherwise line 590 would assert.
There was a problem hiding this comment.
Agree, but I do not want to crash drafter with nullptr on production
There was a problem hiding this comment.
Drafter wouldn't necessarily crash. It would yield to undefined behaviour, which is even worse. Nevertheless, the assert makes the branch impossible to unit test, so either choose an assert, or a conditional.
There was a problem hiding this comment.
And if you use a conditional, that path is definitely untested without unit tests for JSONSchemaVisitor.
There was a problem hiding this comment.
In general, there can be nullptr.
Then I will remove assert
There was a problem hiding this comment.
There is problem with testing, it is private function. We need bigger refactoring for this.
There was a problem hiding this comment.
At first look, it seem it is independent function and it can be isolated out of class
There was a problem hiding this comment.
So it use members fixed, fixedType, pDefs.
Will try to fix it, but not sure if able finish before sprint end
src/refract/JSONSchemaVisitor.cc
Outdated
| @@ -637,5 +643,10 @@ namespace refract | |||
| throw LogicError("Invalid member type of object in MSON definition"); | |||
| } | |||
| } | |||
|
|
|||
| std::for_each(required.begin(), required.end(), | |||
There was a problem hiding this comment.
Use std::transform with std::inserter.
klokane
force-pushed
the
klokane/fix-493
branch
from
9bd9bce to
c446b59
Compare
Use internally
std::setto catch all "required" members and avoid repeat value