Return correct HTTP status code on errors

[iBobik]

API returns 200 even in case of errors, so client needs to handle them differently (not just try catch).

I really am not proud of this code for such simple thing:

  try {
    const { result } = await GqlUserLogin({ email: email.value, password: password.value })
    if (result?.result?.token) { // Logged in successfully
      await $auth.setSession(result.result.token)
      await $auth.redirectBack()
    } else if (result?.error?.code) { // Request is ok, but login failed
      error.value = result.error.code
    }
  }
  catch (e) { // Request is not ok
    await $auth.updateSession()
    if ($auth.isLoggedIn.value) { // But user was already logged in
      await $auth.redirectBack()
    } else {
      error.value = 'unknown'
      console.error('Login failed', e)
    }
  }

Also all errors should be identifiable by some code, so we can easily handle them. E.g. in case of already logged in user it returns generic status code and human readable message only, so I had to do one more request to handle this case.

[iBobik]

Another example of unexpected behavior:

Status code is 200, but response is:

{
    "errors": [
        {
            "message": "Variable \"$merchant\" got invalid value { name: \"\", phone: \"rr\", addressStreet: \"\", addressCity: \"\", addressPostCode: \"\", addressCountry: \"\", taxNo: \"\", payoutBankAccount: \"dd\", active: false }; Field \"active\" is not defined by type \"MerchantUpdateInput\".",
            "locations": [
                {
                    "line": 2,
                    "column": 41
                }
            ]
        }
    ],
    "extensions": {
        "dbQueries": []
    }
}

But my code expect that errors are in the errorMessage fields because others are there.

[matej21]

Are you sure this returns 200? It is a graphql level error, which should be 400. There is even a test for that (for different errro, but on the same level). Which version are you using?

[matej21]

you were right, this case was not handled correctly, here is a PR with a fix, contember/contember#464

[honzasladek]

We're not sure if we should change it by default (as it's quite a big breaking change), currently we think this might be a better solution: contember/contember#398

Thoughts?

[iBobik]

I think there always be some surface for non-200 errors, so this will not solve it, so better would be to have config to all errors emit non 200 code.

[honzasladek]

@matej21 I think it's worth considering.

[iBobik]

Hi, I think this should not return 200:

mutation (
  $orderCode: String!,
  $offerID: UUID!,
  $data: OrderItemUpdateInput!,
) {
  updateOrderItem(
    by: { orderCode: $orderCode, offer: { id: $offerID } }
    data: $data
  ) {
    ok
    errorMessage
  }
}

{
  "data": {
    "updateOrderItem": {
      "ok": false,
      "orderItem": null,
      "errorMessage": "Execution has failed:\nunknown field: NotFoundOrDenied (for input {\"orderCode\":\"2023002914\",\"offer\":{\"id\":\"38a54a74-5eba-4a77-9bbc-cdd559b9b4c2\"}})"
    }
  },
...

[matej21]

Hi! It's actually expected to get a 200 OK even when something goes wrong in the mutation itself. In GraphQL, the 200 OK just means the server got and understood your request. The actual details about what went right or wrong in the mutation are in the response body, like the "ok": false part you saw. This is just how GraphQL usually does things, keeping the HTTP part simple and handling the specifics in the GraphQL layer.

[iBobik]

Aha, this makes sense. This mutation have not failed, just it's where part returned zero results.