Improve sttpOpenApiClient generator #6684

ghostbuster91 · 2020-06-16T13:41:06Z

We'd like to propose the following changes to the scala-sttp generator. While the existing version works quite well (thanks to all of the contributors!), in a couple of places it diverges from the way sttp is most often used in the wild, and from the original design goals.

List of changes together with reasons behind them:

allow libraries' versions customization - although it is not hard to change them by hand we think that it plays nicely with the rest of the cli options and improves the overal user's experience
return erros by default in a separate error channel - being explicit in terms of returned error types is the way to go in scala (there is a switch which allows to restore the old behavior)
some of the helper functions were incorporated into sttp directly
some of the helper functions were removed as they were redundant/unnecessary
auth parameters are passed explicitly rather than implicitly - we belive that the rule of tumb to follow when choosing between imlicit vs explicit arguments is whether they are business or techincal one. Auth parameters belongs to the first category.
because credentials were no longer passed implicitly there was no need to keep custom auth releated classes
json processing related instances are now imported via single import statement
fixed multipart requests
removed ApiModel class - it was artifical and redundant
added circe as another json library - circe is as popular as json4s if not more
removed sttpClientCodegen inheritance from akkaClientCodegen
extract classes to deal with cli properties - Dealing with them is quite cumbersome and what is worse it is error-prone. It is easy to make a mistake, especially when dealing with properties which mutually exlude themselfs. Having another layer of abstraction on top of them makes things much easier.
build.sbt was reduced to minimum

Despite breaking changes I opened this PR against master branch since the sttp generator is marked as beta. But let me know if that needs to be changed.

PR checklist

Read the contribution guidelines.
If contributing template-only or documentation-only changes which will change sample output, build the project beforehand.
Run the shell script ./bin/generate-samples.shto update all Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/config/java*. For Windows users, please run the script in Git BASH.
File the PR against the correct branch: master
Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Summoning technical committee:
@clasnake @jimschubert , @shijinkui , @ramzimaalej , @chameleon82 , @Bouillie

- remove apiModel - do not preinline response type - define sbt version - replace ApiRequestT with Request - update sttp version

chameleon82

@ghostbuster91 thanks for the PR
Had no chance to test compiled changes yet, but generally there is a lot of good improvements. Thanks
I leaved some comments and will try to check compiled client soon.

modules/openapi-generator/src/main/resources/scala-sttp/build.sbt.mustache

chameleon82 · 2020-06-21T14:40:42Z

...enapi-generator/src/main/java/org/openapitools/codegen/languages/ScalaSttpClientCodegen.java

