-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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
GraphQL client: "endpoint" parameter in "@GraphQLClientApi" has precedence over "application.properties" #44334
Comments
/cc @jmartisk (graphql), @phillip-kruger (graphql) |
@jmartisk If you point me to the location of the integration test in quarkus, I could try to add a unit test to validate my observations (and if the expectation is that "application.properties" should win, then this test should fail) |
@jmini a good similar test to get started from would be, for example, https://github.com/quarkusio/quarkus/blob/main/extensions/smallrye-graphql-client/deployment/src/test/java/io/quarkus/smallrye/graphql/client/deployment/DynamicGraphQLClientInjectionWithQuarkusConfigTest.java - you will just need to make it use a different Client API interface - one that specifies a hardcoded URL |
@jmartisk I think my test pushed here: Test result from the CI
Source: https://github.com/jmini/quarkus/actions/runs/11701187729/job/32587425640 Any idea what I should investigate for the fix? Maybe the place where the configs are merged? |
Thanks @jmini !! @mskacelik has already started looking into it. It's a bit tricky, but I think we will figure it out |
Then maybe we can integrate my test after the SmallRye GraphQL client with the fix has been released and updated in Quarkus. |
Fixed via smallrye/smallrye-graphql#2223, so it will be included in Quarkus 3.17 (probably) @jmini do you want to submit a test yourself (you'd need to change your commit with the test to add a new test instead of changing an existing one)? The fix is not in Quarkus yet, but if you want to do it now, I can start a new Quarkus branch that will serve for capturing changes related to SmallRye GraphQL 2.12, and you could send a PR to that branch. Or we can tell @mskacelik to take care of it, whatever you prefer. |
Don't you need a release of SmallRye GraphQL I am happy to contribute my test, I will split the test as required and open a draft MR because it is easier to discuss on code changes. (the draft PR can remain open until there is a SmallRye release) From my side the release and the quarkus update is not urgent, the work-around of removing the |
We usually do it the way that I maintain a custom branch of Quarkus that captures the changes related to the next smallrye-graphql upgrade, and the branch depends on a snapshot of the upcoming release version. But you can as well just send a draft PR straight to Quarkus (without actually changing the dependency version, so the test will just fail for now) and I will make sure to merge it after the smallrye-graphql upgrade. That will most likely be easier in this case |
If you already created that branch, I can target this with my PR and then the test should be green. |
That won't work like this OOTB because the Quarkus CI doesn't know how to build a snapshot of smallrye-graphql, and we don't deploy snapshots into public Maven repositories. |
This was my first proposal. 😉
I don't have that setup on my machine. But maybe you can try my branch on your machine locally. Otherwise we hope that the change in jmini/gitlab-experiments@a79a36c is good, but we will only be sure after the smallrye release. |
Ok I'll do a test run with jmini@26d61f6 and let you know |
Yeah it worked, just feel free to send a draft PR with jmini@26d61f6 and say in the description that it requires an upgrade SmallRye GraphQL 2.11.1 or 2.12.0, and I'll take care of it |
See #44441 |
From comment #44293 (comment):
When testing the GraphQL client I discovered that the endpoint defined in the annotation:
Has precedence over what is defined in the application properties:
Example commit to "move" the config property:
jmini/gitlab-experiments@a79a36c
According to @jmartisk it should be the other way around.
Note that the settings made in TypesafeGraphQLClientBuilder wins over what is defined in the application properties, but I think that this is correct:
The text was updated successfully, but these errors were encountered: