Skip to content
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

Adding error objects to request responses #61

Merged
merged 9 commits into from
Dec 11, 2015

Conversation

DEGoodmanWilson
Copy link
Contributor

Sometimes CURL just gives up, and it would be nice to have some understanding of why. It would also be nice if we could do it in a non-CURL-centric way. This PR introduces a new Error object to encapsulate specifically internal and HTTP errors that can occur when performing a request. It also introduces a new enum ErrorCode to provide a machine-readable way of assessing errors that is also independent of the CURL codes.

Not all CURL errors map to ErrorCodes. This is because many of the errors are for protocols beyond the current scope of cpr (e.g. FTP). Many are due to errors on the server which are already suitably handled by the HTTP status_code feature. And a lot of them are CURL specific, of course, and so get rolled up into the generic INTERNAL_ERROR. (Also, many of the SSL errors are very specific, and I wasn't sure if the way CURL granulated them was both universally applicable and user friendly, so I just lumped them together under a small handful of SSL errors). The CURL-generated human readable text is placed in the ERROR object in case the end developer requires more context.

Test coverage is not 100%, because in many cases I am not certain how to induce certain of the CURL error conditions.

I've tried to stick to the house style, please let me know if there are any formatting mistakes as well! I look forward to reading your comments.

Don Goodman-Wilson added 7 commits November 6, 2015 15:40
@nabijaczleweli
Copy link
Contributor

Looks like here you're referring to CURLE_HTTP2 which doesn't exist, causing the build to fail. (Should it be CURL_HTTP2?)

@DEGoodmanWilson
Copy link
Contributor Author

Ah, I was using the system CURL, for which that value does exist. I suspect it can be safely elided, which I will do.

@DEGoodmanWilson
Copy link
Contributor Author

Hrm. Currently the tests are failing because the test for SSL_CONNECTION_FAILURE is triggering UNSUPPORTED_PROTOCOL instead. Is there an obvious compile-time check I can do to see if SSL is enabled, preferably without querying any defined CURL macros?

@whoshuu
Copy link
Contributor

whoshuu commented Nov 9, 2015

When you say compile-time, do you mean within CMake? I think you can check SSL_ENABLED from here. Pending #31, it might even make sense to set CMAKE_USE_OPENSSL (found here) to OFF.

… is dependent upon features selected at compile time…but then, there really isn't much that can be done about it.
@DEGoodmanWilson
Copy link
Contributor Author

Oh, I just meant conditional compilation by examining some macro definition. When I build using system curl on OSX, I get SSL for free. (And building using the included CURL fails, but that's a different issue). But when I build on Windows with the included CURL, there is no SSL (as you well know). So it doesn't make sense to test SSL functionality where it isn't available (but it seems like it should when it is).

Not my favorite thing to do ever (because it requires us to talk to an implementation detail that oughtn't be exposed), but I'm going to just do runtime check against the feature flags in curl_version_info().

@whoshuu
Copy link
Contributor

whoshuu commented Nov 15, 2015

Just did a thorough read over your PR here. This is a great start! I'm going to build out some support inside the embedded test server to surface the other errors you're having trouble reproducing. Once I get a good hold of that, and get as close to 100% coverage again as possible, I'll merge this in and throw my changes on top of it.

@DEGoodmanWilson
Copy link
Contributor Author

Great! Let me know if there is anything I can do from my end to ease that process.

@DEGoodmanWilson
Copy link
Contributor Author

Oh hello, just wanted to check in and see if there was anything I could do to pitch in on this?

@whoshuu
Copy link
Contributor

whoshuu commented Dec 10, 2015

Hey @DEGoodmanWilson, just been busy with work! I'm gonna cut 1.2.0 and just get it out there, then get this into the master branch and we'll take it from there. Expect this within the next few days. Thanks for your patience!

@whoshuu whoshuu added this to the 1.3.0 milestone Dec 10, 2015
whoshuu added a commit that referenced this pull request Dec 11, 2015
Adding error objects to request responses
@whoshuu whoshuu merged commit bdb877c into libcpr:master Dec 11, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants