Simplify Feign

[codefromthecrypt]

There are a number of ideas in feign that didn't work out very well, are edge case, or pure over-engineering. We should get rid of them as feign is used in production and should focus its code base, failure cases, and WTFs on things relevant to those use cases.

• [x] For those who do not use dagger, or do not wish to, we could expose a builder which defines all dependencies explicitly that can be provided. see issue #34
• [x] Codec types such as Decoder, Encoder, should not have a type parameter. The implicit responsibility of type adapters is better taken by implementations like gson. Moreover, binding to a raw-type Set<Decoder> is confusing when the type parameter is meaningful.
• [x] IncrementalDecoder should be axed. This is even more hairy than above, and even less often used. Streaming is application specific (twitter newlines vs S3 buckets) and rare. There are no feign clients I'm aware of that actually employ incremental parsing. Finally, multiplexed (SPDY, HTTP2) or message oriented (WebSocket) approaches imply less need for really long stream decodes over time.
• [x] Pattern decoders should be axed. Existing use of RegEx decoders was limited to XML responses, so rather than maintain a sizable chunk of code for both SAX and RegEx, let's prefer SAX.
• [x] Observer/Observable should be axed. Due to above, getting an Observer to actually work per-element is application-specific, not format specific, and would imply a lot of confusing feign config. While there is usefulness in exposing the response (or even its reader) as a callback, Observable overshoots this and adds complexity. Similarly to IncrementalDecoder, Observable isn't used in feign client in production I'm aware of! Currently, those wanting Observer could do so by submitting a Callable to a feign method w/ rx.Observer. Experimentation would end up in an extension per https://github.com/Netflix/feign/issues/25 and https://github.com/Netflix/feign/pull/26

This doesn't mean we shouldn't play with ideas or that these ideas will never reach prod. Just that we shouldn't commit all ideas to trunk as that increases the cognitive and maintenance load of feign without helping real production use. This may imply we need a "labs" repo or persistent forks to facilitate research.

[codefromthecrypt]

cc @syed-haq @niteshkant @benjchristensen @digitalsanctum @stonse @jdamick @allenxwang

[thesurlydev]

My thought on these types of debates is that the core project should do as little as possible while still maintaining usefulness so +1 from me.

Fancy pants functionality or addressing edge cases should happen in another dependency/extension/module/plugin.

[davidmc24]

Makes sense to me. My thinking is that open-source utility libraries like this one should strive to be easy to understand, address the most commonly used 90% of use cases out-of-the-box, and provide a clear path through some form of extension point to address the next 8% or so of use cases.

In this case, that would seem to mean cutting down on core code and concepts that are not seeing heavy use, but providing mechanisms to hook in equivalent types of behaviors through extension modules. Over time, if certain extension modules become populate enough, it may be worthwhile to incorporate them into the core.

[etoews]

+1 to less code.

Simplifying also has the added benefit of making it easier to learn.

[benjchristensen]

Perhaps it would be helpful to restate what the intent of feign-core is (in light of https://github.com/Netflix/feign/#why-feign-and-not-x) and then guidelines of how these extensions (such as streaming, callbacks, decoders, etc) will work a way that feels natural?

It would be good to have a story (such as will replace what is currently on the main README) about how each of these things will be accomplished after this refactor:

• https://github.com/Netflix/feign/#request-interceptors
• https://github.com/Netflix/feign/#observable-methods
• https://github.com/Netflix/feign/#multiple-interfaces
• https://github.com/Netflix/feign/#gson and https://github.com/Netflix/feign/#decoders
• https://github.com/Netflix/feign/#jax-rs

[codefromthecrypt]

So these changes move forward the goal of feign as stated in the read me: "reducing the complexity of binding Denominator uniformly to http apis regardless of restfulness" This particularly helps, as every item in this issue is a reduction of configuration and/or complexity.

That said, there are some changes that would take place in the README.

• Intro - remove RxJava ref, as the core wouldn't really have any relationship to it anymore
• How does Feign work? - no changes
• Basics - no changes
• Request Interceptors - no changes
• Observable Methods - remove and continue discussion instead on https://github.com/Netflix/feign/pull/25
• Multiple Interfaces - no changes
• Integrations - no changes
• Decoders - remove type parameter
  • Type-specific Decoders - remove, but add content to top-level Codec wiki; explain that type-specific decoding is an implementation detail + an example of how in gson.
• Incremental Decoding - move, but add content to top-level Codec wiki; explain that incremental decoding is an implementation detail + an example of how to incrementally decode to an Iterator in gson.
• Advanced usage and Dagger - change set binding example to discuss multiple interceptors as there is no longer a need for multiple decoders.
• Logging - no change
• Pattern Decoders - remove, but add content to top-level Codec wiki

I've updated this issue to simply remove Observable, vs replacing it w/Callback. Reason being is that the callback design might be different depending on whether it is cancelable or not, and this is often a transport concern. Rather than implementing another feature that has no production use, it is more congruent to just axe callbacks for now.

[davidmc24]

Makes sense to me. For starting users, implementing #34 and making end-user exposure of Dagger optional except for advanced usage should lower the learning curve. Towards that goal, it might make sense to use the builder in all examples until the "Advanced usage and Dagger" section.

[codefromthecrypt]

+1

[codefromthecrypt]

ps if anyone gets a code-cleansing high out of deleting stuff, feel free to start issuing pull requests against this. My migration path is to move denominator off any decoders/encoders except those w/ Object type param. We'll likely be better off in transitioning just making a single type Codec w/ encode/decode methods in v4 so that we can deprecate the encoder/decoders stuff for removal in v5

[codefromthecrypt]

@davidmc24 ok so code changes are ready except the global encoder/decoder stuff you are working on and the Feign.Builder (aka issue 34). I'm going to be sporadically online a couple days but happy to review any progress as you make it.

[davidmc24]

@adriancole ready for you to take another look when you have a chance

[codefromthecrypt]

@davidmc24 went through it and looks nearly there. a bit of polish then ready to merge I think!

[codefromthecrypt]

great job, @davidmc24! Only thing left is to address issue #34 (Feign.builder()). After that, we can cut feign 5, which I can do on Tuesday morning pacific, if we are ready.

[codefromthecrypt]

@davidmc24 super job on this work.

I think the README is accurate as well. I'm refactoring denominator over this change, now. I suppose last concern might be.. should we bump sax to its own module? It is used twice in denominator, and it has no deps. However, it probably needs a top-level wiki, so this could be best achieved as sax/README.md.

wdyt?

[codefromthecrypt]

@davidmc24 fyi cleared a couple usage glitches sorted in #66 and #67 I'll probably be offline for several hours.

[davidmc24]

@adriancole I think that splitting the SAX decoder into its own module with a README makes sense. Having it be the only non-default'ish encoder/decoder in core feels a strange to me.

[codefromthecrypt]

will do

[codefromthecrypt]

feign 5.0.0-SNAPSHOT published to sonatype.

testing today