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

Added VndErrorException #399

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jmnarloch
Copy link

Hi,

Some time ago I had created this little thingy for handling the VndErrors in Feign calls over SpringCloud: https://github.com/jmnarloch/feign-vnderror-spring-cloud-starter. I though that I would be better if there would be a unified exception that could be handled by multiple clients: Feign, RestTemplate, HttpClient, OkHttp etc. So I would like to propose adding one.

@jmnarloch jmnarloch force-pushed the VndErrorException branch 2 times, most recently from 0b196ce to e3baeca Compare October 17, 2015 10:05
@jmnarloch
Copy link
Author

cc @olivergierke

@odrotbohm
Copy link
Member

What would that exception be used like? Would you mind adding test cases for the code you added? I guess we need to give the VndErrors implementation a bit of a rewrite anyway as it seems like the spec has moved significantly in the meantime.

@jmnarloch
Copy link
Author

So the whole idea is that this introduces a specialized version of HttpStatusCodeException that brings the payload information in a form of VndErrors, I am not expecting anything more that the Spring HATEOAS would just define it. This opens possibilities to try to unify the errors handling using different clients API like already mentioned: RestTemplate, AsyncRestTemplate and for instance Feign clients, but the integration would have be handled elsewhere. This would bring at least the common error representation.

So to clarify I can imagine to be able to process the errors as fallows:

try {

    feignClient.getInvoices();    
} catch (VndErrorException exc) {
    ...
}

with similar case with for instance RestTemplate:

try {

    restOperations.postForObject(...);    
} catch (VndErrorException exc) {
    ...
}

Simply by registering ResponseErrorHandler or ResponseDecoder in case of Feign.

@aheusingfeld
Copy link

I like the idea but could you explain how you avoid that the libraries you mentioned (Feign, RestTemplate, HttpClient, OkHttp etc.) need to have a dependency to the artifact which contains VndErrorException i.e. speing-hateoas?

@jmnarloch
Copy link
Author

Probably we can not avoid that, I made the PR here were the VndErrors is already being defined, though I would consider whether it wouldn't make more sense to move the VndErrors to spring-web at some point.

@jmnarloch
Copy link
Author

@olivergierke Thanks, for your thoughts. I've added extra tests, if you want to I can take a look at updating the implementation and align the VndErrors with the current specs, though I would rather prepare separate PR for that.

Also I was wondering way the exception hasn't been introduced in first place, the spring-web has this hall hierarchy of RestClientException.

@pivotal-issuemaster
Copy link

@jmnarloch Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@pivotal-issuemaster
Copy link

@jmnarloch Thank you for signing the Contributor License Agreement!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants