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.

Sign up for GitHub

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

Jump to bottom

Fail build with HAL resource without serializer #11246 #11248

Merged
merged 1 commit into from
Mar 15, 2021
Merged

Fail build with HAL resource without serializer #11246 #11248

merged 1 commit into from
Mar 15, 2021

Conversation

gytis
Copy link

@gytis gytis commented Aug 6, 2020

gsmet
gsmet previously requested changes Aug 6, 2020
View reviewed changes
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Added some comments inline. Feel free to ping me if something is unclear.

...ache/deployment/src/main/java/io/quarkus/rest/data/panache/deployment/RestDataProcessor.java Outdated
return false;
}

private boolean hasJsonProcessor() {
Copy link
Member

Choose a reason for hiding this comment

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

You should rely on the Capabilities here. The ones existing don't really cover your use case (I checked, even if their names sound appealing, they are not covering your needs) so you will need to add RESTEASY_JACKSON and RESTEASY_JSONB and produce them from the respective processors together with the others.

...ache/deployment/src/main/java/io/quarkus/rest/data/panache/deployment/RestDataProcessor.java Outdated
private void verifyHalResources(IndexView index, List<RestDataResourceBuildItem> resourceBuildItems) {
if (!hasJsonProcessor() && hasHalResource(index, resourceBuildItems)) {
logger.warn("Cannot generate HAL endpoints without a serializer. "
+ "Make sure you have either Jsonb or Jackson RESTEasy extension enabled.");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
+ "Make sure you have either Jsonb or Jackson RESTEasy extension enabled.");
+ "Make sure you have either RESTEasy JSON-B or Jackson extension enabled.");

...ache/deployment/src/main/java/io/quarkus/rest/data/panache/deployment/RestDataProcessor.java Outdated
if (!hasJsonProcessor() && hasHalResource(index, resourceBuildItems)) {
logger.warn("Cannot generate HAL endpoints without a serializer. "
+ "Make sure you have either Jsonb or Jackson RESTEasy extension enabled.");
throw new RestDataBuildException("Cannot generate HAL endpoints without a serializer");
Copy link
Member

Choose a reason for hiding this comment

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

Either you log a warning or you throw an exception. In this case, I would just throw an exception with the proper message.

Also I don't think you need a specific exception for this. An IllegalStateException would be perfectly adequate IMHO.

@gytis gytis force-pushed the rest-data-fail-hal-without-serialiser branch from 7a90402 to 8e132c2 Compare August 7, 2020 08:42
@gytis
Copy link
Author

gytis commented Aug 7, 2020

Thanks for the tip on capabilities, @gsmet. It makes a much more sense and keeps the code much cleaner. I changed the commit to use RESTEASY_JSON capability, I think that should be sufficient.

@gsmet
Copy link
Member

gsmet commented Aug 7, 2020

Nope. That's why I was extremely clear in my comment. The RESTEASY_JSON capability is also produced for Vert.x JSON support and it won't work for you.

@gytis
Copy link
Author

gytis commented Aug 7, 2020

OK I'll add the new capabilities then.

@gytis
Copy link
Author

gytis commented Aug 10, 2020

@gsmet I've just noticed that there are two other capabilities: REST_JACKSON and REST_JSONB. They are produced by their respective server and client deployments both of which also bring in the required serialiser dependencies. I've just tried adding all of them one by one and serialisation worked correctly.
Of course, this could be a side effect which might go away in the future. I can still add new RESTEASY_JACKSON and RESTEASY_JSONB capabilities avoid that. But I just wanted to double check with you to avoid adding any unnecessary code.

@gsmet
Copy link
Member

gsmet commented Aug 20, 2020

@gytis as I mentioned it twice now, I did the digging and you need new capabilities.

@gytis gytis force-pushed the rest-data-fail-hal-without-serialiser branch from 8e132c2 to 19ba0d4 Compare August 20, 2020 13:46
@gytis
Copy link
Author

gytis commented Aug 20, 2020

Sorry for asking multiple times. I just wanted to avoid adding unnecessary code.
I've added the new capabilities as you've said.

@gytis gytis mentioned this pull request Sep 2, 2020
Closed
@gytis
Copy link
Author

gytis commented Oct 5, 2020

Is there something else missing in this PR?

@gytis gytis force-pushed the rest-data-fail-hal-without-serialiser branch from 19ba0d4 to fc66a0d Compare October 26, 2020 12:08
@geoand geoand requested a review from gsmet October 27, 2020 06:35
@gytis gytis force-pushed the rest-data-fail-hal-without-serialiser branch from fc66a0d to b71a894 Compare November 4, 2020 14:35
@gytis gytis force-pushed the rest-data-fail-hal-without-serialiser branch from b71a894 to 76ce627 Compare December 9, 2020 11:04
@gytis gytis force-pushed the rest-data-fail-hal-without-serialiser branch from 76ce627 to 6669cd4 Compare January 20, 2021 14:45
Base automatically changed from master to main March 12, 2021 15:54
@gytis gytis force-pushed the rest-data-fail-hal-without-serialiser branch from 6669cd4 to bda2e56 Compare March 15, 2021 11:43
@geoand geoand dismissed gsmet’s stale review March 15, 2021 11:52

Issues addressed

@geoand geoand added the triage/waiting-for-ci Ready to merge when CI successfully finishes label Mar 15, 2021
@geoand geoand merged commit 087b300 into quarkusio:main Mar 15, 2021
@quarkus-bot quarkus-bot bot added this to the 1.13 - main milestone Mar 15, 2021
@gytis gytis deleted the rest-data-fail-hal-without-serialiser branch March 16, 2021 10:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@geoand geoand geoand approved these changes

@gsmet gsmet Awaiting requested review from gsmet

Assignees
No one assigned
Labels
area/core area/panache area/resteasy triage/waiting-for-ci Ready to merge when CI successfully finishes
Projects
None yet
Milestone
1.13.0.CR1
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@gytis @gsmet @geoand