feat!: drop required property support

[ivan-tymoshenko]

The required property doesn't need for serialization. It is used only for validating schema.
It would be a serious change. If you have any thoughts on why we shouldn't do this, I would like to hear them.

[mcollina]

lgtm

[Uzlopak]

makes sense

[jsumners]

Why isn't it needed for serialization? Is it not possible that a schema is defined for the output of an endpoint, someone else on the team changes code in the endpoint but not the schema, and the result is that they removed a required property? Wouldn't the schema then refuse sending the payload due to the violation?

[ivan-tymoshenko]

@jsumners If I understand you correctly, you ask, what if someone will try to serialize an object without the required property? Then FJS will serialize it without the required property. If you want to be sure that your data matches the schema, you should call validator but not serializer. And then, you will get an error that you don't have the required property.

[jsumners]

Prior to this change, what will happen in the following case?:

fastify.route({
  method: 'GET',
  path: '/example',
  schema: {
    response: {
      200: {
        type: 'object',
        properties: {
          foo: { type: 'string' },
          bar: { type: 'string'}
        },
        required: ['bar']
      }
    }
  }
})

function handler (req, reply) {
  const data = { foo: 'foo' }
  reply.send(data)
}

[ivan-tymoshenko]

Now it throws an error. After merging this PR, it will return {"foo":"foo"}.

[Eomm]

I must say that I would keep the feature opt-in with the option strictRequire.

In my opinion, the serialization process transforms an input into a string, but it should throw an error when the module does not know how to serialize the body.

In this case:

• the schema said that the property is required, but the field is missing: what should we return?

Other supported cases are where errors are thrown:

• the additionalProperties sets how to serialize properties that are not listed in the main schema
• the anyOf and allOf choose the proper schema to apply to an object

So, for this reason, I think there are edge cases between validation and (how to) serialize an object starting from a JSON schema.

makes sense

I agree that we can improve this aspect by removing all the validation logic of the module by setting a clear action to execute, like in this PR by ignoring the required array.

Does it make sense to you?

[jsumners]

Now it throws an error. After merging this PR, it will return {"foo":"foo"}.

Precisely my point. Right now this provides utility by guarding against the accidental removal of required properties in a response payload.

I must say that I would keep the feature opt-in with the option strictRequire.

This seems like a fair compromise.

[ivan-tymoshenko]

guarding against the accidental removal of required properties in a response payload

Serializer shouldn't do this. The validator should.

[ivan-tymoshenko]

@Eomm I agree with the point that we use the validator when we don't know how to serialize the data. But we should have the data to be unaware of how to serialize it.

In anyOf, oneOf, if we have the incoming data and we don't know what to do with it without validation. In this case, we don't have any data, so we don't have any problem.

I'm not obsessed with removing the required properties. I want to be consistent and understand the rules of FJS.

[ivan-tymoshenko]

@Eomm FJS will correctly serialize the data if you pass the correct (previously validated if you need that) data.

1. If you pass correct/previously validated data with anyOf, oneOf ..., we still won't know how to serialize it without validation.
2. If you pass correct/previously validated data with required properties it would be serialized correctly without any validation.

[climba03003]

It would be a serious change. If you have any thoughts on why we shouldn't do this, I would like to hear them.

If this packages is not used inside fastify, I would agree on removing the required check.

It is a safety measure on ensure the API endpoint always return something that we want. At least it should has that field.
I found that it is useful providing an API endpoint, especially for mobile. It just crash when receiving non-expecting data, including missing field.

I hope it would be a toggle to enable it instead of just drop it. Otherwise, people may need an extra check using ajv and duplicate the same set of schema again.

[ivan-tymoshenko]

It is a safety measure on ensure the API endpoint always return something that we want. At least it should has that field.
I found that it is useful providing an API endpoint, especially for mobile. It just crash when receiving non-expecting data, including missing field.

@climba03003 I agree that it is good to know what you return. But FJS does not guarantee that your data would be valid. Why should we check some aspects but not check others? If we really want to be sure that our data is valid, maybe we should check it before serialization.

[ivan-tymoshenko]

We can check an endpoint response with AJV before serializing or if it's too long we can create our own validator that would check only a few rules that we need. Like required properties.

[climba03003]

We can check an endpoint response with AJV before serializing or if it's too long we can create our own validator that would check only a few rules that we need. Like required properties.

Just like my first sentence, if this package is not used inside fastify. I would strongly agree on the removal.

I will create a function if the feature actually drops. But, when we are talking about use-case and why. I think it needs to talk into the consideration how people use it in fastify and what they expect.

I just have few question before my vote.
Can I assume the serializer when taking JSON-Schema into account should return a value that should fulfill the schema?

Does any of the current output actually violate ajv validation?

[ivan-tymoshenko]

I will create a function if the feature actually drops. But, when we are talking about use-case and why. I think it needs to talk into the consideration how people use it in fastify and what they expect.

If we need this in Fastify, we can create it in Fastify. Use our custom validator or use AJV.

Can I assume the serializer when taking JSON-Schema into account should return a value that should fulfill the schema?

I we all agree on that and would make it work like that, I won't be against it. I just want to understand what FJS is and how it should work. And be consistent with that. Just a few general rules like that.

Does any of the current output actually violate ajv validation?

You mean like this https://github.com/fastify/fast-json-stringify/blob/master/test/date.test.js#L268?

[climba03003]

I we all agree on that and would make it work like that, I won't be against it. I just want to understand what FJS is and how it should work. And be consistent with that. Just a few general rules like that.

Then, required property should be keep if we all agree on it.

You mean like this https://github.com/fastify/fast-json-stringify/blob/master/test/date.test.js#L268?

Yes, pretty sad it does have output didn't pass the validation of schema.

I do expect FJS output is able to validate against JSON-Schema. And we can follow it as the guideline on how the behavior should be.
WDYT @mcollina @jsumners @Eomm @Uzlopak

[ivan-tymoshenko]

@climba03003 I think we can open another issue/discussion and discuss how FJS should work.

[Uzlopak]

Tbh. When I compare this with #520 then I dont understand why #520 would be ok but not this PR.

If I use a serializer I expect that the serializer does actually just that - serializing the input. So while I assume this one still serializes accordingly and maybe doesnt throw if the result is not matching a schema, the change in #520 was actually serializing something completely different than what I would have expected. See my comment #520 (review)

I have no issue with still throwing. It avoids the roundtrip to validate the generated Object against ajv.

So maybe yes: We should discuss what we actually expect.

[climba03003]

@climba03003 I think we can open another issue/discussion and discuss how FJS should work.

I have created a discussion #532
Let move forward and discuss inside the discussion thread.

[mcollina]

@jsumners changed my mind, I think this is one of the minimum validation we need to perform.