Feign spring pagination support #1604

Open
wants to merge 16 commits into
from

Projects

None yet

4 participants

@IsNull
IsNull commented Jan 10, 2017 edited

Support using spring Pageable and Page in Feign clients as requested in #556.

@RequestMapping(value = "invoicesPaged", method = RequestMethod.GET, produces = MediaType.APPLICATION_JSON_VALUE)
ResponseEntity<Page<Invoice>> getInvoicesPaged(Pageable pageable);

To support this, a Pageable Encoder and a Page Jackson Decoder has been added.

@IsNull
IsNull commented Jan 10, 2017 edited

The tests currently fail since the assumption that the Encoder is an instance of SpringEncoder is no longer true:

@Test
public void encoderDefaultCorrect() {
      SpringEncoder.class.cast(this.feignContext.getInstance("foo", Encoder.class));
}

encoderDefaultCorrect(org.springframework.cloud.netflix.feign.EnableFeignClientsTests): Cannot cast org.springframework.cloud.netflix.feign.support.PageableSpringEncoder to org.springframework.cloud.netflix.feign.support.SpringEncoder
overrideEncoder(org.springframework.cloud.netflix.feign.FeignClientOverrideDefaultsTests): Cannot cast org.springframework.cloud.netflix.feign.support.PageableSpringEncoder to org.springframework.cloud.netflix.feign.support.SpringEncoder

I could of course rework the PageableEncoder and hide it inside the SpringEncoder, maybe make it pluggable. What do you think?

@IsNull IsNull hide PageableSpringEncoder in SpringEncoder to pass tests
3b055d1
@codecov-io
codecov-io commented Jan 10, 2017 edited

Current coverage is 73.96% (diff: 57.14%)

Merging #1604 into master will decrease coverage by 0.15%

@@             master      #1604   diff @@
==========================================
  Files           193        195     +2   
  Lines          6008       6057    +49   
  Methods           0          0          
  Messages          0          0          
  Branches        904        909     +5   
==========================================
+ Hits           4453       4480    +27   
- Misses         1229       1248    +19   
- Partials        326        329     +3   

Powered by Codecov. Last update 8029deb...2489e06

@@ -64,6 +62,7 @@
@Autowired(required = false)
private Logger logger;
+
@ryanjbaxter
ryanjbaxter Jan 11, 2017 Contributor

Remove rogue new line

@@ -69,6 +69,10 @@
<artifactId>archaius-core</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
@ryanjbaxter
ryanjbaxter Jan 11, 2017 Contributor

should this be optional like the other dependencies?

@IsNull
IsNull Jan 11, 2017

Now this is a good question. If I understand the optional behaviour correctly, then if the user does not specify it (directly or indirectly) he wont have this dependency. If this is the case, then the current implementation would fail horribly due to a ClassNotFoundException in

public boolean supports(Object object, Type bodyType, RequestTemplate template){
        return object instanceof Pageable;
    }

Since Pageable wont be available.

So would it require a ugly check like the one below, before using a class file which has an import to the optional dependency?

try {
 Class.forName( "org.springframework.data.domain.Pageable" );
} catch( ClassNotFoundException e ) {
 // not here
}

If so, is this really desired?

@ryanjbaxter
ryanjbaxter Jan 11, 2017 Contributor

No, you can use Spring Beans and check is the Pageable class is available and then include/exclude encoders based on that.

@IsNull
IsNull Jan 11, 2017

Ah yes, this is simpler. I give it a try.

@spencergibb
spencergibb Jan 11, 2017 Member

+1 to the optional. use @ConditionalOnClass in the auto configuration.

@IsNull IsNull removed unintended newline
dd7bb57
@IsNull
IsNull commented Jan 11, 2017 edited

@ryanjbaxter I noticed several places where the Encoder is hardcoded to SpringEncoder in the tests.

@Test
public void encoderDefaultCorrect() {
	SpringEncoder.class.cast(this.feignContext.getInstance("foo", Encoder.class));
}

// ...

@Test
public void overrideEncoder() {
	Encoder.Default.class.cast(this.context.getInstance("foo", Encoder.class));
	SpringEncoder.class.cast(this.context.getInstance("bar", Encoder.class));
}

// ...

@Test
public void testCustomHttpMessageConverter() {
	SpringEncoder encoder = this.context.getInstance("foo", SpringEncoder.class);
	assertThat(encoder, is(notNullValue()));
	RequestTemplate request = new RequestTemplate();

	encoder.encode("hi", MyType.class, request);

	Collection<String> contentTypeHeader = request.headers().get("Content-Type");
	assertThat("missing content type header", contentTypeHeader, is(notNullValue()));
	assertThat("missing content type header", contentTypeHeader.isEmpty(), is(false));

	String header = contentTypeHeader.iterator().next();
	assertThat("content type header is wrong", header, is("application/mytype"));
}

