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

Jackson2Decoder fails to determine correct target type from default interface method with a generic type #23791

Closed
zhou-hao opened this issue Oct 14, 2019 · 11 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@zhou-hao
Copy link

version: 5.2.0-RELEASE

JavaType javaType = getJavaType(elementType.getType(), contextClass);

image

image

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Oct 14, 2019
@sdeleuze
Copy link
Contributor

Please provide a repro project for the issue you observe.

@sdeleuze sdeleuze added the status: waiting-for-feedback We need additional information before we can continue label Oct 14, 2019
zhou-hao added a commit to zhou-hao/spring-webflux-issue-23791 that referenced this issue Oct 14, 2019
@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Oct 14, 2019
@rstoyanchev
Copy link
Contributor

Thanks for the sample project. I can confirm there is an issue with resolving generic types from a default method via ResolvableType. I was able to further reduce it to the following test:

import java.lang.reflect.Method;

import org.junit.jupiter.api.Test;
import reactor.core.publisher.Mono;

import org.springframework.core.MethodParameter;
import org.springframework.core.ResolvableType;
import org.springframework.web.bind.annotation.PostMapping;
import org.springframework.web.bind.annotation.RequestBody;
import org.springframework.web.bind.annotation.RestController;

import static org.assertj.core.api.Assertions.assertThat;

public class Issue23791Tests {

	@Test
	void name() throws NoSuchMethodException {

		Method method = TestController.class.getMethod("test", Mono.class);
		MethodParameter bodyParam = new MethodParameter(method, 0);

		ResolvableType bodyType = ResolvableType.forMethodParameter(bodyParam);
		ResolvableType elementType = bodyType.getGeneric();

		assertThat(elementType.getSource()).isEqualTo(TestEntity.class);
	}

	public interface GenericController<E> {

		@PostMapping("/test")
		default Mono<String> test(@RequestBody Mono<E> test) {
			return Mono.empty();
		}
	}

	@RestController
	public class TestController implements GenericController<TestEntity> {
	}

	public class TestEntity {
	}
}

The above prints:

Expecting:
 <E>
to be equal to:
 <org.springframework.web.reactive.function.client.Issue23791Tests.TestEntity>
but was not.

If the default method is changed to be a regular interface method, the test passes.

@rstoyanchev rstoyanchev added in: core Issues in core modules (aop, beans, core, context, expression) type: enhancement A general enhancement and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 16, 2019
@rstoyanchev rstoyanchev added this to the 5.2.3 milestone Dec 16, 2019
@philwebb
Copy link
Member

I did a bit of digging that problem is caused by the fact that ResolvableType doesn't know that the method is actually declared in the TestController.

If you put a breakpoint at ResolvableType.forMethodParameter (around line 1326) you'll see that owner is resolved as GenericController. If you change TestController to actually override the test method then you'll see owner is TestController.

Unfortunately it doesn't appear that Method can provide the information that we need. The only way to workaround the problem is to create the parameter as follows:

	MethodParameter param = new MethodParameter(method, 0, TestController.class);

The sets MethodParameter.containingClass which means we can resolve the result.

@philwebb
Copy link
Member

@rstoyanchev I think the only way to fix this is to move up the stack to the point where the ResolvableType is created. I don't think there's anything that can be done in ResolvableType itself.

@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) and removed in: core Issues in core modules (aop, beans, core, context, expression) labels Dec 17, 2019
@rstoyanchev rstoyanchev self-assigned this Dec 17, 2019
rstoyanchev added a commit that referenced this issue Dec 20, 2019
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Dec 20, 2019

Thanks for taking a look @philwebb. My mistake actually, the example above does not accurately reflect what actually happens in Spring MVC where a MethodParameter is obtained from a HandlerMethod which has a MethodParameter sub-class that overrides getContainingClass().

So in the above example, the MethodParameter should be initialized as shown below, after which around line 1326 in ResolvableType.forMethodParameter the owner is TestController:

TestController bean = new TestController();
HandlerMethod handlerMethod = new HandlerMethod(bean, bean.getClass().getMethod("test", Mono.class));
MethodParameter bodyParam = handlerMethod.getMethodParameters()[0];

I have also added a disabled test in the framework that mimics closely the scenario. For debugging, this is where we handle the MethodParameter and resolve its generic, and this is where we obtain the context class but fail.

Would you mind taking another look? It seems that all the information should be available and as far as I know the code is using ResolvableType in the expected ways.

@rstoyanchev
Copy link
Contributor

Oops closed in error. So far the only commit for this issue is 7456fb9.

@rstoyanchev rstoyanchev reopened this Dec 20, 2019
@rstoyanchev rstoyanchev changed the title Decoding Interface Generic Type Bug? Jackson2Decoder fails to determine correct target type from default interface method with a generic type Dec 20, 2019
@philwebb
Copy link
Member

I've not yet been able to pinpoint the exact cause. If I take the failing type and change ConcreteController23791 so that it overrides the test method I can debug into Jackson2CodecSupport.getObjectReader and see the following:

  • elementType is Person
  • param is null
  • contextClass is null

The call to getJavaType gets passes a type of Class with the value Person.

If I run the original I see the following:

  • elementType is Person
  • param is null
  • contextClass is null

The call to getJavaType gets passes a type of TypeVariableImpl with the value E.

In the second case the call to GenericTypeResolver.resolveType(type, contextClass) fails because there is no context class.

So it seems like the getParameter(type) call is failing in both cases, we just get away with it when we're not using default interface methods.

@philwebb
Copy link
Member

It looks like the type passed to getParameter will never be able to get back to the MethodParameter with reactive types. The elementType is ultimately coming from AbstractMessageReaderArgumentResolver.readBody:

ResolvableType elementType = (adapter != null ? bodyType.getGeneric() : bodyType);

At this point, because adaper is not null we're calling bodyType.getGeneric(). This means that the MethodParameter is no longer the source. It's the generic on the method parameter. Calling bodyType.getSource() will give you the MethodParameter but calling elementType.getSource() will give you the E variable type.

@philwebb
Copy link
Member

I'm not sure what the fix is since I don't really understand all of that code. Perhaps if getJavaType fails you can fallback to elementType.resolve() which will give you a Person type.

@rstoyanchev rstoyanchev modified the milestones: 5.2.3, 5.2.4 Jan 13, 2020
@rstoyanchev
Copy link
Contributor

rstoyanchev commented Jan 23, 2020

Thanks @philwebb.

So I found a way to pass the containing class through the "hints" map which resolves this issue. However I still wonder if there is room for improvement in ResolvableType.

The elementType is ultimately coming from AbstractMessageReaderArgumentResolver.readBody:
ResolvableType elementType = (adapter != null ? bodyType.getGeneric() : bodyType);

At this point, because adaper is not null we're calling bodyType.getGeneric(). This means that the MethodParameter is no longer the source. It's the generic on the method parameter.

Right. I found out that if elementType is created like this, it also works:

ResolvableType elementType = ResolvableType.forType(
        GenericTypeResolver.resolveType(
                bodyType.getGeneric().getType(), bodyParam.getContainingClass()));

Considering that bodyType has the MethodParameter source (i.e. bodyParam), and therefore has access to bodyParam.getContainingClass(), then the magic that is in GenericTypeResolver should be possible to do from within ResolvableType.getGeneric() as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

5 participants