RestClient processor with NullPointerException during scope definition

[danielsoro]

Before Quarkus 3.28, when we defined a wrong scope, Quarkus added a warning log to indicate that it wasn't possible to use the defined scope for the RestClient and initialize the Client with the default scope. With the current version, it is returning a NullPointerException, and no warning is thrown.

This change fixes the issue and adds a warning about it during the build.

[geoand]

Thanks for this!

It would be nice to have a test that would have failed with the existing version of the code, but that passes with this fix

[danielsoro]

Thanks for this!

It would be nice to have a test that would have failed with the existing version of the code, but that passes with this fix

@geoand Added test for it. Just to make sure that when we define some wrong-scope it shouldn't return a NPE, but should apply the default scope.

[github-actions]

🎊 PR Preview 021b167 has been successfully built and deployed to https://quarkus-pr-main-50398-preview.surge.sh/version/main/guides/

• Images of blog posts older than 3 months are not available.
• Newsletters older than 3 months are not available.

[geoand]

I'm actually wondering whether we should fail the build instead in these cases... WDYT @gsmet ?

[danielsoro]

I'm actually wondering whether we should fail the build instead in these cases... WDYT @gsmet ?

Any update about it?

[geoand]

We are at a conference this week so responses will be delayed 😉

[gsmet]

I'm actually wondering whether we should fail the build instead in these cases...

I'm on the verge of it. Warnings are easy to miss and this could have consequences so yeah maybe throwing an error is safer.

@danielsoro as a user, what's your take on this?

Maybe our CDI people will have a strong opinion about it? /cc @mkouba @Ladicek @manovotn

[Ladicek]

I'm not sure how the NPE looks like, but a NPE is usually not intentional, so that sounds like a bug.

I'm not sure if we "validate" a scope anywhere else in Quarkus (maybe we do, I just can't remember at the moment), but changing a scope in case the declared one is invalid seems like a bad idea. I'm +1 on turning that into an error.

[danielsoro]

Yes, in the past, a warning was present instead of an error, which is why I kept it the same way.

However, it makes sense to throw an exception; the feedback is much clearer.
I'll take a look at it later.

[mkouba]

+1 for a build failure.

[manovotn]

LGTM
Also 👍 for making it an error to declare wrong scope

[geoand]

Thanks!

[quarkus-bot]

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 4869013.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

[Ladicek]

From the added tests, I take it the RESTEasy Reactive client already failed with an exception if an invalid scope was configured? And the change here to RESTEasy classic "just" unifies the behavior? If so, even better! Thanks!

[quarkus-bot]

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 4869013.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.