fix: handle invalid schema data

[lottamus]

• fixes https://github.com/stoplightio/platform-internal/issues/3501

[stoplight-bot]

🎉 This PR is included in version 3.1.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[XVincentX]

Hmm, I saw this merged but I got a review request so just posting this for the posterity.

This is very wrong to me. The Schema type is already implying that data is an object and not undefined.

If this line is required it means somebody calling this function is lying/casting. That's not this function's problem; it should not be the its job of this function to validate the passed value; otherwise why do we have the types for?

You can see this effectively by the fact that you had to "lie" in the test by doing a ts-ignore.

I'd vote to fix this where the real problem is and not here @lottamus @marbemac

[XVincentX]

@lottamus @marbemac Can you point out some more details about this so I can try to fix it?

[lottamus]

@XVincentX yeah since we are transforming user input, we cannot assume the data property will actually be a Schema without doing some validation on it somewhere in the code. The easiest place was to add it in getExamplesFromSchema, but perhaps we should also update the typing to unknown or add some validator isSchema or something. I'd like to stay pretty lenient and not throw errors on invalid documents, that's why we have Spectral 🤓

• https://github.com/stoplightio/http-spec/blob/master/src/oas2/transformers/getExamplesFromSchema.ts#L5
• https://github.com/stoplightio/http-spec/blob/master/src/oas2/transformers/responses.ts#L22

• http-spec/src/oas2/transformers/params.ts

  Line 80 in f2f0acb

  get(body, 'x-examples') || (body.schema ? getExamplesFromSchema(body.schema) : void 0),

[XVincentX]

I'd like to stay pretty lenient and not throw errors on invalid documents, that's why we have Spectral

I would not rely on this for the following reasons:

1. Spectral is a linter, not a parser
2. The assumption that Spectral has been ran before passing the input is very arguable (Prism does not use Spectral and won't)
3. Spectral rules can be disabled; a Spectral's OK does not imply a valid document.

I wouldn't want to throw an exception either, but we could use the These monad to carry on parse results and errors at the same time (guess what library has such monad implemented).

---

That said — I understand now the issue and I've created a ticket to tame this although to be honest, I do not think we're going to have the time to make this right, but you never know.

[lottamus]

Spectral is a linter, not a parser

True, but http-spec should assume user input will be invalid and we should thus update the typings to reflect. The goal of http-spec is to normalize the user input into a format that can be reliably consumed throughout our ecosystem. This means safely handling invalid documents and producing a consistent interface no matter the input.

The assumption that Spectral has been ran before passing the input is very arguable

The reasoning I was trying to make was that it's not http-specs job to report errors/warnings within the document. That job should be on Spectral.

Spectral rules can be disabled; a Spectral's OK does not imply a valid document.

Exactly, thus http-spec should not assume a valid document. We need to make sure our typings reflect that each part of the document could be completely incorrect lol.

[XVincentX]

True, but http-spec should assume user input will be invalid and we should thus update the typings to reflect. The goal of http-spec is to normalize the user input into a format that can be reliably consumed throughout our ecosystem. This means safely handling invalid documents and producing a consistent interface no matter the input.

Completely agree with this. That's way I was pointing out that Spectral is not enough. I must have misunderstood your comment.

We need to make sure our typings reflect that each part of the document could be completely incorrect lol.

I agree with this also, but if so this specific is going on a different direction, right? (t: Schema but then if (isObject(t))) — and that is dangerous.

---

Seems like we're on the same page.