Skip to content

Conversation

@clasnake
Copy link
Contributor

Based on #5110 (comment)

@clasnake
Copy link
Contributor Author

@wing328 Seems CI failed due to Go client build failure.
[INFO] Go Petstore Client ................................ FAILURE [9.978s]

@wing328
Copy link
Contributor

wing328 commented Mar 19, 2017

@clasnake let me take a look.

Btw, did you see my comment about datetime?

@clasnake
Copy link
Contributor Author

@wing328 Yup. The async support in async-scala uses the serializers/deserializers provided by com.wordnik.swagger swagger-async-httpclient and that works for java.util.Date instead of joda DateTime. Actually this is what makes this change a breaking change. It'd be great if we can rewrite the marshalling logic. Maybe just use jackson. But that'll need some time because we can't just add a layer of facade over async-scala. I'm not sure what the current status of swagger-async-httpclient is? Is it under active development or maintenance?

@wing328
Copy link
Contributor

wing328 commented Mar 19, 2017

Travis CI reported the following issue with the Scala Petstore client:

Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.164 sec

Results :

Tests in error: 
  StoreApi should delete an order(StoreApiTest): order should have been deleted

Tests run: 12, Failures: 0, Errors: 1, Skipped: 0

I could repeat the issue in my machine.

Did you get similar errors when running mvn test under samples/client/petstore/scala?

@wing328
Copy link
Contributor

wing328 commented Mar 19, 2017

I don't think https://github.com/swagger-api/swagger-async-httpclient is actively maintained as last change was almost a year ago.

https://www.implicitdef.com/2015/11/19/comparing-scala-http-client-libraries.html compares differnet Scala HTTP library and the comments mentioned about akka-http, which is used by the 'akka-scala` generator in Swagger Codegen.

I would suggest using akka-http instead so that we can consolidate all 3 generators (scala, async-scala, akka-http) into one. (this can be done in a separate PR after this PR is merged)

importMapping.put("DateTime", "org.joda.time.DateTime");
importMapping.put("ListBuffer", "scala.collection.mutable.ListBuffer");
importMapping.put("Date", "java.util.Date");
importMapping.put("ListBuffer", "scala.collections.mutable.ListBuffer");

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change was probably missed during a merge conflict. It should be scala.collection.mutable.ListBuffer

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codeniko thanks for reviewing.

@clasnake can you please take a look when you've time?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No problem. I'll change the underlying library to akka-http and try to consolidate all three Scala clients into one(In a separate PR later). In fact that's the one I use in my daily work.

@wing328
Copy link
Contributor

wing328 commented Apr 1, 2017

@clasnake I've looked into the test failure a bit more. Here is part of the result I got when running sbt "test:testOnly *StoreApiTest"

[info] StoreApiTest:
[info] StoreApi
[info] - should place and fetch an order
[info] - should delete an order *** FAILED ***
[info]   order should have been deleted (StoreApiTest.scala:65)

I used tcpdump to troubleshoot and here is what I got for the last request (to see if the order has been deleted):

GET /v2/store/order/1001 HTTP/1.1
Host: petstore.swagger.io
Content-Type: application/json;charset=utf-8
Accept-Encoding: gzip,deflate
Connection: keep-alive
Accept: */*
User-Agent: Reverb SwaggerClient / 0.3.5
FC'dp
tHTTP/1.1 404 Not Found
Date: Sat, 01 Apr 2017 10:52:07 GMT
Access-Control-Allow-Origin: *
Access-Control-Allow-Methods: GET, POST, DELETE, PUT
Access-Control-Allow-Headers: Content-Type, api_key, Authorization
Content-Type: application/json
Connection: close
Server: Jetty(9.2.9.v20150224)
{"code":1,"type":"error","message":"Order not found"}

The server did return 404, which indicates the order (with id 1001) is not found but somehow the Scala API client was not able to handle the 4xx status code correctly.

@clasnake
Copy link
Contributor Author

clasnake commented Apr 2, 2017

@wing328 Thanks very much. This is very useful. I haven't got time to start the refactoring to use akka-http as the underlying library yet. I'll look into this issue during the refactoring as well.

@wing328
Copy link
Contributor

wing328 commented Apr 3, 2017

@clasnake np. I'll comment out the test case and open a new issue to track this.

@wing328 wing328 merged commit 39bd017 into swagger-api:2.3.0 Apr 3, 2017
@wing328 wing328 changed the title Pr4855 cherrypick [Scala] add async support to Scala API client Apr 3, 2017
@wing328
Copy link
Contributor

wing328 commented Apr 3, 2017

@clasnake PR merged into 2.3.0. I've commented out the test via d1350b0

@clasnake
Copy link
Contributor Author

clasnake commented Apr 4, 2017

@wing328 Cool thanks@@

@wing328
Copy link
Contributor

wing328 commented Apr 6, 2017

Just to clarify. Normally we don't comment out test cases. In this case, I want to avoid the PR opening for too long (as more likely merge conflicts will occur later). Scala users should use the master or stable version for the time being.

I'll also update the help text to mark the Scala client generator in 2.3.0 as "beta"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants