feat(openapi): provide default 400 responses in docs

[MichalPawlicki]

If endpoint input is validated, the endpoint may respond with a 400 status code. Provide a default 400 response in OpenAPI docs to make them more consistent with the actual endpoint behavior.

Partly resolves #1053.

[adamw]

Thanks! And sorry for the delay. We definitely needs something like this :)

Couple of questions/ideas:

• maybe this should be part of the OpenAPIDocsOptions, allowing users to specify the description string - or more generally, provide an output which will be used for 400 errors, possibly the same that is used in the DecodeFailureHandler.
• what should be the default? Maybe we should only add it optionally, leaving the default as-is. On the other hand, the upcoming 0.18 gives as an opportunity to introduce a breaking change
• what if a 400 output is already defined? I think we'd have to check, that it is not overriden
• maybe we should always add this type of input? decode failures can happen not only because of validation, but also if we try to e.g. parse an Int. On the other hand, most of the time you extract Strings, so this would produce the additional output too eagerly ... maybe a good heuristic would be checking for either validation, or if the schema format is non-empty (hinting at additional parsing that is happening)?

[adamw]

cc @matwojcik

Yes, you could generate a nice representation of the validator for a schema. See the Validator.show method

[MichalPawlicki]

No problem, thanks for the response. 🙂

• maybe this should be part of the OpenAPIDocsOptions, allowing users to specify the description string - or more generally, provide an output which will be used for 400 errors, possibly the same that is used in the DecodeFailureHandler.
• what should be the default? Maybe we should only add it optionally, leaving the default as-is. On the other hand, the upcoming 0.18 gives as an opportunity to introduce a breaking change

I think adding a field of type EndpointOutput.Basic[_] to OpenAPIDocsOptions may be a good idea. This way we could specify a default response and give users the option to override it.

• what if a 400 output is already defined? I think we'd have to check, that it is not overriden

Sure, I'll add a test for it.

• maybe we should always add this type of input? decode failures can happen not only because of validation, but also if we try to e.g. parse an Int.

Makes sense. Users have the option to use infallibleEndpoint if they think no error can occur, so how about we always add the default response to "fallible" endpoints?

[adamw]

I think adding a field of type EndpointOutput.Basic[_] to OpenAPIDocsOptions may be a good idea. This way we could specify a default response and give users the option to override it.

Or more generally EndpointOutput, which can then be converted to basic outputs using the methods in sttp.tapir.internal. When creating a DecodeFailureHandler, or even customising the DefaultDecodeFailureHandler, you can use any output - potentially with multiple basic outputs (such as a body & a header)

Makes sense. Users have the option to use infallibleEndpoint if they think no error can occur, so how about we always add the default response to "fallible" endpoints?

That's a different axis - even for infallibleEndpoints, you can get a decode failure, in decoding an input fails.

[MichalPawlicki]

maybe a good heuristic would be checking for either validation, or if the schema format is non-empty (hinting at additional parsing that is happening)?

It seems that a schema format is rarely defined, at least in the tests, so it might not be a good "predictor" of possible parsing/decoding failures. For now, I pushed a version where a 400 response is added for any basic input other than FixedMethod, FixedPath or EndpointIO.Empty. This works pretty well, though it sometimes adds the response when it shouldn't (e.g. for a path-captured string, or an optional string query param). I'll check if this could be improved. Also, I'll add the option to OpenAPIDocsOptions.

[adamw]

Thanks for the updates :)

I'm still thinking of what would be a good way to decide whether to include the error output. But I can't think of anything. I don't really like the current way - it's too broad and gives imprecise information.

Maybe then we should extend the Codec with the appropriate metadata? Sth like decodeFailureDescription: Option[String]. A None would indicate that the codec can't fail, otherwise we would know that it can. This description (combined possibly with existence or absence of validators) could then be used to create the documentation message.

But these changes would be quite far-reaching. We would need new .mapDecode variants which would additionally add the description (.map can't return a failure, except for exceptions - but let's gloss over it for now, that's not verifiable in any way). What do you think?

[adamw]

I think I'd still make this optional. Maybe defaultDecodeFailureOutput would be a better name. Or maybe we could move the logic here, having a EndpointInput => Option[ValuedEndpointOuput[_]] function in the options. The default implementation would contain partially what's now in inputToDefaultErrorResponses. WDYT?

The logic would be nicely pluggable then, providing a way to implement the decision logic in an app-specific way.

But then, maybe that's not a place where you need the flexibility?

[MichalPawlicki]

Yeah, I'll make this optional in case users don't like or need the functionality. I think this could be an EndpointInput[_] => Option[EndpointOutput[_]], potentially implemented using DefaultDecodeFailureHandler.FailureMessages.failureSourceMessage. And yes, I think I'll move some logic from inputToDefaultErrorResponses.

(I think we don't need to return ValuedEndpointOutput, because we don't need the value returned by the endpoint to generate the docs, is that correct?)

[MichalPawlicki]

I made some changes to the logic (here). It's now a bit better at detecting potential failures and gives a bit more detailed descriptions, e.g. Invalid value for: body. Maybe this would be enough for now? (Ofc, I'm going to refactor this.)

Maybe then we should extend the Codec with the appropriate metadata? Sth like decodeFailureDescription: Option[String].

Is it ok to add a method to a trait that's used in so many places? What if it turns out later that it was not a good idea, would it be easy to change its signature or move the method somewhere else? If so, I could give it a try.

Also, do you plan any deadline for introducing breaking changes? I think extending Codec could take me some time since I don't know the codebase too well.

[adamw]

I was just reading the code :) I would be reluctant to add new methods to Codec as well, but if that would be necessary, it's better to do this now, than post-1.0 ;)

The current heuristics looks much better. Let's try it and we'll see what the feedback is. The only missing piece is I think adding some mention in the docs on how to configure this feature.

[MichalPawlicki]

I refactored the code and added a short explanation to the docs, let me know if this works.