SmallRye GraphQL 1.2.8 and client configuration fixes

[jmartisk]

Fixes #18457

[gsmet]

@jmartisk for my understanding, how does it fix the native issue?

[jmartisk]

I'm just finding out I don't know that either. Hold on, might still need some adjustments

[quarkus-bot]

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building f35b85f

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Gradle Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	Gradle Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	JVM Tests - JDK 16	Build	⚠️ Check →	Logs	Raw logs
⌛	Maven Tests - JDK 11	Build	⚠️ Check →	Logs	Raw logs
⌛	Maven Tests - JDK 11 Windows	Build	⚠️ Check →	Logs	Raw logs
⌛	MicroProfile TCKs Tests	Verify	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Amazon		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Cache		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data1	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data2	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data3	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data4	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data5	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data6	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Data7	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - HTTP		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Main	Build	⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Messaging2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Misc4		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security1		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security2		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Security3		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Spring		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - Windows - hibernate-validator		⚠️ Check →	Logs	Raw logs
⌛	Native Tests - gRPC		⚠️ Check →	Logs	Raw logs

[jmartisk]

Ok so in the end the previous fix was good, but I've improved the integration tests to be sure.

The first problem was caused by a bad regular expression that did not expect that a configured client name can contain dashes. Now it should accept dashes, numbers and underscores in addition to the English alphabet.

The second problem (with native mode) is because we were depending on TCCL as a key for storing the client configuration map in a static map (this is to be able to support app servers with potentially multiple applications). But in native mode, this becomes a problem because at build-time, the class loader is different than the one at runtime. I changed that so that in Quarkus, we assume there is only one application, so no need to distinguish using the TCCL. The reason why native mode tests in quarkus core didn't catch this is that there was a fallback - if the configuration-storing bean is broken or inaccessible, we just ignored it. And that worked in the integration-tests module, because there was no client configuration in application.properties (I now added a test that actually needs some config from there to be able to pass properly).

Generally I had to rework the integration tests quite a bit, because now that the client depends more heavily on CDI (in most cases it's necessary), we hit the problem that CDI does not work in native mode tests on the "client" side (from the @Test method). So I changed the tests in the way that the @Test method calls a REST endpoint, which then performs all the GraphQL-client-related work - CDI is available then.

I also fixed one minor problem where the client's url was considered required by Quarkus, but it should not - it should be possible to define the URL dynamically using the client builder.

[quarkus-bot]

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building f35b85f

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build	Set up JDK 11	⚠️ Check →	Logs	Raw logs
⌛	CI Sanity Check	Build	⚠️ Check →	Logs	Raw logs

[quarkus-bot]

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

🚫 This workflow run has been cancelled.

Failing Jobs - Building 7924966

⚠️ Artifacts of the workflow run were not available thus the report misses some details.

Status	Name	Step	Test failures	Logs	Raw logs
⌛	Initial JDK 11 Build		⚠️ Check →	Logs	Raw logs
⌛	Attach pull request number		⚠️ Check →	Logs	Raw logs
⌛	CI Sanity Check		⚠️ Check →	Logs	Raw logs

[jmartisk]

A pair of eyes to try the quickstart with this change would be welcome. I tested it, but, you know...

[gsmet]

Hmmm, I don't know if it's normal but before if the URL was not configured, I had a nice (but misleading as reported) error message.

Now I get:

Caused by: java.lang.NullPointerException
	at java.base/java.util.Objects.requireNonNull(Objects.java:221)
	at org.jboss.resteasy.reactive.client.impl.ClientImpl.target(ClientImpl.java:150)
	at io.smallrye.graphql.client.typesafe.jaxrs.JaxRsTypesafeGraphQLClientBuilder.build(JaxRsTypesafeGraphQLClientBuilder.java:81)
	at io.quarkus.smallrye.graphql.client.runtime.SmallRyeGraphQLClientRecorder.lambda$typesafeClientSupplier$0(SmallRyeGraphQLClientRecorder.java:20)

I don't think this is an improvement :).

To reproduce, just comment the first URL in the quickstart application.properties.

[jmartisk]

Hey, in the first case you were referring to the dynamic client, this example is for a typesafe client. Yeah, they behave a bit differently, which is probably also bad.
I filed smallrye/smallrye-graphql#903 to unify and improve, but it will need a new SmallRye release.

[gsmet]

@jmartisk OK so I also commented the other one and I get:

SRGQLDC035001: URL not configured for client named 'star-wars-dynamic'. Please define the property smallrye.graphql.client.CLIENT_NAME/url

and if I define:

smallrye.graphql.client.star-wars-dynamic/url=https://swapi-graphql.netlify.app/.netlify/functions/index

as recommended by the message, I still get the error as I initially reported it.

Did I miss something?

[jmartisk]

Oh fuck me, I realize the error message is wrong. It should be CLIENT_NAME/mp-graphql/url, not smallrye.graphql.client.CLIENT_NAME/url (I changed that at one point in the implementation but forgot to correspondingly update the error message). Thanks for noticing. Will fix that within smallrye/smallrye-graphql#903

[gsmet]

@jmartisk and in an ideal world, it would be even cooler if we could recommend the Quarkus version of the property :).

[gsmet]

OK, let's merge this, it fixes the native issue, which is good. I'll let @jmartisk iterate on the rest of the issues.