fix: add ability to parse empty response

[shayff]

Add the ability to have a response schema with no properties for HTTP code 204

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

[shayff]

@mcollina @Eomm I'm not sure it's the best way to fix it but I would like to hear your though, also I couldn't link this PR to the issue, can you do it?
#4833

[shayff]

One more thing, should we also address 304?

[mcollina]

lgtm

[mcollina]

304?

[Uzlopak]

Imho i disagree.

It should not determine by status code

[shayff]

Imho i disagree.

It should not determine by status code

What do you suggest? as far as I understand only in 204 there is no response and we should allow properties to be empty, so it somehow how to determine by the status code

[Eomm]

I wrote here my idea: #4864 (comment)

2 PRs at the same time 😅

Maybe let's try to sync with @giuliowaitforitdavide in order to work together on the same PR

[giuliowaitforitdavide]

Add this test inside test/internals/reply.test.js

test('reply should allow empty responses', t => {
  t.plan(3)
  const fastify = require('../..')()
  fastify.route({
    method: 'GET',
    url: '/',
    schema: {
      response: {
        204: {
          description: 'Session Deleted'
        }
      }
    },
    handler: (_, reply) => {
      reply.code(204).send()
    }
  })

  fastify.inject({
    method: 'GET',
    url: '/'
  }, (err, res) => {
    t.error(err)
    t.equal(res.statusCode, 204)
    t.equal(res.payload, '')
  })
})

[giuliowaitforitdavide]

Here we can consider the structure of the JSONSchemaDraft7

On my PR I was working on something like considering the keys

const draft7Keys = ["$comment", "$id", "$ref", "$schema", "additionalItems", "additionalProperties", "allOf", "anyOf", "const", "contains", "contentEncoding", "contentMediaType", "default", "definitions", "dependencies", "description", "else", "enum", "examples", "exclusiveMaximum", "exclusiveMinimum", "format", "if", "items", "maxItems", "maxLength", "maxProperties", "maximum", "minItems", "minLength", "minProperties", "minimum", "multipleOf", "not", "oneOf", "pattern", "patternProperties", "properties", "propertyNames", "readOnly", "required", "then", "title", "type", "uniqueItems"] 

This could be an idea but I don't know if others agree with me. @Uzlopak what do you think about it?

[giuliowaitforitdavide]

@Eomm the main thing about your suggestion is that I'm missing the point when we pass from the getSchemaSerializer function in case of empty schemas

[shayff]

@giuliowaitforitdavide Thanks!
@Eomm I read your suggestion, as I understand we call getSchemaSerializer on income request, the problem here is that we fail to start the server.

[Eomm]

I will check later - I think we run that function (or similar) during the startup to compile the serialize function.

[Eomm]

Found, here is where the magic happen:

fastify/lib/validation.js

Line 23 in 5b7fd18

context[responseSchema] = Object.keys(context.schema.response)

if we skip adding a function to the acc(accumulator) the response payload will not be blocked

I would add a test with a response schema with a typo on the schema, such as propppertie

[shayff]

@Eomm Thanks, i'll check that later.
But do we want to skip the whole schema check?

[Uzlopak]

E.g. 202 DELETED is also empty response. Also 200 can be empty response.

[Uzlopak]

THe documentation of openapi says in https://swagger.io/docs/specification/describing-responses/

Some responses, such as 204 No Content, have no body. To indicate the response body is empty, do not specify a content for the response:

"such as" means, that it is open for more than 204. So every statuscode which has no content attribute is considered an empty response. So I could even do

      responses:
        '200':
          description: The resource was processed successfully.

Ok, we have I guess an issue in fastify, because we can assign directly the content schema to the status code so we have to somehow ducktype if the schema provided can be a content schema. Thats bad, but that is imho the way to go.

[giuliowaitforitdavide]

LGTM test cases, need to work on the statusCode part

[Fdawgs]

Would this count as a fix or a feature?

[mcollina]

@Eomm can this ship?

[Uzlopak]

@mcollina

The question is, which of the solutions we should choose. This or #4871

[Eomm]

I don't have time to work on this before the next week.

I think we must not rush on this issue and take a bit of time

The @shayff question about my review is good: if we skip the serialize function, should we set a default serializer function that forces and empty body or should we just "trust" that the user will not return anything potentially breaking the schema contract?

Anyway I was thinking that maybe this issue may disappear if the user set a "type: null" beside the "description" property - did not have time to try it

In general I think if the user set and empty schema, the application/json content type must not be assumed, so the fix could be somewhere else here in the code.

Need to get few hours to play a bit, but as said I can't before the next week - summer is a busy season 😂

[Eomm]

Blocking per #4833 (comment)

[Eomm]

Closing due #4833 (comment)

Thanks for the effort - there are other good-first-issue on our issue list 🙏