-
Notifications
You must be signed in to change notification settings - Fork 18
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 JSON decoding error when empty optional is returned. #16
Fix JSON decoding error when empty optional is returned. #16
Conversation
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.
Thanks for the PR! I think that we may want to make this change in conjure-python-client instead though.
I'd like to keep the generator as simple as possible, and this would further leak details of the requests implementation into to the generic code gen
@ferozco I thought about this. This would mean moving the Conjure decoding into the client or having the client at least have some knowledge of the return type (assuming you wouldn't want to return |
@tom-s-powell I've just enabled CircleCI's build forks setting, so you should get builds from now on! |
I think you're right @tom-s-powell, for now this is probably the best way forward |
I'd really like to get the proper test cases implemented before we merge this... I think we'll need to build some dedicated endpoints on the verification-server that will return 204s for map, list and set returns (and also alias of map, list and set). Tracking here: palantir/conjure-verification#38 |
@iamdanfox is there a timeline on this change in conjure-verification? |
@tom-s-powell, there is a pr for this, palantir/conjure-verification#44 |
@@ -88,6 +88,9 @@ public PythonClass generateClient( | |||
} | |||
return false; | |||
}).orElse(false)) | |||
.isOptionalReturnType(ed.getReturns() | |||
.map(rt -> rt.accept(TypeVisitor.IS_OPTIONAL)) |
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 doesn't work on the test-case for this in conjure-verification 0.10.0 because it is an alias to an optional type. This type visitor is used in a handful of places to determine whether or not a value is optional. Should the visitor also return true if the type is an alias to an optional? Should there be a separate visitor which handles the alias case?
{
"type" : "alias",
"alias" : {
"typeName" : {
"name" : "RawOptionalExample",
"package" : "com.palantir.conjure.verification"
},
"alias" : {
"type" : "optional",
"optional" : {
"itemType" : {
"type" : "primitive",
"primitive" : "INTEGER"
}
}
}
}
}
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.
@ferozco @iamdanfox I'm not actually sure what the right thing to do is
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.
We encountered something similar in conjure-core and made: https://github.com/palantir/conjure/blob/develop/conjure-core/src/main/java/com/palantir/conjure/defs/DealiasingTypeVisitor.java . Perhaps we can factor that out into the generator commons and you can then use it here
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.
👍 would really like to see that in generator-commons
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.
4.2.0 is out with the new visitor
|
It wasn't that the verification server doesn't accept $ curl -s http://localhost:8000/confirm/receiveRawOptionalExample/0 -XPOST -H 'content-type: application/json'
{"errorCode":"INVALID_ARGUMENT","errorName":"Conjureverification:InvalidRequestBody","errorInstanceId":"ec3a4ec0-4bbd-42da-8058-14357fb1014a","parameters":{}}% |
Chatting in person, I've filed #18 to make sure we finish off the tests here, without blocking these guys too much! |
Let me know if you'd prefer some kind of helper to be pulled out.