morph adapter errors shouldn't fail the query if there are also valid results

[balmas]

reported my @monzug . Reproducible with a lookup of the Latin word 'caro'

For this word, the test on if (adapterMorphRes.errors.length === 0) { at https://github.com/alpheios-project/alpheios-core/blob/master/packages/components/src/data-model/word-query/lexical-data/data-objects/tufts-morphology-data.js#L50 returns false because there are some errors in the morph response but there are also valid lexemes.

We should not entirely fail the query in this case, but proceed with the valid data results.

[monzug]

other words that are failing: nero, qui, quibuscum (also double-click), quis and many more

[kirlat]

@balmas, @irina060981: To handle situations like that gracefully we should probably start to use warnings along with errors in our GraphQL API. I think those could have the following meanings:

• Warning. Something went wrong, but the error is not critical and we can continue the normal execution despite what happened. We should probably take notice, maybe report the issue in the logs, and maybe tweak an execution path a little bit to accommodate for the issue.
• Error. The problem is so serious that we cannot continue the normal execution under any circumstances and need to bail out. No morphological results will be shown to the user and, most likely, a user will be presented with the error message in the UI that will explain the situation somehow.

What do you think about an approach like the above? If it seems reasonable, I can add it into the GraphQL interface and, possibly, to other places.

[balmas]

This makes sense to me.

[irina060981]

I like this suggestion too

[monzug]

Kirlat, Question: in the case of "caro" a warning will be issued. for the user, the warning is totally transparent, correct?

[kirlat]

Kirlat, Question: in the case of "caro" a warning will be issued. for the user, the warning is totally transparent, correct?

Yes, the warning will be used internally within the system but will not be shown to the user. Users will see no special messages in this case. The UX of the user would be exactly the same as in cases where things will go smoothly and there will be no warnings at all.

The user will be notified only of errors that prevent morphological data from being displayed. Such notifications would explain to the user why no data for the user's request was retrieved.

[monzug]

Thanks!!!

[kirlat]

The ultimate source of truth for error handling is in GraphQL specifications: http://spec.graphql.org/draft/#sec-Errors. Sometimes Apollo error response matches it (https://www.apollographql.com/docs/apollo-server/data/errors/), sometimes, it seems, doesn't: https://www.apollographql.com/blog/full-stack-error-handling-with-graphql-apollo-5c12da407210/ The last source is, however, from 2018, when the GraphQL docs did not have the errors specs published, so maybe things have changed since then.

I think we should comply with GraphQL specs first and foremost and only then think about following the Apollo practices: we might use GraphQL solutions other than Apollo and interoperability is a must.

Based on what I've read and know so far I suggest the following approach:

a. If the query completed successfully and no errors occurred during its execution, the response will have a homonyms array and no errors field:

{
  "data": {
    "homonyms": [
      { ... homonym one },
      { ... homonym two }
    ]
  }
}

b. If the query succeeded partially and there were some errors during its executions, the response will have both a homonyms and an errors arrays. The homonyms array will hold whatever partial results are available and the errors would hold errors and warnings presented as errors because GraphQL has no notion of a warning:

{
  "errors": [
    { ... error one},
    { ... error two}
  ],
  "data": {
    "homonyms": [
      { ... homonym one },
      { ... homonym two }
    ]
  }
}

c. If the query failed completely and no homonym info can be returned, then the response will contain a homonym field that will have a value of null and the errors array containing at least one error object with the information about the error:

{
  "errors": [
    { ... error one},
    { ... error two}
  ],
  "data": {
    "homonyms": null
  }
}

Then on the client side, we should check if the homonyms side is null or not. If it is null, we should assume that the request failed completely and check for the error info in the errors array. In that case we should stop processing the lexical query and show an error message to the user.

If homonyms is an array (i.e. at least partial results are available), we should use the data from the homonyms to render morphological output in a popup and take notice of any errors in the errors array if the latter are present.

Regarding the format of the error object, there are no warnings in GraphQL. It can have errors only. Also, names of its top-level fields (message, locations, path) should match the specs and thus we can't add our own fields to the top level of the error object. But we can use an extensions field to supply our own error information. To distinguish between an error and a warning we can introduces a severity field that have a value of either error or warning:

{
    "message": "An error message",
    "locations": [],
    "path": [
        "queryFieldThatCausedTheErrorToOccur"
     ],
     "extensions": {
         // Fields from our Error object
         "severity": "warning", // The other value can be "error"
         "errCode": "TheErrorCode", // The warning code, in this case
         "origin": "ClientAdapters", // In what part of our application did the error occurred
         // Two next fields are specific for the origin listed, the client adapters
         "adapter": "AdapterName",
         "methodName" "TheMethodName"
     }
}

@balmas, @irina060981: does all of the above make sense to you?

[irina060981]

I believe it is relly very similiar to the way client-adapter returns a morphological result. With the only difference - it has a more general field - result, because the result could be not only the homonym. And an errors array has no severity - because a client-adapter is separated from other repositories by business logic. And it couldn't now what errors are important or not for the application that uses it for retrieving the data.

Do you suggest to change the response on client-adapter's side or duplicate it with described difference on the components side?

[balmas]

I agree it is very similar to the way it is currently handled with the client adapters. The proposed error extensions structure looks fine to me. One thing to consider here is whether it adds any value to pass a non-critical warning on to the QraphQL API in this case. It has no value for the end user since we never show them the error and if we did they couldn't do anything about it. However it is very helpful for debugging. That's the only reason to include it IMHO.

[kirlat]

I don't think it would make sense to change client adapters to match GraphQL specification exactly. Those are different domains, and we might use something different along with GraphQL as well. Different formats should be able to coexist peacefully.

It is a very good point that the client adapters may not know what error is critical for the client app and what is not. But I think from client adapter's point of view an error is something that prevents results from being obtained: a network error, an incorrect parameter or a bad parameter set. If client adapters experienced an error it should return no results at all. The warning, on the other hand, might be an indication that the data returned from client adapters might be incomplete (partial) due to some non-critical error conditions. But the data could still have value to the client. A warning would notify the client that the data is incomplete and it might do an extra effort to retrieve some additional data somewhere, or let the user know that parts of data might be missing.

So, considering the above, I think it would probably be good to change the client adapters to support warnings. What do you think about that?

[balmas]

So, considering the above, I think it would probably be good to change the client adapters to support warnings. What do you think about that?

Yes I agree

[irina060981]

So, considering the above, I think it would probably be good to change the client adapters to support warnings. What do you think about that?

As we talked on the meeting it could be useful, but I think that such warnings could be catched in the following cases:

• they could be retrieved from the remote service, but for now I think that we have no service that supports warnings
• they could be retrieved from some conversion steps, and such warnings could be produced from data-models methods;

This way I think that such warnings would appear mostly from user's issues, because they are needed to have more specific knowledge.

[kirlat]

This should be fixed by #568. The changes have been merged to master.