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.

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

When no schemas of a oneOf schema element match null is returned rather than raising an error. #342

Closed
2 tasks done
rustyconover opened this issue Jul 17, 2021 · 5 comments · Fixed by #497
Closed
2 tasks done
Labels
bug Confirmed bug good first issue Good for newcomers

Comments

@rustyconover
Copy link

Prerequisites

  • I have written a descriptive issue title
  • I have searched existing issues to ensure it has not already been reported

Fastify version

3.18.1

Plugin version

No response

Node.js version

14.17.3

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

11.4

Description

Assume using Fastify, there is a response schema for a 200 status code to a route that has a the following JSON schema specified:

{
  "type": "object",
  "additionalProperties": false,
  "required": ["data"],
  "properties": {
    "data": {
      "type": "array",
      "minItems": 1,
      "items": {
        "oneOf": [
          {
            "type": "string"
          },
          {
            "type": "number"
          }
        ]
      }
    }
  }
}

This says that the response should be an object with a property called "data" which consists of an array where items can be a string or a number.

If the function generating the response returns a response like {"data": [false, "testing"]} the response generated by fast-json-stringify will be {"data": [null]} which violates the schema of the response. This will likely surprise the callers of the route.

No exception will be raised, which seems unsound.

This is because of this logical branch:

code += `

If none of the available oneOf schemas match, an error isn't raised a null is just returned for the the produced portion of the schema.

Steps to Reproduce

Run the example above.

Expected Behavior

I think the right thing to do is raise an exception that the response does not match the expected schema and return a different HTTP status code such as 500 rather than returning a response that violates the schema.

@MSE99
Copy link
Contributor

MSE99 commented Jul 17, 2021

fast-json-stringify is more of a serializer it doesn't and shouldn't do a lot of validation work, users who want to validate their inputs must do so before passing it to fjs.

I think it should have returned an empty array instead of an array with null in it.

@rustyconover
Copy link
Author

Hi @MSE99,

Thanks for getting back to me. You're right fast-json-stringify is a serializer, but in this case its completely violating the schema that its output is supposed to follow.

In the case of oneOf handling, fast-json-stringify is actually validating each possible schema (using Ajv) that is part of the oneOf specification to determine which one to use, so in this case it actually is validating the data. As such I disagree with the responsibility of validation being completely on the user.

Right now, this behavior is violating the principal of least surprise.

Rusty

@Eomm
Copy link
Member

Eomm commented Jul 17, 2021

I think the right thing to do is raise an exception that the response does not match the expected schema and return a different HTTP status code such as 500 rather than returning a response that violates the schema.

For this purpose exists this plugin https://github.com/fastify/fastify-response-validation

I agree that FJS should return an empty array in this case: we are avoiding introducing a cannot serialize error since it would be a validation phase that was not designed in the module

@Eomm Eomm added bug Confirmed bug good first issue Good for newcomers labels Jul 17, 2021
@joaopedrocampos
Copy link
Contributor

Hey @Eomm! If possible I would love to try this fix 🙏.

Just a question to understand what is the expected result on this case, using the issue example:

{
  "type": "object",
  "additionalProperties": false,
  "required": ["data"],
  "properties": {
    "data": {
      "type": "array",
      "minItems": 1,
      "items": {
        "oneOf": [
          {
            "type": "string"
          },
          {
            "type": "number"
          }
        ]
      }
    }
  }
}

If the stringified schema is

const schemaStringify = {
    data: [false, 'foo']
}

What should be the expected result?

{
    "data": []
}

or (with just the valid value)

{
    "data": ["foo"]
}

@Eomm
Copy link
Member

Eomm commented Jul 26, 2021

Go for it!

I think the "data": ["foo"] is more correct as you find out! 👍🏼

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants