-
-
Notifications
You must be signed in to change notification settings - Fork 7.3k
#1023 - [Scala] Use status family during response processing #1024
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -13,12 +13,13 @@ import com.sun.jersey.multipart.file.FileDataBodyPart | |
| import com.wordnik.swagger.client._ | ||
| import com.wordnik.swagger.client.ClientResponseReaders.Json4sFormatsReader._ | ||
| import com.wordnik.swagger.client.RequestWriters.Json4sFormatsWriter._ | ||
| import javax.ws.rs.core.Response.Status.Family | ||
|
|
||
| import java.net.URI | ||
| import java.io.File | ||
| import java.util.Date | ||
| import java.util.TimeZone | ||
| import javax.ws.rs.core.MediaType | ||
| import javax.ws.rs.core.{MediaType, Response} | ||
|
|
||
| import scala.concurrent.ExecutionContext.Implicits.global | ||
| import scala.concurrent._ | ||
|
|
@@ -144,7 +145,11 @@ class {{classname}}AsyncHelper(client: TransportClient, config: SwaggerConfig) e | |
|
|
||
| val resFuture = client.submit("{{httpMethod}}", path, queryParams.toMap, headerParams.toMap, {{#bodyParam}}writer.write({{paramName}}){{/bodyParam}}{{^bodyParam}}"{{emptyBodyParam}}"{{/bodyParam}}) | ||
| resFuture flatMap { resp => | ||
| process(reader.read(resp)) | ||
| val status = Response.Status.fromStatusCode(resp.statusCode) | ||
| status.getFamily match { | ||
| case Family.SUCCESSFUL | Family.REDIRECTION | Family.INFORMATIONAL => process(reader.read(resp)) | ||
| case _ => throw new ApiException(resp.statusCode, resp.statusText) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You're right, I pushed a fix for informational and redirection statuses, please take a look. |
||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,237 @@ | ||
| /** | ||
| * OpenAPI Petstore *_/ ' \" =end -- \\r\\n \\n \\r | ||
| * This spec is mainly for testing Petstore server and contains fake endpoints, models. Please do not use this for any other purpose. Special characters: \" \\ *_/ ' \" =end -- | ||
| * | ||
| * OpenAPI spec version: 1.0.0 *_/ ' \" =end -- \\r\\n \\n \\r | ||
| * Contact: [email protected] *_/ ' \" =end -- \\r\\n \\n \\r | ||
| * | ||
| * NOTE: This class is auto generated by OpenAPI Generator (https://openapi-generator.tech). | ||
| * https://openapi-generator.tech | ||
| * Do not edit the class manually. | ||
| */ | ||
|
|
||
| package org.openapitools.client | ||
|
|
||
| import com.sun.jersey.api.client.Client | ||
| import com.sun.jersey.api.client.ClientResponse | ||
| import com.sun.jersey.api.client.config.ClientConfig | ||
| import com.sun.jersey.api.client.config.DefaultClientConfig | ||
| import com.sun.jersey.api.client.filter.LoggingFilter | ||
|
|
||
| import com.sun.jersey.core.util.MultivaluedMapImpl | ||
| import com.sun.jersey.multipart.FormDataMultiPart | ||
| import com.sun.jersey.multipart.file.FileDataBodyPart | ||
|
|
||
| import java.io.File | ||
| import java.net.URLEncoder | ||
| import java.util.UUID | ||
| import javax.ws.rs.core.MediaType | ||
|
|
||
| import scala.collection.JavaConverters._ | ||
| import scala.collection.mutable | ||
|
|
||
| import com.fasterxml.jackson.module.scala.DefaultScalaModule | ||
| import com.fasterxml.jackson.datatype.joda.JodaModule | ||
| import com.fasterxml.jackson.core.JsonGenerator.Feature | ||
| import com.fasterxml.jackson.databind._ | ||
| import com.fasterxml.jackson.annotation._ | ||
| import com.fasterxml.jackson.databind.annotation.JsonSerialize | ||
|
|
||
| object ScalaJsonUtil { | ||
| def getJsonMapper: ObjectMapper = { | ||
| val mapper = new ObjectMapper() | ||
| mapper.registerModule(new DefaultScalaModule()) | ||
| mapper.registerModule(new JodaModule()) | ||
| mapper.setSerializationInclusion(JsonInclude.Include.NON_NULL) | ||
| mapper.setSerializationInclusion(JsonInclude.Include.NON_DEFAULT) | ||
| mapper.configure(SerializationFeature.WRITE_DATES_AS_TIMESTAMPS, false) | ||
| mapper.configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false) | ||
| mapper.setSerializationInclusion(JsonInclude.Include.NON_EMPTY) | ||
| mapper | ||
| } | ||
| } | ||
|
|
||
| class ApiInvoker(val mapper: ObjectMapper = ScalaJsonUtil.getJsonMapper, | ||
| httpHeaders: mutable.HashMap[String, String] = mutable.HashMap(), | ||
| hostMap: mutable.HashMap[String, Client] = mutable.HashMap(), | ||
| asyncHttpClient: Boolean = false, | ||
| authScheme: String = "", | ||
| authPreemptive: Boolean = false | ||
| ) { | ||
|
|
||
| var defaultHeaders: mutable.HashMap[String, String] = httpHeaders | ||
|
|
||
| def escape(value: String): String = { | ||
| URLEncoder.encode(value, "utf-8").replaceAll("\\+", "%20") | ||
| } | ||
| def escape(values: List[String]): String = { | ||
| values.map(escape).mkString(",") | ||
| } | ||
|
|
||
| def escape(value: Long): String = value.toString | ||
| def escape(value: Double): String = value.toString | ||
| def escape(value: Float): String = value.toString | ||
| def escape(value: UUID): String = value.toString | ||
|
|
||
| def deserialize(json: String, containerType: String, cls: Class[_]) = { | ||
| if (cls == classOf[String]) { | ||
| json match { | ||
| case s: String => | ||
| if (s.startsWith("\"") && s.endsWith("\"") && s.length > 1) { | ||
| s.substring(1, s.length - 1) | ||
| } else { | ||
| s | ||
| } | ||
| case _ => null | ||
| } | ||
| } else { | ||
| containerType.toLowerCase match { | ||
| case "array" => | ||
| val typeInfo = mapper.getTypeFactory.constructCollectionType(classOf[java.util.List[_]], cls) | ||
| val response = mapper.readValue(json, typeInfo).asInstanceOf[java.util.List[_]] | ||
| response.asScala.toList | ||
| case "list" => | ||
| val typeInfo = mapper.getTypeFactory.constructCollectionType(classOf[java.util.List[_]], cls) | ||
| val response = mapper.readValue(json, typeInfo).asInstanceOf[java.util.List[_]] | ||
| response.asScala.toList | ||
| case _ => | ||
| json match { | ||
| case e: String if "\"\"" == e => null | ||
| case _ => mapper.readValue(json, cls) | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| def serialize(obj: AnyRef): String = { | ||
| if (obj != null) { | ||
| obj match { | ||
| case e: List[_] => mapper.writeValueAsString(obj.asInstanceOf[List[_]].asJava) | ||
| case _ => mapper.writeValueAsString(obj) | ||
| } | ||
| } else { | ||
| null | ||
| } | ||
| } | ||
|
|
||
| def invokeApi( | ||
| host: String, | ||
| path: String, | ||
| method: String, | ||
| queryParams: Map[String, String], | ||
| formParams: Map[String, String], | ||
| body: AnyRef, | ||
| headerParams: Map[String, String], | ||
| contentType: String | ||
| ): String = { | ||
| val client = getClient(host) | ||
|
|
||
| val querystring = queryParams.filter(k => k._2 != null).map(k => escape(k._1) + "=" + escape(k._2)).mkString("?", "&", "") | ||
| val builder = client.resource(host + path + querystring).accept(contentType) | ||
| headerParams.map(p => builder.header(p._1, p._2)) | ||
| defaultHeaders.foreach(p => { | ||
| if (!headerParams.contains(p._1) && p._2 != null) { | ||
| builder.header(p._1, p._2) | ||
| } | ||
| }) | ||
| var formData: MultivaluedMapImpl = null | ||
| if (contentType == "application/x-www-form-urlencoded") { | ||
| formData = new MultivaluedMapImpl() | ||
| formParams.foreach(p => formData.add(p._1, p._2)) | ||
| } | ||
|
|
||
| val response: ClientResponse = method match { | ||
| case "GET" => builder.get(classOf[ClientResponse]) | ||
| case "POST" => | ||
| if (formData != null && formData.size() > 0) { | ||
| builder.post(classOf[ClientResponse], formData) | ||
| } else if (body != null && body.isInstanceOf[File]) { | ||
| val file = body.asInstanceOf[File] | ||
| val form = new FormDataMultiPart() | ||
| form.field("filename", file.getName) | ||
| form.bodyPart(new FileDataBodyPart("file", file, MediaType.MULTIPART_FORM_DATA_TYPE)) | ||
| builder.post(classOf[ClientResponse], form) | ||
| } else { | ||
| if (body == null) { | ||
| builder.post(classOf[ClientResponse], serialize(body)) | ||
| } else { | ||
| builder.`type`(contentType).post(classOf[ClientResponse], serialize(body)) | ||
| } | ||
| } | ||
| case "PUT" => | ||
| if (formData != null) { | ||
| builder.post(classOf[ClientResponse], formData) | ||
| } else if (body == null) { | ||
| builder.put(classOf[ClientResponse], null) | ||
| } else { | ||
| builder.`type`(contentType).put(classOf[ClientResponse], serialize(body)) | ||
| } | ||
| case "DELETE" => builder.delete(classOf[ClientResponse]) | ||
| case "PATCH" => | ||
| if(formData != null) { | ||
| builder.header("X-HTTP-Method-Override", "PATCH").post(classOf[ClientResponse], formData) | ||
| } else if(body == null) { | ||
| builder.header("X-HTTP-Method-Override", "PATCH").post(classOf[ClientResponse], null) | ||
| } else { | ||
| builder.header("X-HTTP-Method-Override", "PATCH").`type`(contentType).post(classOf[ClientResponse], serialize(body)) | ||
| } | ||
| case _ => null | ||
| } | ||
| response.getStatusInfo.getStatusCode match { | ||
| case 204 => "" | ||
| case code: Int if Range(200, 299).contains(code) => | ||
| if (response.hasEntity) { | ||
| response.getEntity(classOf[String]) | ||
| } else { | ||
| "" | ||
| } | ||
| case _ => | ||
| val entity = if (response.hasEntity) { | ||
| response.getEntity(classOf[String]) | ||
| } else { | ||
| "no data" | ||
| } | ||
| throw new ApiException(response.getStatusInfo.getStatusCode, entity) | ||
| } | ||
| } | ||
|
|
||
| def getClient(host: String): Client = { | ||
| if (hostMap.contains(host)) { | ||
| hostMap(host) | ||
| } else { | ||
| val client = newClient(host) | ||
| // client.addFilter(new LoggingFilter()) | ||
| hostMap += host -> client | ||
| client | ||
| } | ||
| } | ||
|
|
||
| def newClient(host: String): Client = if (asyncHttpClient) { | ||
| import com.ning.http.client.Realm | ||
| import org.sonatype.spice.jersey.client.ahc.AhcHttpClient | ||
| import org.sonatype.spice.jersey.client.ahc.config.DefaultAhcConfig | ||
|
|
||
| val config: DefaultAhcConfig = new DefaultAhcConfig() | ||
| if (!authScheme.isEmpty) { | ||
| val authSchemeEnum = Realm.AuthScheme.valueOf(authScheme) | ||
| config | ||
| .getAsyncHttpClientConfigBuilder | ||
| .setRealm(new Realm.RealmBuilder().setScheme(authSchemeEnum) | ||
| .setUsePreemptiveAuth(authPreemptive).build) | ||
| } | ||
| AhcHttpClient.create(config) | ||
| } else { | ||
| Client.create() | ||
| } | ||
| } | ||
|
|
||
| object ApiInvoker extends ApiInvoker( | ||
| mapper = ScalaJsonUtil.getJsonMapper, | ||
| httpHeaders = mutable.HashMap(), | ||
| hostMap = mutable.HashMap(), | ||
| asyncHttpClient = false, | ||
| authScheme = "", | ||
| authPreemptive = false | ||
| ) | ||
|
|
||
| class ApiException(val code: Int, msg: String) extends RuntimeException(msg) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,20 @@ | ||
| package org.openapitools.client | ||
|
|
||
| import org.openapitools.client.api._ | ||
|
|
||
| import com.wordnik.swagger.client._ | ||
|
|
||
| import java.io.Closeable | ||
|
|
||
| class AsyncClient(config: SwaggerConfig) extends Closeable { | ||
| lazy val locator: ServiceLocator = config.locator | ||
| lazy val name: String = config.name | ||
|
|
||
| private[this] val client = transportClient | ||
|
|
||
| protected def transportClient: TransportClient = new RestClient(config) | ||
|
|
||
| def close() { | ||
| client.close() | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
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 theClientExceptionandServerExceptionthat exist in the kotlin client code from above. One could argue that the result of calling an API invocation method is expected to beFamily.SUCCESSFULand 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 Continueoccurs after the server receives the request headers and determines whether or not it will continue processing. If the client sends the headersExpect: 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 support100 Continue.101 Switching Protocolswould 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 Processingis a WebDAV extension (see RFC 2518 Section10.1), so I consider it a low priority (if any) to support.Redirection
300 Multiple Choicesoccurs 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 Permanentlywith no standardized way to define where the resource has moved (spec only says it SHOULD be inLocationheader), 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 FoundI've never seen this used without automatically following redirects. If we receive this, the new location would be in theLocationheader, 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 aLocationheader, the spec suggests we could leave this as-is.303 See Otherindicates the server wants you to perform aGETrequest to some other URI. This could result in a different deserialized type, so this type of redirect would need to be defined in theresponsesobject 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 Modifiedwould 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, soprocess(reader.read(resp))would fail.305 Use Proxy: requires querying via proxy defined in theLocationheader. I understand 305 is widely considered a security concern, and is therefore not very common.307 Temporary Redirectand308 Permanent Redirect: being redirects, I would think the http-client framework would handle these directly.