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

DispatcherHandler invoke wrong method when discriminating request with params [SPR-16210] #20758

Closed
spring-issuemaster opened this issue Nov 17, 2017 · 7 comments

Comments

Projects
None yet
2 participants
@spring-issuemaster
Copy link
Collaborator

commented Nov 17, 2017

Jocelyn Ntakpe opened SPR-16210 and commented

When having to controller methods with almost the same parameters and return type, DispatcherHandler doesn't apply the right JsonView.
For instance :

@RestController
    @RequestMapping("/users")
    class UserController {

        @JsonView(Public::class)
        @GetMapping("/{username}")
        fun findPublic(@PathVariable username: String) = User(username, "pwd").toMono()

        @JsonView(Internal::class)
        @GetMapping("/{username}", params = arrayOf("withpwd"))
        fun findInternal(@PathVariable username: String) = User(username, "pwd").toMono()

    }

    data class User(val username: String, @field:JsonView(Internal::class) val password: String?)

    interface Views {

        interface Public

        interface Internal : Public
    }

If you run tests depending on tests order the code above will fail on one of the two endpoints.

!webflux_bug.png|thumbnail!

It seems HandlerResult method (findPublic) differs from getReturnTypeSource (findInternal) because in HandlerResult constructor handler.getMethod() doesn't return the same that ResolvableType.forMethodParameter(returnType).getSource().

The code works fine if the endpoints returns User instead of Mono<User>.

The issue looks like #20647 but the fix doesn't solve this one


Affects: 5.0.2

Reference URL: https://github.com/jntakpe/webflux-bug-report

Attachments:

Issue Links:

  • #19392 ResolvableType equals method should not consider TypeProvider source
  • #21001 Resolvable type cannot resolve generic between different collection types

Referenced from: commits 3f3141c, 4c74148

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 20, 2017

Sébastien Deleuze commented

I made some tests using your repro project, but was not able to reproduce the described behavior:

  • Without changing anything, I get a NPE at org.springframework.restdocs.cli.CliOperationRequest.allowedHeader(CliOperationRequest.java:100). Notice that the body is currently consumed 2 times and I don't think it should be (even if it does not fix the NPE)
  • If I remove Spring REST Docs from the test, WebFlux behaves as expected, tests are green and a manual test with a browser confirm it behaves as expected

Based on these checks, I tend to think WebFlux behaves correctly. Could you please double check you have the same behavior than me with your repro project?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Nov 20, 2017

Jocelyn Ntakpe commented

Hi Sébastien,

My bad, you are not able to reproduce the bug because I changed the repo to report a rest docs bug :(

I just reverted the rest docs modifications. Now you should be able to reproduce the bug.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Dec 5, 2017

Jocelyn Ntakpe commented

@sdeleuze Have you tried with the updated project version ?

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 16, 2018

Sébastien Deleuze commented

Indeed I observe the same behavior with your updated repro project, and only when async wrappers like Mono or CompletableFuture are used.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 18, 2018

Juergen Hoeller commented

Sébastien Deleuze, could HandlerResult simply store its given MethodParameter as a separate field? And then access that field in getReturnTypeSource instead of downcasting the ResolvableType source there? That would allow ResolvableType caching to do whatever it wants, reliably accessing the original MethodParameter given to the HandlerResult constructor in any case.

The only other place where we downcast a ResolvableType source is Jackson2CodecSupport. We might have to revisit that one as well.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 19, 2018

Juergen Hoeller commented

I've modified ResolvableType's caching to always return an independent clone (the lookup key instance which it needs to create in any case), just pre-resolving it from cached state if possible. This means that getSource() reliably returns the original local source on any call to a ResolvableType factory method now, with no caching side effects leaking through. I've also revised ResolvableType for concurrent access, marking lazily resolvable parts (super type, interfaces, generics) as volatile now and defensively accessing them.

@spring-issuemaster

This comment has been minimized.

Copy link
Collaborator Author

commented Jan 22, 2018

Sébastien Deleuze commented

Thanks for these changes Juergen Hoeller, I confirm it solves the issue seen in the repro project. I have also added an additional test that checks ResolvableType.forMethodParameter now behaves correctly for the reported use case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.