bug: GraphQLClientException thrown by RestClientGraphQLClient doesn't use correct URL

[GFriedrich]

Expected behavior

The URL within the GraphQLClientException thrown by the RestClientGraphQLClient isn't showing any useful value.

Actual behavior

The URL should be the actual URL that was used to request the endpoint.

Steps to reproduce

Simply use the RestClientGraphQLClient and make the endpoint return a non 2xx response.

Reason

The RestClientGraphQLClient is using a toString() call at:

dgs-framework/graphql-dgs-client/src/main/kotlin/com/netflix/graphql/dgs/client/RestClientGraphQLClient.kt

Line 102 in 6450c31

url = restClient.toString(),

This of course doesn't return the expected URL, but some non-useful string instead.
I do understand that it is not possible to access the URL at this point from the RestClient but maybe either the user should be asked to hand in the original URL to the client (which then could be used on the restClient....retrieve() call too), or the exception shouldn't contain this property at all (or at least it should be nullable). Another solution (probably the most clean one) is to use the onStatus callback which allows some access to the previously used URI.

[paulbakker]

@GFriedrich Could you share how you were able to trigger this case? From what I'm testing now the RestClient itself actually throws an exception on anything other than a 200 status code, which implies that this error handling is effectively dead code.

Another thing we should consider is if we should catch those RestClient exceptions and wrap them in GraphQLClientException for consistency. Any thoughts about this?

[GFriedrich]

@paulbakker I was actually expecting that you've made the assumption that a user might use a RestClient that doesn't use the DefaultResponseErrorHandler of the RestClient but rather one that just returns. In this case you would end up there. But you're absolutely right that a "default" RestClient would never get there.

I've not actually got to this place, but I simply found the weird code when looking at the issue of #2060

When it comes to wrapping the exception: I've used the CustomGraphQLClient before which (in my case) used an executor which internally used a RestTemplate. And this was also directly throwing the exception as the CustomGraphQLClient was also not wrapping any exception coming from the executor.
So if you decide to wrap things, I think this would need to happen in a few more places and it would definitely be a breaking change to the interface.

[paulbakker]

Addressed in #2069.

[paulbakker]

Thanks for the quick response @GFriedrich!