-
Notifications
You must be signed in to change notification settings - Fork 54
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
fix required attribute processing for json schema of oneOf #453
Conversation
@w-vi I am adding you as a reviewer too. |
src/refract/JSONSchemaVisitor.cc
Outdated
JSONSchemaVisitor renderer(pDefs, fixed); | ||
Visit(renderer, *(*it)); | ||
ObjectElement *o1 = TypeQueryVisitor::as<ObjectElement>(renderer.get()); | ||
if (!o1->value.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line before this.
src/refract/JSONSchemaVisitor.cc
Outdated
ObjectElement *o1 = TypeQueryVisitor::as<ObjectElement>(renderer.get()); | ||
if (!o1->value.empty()) { | ||
MemberElement *m1 = TypeQueryVisitor::as<MemberElement>(o1->value[0]->clone()); | ||
if (m1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line before this.
src/refract/JSONSchemaVisitor.cc
Outdated
m1->renderType(IElement::rCompact); | ||
o->push_back(m1); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unnecessary empty line.
src/refract/JSONSchemaVisitor.cc
Outdated
switch (type.get()) { | ||
case TypeQueryVisitor::Member: { | ||
MemberElement *mr = static_cast<MemberElement*>(*it); | ||
if (IsTypeAttribute(*(*it), "required") || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line before this.
src/refract/JSONSchemaVisitor.cc
Outdated
((fixed || fixedType) && !IsTypeAttribute(*(*it), "optional"))) { | ||
|
||
StringElement *str = TypeQueryVisitor::as<StringElement>(mr->value.first); | ||
if (str) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line before this.
src/refract/JSONSchemaVisitor.cc
Outdated
|
||
pObj = new ObjectElement; | ||
pObj->renderType(IElement::rCompact); | ||
|
||
addMember("properties", props); | ||
if (!reqVals.empty()) { | ||
addMember("required", new ArrayElement(reqVals, IElement::rCompact)); | ||
} | ||
if (!oneOfMembers.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line before this.
src/refract/JSONSchemaVisitor.cc
Outdated
|
||
pObj = new ObjectElement; | ||
pObj->renderType(IElement::rCompact); | ||
|
||
addMember("properties", props); | ||
if (!reqVals.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add an empty line before this.
} | ||
} | ||
else { | ||
Visit(*this, *(*it)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic is missing in the refactor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, okay. Add another test case as I requested and I can merge this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
- Properties | ||
- foo: A (string, required) | ||
|
||
- Properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try a test case with removing only the above line. Properties
is not always needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I add one more test with such data
# GET /ab
- response 200 (application/json)
- Attributes (Test)
# Data Structures
## Test (object)
- One Of
- Properties
- foo: A (string, required)
- bar: B (string)
or change this one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add one more please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just 1 minor change.
test/fixtures/oneof/required2.apib
Outdated
- Properties | ||
- foo: A (string, required) | ||
|
||
- bar: B (string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add required here.
@goganchic Can you force push and trigger the CI again please? Thanks. |
done |
Should've been squashed but otherwise it's all good. Cheers @goganchic |
Is it possible to include this fix in the next release? |
Will do. |
Thanks a lot, Pavan! Спасибо! |
This PR solves problem described in #452, but I think a bit of refactoring is required. It's possible to make it in separate PR or in this one.
Functions
void JSONSchemaVisitor::operator()(const OptionElement& e)
andvoid JSONSchemaVisitor::operator()(const ObjectElement& e)
have a lot of common code (MemberElement & SelectElement processing, required attributes calculation) and I think this code a bit dirty: it's difficult to understand Select -> Option -> Select chain processing. This refactoring may take some time and it's difficult to estimate how much time it will be.// UPDATE: refactoring complete