-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Add RestResponse<T> type to RESTEasy Reactive #18269
Conversation
Nice! I'll take a look tomorrow! |
...ment/src/main/java/io/quarkus/resteasy/reactive/server/deployment/CustomFilterGenerator.java
Outdated
Show resolved
Hide resolved
I like it. I think it's worth having it for the client side too. |
RestResponse<T>
RestResponse<T>
type to RESTEasy Reactive
RestResponse<T>
type to RESTEasy Reactive
It seems like the PR needs a formatting update |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is great! I added some small comments
...resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/RestResponse.java
Outdated
Show resolved
Hide resolved
...resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/RestResponse.java
Outdated
Show resolved
Hide resolved
...resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/RestResponse.java
Outdated
Show resolved
Hide resolved
...resteasy-reactive/common/runtime/src/main/java/org/jboss/resteasy/reactive/RestResponse.java
Outdated
Show resolved
Hide resolved
...nt/src/test/java/io/quarkus/resteasy/reactive/server/test/response/RestResponseResource.java
Outdated
Show resolved
Hide resolved
@@ -94,6 +94,11 @@ | |||
<artifactId>quarkus-jaxrs-client-reactive-deployment</artifactId> | |||
<scope>test</scope> | |||
</dependency> | |||
<dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure this works properly? Just want to make sure it doesn't create any cyclic dependencies or anything
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's a test dep, and it seems to build… 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK :)
I think it's best as is
I don't think it's worth it.
Yeah, it does sound useful but can definitely done in a follow-up
|
Ok great ! We can add support for it a.s.a.p. I'll open a ticket in SmallRye. |
679b427
to
5deff40
Compare
This workflow status is outdated as a new workflow run has been triggered. |
This workflow status is outdated as a new workflow run has been triggered. 🚫 This workflow run has been cancelled. Failing Jobs - Building 5deff40
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
Huh…
|
Likely:
which is what I thought would happen because of the jackson dependency you added |
Huh, I wonder how I didn't notice this before. Fixed now, let's see. |
Oh FFS. Well this is even worse now. |
OK, moved them to the jackson module. Same player shoots again. |
Nothing but net :) |
Now, that was a majorly screwed up rebase. Took me forever to untangle it. WOW. |
Fixes #16718.
RestResponse
type, which is generic with the entity typeResponse
type with generics added and a few modificationsRestResponse
return a fully-builtRestResponse
instead of aResponseBuilder
(shortcuts)RestResponse.ResponseBuilder
allowing more complex use-casesRestResponse.Status
andRestResponse.StatusCode
to list new HTTP statuses and have them asint
variants (yay).Response RestResponse.toResponse()
to convert to a JAX-RSResponse
Response
was supported)Questions/remaining:
Response
and impls, but perhaps it could just wrap one instead?Response
, not sure if we should turn it around or not. Not sure it's worth it.Request
(for preconditions) returnsResponse
types, so I haven't updated the docs example to useRestResponse
. We could extend it to offer variants that deal withRestResponse
but given that the return type would likely beRestResponse<Void>
since those status codes don't have any entity, it would likely be awkward and the endpoints using them would have to returnRestResponse<?>
in order to allow both variants…smallrye-openapi
.