[#1965] Filter out the null elements from the list of errors in Graph…

[api-from-the-ion]

…QLClientException

[jmartisk]

I agree this kinda makes sense, but I'm not completely sure I understand the point - is there a real possibility that, with a regular usage of the GraphQL client, the library tries to create a GraphQLClientException where one of the errors is null? Has that happened to you (except cases where you construct the exception directly)?

[jmartisk]

And for consistency, if the variant that takes a single GraphQLError throws an exception on a null error, shouldn't the other one (that takes a list of errors) also throw an error instead of ignoring the null value? (Just thinking... in any case it would be better if we avoided the possibility of receiving any null errors here altogether)

[api-from-the-ion]

For explanation, how we get here. We are writing some test for our service, that use other services (noting strange here). And we try to test the reaction of our service in the case that foreign service call have no result but an error. And here we're creating our own exception's instance of this class for the mock, for example.

Now I see our test is failing, but not the way that we like. It fails not because of reaction of our code on the error, but because logging or printing of the exception or something with toString() of the exception, caused another exception.

So, back to your questions: I think that this is possible in some business case, but would be unusual. But in any case, especially in testing, I would like to have an error (an exception in our case) as early as possible. Not in toString() call, but in the call of the constructor. Then I would immediately know that something went wrong.

To your other question: you are completely right. If we have an exception in one case, we should have an exception in other case also. Or no exception at all in any of these cases. Should I change nonNull() to requireNonNull()?

[jmartisk]

Yeah I think that we should throw an exception if any of the errors is null. If there's a null error in the list, something probably went really wrong, because that's normal at all, and we shouldn't silently ignore it.

[api-from-the-ion]

So, I made the change and it now in the PR. I had the failure in the Quarkus tests, but I re-run them and now it's over. Looked like a temporary trouble with Quarkus image itself.

I thought once again about the whole situation. I already described what does it mean in the test environment. How can we get here in the live environment? We have a response where: error is null, or errors are null or some of the errors in the list are null. The attribute isn't missing, but null. Is this normal? I don't think so. The information should be here, but absent. So it would be OK even in a live system to have an exception because something is wrong with an error's structure. And this directly on the JSON mapping of the error.

[jmartisk]

Thanks for your contribution!