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

Create quarkus hal extension for resteasy and reactive #25396

Merged
merged 1 commit into from
May 11, 2022

Conversation

Sgitario
Copy link
Contributor

@Sgitario Sgitario commented May 5, 2022

The quarkus-hal is a new extension that supports both resteasy and resteasy reactive.

  • New extension quarkus-hal (mostly copied from quarkus rest data panache). The only change I made is that before, in Rest Data Panache, the jsonb and jackson serializers were serializing the entity into hal and also resolving the hal links. Now, the jsonb and jackson serializers are only serializing the entity that should have been previously populated with the hal links (this is done via server filters in resteasy classic and resteasy reactive).
  • Documented in Quarkus Restasy Reactive and Quarkus Resteasy guides

This is a follow-up action from #25217 (comment)

@quarkus-bot quarkus-bot bot added area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/panache area/rest labels May 5, 2022
@quarkus-bot

This comment has been minimized.

@Sgitario Sgitario marked this pull request as draft May 5, 2022 19:00
@quarkus-bot quarkus-bot bot added the area/spring Issues relating to the Spring integration label May 6, 2022
@geoand
Copy link
Contributor

geoand commented May 6, 2022

Thanks a lot @Sgitario.

I'll have a look next week.

@Sgitario Sgitario marked this pull request as ready for review May 6, 2022 05:56
@quarkus-bot

This comment has been minimized.

@quarkus-bot

This comment has been minimized.

@Sgitario
Copy link
Contributor Author

Sgitario commented May 8, 2022

A possible solution to not add breaking changes in Resteasy Classic is to not remove the auto-generated hal resources in quarkus-hibernate-rest-data-panache. Therefore, this dependency would only use the hal API from quarkus-hal and nothing else. The disadvantage of this is that we would be still duplicating the resources implementations for JSON and hal, and duplicating the hal JSON serializers in quarkus-hal and in quarkus-hibernate-rest-data-panache (as the serializers in quarkus-hibernate-rest-data-panache are also gathering the HAL links).

And another solution could be to change quarkus-hibernate-orm-rest-data-panache to use quarkus-resteas-reactive by default, so we could add quarkus-resteasy-reactive-links. Of course, we'd need to document how to keep using resteasy classic.

@geoand
Copy link
Contributor

geoand commented May 9, 2022

@Sgitario perhaps the description in #20874 will give you more insight into why things have been done the way they have

<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy-links-deployment</artifactId>
<scope>test</scope>
Copy link
Contributor Author

@Sgitario Sgitario May 9, 2022

Choose a reason for hiding this comment

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

I had to explicitly add this dependency, I guess because we had to use the -deployment version. However, this is not necessary to be added when using quarkus-hibernate-orm-rest-data-panache (there are tests to verify this).

<dependency>
<groupId>io.quarkus</groupId>
<artifactId>quarkus-resteasy-links-deployment</artifactId>
<scope>test</scope>
Copy link
Contributor Author

@Sgitario Sgitario May 9, 2022

Choose a reason for hiding this comment

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

I had to explicitly add this dependency, I guess because we had to use the -deployment version. However, this is not necessary to be added when using quarkus-spring-data-rest (there are tests to verify this).

@Sgitario
Copy link
Contributor Author

Sgitario commented May 9, 2022

@Sgitario perhaps the description in #20874 will give you more insight into why things have been done the way they have

PR updated

Copy link
Contributor

@geoand geoand left a comment

Choose a reason for hiding this comment

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

This is great work, thanks!

I've just added a few comments

return acceptMediaType.contains(RestMediaType.APPLICATION_HAL_JSON);
}

private Class<?> findEntityClass() {
Copy link
Contributor

Choose a reason for hiding this comment

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

We can leave addressing this for a following PR, but in RESTEasy Reactive we generally try to avoid performing reflection as much as possible as it has a negative effect on both the execution speed and memory consumption. This data is generally available at build time, so we try to leverage it as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed the second part of this method. For the first part, we can do it in the next iteration.

Copy link
Contributor

Choose a reason for hiding this comment

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

So the second part was not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it wasn't needed at all.

The quarkus-hal is a new extension that supports both resteasy and resteasy reactive.
@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 10, 2022
@quarkus-bot

This comment has been minimized.

@geoand geoand merged commit 1d6fc0b into quarkusio:main May 11, 2022
@quarkus-bot quarkus-bot bot added this to the 2.10 - main milestone May 11, 2022
@quarkus-bot quarkus-bot bot removed the triage/waiting-for-ci Ready to merge when CI successfully finishes label May 11, 2022
@Sgitario Sgitario deleted the links_hal branch May 11, 2022 05:56
@rsvoboda
Copy link
Member

@gsmet this wasn't mentioned in https://quarkus.io/blog/quarkus-2-10-0-final-released/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/core area/dependencies Pull requests that update a dependency file area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/documentation area/panache area/rest area/spring Issues relating to the Spring integration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants