René's URL Explorer Experiment

Title: Apply review feedback to the multi-encoder and multi-decoder surface by velo · Pull Request #3534 · OpenFeign/feign · GitHub

Open Graph Title: Apply review feedback to the multi-encoder and multi-decoder surface by velo · Pull Request #3534 · OpenFeign/feign

X Title: Apply review feedback to the multi-encoder and multi-decoder surface by velo · Pull Request #3534 · OpenFeign/feign

Description: @trumpetinc's review of #3527, applied to both sides of the codec surface. Everything here is still @Experimental, so this is the moment to move it. Taken A multi-codec is itself predicated. MultiEncoder implements PredicatedEncoder and MultiDecoder implements PredicatedDecoder, accepting whatever any of their delegates accepts, so one nests inside another. That is the composition you asked for — a library can ship a set as a single unit: Feign.builder().encoders(StreamingFeign.encoders(), new JacksonEncoder()); The winning delegate's canEncode is evaluated twice, once by the outer anyMatch and once by the inner loop. Predicates are already documented as cheap and free of side effects, so that is acceptable, but it is worth saying out loud. Nested sets are unfolded in the failure message, so contributing a bundle does not turn it into one opaque line: Encoders tried, in order: - JacksonEncoder - MultiEncoder: - StreamingStringEncoder when the return type is a Stream - StreamingBytesEncoder when Content-Type is application/octet-stream DefaultEncoder and DefaultDecoder declare themselves. You were right and I was wrong: the predicate is not a yes-man, it is exactly what encode/decode handles. This makes raw passthrough composable, which is genuinely useful — a String body reaching Jackson comes out quoted, so putting the default first fixes that: Feign.builder().encoders(new DefaultEncoder(), new JacksonEncoder()); One correction to the predicate you proposed: DefaultEncoder.encode also accepts a null body, which it sends as no body at all. Dropping that case would break @RequestLine("POST /orders") void create(Order order); client.create(null); so the predicate is bodyType == String.class || bodyType == byte[].class || object == null. DefaultDecoder gets the same treatment, including 404/204 and a null body, which it handles for any type. An empty builder fails at wiring time, IllegalStateException from build(), rather than producing a codec that can only ever throw on the first request. encoders()/decoders() with no arguments fail the same way. Builder.narrow(predicate, codec), so the replace/AND pair sits side by side where the difference is visible, and the javadoc on of and narrowing now says which is which in one line. More on that below. The formRequests() javadoc wording, and a note on the FormEncoder(Encoder) constructor that chaining belongs in encoders(...) and the delegate is expected to be deprecated once this surface stops being experimental. Pushed back on narrowing is not redundant with add(predicate, encoder). PredicatedEncoder.of replaces whatever the encoder declares — deliberately, so you can point a GsonEncoder at a vendor content type it would otherwise refuse. narrowing is the AND. Both are needed. That said, the fact that it read as redundant is the finding: the names were not carrying the difference. Hence the builder narrow and the sharper javadoc rather than a rename, since the statics are already published. The failure message reports Content-Type and Accept, not every header. EncodeException messages land in logs and the header map routinely holds Authorization. The detail is not lost — every predicate carries a description and is printed alongside the codec it guards, so a predicate keyed on an unusual header says so itself: .add(EncoderPredicate.describedAs("X-Acme-Format is protobuf", ...), protoEncoder) Unable to encode com.acme.Order (Content-Type: text/plain, Accept: application/json) for POST /orders. Encoders tried, in order: - JacksonEncoder - ProtobufEncoder when X-Acme-Format is protobuf Register an encoder that accepts it, or add a catch-all (EncoderPredicate.any()) last. The hint wording is softened too, since .any() is an implementation detail for anyone who came in through encoders(...). On the decode side the second header is the request's Accept, which is what the caller asked for. Not done DefaultEncoder as a MultiEncoder — you flagged it as an observation rather than a proposal, and I agree it is one. It trades a ten-line class for a list allocation and makes the failure message worse. Centralizing content-type parsing on the parser in #3494 — agreed in principle, but that PR is still open. The Util helpers are @Experimental, so whichever lands second swaps the internals; no need to couple them now. mvn clean install passes.

Open Graph Description: @trumpetinc's review of #3527, applied to both sides of the codec surface. Everything here is still @Experimental, so this is the moment to move it. Taken A multi-codec is itself predicated. Mu...

X Description: @trumpetinc's review of #3527, applied to both sides of the codec surface. Everything here is still @Experimental, so this is the moment to move it. Taken A multi-codec is itself predicated...

Opengraph URL: https://github.com/OpenFeign/feign/pull/3534

X: @github

direct link

Domain: github.com

Links:

Viewport: width=device-width