Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: add ability to parse empty response #4865

Closed
wants to merge 6 commits into from
Closed

fix: add ability to parse empty response #4865

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f74db49
Add ability to parse empty response
shaypep Jul 1, 2023
e454198
Merge branch 'main' into allow-empty-response-in-schema
shayff Jul 1, 2023
ea96124
Fix
shaypep Jul 1, 2023
8c1451f
Merge branch 'allow-empty-response-in-schema' of https://github.com/s…
shaypep Jul 1, 2023
06e8531
add missing code
shaypep Jul 1, 2023
03f32aa
Added @giuliowaitforitdavide test case
shaypep Jul 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions lib/schemas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@ function normalizeSchema (routeSchemas, serverOptions) {
}

if (!hasContentMultipleContentTypes) {
routeSchemas.response[code] = getSchemaAnyway(routeSchemas.response[code], serverOptions.jsonShorthand)
routeSchemas.response[code] = getSchemaAnyway(routeSchemas.response[code], serverOptions.jsonShorthand, code)
}
}
}
Expand All @@ -129,9 +129,9 @@ function generateFluentSchema (schema) {
}
}

function getSchemaAnyway (schema, jsonShorthand) {
function getSchemaAnyway (schema, jsonShorthand, statusCode) {
if (!jsonShorthand || schema.$ref || schema.oneOf || schema.allOf || schema.anyOf || schema.$merge || schema.$patch) return schema
if (!schema.type && !schema.properties) {
if (!schema.type && (!schema.properties && statusCode !== '204')) {
Copy link
Contributor

@giuliowaitforitdavide giuliowaitforitdavide Jul 1, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return {
type: 'object',
properties: schema
Expand Down
22 changes: 22 additions & 0 deletions test/internals/validation.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,28 @@ test('build schema - payload schema', t => {
t.equal(typeof opts[symbols.bodySchema], 'function')
})

test('build schema - 204 empty response', t => {
t.plan(1)
const serverConfig = {
jsonShorthand: true
}
const opts = {
schema: {
response: {
204: {
description: 'No Content'
}
}
}
}
opts.schema = normalizeSchema(opts.schema, serverConfig)
t.same(opts.schema.response, {
204: {
description: 'No Content'
}
})
})

test('build schema - avoid repeated normalize schema', t => {
t.plan(3)
const serverConfig = {
Expand Down