Document Default Error Responses

[muya]

Include documentation for 500 & 503 responses in default schema definition

This ensures that they get included in the Open API documentation generated.

• chore: update tests to match updated responses

Demo / Screenshots

Before	After

Notes to Reviewer(s)

• Addition of docs for other responses make for more complete API documentation when using the healthcheck feature
• Open to suggestions on wording of descriptions
• Try out the updated documentation version here: muya/fastify-under-pressure-update-demo

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[mcollina]

lgtm

[Uzlopak]

this seems odd. statuscode will always be HTTP respone code.

[muya]

@Uzlopak This is updated ✅

[Uzlopak]

I kind of doubt that this so that much correct. It is good to add the 500 and 503 status codes to the schema. But the errors are thrown so the default error handler takes as we did not specify setErrorHandler for this plugin.

So if somebody changes the default errorHandler than the schema is not correct as the error handler will be generating a different response.

So we have to add an error Handler with setErrorHandler for this plugin, to ensure that the api contract is ensured even if the defaultErrorHandler is changed.

[Uzlopak]

@Eomm
Do you concur?

[muya]

I kind of doubt that this so that much correct. It is good to add the 500 and 503 status codes to the schema. But the errors are thrown so the default error handler takes as we did not specify setErrorHandler for this plugin.

So if somebody changes the default errorHandler than the schema is not correct as the error handler will be generating a different response.

So we have to add an error Handler with setErrorHandler for this plugin, to ensure that the api contract is ensured even if the defaultErrorHandler is changed.

Hi @Uzlopak, thank you for taking a look.

I agree that this implementation only allows for the default responses.

However, I think that this is okay, since an API spec is meant to define the "schema", and not the actual response that will be returned.
For example, if an API returns 10 different variations of a message, then we don't have to document all the 10 responses, we only have to define the "structure".

Since there's currently no support for defining a custom error handler, the API is always returning the same payload in case of 500 & 503 errors.

If we decide to add the error handler, we would then also update the schema definition to accept a custom payload (i.e. in the same way we're combining the custom success response in the 200 OK response).

What do you think?

[Uzlopak]

But afaik specifiying a plugin specific errorhandler is possible by using the setErrorHandler method. So we could specify a custom errorhandler and uphold the api contract even if the default error handler is modified.

[Uzlopak]

Oh, I get it now. If you define another error handler, then the dev should specify the schema by himself?

[Uzlopak]

I had a second look at it seems we have bugs in that part, before you even touched it.

under-pressure/index.js

Line 107 in 26677ab

schema: Object.assign({}, opts.exposeStatusRoute.routeSchemaOpts, {

When opts.exposeStatusRoute.routeSchemaOpts is in Object.assign before our changes, than it means we can not overwrite the error statuscodes.

The same here:

under-pressure/index.js

Line 113 in 26677ab

opts.exposeStatusRoute.routeResponseSchemaOpts

We could set a custom error handler like this:

    fastify.setErrorHandler((err, request, reply, done) => {
      reply.status(err.statusCode)
        .send({
          code: err.code,
          message: err.message
        })
      done()
    })

a potential unit test would be:

test('check error handler', async t => {
  t.plan(1)
  const fastify = Fastify({ exposeStatusRoute: true })

  fastify.register(underPressure, {
    healthCheck: () => {
      throw new Error('Arbitrary Error')
    },    
    exposeStatusRoute: {
      routeOpts: {
        logLevel: 'silent'
      }
    }
  })

  t.equal((await fastify.inject().get('/').end()).body, '{"code":"FST_UNDER_PRESSURE","message":"Service Unavailable"}')
})

[Uzlopak]

@mcollina
What should we do? Merge and open a new issue? Or should we request to fix also this bug?

[mcollina]

If what is in this pr is ok, let's land it and then you can open a fresh issue with the fix.