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

[RESTEASY-2612] Support CompletableFuture resource method return type #2727

Merged
merged 1 commit into from
Sep 12, 2023

Conversation

spyrkob
Copy link
Contributor

@spyrkob spyrkob commented Apr 9, 2021

@ronsigal ronsigal added main main and removed main labels Apr 9, 2021
@wildfly-ci
Copy link
Collaborator

Hello, spyrkob. I'm waiting for one of the admins to verify this patch with /ok-to-test in a comment.

@@ -1115,17 +1115,18 @@ public <T extends Throwable> ExceptionMapper<T> getExceptionMapperForClass(Class
public <T> AsyncResponseProvider<T> getAsyncResponseProvider(Class<T> type)
{
Class asyncType = type;
AsyncResponseProvider<T> mapper = null;
while (mapper == null)
if (getAsyncResponseProviders() == null)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should the three calls to getAsyncResponseProviders() be stored in a local and re-used?

* @tpSince RESTEasy 4.7
*/
@Test
public void testCompletabeFutureText() throws Exception
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo, Completabe

@spyrkob
Copy link
Contributor Author

spyrkob commented Jun 16, 2023

Thanks for the review @stevenschlansker, I updated and rebased the PR

@@ -1021,17 +1021,16 @@ public <T extends Throwable> ExceptionMapper<T> getExceptionMapperForClass(Class
// @Override
public <T> AsyncResponseProvider<T> getAsyncResponseProvider(Class<T> type) {
Class asyncType = type;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could be removed and the usage below could just be replaced by the type argument.

}
return mapper;
return null;
}

public <T> AsyncClientResponseProvider<T> getAsyncClientResponseProvider(Class<T> type) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably do the same thing from above here. Possibly with the getAsyncStreamProvider() too.

for (Class<?> aClass : asyncResponseProviders.keySet()) {
if (aClass.isAssignableFrom(asyncType)) {
return asyncResponseProviders.get(aClass);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should remove the type.getSuperClass() checking. That's a change in behavior. My assumption is this is done so we don't check the CompletionStatge, but maybe I'm wrong.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jamezp sorry for delay, I must have missed the notification

I added back the type.getSuperClass check. I assumed it was an attempt to check if the return type extends CompletionStage but maybe there's another case that it was meant to handle

@florian-signoret-ibm
Copy link

Hello,
Is there a plan to complete this PR?
Developers often forget to use CompletionStage instead of CompletableFuture and hit the issue.
Thank you!

@jamezp
Copy link
Contributor

jamezp commented Sep 6, 2023

Hello, Is there a plan to complete this PR? Developers often forget to use CompletionStage instead of CompletableFuture and hit the issue. Thank you!

Hi @florian-signoret-ibm,
I plan on looking at this by the end of the week. I'll back port this to 6.2 and likely 5.0 as well.

@jamezp jamezp merged commit 5534519 into resteasy:main Sep 12, 2023
11 of 12 checks passed
@spyrkob spyrkob deleted the RESTEASY-2612 branch September 12, 2023 20:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
6 participants