#1023 - [Scala] Use status family during response processing

[luzhanov]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\.
• Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@clasnake @jimschubert @shijinkui @ramzimaalej

Description of the PR

Issue: #1023

[wing328]

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

[luzhanov]

Let me know if you need help fixing it.

Yes, that will be very nice if you will help me to fix this. Thank you!

[ramzimaalej]

it's semantically incorrect to treat informational and redirection as errors. there is valuable information in the headers that should be sent back to the client. for instance, when you get a redirect, the header location will tell you where to go. can you please shed some light on the motivation for this change?

[luzhanov]

You're right, I pushed a fix for informational and redirection statuses, please take a look.

[jimschubert]

*.iml should not be ignored. See reasoning: toptal/gitignore.io#186 (comment)

[jimschubert]

This assumes that a 1xx and 3xx status results in the same body structure as a 2xx message. I doubt this will ever be the case, and this would most likely always result in an deserialization error.

Unfortunately, I don't have a great solution to this. I left these cases as TODOs that throw not-implemented exceptions in the Kotlin client:

return when (response.responseType) {
    ResponseType.Success -> {{#returnType}}(response as Success<*>).data as {{{returnType}}}{{/returnType}}{{^returnType}}Unit{{/returnType}}
    ResponseType.Informational -> TODO()
    ResponseType.Redirection -> TODO()
    ResponseType.ClientError -> throw ClientException((response as ClientError<*>).body as? String ?: "Client error")
    ResponseType.ServerError -> throw ServerException((response as ServerError<*>).message ?: "Server error")
    else -> throw kotlin.IllegalStateException("Undefined ResponseType.")
}

What makes these cases difficult is the potentially different type structure for all response families and the lack of common response structures for those status codes that support response bodies. In Scala, we could maybe solve this with an Either[A,B], but I've never liked that pattern for potentially unlikely use cases. While it's true that informational and redirects are not errors, I'm inclined to create a typed exception along the lines of the ClientException and ServerException that exist in the kotlin client code from above. One could argue that the result of calling an API invocation method is expected to be Family.SUCCESSFUL and all other families are unexpected, with maybe 2 or 3 special cases.

A reason I am comfortable leaving these to throw exceptions in the kotlin client is that I haven't personally seen a use case for a majority of the 1xx and 3xx statuses. The few others would require some standardized way of handling them across all generators.

Informational

• 100 Continue occurs after the server receives the request headers and determines whether or not it will continue processing. If the client sends the headers Expect: 100-Continue, then it should handle waiting for the server's response. If the client didn't send this header, RFC 7231 Section 6.2.1 says the interim response can safely be ignored. All of this requires the server to support 100 Continue.
• 101 Switching Protocols would occur if upgrading from HTTP 1.1 to 2.0, or doing a switch from HTTP to websockets, or other similar "upgrades". I've never experienced the first case implemented correctly by a REST API, and I don't know that the second case would be supported by an HTTP Client.
• 102 Processing is a WebDAV extension (see RFC 2518 Section10.1), so I consider it a low priority (if any) to support.

Redirection

• 300 Multiple Choices occurs when a server has multiple representations of a selected resource. The spec says that a non-HEAD request SHOULD provide a body explaining these representations (source). However, this is supposed to be either left up to the user agent or the user directly. The body logically wouldn't have the same structure as a 2xx body, so this would result in a deserialization error.
• 301 Moved Permanently with no standardized way to define where the resource has moved (spec only says it SHOULD be in Location header), I think an exception is most sufficient here because it indicates that the API for which the client was generated has changed. We could redirect to a new location if the header is present, but this begs the question about what else has changed with the underlying API; a blind redirect could potentially result in bad data or a loss of data (e.g. if request body has changed structure and server drops unknown properties).
• 302 Found I've never seen this used without automatically following redirects. If we receive this, the new location would be in the Location header, and we could automatically re-query… but the specification says the client should continue using the original Request-URI and suggests that the new URI be considered temporary or able to change in the future. So aside from potentially following a Location header, the spec suggests we could leave this as-is.
• 303 See Other indicates the server wants you to perform a GET request to some other URI. This could result in a different deserialized type, so this type of redirect would need to be defined in the responses object of the OpenAPI specification. The likelihood that this happens (an API defines a successful type as well as a 'see this other endpoint' definition) is pretty slim.
• 304 Not Modified would indicate that the client can use a cache of its previously known copy of an object. I don't know that any of our client generators have client-side caching built in, so I doubt we're supporting 304s outside of whatever support exists in specific client frameworks. Also, 304s are not allowed to contain bodies, so process(reader.read(resp)) would fail.
• 305 Use Proxy: requires querying via proxy defined in the Location header. I understand 305 is widely considered a security concern, and is therefore not very common.
• 307 Temporary Redirect and 308 Permanent Redirect: being redirects, I would think the http-client framework would handle these directly.

[wing328]

@luzhanov thanks for the PR, which has been merged into master.

The scala-httpclient generator has been deprecated since it's depending on an unmaintained HTTP lib.

Please switch to scala-akka instead.