fastify / fast-json-stringify

2x faster than JSON.stringify()
MIT License
3.52k stars 208 forks source link

Validation issue with enum #253

Closed JSteunou closed 4 years ago

JSteunou commented 4 years ago

🐛 Bug Report

Fastify through an error if the validation schema contains an enum without enforced type.

,{"type":"object","properties":{"monitor":{"type":"object","properties":{"status":{"enum":["ok","ko"]}},"required":["status"]}},"required":["monitor"]}
,FastifyError [Error]: Failed building the serialization schema for GET: /check/ready, due to error undefined unsupported
,    at Boot.<anonymous> (/project/node_modules/fastify/lib/route.js:271:19)
,    at /project/node_modules/avvio/boot.js:153:12
,    at Object.onceWrapper (events.js:416:28)
,    at Boot.emit (events.js:322:22)
,    at /project/node_modules/avvio/plugin.js:269:7
,    at done (/project/node_modules/avvio/plugin.js:201:5)
,    at check (/project/node_modules/avvio/plugin.js:225:9)
,    at internal/process/task_queues.js:153:7
,    at AsyncResource.runInAsyncScope (async_hooks.js:185:9)
,    at AsyncResource.runMicrotask (internal/process/task_queues.js:150:8) {
,  code: 'FST_ERR_SCH_SERIALIZATION_BUILD',
,  statusCode: 500
,}

To Reproduce

  1. add a route with this validation schema
{
    schema: {
        response: {
            200: {
                type: 'object',
                properties: {
                    monitor: {
                        type: 'object',
                        properties: {status: {enum: ['ok', 'ko']}},
                        required: ['status'],
                    },
                },
                required: ['monitor'],
            },
        },
    },
}

Expected behavior

no error at sever start

Your Environment

The only way to avoid this issue for now is to add a type: string along with the enum, but per the spec, type is optional and is set only to enforce a certain type along with the enum values. Enum can be mixed type.

This detail seems not that important, but it has a lot when combined to TypeScript and OpenAPI auto generated from the schema. In this case status is shown as string instead of enum in TypeScript and documentation just to make the schema works for fastify.

fox1t commented 4 years ago

I've encountered this error in the past using https://github.com/sinclairzx81/typebox to generate schemas and, yes, it is not compliant with the spec.

JSteunou commented 4 years ago

This is exactly while using this library that I have spotted this error. When you say it is not compliant with the spec, do you refer to fastify or typebox?

To me typebox is compliant here, type: xxx is not mandatory with enum per the spec. It should only be here when you need to enforce a single type within the enum.

fox1t commented 4 years ago

I think this is a json-fast-stringify not being compliant to that spec (It is used internally by fastify).

The spec itself is ok.

JSteunou commented 4 years ago

Ok I understand better, thank you.

fox1t commented 4 years ago

This is the line that has that problem https://github.com/fastify/fast-json-stringify/blob/master/index.js#L134 Would you like to make a PR?

JSteunou commented 4 years ago

I could try, but what should be the default then? Should {enum: [...]} be a special case, or every object without type should have the same logic? For example {} which is totally empty and the result of TypeBox Type.Any() should throw or pass?

JSteunou commented 4 years ago

Naïve question ahead:

Why cannot AJV be used as the serializer instead of fast-json-stringify? Just for speed reasons?

Because testing

{
    "type": "object",
    "properties": {
        "monitor": {
            "type": "object",
            "properties": {"status": {"enum": ["ok", "ko"]}},
            "required": ["status"]
        },
        "optionalany": {}
    },
    "required": ["monitor"]
}

compiles and validates very fine with AJV. As it is already used for input validation, maybe it could be used for output serialisation, would avoid maintenance and double work?

jsumners commented 4 years ago

AJV is not a serializer. It is only a validator.

JSteunou commented 4 years ago

Makes sense.

I though it, or some part of it, could be reuse for the serialisation part.

jsumners commented 4 years ago

Both modules work the same way: they read the JSON Schema DSL and generate code from it. In the case of the serializer, the generated code is a function that "knows" the shape of data to be returned from it. AJV returns a function that verifies the shape is what it is supposed to be. Subtle but significant difference.

Eomm commented 4 years ago

I could try, but what should be the default then? Should {enum: [...]} be a special case, or every object without type should have the same logic? For example {} which is totally empty and the result of TypeBox Type.Any() should throw or pass?

I think the spec helps to decide what to do in these cases.

But we need to remember that we are a serializer, not a validator, and if you want to serialize an object you must know what the output is, otherwise we would be like JSON.stringify().

The enum aims to validate that the output of the server is fine with the schema and for this reason, I would not include the enum support to fast-json-stringify but I would suggest:

A final thought: I think we should not crash and manage this special case (object without type but with enum) as an additional-properties object by default.

const fjs = require('fast-json-stringify')

const x = fjs({
  type: 'object',
  properties: {
    kind: { type: 'string', enum: ['foobar'] } // if we remove `type` we should not crash an leave all the feature as is now
  }
})

console.log(x({ kind: 'hello' })) // works
console.log(x({ kind: 'foobar' })) // works as expected
fox1t commented 4 years ago

@Eomm nice idea! I agree with you!

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

JSteunou commented 4 years ago

Can this issue be re-opened plz?

Eomm commented 4 years ago

Moved the issue as bug to fast-json-stringify: enum may not have the type property: details here

Here a snippet to reproduce the issue: https://runkit.com/embed/g2imhxqi00tb