I think the tests should not depend on the specific type. If we need to be sure it is an encoder from Spring there might be another way. But I am not sure about the benefit of these tests.

Since my Encoder pipe will be built dynamically, I also don't know which Encoder will be instantiated so these tests are really annoying.

@IsNull IsNull support optional decoder
7300cd4
@@ -69,6 +69,10 @@
<artifactId>archaius-core</artifactId>
<optional>true</optional>
</dependency>
+ <dependency>
@spencergibb
spencergibb Jan 11, 2017 Member

+1 to the optional. use @ConditionalOnClass in the auto configuration.

@@ -57,6 +59,11 @@ public FeignContext feignContext() {
return context;
}
+ @Bean
@spencergibb
spencergibb Jan 11, 2017 Member

A @ConditionalOnClass should probably go here as well.

@ConditionalOnMissingBean
public Encoder feignEncoder() {
return new SpringEncoder(this.messageConverters);
}
@Bean
+ @ConditionalOnClass(name = "org.springframework.data.domain.Pageable")
@spencergibb
spencergibb Jan 11, 2017 Member

You can use the actual class here, boot handles it just fine.

@IsNull
IsNull Jan 11, 2017

Thank you for the info, I will change it.

@IsNull IsNull improved conditional configuration
54fab60
@ryanjbaxter
Contributor

So are those tests now failing? I would assume the Encoder would not be effected in the existing tests. If not it might be an indication your changes broke something...

@IsNull
IsNull commented Jan 12, 2017

@ryanjbaxter Yes the tests are failing and I understand why and explained the reason above (at least I tried) In my opinion the tests need to be fixed, so before I do that Id like to consult with you :)

The new Encoder configuration looks like this and is the reason why some tests fail:

@Bean
@ConditionalOnMissingClass("org.springframework.data.domain.Pageable")
@ConditionalOnMissingBean
public Encoder feignEncoder() {
	return new SpringEncoder(this.messageConverters);
}

@Bean
@ConditionalOnClass(Pageable.class)
@ConditionalOnMissingBean
public Encoder feignEncoderPageable() {
	return new PageableSpringEncoder(new SpringEncoder(this.messageConverters));
}

Now, there are 3 tests which expect the Encoder to be SpringEncoder, which is IMHO too specific and prevents me to do it this way.

On a similar note, the Decoder already exposes something different than the SpringDecoder:

@Bean
@ConditionalOnMissingBean
public Decoder feignDecoder() {
	return new ResponseEntityDecoder(new SpringDecoder(this.messageConverters));
}

So if you and @spencergibb are fine with my implementation, I go ahead and fix the tests. I just wasn't sure if there is any other reasoning why the tests explicitly expect a SpringDecoder instance.

@ryanjbaxter
Contributor

They probably expect SpringDecorder because there was no other decoder. I would expect that SpringDecorder would still be returned in those tests. Is PageableSpringEncoder always being returned now since Pageable is on the classpath?

@IsNull
IsNull commented Jan 12, 2017 edited

They probably expect SpringDecorder because there was no other decoder.

If you mean SpringEncoder then yes, the tests seem that way.
(I did not change anything about the SpringDecoder and as a matter of fact, the tests are already not expecting SpringDecoder but the enhanced ResponseEntityDecoder)

I would expect that SpringDecorder would still be returned in those tests.

Assuming you mean SpringEncoder. 😄
=> Well, since the SpringDecoder is also not returned, it would be inconsequent to imply that the Encoder must always be the SpringEncoder.

Is PageableSpringEncoder always being returned now since Pageable is on the classpath?

No as per your request above, the Pageable dependency (spring-data-commons) is now optional, and thus the actual Encoder instance depends on whenever Pageable is available or not.
The current implementation will either yield PageableSpringEncoder or the original SpringEncoder.

IsNull added some commits Jan 12, 2017
@IsNull IsNull fixed tests 315eefe
@IsNull IsNull Merge remote-tracking branch 'spring-cloud/master' into feign-spring-…
…pagination-support
2489e06
@IsNull
IsNull commented Jan 12, 2017

I have fixed the unit tests, please have a look if this is feasible.

@ryanjbaxter

Can you try doing something like this to test what happens when the spring-data jars are not on the class path? https://github.com/spring-cloud/spring-cloud-commons/pull/137/files#diff-5ae04146d605077bb4ccf022b15c801cR18

If you modify the existing tests to exclude the jars I would imagine they would not fail. Then you can create new tests that test with the new encoder.

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