Allow to disable contextual error messages in reactive rest client #23171

TomasHofman · 2022-01-25T10:31:51Z

By default, contextual error messages are enabled.
Can be disabled by setting configuration property quarkus.rest-client.disable-contextual-error-messages=true, or a system property of the same name.

Fixes #22777.

gsmet

Added a small suggestion.

...rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java

...time/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java

TomasHofman · 2022-01-25T10:38:00Z

Hello @geoand and @michalszynkiewicz, would you review please?

.../runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java

quarkus-bot · 2022-01-25T10:56:19Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `ea67b5a`

Status	Name	Step	Failures	Logs	Raw logs
✖	Initial JDK 11 Build	`Build`	Failures	Logs	Raw logs

Failures

⚙️ Initial JDK 11 Build #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: docs extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment and 11 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

✖ Failed to execute goal net.revelc.code.formatter:formatter-maven-plugin:2.17.1:validate (default) on project quarkus-rest-client-reactive-deployment: File '/home/runner/work/quarkus/quarkus/extensions/resteasy-reactive/rest-client-reactive/deployment/src/test/java/io/quarkus/rest/client/reactive/ContextualErrorMessagesTest.java' has not been previously formatted. Please format file and commit before running validation!

geoand · 2022-01-25T15:48:53Z

...src/test/java/io/quarkus/rest/client/reactive/ContextualErrorMessagesSystemPropertyTest.java

+                            "application.properties"));
+
+    static {
+        System.setProperty("quarkus.rest-client.disable-contextual-error-messages", "true");


This is quite dangerous to be honest. Would it not work to have this in a @BeforeAll call and also unset or reset the property in an @AfterAll?

I will look at that.

Regarding "dangerous", do you mean the system property could leak into other tests or is it something else? Thanks :)

@BeforeAll doesn't work, it looks like config root is initialized sooner still. I will try to think about it more tomorrow, in the worst case we can remove this one test...

Yeah exactly, I'm worried about system property leaks

You can set the property in the test setup itself as you have done with other tests.

quarkus-bot · 2022-01-25T17:51:47Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `fd2be39`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Failures	Logs	Raw logs
✖	JVM Tests - JDK 17	`Build`	Failures	Logs	Raw logs
✖	MicroProfile TCKs Tests	`Verify`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment extensions/resteasy-reactive/rest-client-reactive-kotlin-serialization/deployment and 9 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

✖ io.quarkus.rest.client.reactive.ContextualErrorMessagesTest.errorMessageContainsContext line 35 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment extensions/resteasy-reactive/rest-client-reactive-kotlin-serialization/deployment and 9 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

✖ io.quarkus.rest.client.reactive.ContextualErrorMessagesTest.errorMessageContainsContext line 35 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

⚙️ JVM Tests - JDK 17 #

- Failing: extensions/resteasy-reactive/rest-client-reactive/deployment 
! Skipped: extensions/oidc-client-reactive-filter/deployment extensions/resteasy-reactive/rest-client-reactive-jackson/deployment extensions/resteasy-reactive/rest-client-reactive-kotlin-serialization/deployment and 9 more

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

✖ io.quarkus.rest.client.reactive.ContextualErrorMessagesTest.errorMessageContainsContext line 35 - More details - Source on GitHub

java.lang.AssertionError: 

Expecting actual:

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-rest-client-reactive

📦 tcks/microprofile-rest-client-reactive

✖ org.eclipse.microprofile.rest.client.tck.CallMultipleMappersTest.testCallsTwoProvidersWithTwoRegisteredProvider line 69 - More details - Source on GitHub