+
+        @Override
+        public void updateAdditionalProperties(Map<String, Object> additionalProperties) {
+            if (additionalProperties.containsKey(name)) {


seems like an error as mainPackage is missed here.
also when you fix this next scenario will probably failed:
"--mainPackage=my.custom.package&--apiPackage=my.custom.api.different.package"
because Map can have those properties in unwanted order
Could you please cover this method with unit tests?

Hmm, this is definitively a good idea to cover it with some tests

It isn't an error since the mainPackage property isn't used anywhere. All packages are specified by apiPackage, modelPackage and invokerPackage properties

chameleon82 · 2020-06-21T14:42:11Z

...enapi-generator/src/main/java/org/openapitools/codegen/languages/ScalaSttpClientCodegen.java

+        importMapping.remove("Set");
+        importMapping.remove("Map");
+
+        typeMapping = new HashMap<>();


some of those imports and mappings already exists in AbstractScalaCodegen. Is it possible to reuse them instead of overwrite with the same values?

Actually there are not the same. In AbstractScalaCodegenonly importMappings are defined, while here we remove some of them and redefine them as typeMappings or instantiationTypes.

samples/client/petstore/scala-sttp/src/main/scala/org/openapitools/client/api/PetApi.scala

chameleon82 · 2020-06-21T14:58:48Z

samples/client/petstore/scala-sttp/src/main/scala/org/openapitools/client/core/ApiInvoker.scala

-    */
-  implicit class ApiRequestImprovements[R[_], T](request: RequestT[Identity, Either[ResponseError[Exception], T], Nothing]) {
-
-    def result(implicit backend: SttpBackend[R, Nothing, Nothing]): R[T] = {


Could we have kinda helper to get final results? Otherwise it could lead code duplication in applications or be implemented there

The way sttp-client is normally used (at least, what we've seen on gitter, issue reports etc.) is that people simply call .send() with an implicit backend in scope. What kind of functionality would the helper add?

I think that helper can be on the app side as error transformations can be different from app to app.
The main idea of that helper is to get successful result directly. For example for Try framework it could be something like

val pet: Pet = new PetApi("http://swagger").getPetById(1).result()

instead of code like next for every method

new PetApi("http://swagger").getPetById(1).send() .flatMap { case Right(pet) => pet doSomething(pet) case ... }

For that purpose we've introduced the separateErrorChannel option: using sttp's mechanism (response specifications), requests can be modeled to return full error information (so the response will be roughly an Either[Error, Success]), or the errors can be represented as failed effects (as the errors are Throwables).

Of course people might want to generate code using full error information and then flatten the result anyway. Though here, as you write, it's quite an application-specific thing. I think if I wanted to flatten the results, I'd write an extension method, which would convert F[Either[Error, Success]] into a F[Success], e.g.:

new PetApi("http://swagger").getPetById(1).send().flattenErrors

where

implicit class TryOps[E <: Throwable, S](r: Try[Either[E, S]]) { def flattenErrors: Try[S] = r.flatMap { case Left(e) => Failed(e) case Right(s) => Success(s) } }

but I think this would belong to the specific app - not in the generated code. Otherwise we should have this in sttp directly :)

chameleon82 · 2020-06-21T15:05:17Z

modules/openapi-generator/src/main/resources/scala-sttp/api.mustache

      .body({{paramName}}){{/bodyParam}}
-      .response(asJson[{{>operationReturnType}}])
+      .response({{#separateErrorChannel}}asJson{{/separateErrorChannel}}{{^separateErrorChannel}}asJsonAlwaysUnsafe{{/separateErrorChannel}}[{{>operationReturnType}}])


Looks good. Could you help to separate api errors as exceptions as described here softwaremill/sttp-model#5 ? The main idea is to split application exceptions (ideally with models) from transport level exceptions by using expected status codes.
Thanks

Transport exceptions are already modeled using SttpClientException. These exceptions are thrown when read or connection exceptions occur, and result in a failed effect (failed Future or IO).

Though you are right that there's a piece missing here, if the OpenAPI description contains an e.g. JSON representation of an error? Then yes, we will need a custom hierarchy (as opposed to the standard ResponseError), covering three possibilities:

DeserializationError[T](body: String, error: T) - if deserializing the success or error json fails

UnknownHttpError(body: String) - for unmapped status codes

HttpError[T](body: T) - for mapped status codes

Then, the overall request type would be: Request[Either[ResponseError[T, U], V]], where:

T is the type of deserialization errors, e.g. io.circe.Error

U is the type of the http errors - but as there might be a couple of these, probably this needs to be a sealed trait, which is a implemented by all possible errors for this endpoint? This could be tricky, as each endpoint might have different error cases (e.g. NotFound).

V is the type of the deserialized http success value (e.g. User)

Of course we'd need better naming for these :)

how about exposing other http metadata inside errors, like status / status code, headers (some APIs expose debug/tracing information), hostname, etc?

another question (and I'm not sure whether OpenAPI Spec supports it) to return same info for correct (V) responses

That's always returned in the Response object which wraps the body. So the type of the result of a call is e.g. Response[Either[ResponseError, V]], where V is the type of success

chameleon82 · 2020-06-22T11:56:24Z

@ghostbuster91 I've tested compiled code, looks very good.
As a suggestion it might be good to have simple example of simple app for beginners in README file.
Please also check comments above and re-trigger CI's

ghostbuster91 · 2020-06-23T05:51:28Z

@chameleon82 Thanks for the review and your suggestions, I'm going to start working on it.

…class

chameleon82 · 2020-06-24T03:05:48Z

modules/openapi-generator/src/main/resources/scala-sttp/project/build.properties.mustache

@@ -0,0 +1 @@
+sbt.version=1.2.4


This file/version might be the cause of CI build failed with next error:

https://app.circleci.com/pipelines/github/OpenAPITools/openapi-generator/3736/workflows/1d16cd64-dd72-4f2b-8a68-92adfbdb29b7/jobs/17104/steps

[warn] :::::::::::::::::::::::::::::::::::::::::::::: [warn] :: UNRESOLVED DEPENDENCIES :: [warn] :::::::::::::::::::::::::::::::::::::::::::::: [warn] :: org.scala-sbt#compiler-bridge_2.13;1.2.3: not found [warn] :::::::::::::::::::::::::::::::::::::::::::::: [error] ## Exception when compiling 14 sources to /home/circleci/OpenAPITools/openapi-generator/samples/client/petstore/scala-sttp/target/scala-2.13/classes [error] The compiler bridge sources org.scala-sbt:compiler-bridge_2.13:1.2.3:compile could not be retrieved. [error] [error] Note: Unresolved dependencies path: [error] org.scala-sbt:compiler-bridge_2.13:1.2.3 [error] +- org.scala-sbt.temp:temp-module-52f238aede5a1d8d7a484c2bec27a144810e2ac0:1.2.3

Could you try sbt.version=1.2.8? Should we even specify this version explicitly?

Yes, the sbt version should be specified in project/build.properties so that the version is fixed and not platform-dependent. However, we should probably use 1.3.12, which is the newest one.

eugeniyk · 2020-06-26T04:16:09Z

...enapi-generator/src/main/java/org/openapitools/codegen/languages/ScalaSttpClientCodegen.java

+            "joda-time library", "2.10.6");
+    private static final StringProperty JSON4S_VERSION = new StringProperty("json4sVersion", "The version of json4s " +
+            "library", "3.6.8");
+    private static final StringProperty CIRCE_VERSION = new StringProperty("circeVersion", "The version of circe " +


oh that's nice
0.12+ stops supporting scala 2.11

eugeniyk · 2020-06-26T04:55:29Z

modules/openapi-generator/src/main/resources/scala-sttp/api.mustache

      .body({{paramName}}){{/bodyParam}}
-      .response(asJson[{{>operationReturnType}}])
+      .response({{#separateErrorChannel}}asJson{{/separateErrorChannel}}{{^separateErrorChannel}}asJsonAlwaysUnsafe{{/separateErrorChannel}}[{{>operationReturnType}}])


how about exposing other http metadata inside errors, like status / status code, headers (some APIs expose debug/tracing information), hostname, etc?

eugeniyk · 2020-06-26T04:56:42Z

modules/openapi-generator/src/main/resources/scala-sttp/api.mustache

      .body({{paramName}}){{/bodyParam}}
-      .response(asJson[{{>operationReturnType}}])
+      .response({{#separateErrorChannel}}asJson{{/separateErrorChannel}}{{^separateErrorChannel}}asJsonAlwaysUnsafe{{/separateErrorChannel}}[{{>operationReturnType}}])


another question (and I'm not sure whether OpenAPI Spec supports it) to return same info for correct (V) responses

samples/client/petstore/scala-sttp/build.sbt

Co-authored-by: eugeniyk <[email protected]>

ghostbuster91 · 2020-06-29T11:33:01Z

@chameleon82 I think that I fixed/reviewed all your suggestions apart from translating application's errors from openapi to sttp.
As @adamw mentioned this might not be a trivial thing to do as each endpoint can have its own errors' hierarchy.

Still we think that it would be a great thing to have, so we would like to implement it, but to not extend this PR too much, we would like to do it in a separate one. Also to make this task a little bit easier we will first introduce some modification to the core of sttp (see softwaremill/sttp#618).

Having said that, let me know if there is still anything which should be done before we merge this PR?

ghostbuster91 · 2020-06-29T11:47:33Z

@ghostbuster91 I've tested compiled code, looks very good.
As a suggestion it might be good to have simple example of simple app for beginners in README file.
Please also check comments above and re-trigger CI's

Is there any other backend which has such simple example app in the README so I can look at it?

chameleon82 · 2020-06-29T16:35:59Z

Still we think that it would be a great thing to have, so we would like to implement it, but to not extend this PR too much, we would like to do it in a separate one. Also to make this task a little bit easier we will first introduce some modification to the core of sttp (see softwaremill/sttp#618).

Yeah, that definitely better to create separate PR for this improvement

Is there any other backend which has such simple example app in the README so I can look at it?

Not sure we have good example.

@wing328 how do you think about having some "basic example" in generated README file or to have specific client documentation somewhere? It might be not easy for newbies to start using generated client properly

Having said that, let me know if there is still anything which should be done before we merge this PR?

Imo looks pretty good. Approved.

@jimschubert could you help with review and merge?

jimschubert

Looks good, just a few minor comments.

jimschubert · 2020-07-01T01:10:03Z

...enapi-generator/src/main/java/org/openapitools/codegen/languages/ScalaSttpClientCodegen.java

+        }
+    }
+
+    private static abstract class CustomLambda implements Mustache.Lambda {


Note that at we have well-tested lambdas in the org.openapitools.codegen.templating.mustache package which handle camel case, title case, uppercase, lowercase, etc. We should probably move these to use the tested lambdas at some point.

I agree. I just copy-pasted it from scala-akka generator AFAIR. We should change it there as well.

jimschubert · 2020-07-01T01:11:22Z

modules/openapi-generator/src/main/resources/scala-sttp/requests.mustache

@@ -1,48 +0,0 @@
-{{>licenseInfo}}


removal of these built-in templates is a breaking change, so this needs to get into 5.0

Is compatibility maintained between generators which aren't stable? (sttp generator is marked as beta)

jimschubert · 2020-07-01T01:12:59Z

samples/client/petstore/scala-sttp/build.sbt

 )

 scalacOptions := Seq(
  "-unchecked",
  "-deprecation",
  "-feature"
 )
-
-publishArtifact in (Compile, packageDoc) := false


We should consider adding this back. Default behavior should be to avoid accidentally publishing content. Users can then configure publishing and ignore the build file, or enable published at invocation.

Right, well our goal here was to create a minimal working project, keeping things as simple as possible, usable for an application developer (who doesn't normally publish artifacts, or has a custom publishing pipeline anyway). But maybe our approach here is incorrect - are there some guidelines as to what should be the functionality of the generated build?

Thanks!

jimschubert · 2020-07-01T01:14:23Z

I left a few minor comments. We can address them at any point in the future, like next time we modify this generator.

adamw · 2020-07-01T06:17:01Z

Thanks for merging!

As a next step, as @ghostbuster91 mentioned, we'd like to work on improving the typed-errors in sttp-client itself, and then building on that, improving the generator.

Another direction is creating an sbt plugin which would generate the client as part of an existing project's build (so not a self-contained build, but only the sources, much like e.g. the gRPC sbt plugin works). This is what we described in #6685. Is this something that we could work on as part of OpenAPITools, and if so, should this be scoped only to this generator, or maybe it would be usable by other generators as well?

* master: (142 commits) update python samples clarify direction of py client side validation flag (#6850) fix erronous cmd arg example for docker in readme (#6846) [BUG] [JAVA] Fix multiple files upload (#4803) (#6808) [kotlin][client] fix retrofit dependencies (#6836) [PowerShell] add more fields to be customized (#6835) [Java][WebClient]remove the dead code from java ApiClient.mustache (#6556) [PHP] Better handling of invalid data (array) (#6760) Make ApiClient in retrofit2 be able to use own OkHttpClient (#6699) mark python2 support in flask as deprecated (#6653) update samples [Java][jersey2] Add a getter for the User-Agent header value (#6831) Provides a default nil value for optional init parameters (#6827) [Java] Deprecate feignVersion option (#6824) [R] Enum R6Class Support, closes #3367 (#5728) [Rust][Client] Unify sync/async client structure (#6753) [php-ze-ph] Set required PHP version to ^7.2 (#6763) [Java][client][native][Gradle] Add missing jackson-databind-nullable (#6802) Improve sttpOpenApiClient generator (#6684) Update docker-tag-latest-release.yml ...

ghostbuster91 added 30 commits June 16, 2020 15:44

Base changes

a09822e

- remove apiModel - do not preinline response type - define sbt version - replace ApiRequestT with Request - update sttp version

Fix rendering too many colons

a412444

Fix formatting

e8c3890

Allow sttp version customization

e3a0654

Add option to return errors in terms of monadError

4ac8894

Add option to customize jodaTime version

88908e2

Add option to customize json4s version

5c255e4

Simplify jsonHandling

6c5d792

Remove helpers as such functionality was added to sttp

3b27bc5

Rename file

cac19fc

Fix uploading multipart data

86ec499

Extract common abstraction for generator properties

97fd17f

Add partial circe support

a7a15a2

Remove unsed method

551bb35

Refactor picking up json library

2a91b11

Add missing DateSerializers for circe

a380531

Simplify credentials

5cab35d

Simplify code

714f930

Update docs

a5f0a05

Fix auto formatting

dfca277

Simplify code

5efad87

SttpCodegen should not depend on akka

3c836f9

Rewrite properties handling

a377921

Fix mainProperty application

d176eeb

Pass credentials to methods as regular parameters

f7c384c

Put auth params first

4a5ff27

[Sttp] SeparateErrorChannel is true by default

e2df636

[Sttp] Simplify generated build.sbt

b3017bb

[Sttp] Regenerate project

86150cb

[Sttp] Update default version to 2.2.0

4588362

ghostbuster91 force-pushed the better-sttp branch from 72247e9 to 4588362 Compare June 16, 2020 13:47

ghostbuster91 changed the title ~~Better sttp~~ Improve sttpOpenApiClient generator Jun 16, 2020

Regenerate petstore sample

b572673

ghostbuster91 mentioned this pull request Jun 16, 2020

[REQ] Allow generating only src/main/scala in sttp generator #6685

Open

wing328 added Client: Scala Enhancement: General labels Jun 16, 2020

chameleon82 reviewed Jun 21, 2020

View reviewed changes

ghostbuster91 added 4 commits June 23, 2020 08:15

Restore cross-compilation to scala 2.12

91dd9b0

Restore syntax with companion object for api definitions

9242bc4

Remove specifying reservedWords as it was already specified in upper …

de82caf

…class

Refactor mainPackageProperty and add tests for it

d868eda

chameleon82 reviewed Jun 24, 2020

View reviewed changes

eugeniyk reviewed Jun 26, 2020

View reviewed changes

ghostbuster91 and others added 2 commits June 29, 2020 13:04

Update samples/client/petstore/scala-sttp/build.sbt

1f9cd3e

Co-authored-by: eugeniyk <[email protected]>

Update sbt to 1.3.12

a6647d2

ghostbuster91 mentioned this pull request Jun 29, 2020

Extend ReponseError to allow arbitrary types for error case softwaremill/sttp#618

Closed

Run bin/utils/ensure-up-to-date and commit changes

8f5a98f

chameleon82 approved these changes Jun 29, 2020

View reviewed changes

jimschubert approved these changes Jul 1, 2020

View reviewed changes

jimschubert added this to the 5.0.0 milestone Jul 1, 2020

jimschubert added the Breaking change (without fallback) label Jul 1, 2020

jimschubert merged commit 323cd38 into OpenAPITools:master Jul 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve sttpOpenApiClient generator #6684

Improve sttpOpenApiClient generator #6684

ghostbuster91 commented Jun 16, 2020 •

edited

Loading

chameleon82 left a comment

chameleon82 Jun 21, 2020

ghostbuster91 Jun 23, 2020

ghostbuster91 Jun 23, 2020

chameleon82 Jun 21, 2020

ghostbuster91 Jun 23, 2020 •

edited

Loading

chameleon82 Jun 21, 2020

adamw Jun 22, 2020

chameleon82 Jun 22, 2020

adamw Jun 22, 2020

chameleon82 Jun 21, 2020

adamw Jun 22, 2020

eugeniyk Jun 26, 2020

eugeniyk Jun 26, 2020

adamw Jun 26, 2020

chameleon82 commented Jun 22, 2020

ghostbuster91 commented Jun 23, 2020

chameleon82 Jun 24, 2020

adamw Jun 24, 2020

eugeniyk Jun 26, 2020

eugeniyk Jun 26, 2020

eugeniyk Jun 26, 2020

ghostbuster91 commented Jun 29, 2020

ghostbuster91 commented Jun 29, 2020 •

edited

Loading

chameleon82 commented Jun 29, 2020

jimschubert left a comment

jimschubert Jul 1, 2020

ghostbuster91 Jul 1, 2020

jimschubert Jul 1, 2020

ghostbuster91 Jul 1, 2020

jimschubert Jul 1, 2020

adamw Jul 1, 2020

jimschubert commented Jul 1, 2020

adamw commented Jul 1, 2020

		@@ -0,0 +1 @@
		sbt.version=1.2.4

Improve sttpOpenApiClient generator #6684

Improve sttpOpenApiClient generator #6684

Conversation

ghostbuster91 commented Jun 16, 2020 • edited Loading

PR checklist

chameleon82 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghostbuster91 Jun 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chameleon82 commented Jun 22, 2020

ghostbuster91 commented Jun 23, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghostbuster91 commented Jun 29, 2020

ghostbuster91 commented Jun 29, 2020 • edited Loading

chameleon82 commented Jun 29, 2020

jimschubert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jimschubert commented Jul 1, 2020

adamw commented Jul 1, 2020

ghostbuster91 commented Jun 16, 2020 •

edited

Loading

ghostbuster91 Jun 23, 2020 •

edited

Loading

ghostbuster91 commented Jun 29, 2020 •

edited

Loading