search

OpenFeign / feign-annotation-error-decoder

Apache License 2.0
51 stars 9 forks source link

with move to feign-core 10.x.x, support capturing the request in Exception constructors #18

Open saintf opened 5 years ago

saintf commented 5 years ago

Feign core 10.X.X allows capturing the Request made from a Response - meaning that we can include the original request into an exception if required.

The requirement here is to detect if one of the parameters in the constructor for the exception is of type Request, and if so, put the original request into that field.

In a later iteration we can ask people want us to do more parsing (like only include request headers based on a @RequestHeaders tag or only the request body based on a @RequestBody tag) - though for now we expect handing over the Request itself is probably more than enough (would want to see usecases for anything further).

Hence - put simply - when an exception is defined as:

public class MyException {

   public MyException(Request request) {
      ...
   }
}

then the AnnotationErrorDecoder should inject the Request.

StefanFellinger commented 5 years ago

Hi saintf, i think we have time to implement that future request, and create a pull request for you. Do you aggree with it, or fo you prefer to implement it by yourself?

saintf commented 5 years ago

@StefanFellinger Absolutely happy for you to give it a go :)

StefanFellinger commented 5 years ago

hey @saintf,

i've added logic to the ExceptionGenerator and updated your tests. How can i publish my changes as an pull request or something else. Can you please authorize me to do it.

saintf commented 5 years ago

hi @StefanFellinger - take a look at:

Basically - fork the codebase, make the changes, make sure code style matches/etc, everything is tested. once you're good, raise a PR from your fork, I'll review it, and then when we're done (review complete/etc) I'll merge it and release it :)

saintf commented 5 years ago

Merged https://github.com/OpenFeign/feign-annotation-error-decoder/pull/23

saintf commented 5 years ago

@StefanFellinger - Merged and released. It's in maven central (though it will take it about 30 minutes to actually show up in searches since it takes a while for the indexes to be updated/etc).

Make sure it's good to go and feel free to close this issue! Also - if it makes sense, also close https://github.com/OpenFeign/feign-annotation-error-decoder/issues/22?

Thanks for the help!