java.lang.AssertionError: The message should be sourced from class org.eclipse.microprofile.rest.client.tck.providers.TestResponseExceptionMapper expected [A 200 OK was received, but I'm throwing an exception] but found [Received: 'A 200 OK was received, but I'm throwing an exception' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

✖ org.eclipse.microprofile.rest.client.tck.DefaultExceptionMapperTest.testLowerPriorityMapperTakesPrecedenceFromDefault line 136 - More details - Source on GitHub

java.lang.AssertionError: class org.eclipse.microprofile.rest.client.tck.providers.LowerPriorityTestResponseExceptionMapper should be in the message expected [LowerPriorityTestResponseExceptionMapper] but found [Received: 'LowerPriorityTestResponseExceptionMapper' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

✖ org.eclipse.microprofile.rest.client.tck.ExceptionMapperTest.testWithOneRegisteredProvider line 75 - More details - Source on GitHub

java.lang.AssertionError: The message should be sourced from class org.eclipse.microprofile.rest.client.tck.providers.TestResponseExceptionMapper expected [A 200 OK was received, but I'm throwing an exception] but found [Received: 'A 200 OK was received, but I'm throwing an exception' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

✖ org.eclipse.microprofile.rest.client.tck.ExceptionMapperTest.testWithTwoRegisteredProviders line 105 - More details - Source on GitHub

java.lang.AssertionError: The message should be sourced from class org.eclipse.microprofile.rest.client.tck.providers.TestResponseExceptionMapper expected [A 200 OK was received, but I'm throwing an exception] but found [Received: 'A 200 OK was received, but I'm throwing an exception' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

quarkus-bot · 2022-01-26T12:15:23Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `d57a488`

Status	Name	Step	Failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Failures	Logs	Raw logs
✔️	JVM Tests - JDK 17
✖	MicroProfile TCKs Tests	`Verify`	Failures	Logs	Raw logs
✖	Native Tests - Data6	`Build`	Failures	Logs	Raw logs

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 #

- Failing: integration-tests/grpc-hibernate

📦 integration-tests/grpc-hibernate

✖ com.example.grpc.hibernate.BlockingRawTest.shouldAdd line 60 - More details - Source on GitHub

org.awaitility.core.ConditionTimeoutException: Condition with lambda expression in com.example.grpc.hibernate.BlockingRawTest that uses java.util.List was not fulfilled within 30 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:164)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)

⚙️ MicroProfile TCKs Tests #

- Failing: tcks/microprofile-rest-client-reactive

📦 tcks/microprofile-rest-client-reactive

✖ org.eclipse.microprofile.rest.client.tck.CallMultipleMappersTest.testCallsTwoProvidersWithTwoRegisteredProvider line 69 - More details - Source on GitHub

java.lang.AssertionError: The message should be sourced from class org.eclipse.microprofile.rest.client.tck.providers.TestResponseExceptionMapper expected [A 200 OK was received, but I'm throwing an exception] but found [Received: 'A 200 OK was received, but I'm throwing an exception' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

✖ org.eclipse.microprofile.rest.client.tck.DefaultExceptionMapperTest.testLowerPriorityMapperTakesPrecedenceFromDefault line 136 - More details - Source on GitHub

java.lang.AssertionError: class org.eclipse.microprofile.rest.client.tck.providers.LowerPriorityTestResponseExceptionMapper should be in the message expected [LowerPriorityTestResponseExceptionMapper] but found [Received: 'LowerPriorityTestResponseExceptionMapper' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

✖ org.eclipse.microprofile.rest.client.tck.ExceptionMapperTest.testWithOneRegisteredProvider line 75 - More details - Source on GitHub

java.lang.AssertionError: The message should be sourced from class org.eclipse.microprofile.rest.client.tck.providers.TestResponseExceptionMapper expected [A 200 OK was received, but I'm throwing an exception] but found [Received: 'A 200 OK was received, but I'm throwing an exception' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

✖ org.eclipse.microprofile.rest.client.tck.ExceptionMapperTest.testWithTwoRegisteredProviders line 105 - More details - Source on GitHub

java.lang.AssertionError: The message should be sourced from class org.eclipse.microprofile.rest.client.tck.providers.TestResponseExceptionMapper expected [A 200 OK was received, but I'm throwing an exception] but found [Received: 'A 200 OK was received, but I'm throwing an exception' when invoking: Rest Client method: 'org.eclipse.microprofile.rest.client.tck.interfaces.SimpleGetApi#executeGet']
	at org.testng.Assert.fail(Assert.java:96)
	at org.testng.Assert.failNotEquals(Assert.java:776)

⚙️ Native Tests - Data6 #

- Failing: integration-tests/hibernate-search-orm-elasticsearch-coordination-outbox-polling

📦 integration-tests/hibernate-search-orm-elasticsearch-coordination-outbox-polling

✖ Failed to execute goal io.fabric8:docker-maven-plugin:0.38.1:start (docker-start) on project quarkus-integration-test-hibernate-search-orm-elasticsearch-coordination-outbox-polling: I/O Error

geoand · 2022-01-28T07:26:20Z

The TCK test failures do seem related

TomasHofman · 2022-01-28T10:24:35Z

Yes, they do, sorry for delay, will get back to this soon.

geoand · 2022-01-28T10:58:29Z

No rush :)

TomasHofman · 2022-02-01T15:48:44Z

I'm still getting side tracked by different tasks, but I have this in my queue.

The issue is that current changes somehow cause that setting the quarkus.rest-client.disable-contextual-error-messages=true system property has no effect in the TCK tests, and the feature remain disabled, as if the property is false.

I couldn't figure out why the property is not seen, and I can't figure out how to debug RestClientCDIDelegateBuilder#configureCustomProperties method in the TCK tests (they are arquillian tests, and breakpoints in that method just don't catch).

Anyway, going to investigate further soon.

* By default, contextual error messages are enabled. * Can be disabled by setting configuration property "quarkus.rest-client.disable-contextual-error-messages=true", or a system property of the same name.

TomasHofman · 2022-02-02T14:02:08Z

So, situation is quite simple after all. We need to keep the system property check in RestClientRequestContext.java (https://github.com/quarkusio/quarkus/pull/23171/files#diff-d4ff9fcb3ceededf539684304aa23d72f0550f593610eeeb34cfccdc86f6de08), because the TCK tests create the clients via builder (obviously), therefore anything we do with config roots will be bypassed. Also we have no option (I guess) to modify the test code itself. I updated the PR accordingly.

geoand · 2022-02-02T14:32:24Z

Gotcha, thanks!

quarkus-bot bot added the area/rest label Jan 25, 2022

gsmet reviewed Jan 25, 2022

View reviewed changes

...rest-client-config/runtime/src/main/java/io/quarkus/restclient/config/RestClientsConfig.java Outdated Show resolved Hide resolved

...time/src/main/java/io/quarkus/rest/client/reactive/runtime/RestClientCDIDelegateBuilder.java Outdated Show resolved Hide resolved

TomasHofman force-pushed the disabling-contextual-error-messages branch from 80bc6c8 to ea67b5a Compare January 25, 2022 10:40

geoand reviewed Jan 25, 2022

View reviewed changes

.../runtime/src/main/java/org/jboss/resteasy/reactive/client/impl/RestClientRequestContext.java Show resolved Hide resolved

TomasHofman force-pushed the disabling-contextual-error-messages branch from ea67b5a to fd2be39 Compare January 25, 2022 15:22

geoand reviewed Jan 25, 2022

View reviewed changes

TomasHofman force-pushed the disabling-contextual-error-messages branch from fd2be39 to d57a488 Compare January 26, 2022 09:44

Allow to disable contextual error messages in reactive rest client

59a2e2b

* By default, contextual error messages are enabled. * Can be disabled by setting configuration property "quarkus.rest-client.disable-contextual-error-messages=true", or a system property of the same name.

TomasHofman force-pushed the disabling-contextual-error-messages branch from d57a488 to 59a2e2b Compare February 2, 2022 12:02

geoand approved these changes Feb 2, 2022

View reviewed changes

geoand merged commit b42be6d into quarkusio:main Feb 2, 2022

quarkus-bot bot added this to the 2.8 - main milestone Feb 2, 2022

Allow to disable contextual error messages in reactive rest client #23171

Allow to disable contextual error messages in reactive rest client #23171

Conversation

TomasHofman commented Jan 25, 2022

gsmet left a comment

Choose a reason for hiding this comment

TomasHofman commented Jan 25, 2022

quarkus-bot bot commented Jan 25, 2022 • edited Loading

Failing Jobs - Building ea67b5a

Failures

⚙️ Initial JDK 11 Build #

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

geoand Jan 25, 2022

Choose a reason for hiding this comment

TomasHofman Jan 25, 2022

Choose a reason for hiding this comment

geoand Jan 25, 2022

Choose a reason for hiding this comment

TomasHofman Jan 25, 2022

Choose a reason for hiding this comment

TomasHofman Jan 25, 2022

Choose a reason for hiding this comment

geoand Jan 25, 2022

Choose a reason for hiding this comment

geoand Jan 25, 2022 • edited Loading

Choose a reason for hiding this comment

quarkus-bot bot commented Jan 25, 2022 • edited Loading

Failing Jobs - Building fd2be39

Failures

⚙️ JVM Tests - JDK 11 #

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

⚙️ JVM Tests - JDK 17 #

📦 extensions/resteasy-reactive/rest-client-reactive/deployment

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-rest-client-reactive

quarkus-bot bot commented Jan 26, 2022 • edited Loading

Failing Jobs - Building d57a488

Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/grpc-hibernate

⚙️ MicroProfile TCKs Tests #

📦 tcks/microprofile-rest-client-reactive

⚙️ Native Tests - Data6 #

📦 integration-tests/hibernate-search-orm-elasticsearch-coordination-outbox-polling

geoand commented Jan 28, 2022

TomasHofman commented Jan 28, 2022

geoand commented Jan 28, 2022

TomasHofman commented Feb 1, 2022

TomasHofman commented Feb 2, 2022

geoand commented Feb 2, 2022

quarkus-bot bot commented Jan 25, 2022 •

edited

Loading

Failing Jobs - Building `ea67b5a`

geoand Jan 25, 2022 •

edited

Loading

quarkus-bot bot commented Jan 25, 2022 •

edited

Loading

Failing Jobs - Building `fd2be39`

quarkus-bot bot commented Jan 26, 2022 •

edited

Loading

Failing Jobs - Building `d57a488`