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

Fix parameter bug of handler that Kotlin DSL defines as a parameter for the filterFunction. #26857

Conversation

ShindongLee
Copy link
Contributor

@ShindongLee ShindongLee commented Apr 24, 2021

Resolve #26838
This PR fixes following bug.

fun filter(filterFunction: (ServerRequest, (ServerRequest) -> ServerResponse) -> ServerResponse) {
builder.filter { request, next ->
filterFunction(request) {
next.handle(request)
}
}

fun filter(filterFunction: suspend (ServerRequest, suspend (ServerRequest) -> ServerResponse) -> ServerResponse) {
builder.filter { serverRequest, handlerFunction ->
mono(Dispatchers.Unconfined) {
filterFunction(serverRequest) {
handlerFunction.handle(serverRequest).awaitSingle()
}
}
}
}

  1. Function filterFunction takes serverRequest and function handler as parameters. (I just named them.)
  2. Function handler also takes serverRequest as parameter.
  3. These DSLs define handler.
  4. Function handlers that these DSLs defined are now using mistakenly filterFunction's serverRequest, not their own serverRequest parameters, which makes their own parameters meaningless.

This bug only exists in Kotlin DSL.

You can see java codes have no problem as follows.

R filter(ServerRequest request, HandlerFunction<T> next) throws Exception;

In java you can override filter something like

ServerRequest newRequest = someOperation(request)
return next.handle(newRequest)

but in Kotlin DSL, even if you write

filter { serverRequest, handlerFunction ->
    val newServerRequest = someOperation(serverRequest)
    handlerFunction(newServerRequest)
}

handlerFunction will use original serverRequest, not the newServerRequest

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 24, 2021
@ShindongLee ShindongLee changed the title Fix parameter bug of handler inside the filterFunction Kotlin DSL Fix parameter bug of handler that Kotlin DSL defines as a parameter for the filterFunction. Apr 24, 2021
@sdeleuze sdeleuze self-requested a review April 28, 2021 12:04
@sdeleuze
Copy link
Contributor

Thanks for this PR, could you please add related tests for both WebMvc and WebFlux that show a failure with current implementation and would be green with your fix in the 2 RouterFunctionDslTests and CoRouterFunctionDslTests?

@sdeleuze sdeleuze removed the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 28, 2021
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Apr 28, 2021
@ShindongLee
Copy link
Contributor Author

ShindongLee commented Apr 28, 2021

@sdeleuze I added tests for Spring MVC (9d0fbbe) and Spring Webflux (b7e6d23). Thank you !

ShindongLee and others added 2 commits April 29, 2021 05:37
Co-authored-by: hojongs <hojong.jjh@gmail.com>
Co-authored-by: bjh970913 <bjh970913@gmail.com>
Co-authored-by: hojongs <hojong.jjh@gmail.com>
Co-authored-by: bjh970913 <bjh970913@gmail.com>
@ShindongLee ShindongLee force-pushed the bugfix/handler-function-parameter-bugfix branch from b916a57 to b7e6d23 Compare April 28, 2021 20:39
@ShindongLee
Copy link
Contributor Author

@sdeleuze Do you have any updates or plans for this ...?

@sdeleuze sdeleuze removed the status: waiting-for-triage An issue we've not yet triaged or decided on label May 7, 2021
@sdeleuze
Copy link
Contributor

sdeleuze commented May 7, 2021

Yes it is planned for inclusion in Spring Framework 5.3.7 as mentioned in #26838.

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

Merged via 07ba957 (main) and 3ab270e (5.2.x), thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Kotlin filter parameter bug in Router DSLs
3 participants