Support top level Reference Element when translating API Elements to JSON & JSON Schema #710
Conversation
| so::Object& renderSchema(so::Object& schema, const IElement& e, TypeAttributes options); | ||
|
|
||
| template <typename T> | ||
| void renderProperty(ObjectSchema&, const T& e, so::Object&, TypeAttributes) | ||
| { | ||
| LOG(error) << "invalid property element: " << e.element(); | ||
| assert(false); | ||
| abort(); |
There was a problem hiding this comment.
Can you explain the rationale behind changing assert to abort please, not sure I understand the reason
There was a problem hiding this comment.
Here we want to avoid continuing the program flow because we should never arrive there. Specifically, bug #709 caused a SIGSEGV by overflowing the stack through unchecked recursion - with an abort here, there would be the much nicer SIGABORT.
There was a problem hiding this comment.
I see, perhaps it would be better to avoid exiting the process though? Perhaps throwing std::exception would be better, the drafter_parse/validate function could catch and return appropriate error to caller?
| @@ -284,6 +285,12 @@ namespace | |||
| return renderValue(*merged, options); | |||
| } | |||
|
|
|||
| so::Value renderValueSpecific(const RefElement& element, TypeAttributes options) | |||
There was a problem hiding this comment.
I wonder, could there be other cases where we would render a ref element outside an enum here? Trying to understand if there is perhaps some other test-coverage we should add for other types.
There was a problem hiding this comment.
Perhaps we should explore that. Also, minim has that generateValue function that probably manifests the same bug.
Fixes #709 .