Frontend: Handle auth errors in a type safe manner

The suggestion

A type-safe way to handle auth errors in a common place, and then let the calling component know that there was an error, but it's been handled...

The problem

In the GQLContext component - if we get an error from the API call, MOST of the time, we throw a GqlStandardError - however if it was an auth error (unauthenticated/unauthorized) we instead swallow the error. e.g.:

https://github.com/msupply-foundation/open-msupply/blob/01a8e81812c688597ed9e5a6de529b133f74aa47/client/packages/common/src/api/GqlContext.tsx#L63-L66

We do handle the error - we set an auth error in local storage, that's what results in the permission denied popup. The kicker is further down, at this line:

https://github.com/msupply-foundation/open-msupply/blob/01a8e81812c688597ed9e5a6de529b133f74aa47/client/packages/common/src/api/GqlContext.tsx#L143

Those empty returns instead of throwing after the auth errors above mean we get here, returning emptyData ({}) but PRETENDING ITS OF TYPE T (the query success type!)

NOW you want to do a mutation, e.g.

const apiResult = await api.deleteVaccineCourse({ vaccineCourseId });

Per the generated types, apiResult should be of type DeleteVaccineCourseMutation, which looks something like this - note, all are required/definitely available fields:

type DeleteVaccineCourseMutation = { 
  centralServer: {  
    vaccineCourse: { 
      deleteVaccineCourse: { id: string } 
    }
  }
};

SO you confidently access those definitely available fields:

const result = apiResult.centralServer.vaccineCourse.deleteVaccineCourse;

EXCEPT you didn't have the correct permission, so the GQL context handled the auth error, and then returned you an empty object! Your app crashes with a could not read property 'vaccineCourse of undefined error 😭

This can be worked around with some kind of if (!apiResult.centralServer) { ...handle error } logic... but this is easy to miss, or remove later, becuase per the types it shouldn't be needed!

Potential solution

@jmbrunskill and I had a little play - we think it might be better if we still throw after the auth error? That way at least the caller knows something went wrong.

Though, that does mean we require all API calls to be wrapped in a try catch block. I guess we should anyway, for the other standard errors, I just know that we don't have this everywhere... at least not consistently. Some are caught inside the useMutation, some in the calling component, some not at all?

Perhaps it could throw a particular kind of error, which we can check for in a catch block in the api calling code? Would be nice to have a common api error mapping method that can be used in the api calls, rather than having the logic like this:

Why should we invest time in this?

We should be able to rely on our types. I can see this being an easy regression bug - you come along and remove the superfluous nullable checks because the types say you don't need them!

Are there any risks associated with this change?

Don't want to introduce even more indirection in the api result/error handling. We already have the GqlContext, the QueryErrorHandler, the ErrorAlert, error handling in the api call... feels like it needs some considered design 👀

How much effort is required?

A day? More so on the design side than the implementation side I think, but estimate could change a lot depending on the agreed solution - if we need to change code in every API call that would be more work 😅

msupply-foundation / open-msupply