Error determining logic needs improvement

[radosavljevic]

Hi everyone!

First I'd like to thank you for creating and sharing this module, it's very useful. I've come across one bug so I'd like to share it.

Actual: When used with apollo-server and apollo rest data sources, logic doesn't determine the error correctly, it returns false even if it's an error.

Expected: isError should return true when response object has errors

Details:: Specifically this line of code should be changed

graphql2rest/src/errorHandling.js

Line 11 in fcd58e7

if (!response.data && response.errors) return true;

should be

if (response.errors && Array.isArray(response.errors) && response.errors.length > 0) {
   return true;
}

It's because the response that comes from RestDataSource has both error and data properties.

[roy-mor]

Hi @radosavljevic, thanks for using the module and for taking the time to open an issue.
It looks to me that what you're describing as an issue for improvement is expected behavior, by design.

As described in the documentation and in the comments in the code section you mentioned, we consider a GraphQL response to be a REST error only if there's a non-empty errors array, AND the data object is missing, null, or empty, or all its keys are null/undefined.

The rationale behind it is that while in REST HTTP a request can either succeed (2XX status) or fail (4XX or 5XX status), in GraphQL a request can partially succeed (or fail). This can happen, for example, when we get a full data object in the response but one field is null because the specific resolver for that field has failed, while all other fields have full, expected values. In this case an errors array will include the error details for that specific resolver/field. We would not want to classify this response as an error and fail the request with a 4XX or 5XX error code, denying the client of the data (when a client sees a non-2XX status code they usually ignore the data). This can perhaps be classified as a "warning", if there was ever a notion of warnings in REST (or GraphQL...).

This is especially common in the case of GraphQL2REST, where generated client queries are "fully exploded queries" including all possible fields, so it's not uncommon for one sometimes unused client field/resolver to fail, generating a completely usable response but also a non-null errors array. We would not want to fail the REST operation wrapping it in that case. So that's why I adopted that logic. In any case graphql2rest logs will show the original response from GraphQL including the errors array, for debugging purposes.

Also see this article for reference: https://itnext.io/the-definitive-guide-to-handling-graphql-errors-e0c58b52b5e1

I saw your fork changed this logic, which might be required for your purposes. In any case let me know if this is clear so I can close the issue as "not a bug". Thanks!!