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

JENA-1321: Exception rewrapping in HttpQuery masks error response from the server #241

Closed
wants to merge 2 commits into from

Conversation

ajs6f
Copy link
Member

@ajs6f ajs6f commented Apr 17, 2017

It's a bit odd-feeling to couple HttpException so tightly, but given that it is an Atlas type, I don't feel terrible about it.

@ajs6f
Copy link
Member Author

ajs6f commented Apr 17, 2017

@rmorrise, will this suit you?

@rmorrise
Copy link

Code looks good!

I am new to git, but if I understood correctly, you took my change plus adding the responseCode, but did not take my changes to TestService. I think this will fail the unit tests? Please correct me if I am reading the diff wrong.

Do I need to do anything for the changes to be applied to the main repo?

@ajs6f
Copy link
Member Author

ajs6f commented Apr 17, 2017

No, actually, I did not start with your code-- if you look at what I did, I added fields and accessors and a constructor to QueryExceptionHTTP instead as we discussed. You need do nothing further. I will merge this in and it will appear in our next release. (In the meantime, you can just git pull the changes from the main repo (once I merge them).)

@ajs6f
Copy link
Member Author

ajs6f commented Apr 17, 2017

The tests pass fine, because as @afs prescribed, we aren't changing the wrapping of exceptions, just adding useful information to "outer" ones.

@rmorrise
Copy link

My mistake! I didn't notice the call to httpEx.getCause() in the constructor.

public String getResponse() { return response ; }

/** The status line for the response for this exception
* @return status line
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"The status line for the response if available from HTTP"

* @return message
*/
public String getResponseMessage() { return responseMessage ; }

/** The response for this exception
* @return response
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this can be null if the original exception was not from the HTTP protocol, but from e.g. an IOException why doing an HTTP operation.

"The response for this exception if available from HTTP"

@ajs6f ajs6f closed this Apr 24, 2017
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