Skip to content

New generator for Scala3 + sttp4 + jsoniter-scala #20006

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

Open
wants to merge 15 commits into
base: master
Choose a base branch
from

Conversation

lbialy
Copy link

@lbialy lbialy commented Oct 31, 2024

I have extended scala-sttp4 to work with Scala 3 and latest sttp4 milestone (M19) using jsoniter-scala as the JSON layer. This work is based on scala-sttp4 generator and I've fixed a lot of issues and bugs it had. It's still far from being perfect and it does contain some hacks that were necessary to generate a compiling package for the Mattermost OpenAPI 4.0 doc (most notably - it tries to infer path parameters that are not provided in OAS document and types them as Strings). It works for the petstore example too. Are there any other sample OpenAPI docs I should test it against? I would like to provide a modern generator of efficient OpenAPI client libs for Scala 3 so I can improve this further if there are any issues.

I specifically submit this as a separate generator as I don't want to merge it with scala-sttp4 because maintenance of Scala 2 / Scala 3 generator would be quite troublesome, especially around enums. This could be also called scala3-sttp4.

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh ./bin/configs/*.yaml
    ./bin/utils/export_docs_generators.sh
    
    (For Windows users, please run the script in Git BASH)
    Commit all changed files.
    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/configs/java*.
    IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
  • File the PR against the correct branch: master (upcoming 7.x.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@clasnake @shijinkui @ramzimaalej @chameleon82 @Bouillie @Fish86

@wing328
Copy link
Member

wing328 commented Nov 1, 2024

thanks for the PR

what about adding samples similar to ./bin/configs/scala-sttp4.yaml and add the new sample folder to the workflow (https://github.com/OpenAPITools/openapi-generator/blob/master/.github/workflows/samples-scala.yaml)?

@lbialy
Copy link
Author

lbialy commented Nov 1, 2024

Let me do just that!

There's also a problem with classes generated having the same name with only case of some letters being different, that leads to warnings like this

Generated class org.sample.mm.OAuthApi$ differs only in case from org.sample.mm.OauthApi$ (defined in Oauth0Api.scala).
  Such classes will overwrite one another on case-insensitive filesystems.

and that's what does, in fact, happen on MacOS X. I'm wondering where I could tune that so that classname is also deduplicated like the nickname (processOperations in DefaultGenerator that uses getAddSuffixToDuplicateOperationNicknames alters nickname only and changing that would be a significant breaking change I think).

@wing328
Copy link
Member

wing328 commented Nov 1, 2024

There's also a problem with classes generated having the same name with only case of some letters being different, that leads to warnings like this

users can use operationIdNameMappings option to have complete control of the naming: https://github.com/openapitools/openapi-generator/blob/master/docs/customization.md#name-mapping

@lbialy
Copy link
Author

lbialy commented Nov 1, 2024

That does not seem to work for top level classes with *Api suffix, I tried both:
--name-mappings Ldap=LDAPGroup,Oauth=OAuthConnection,OAuth=OAuthApp \
and
--name-mappings LdapApi=LDAPGroupApi,OauthApi=OAuthConnectionApi,OAuthApi=OAuthAppApi \

I still get:

Generated class org.sample.mm.LDAPApi differs only in case from org.sample.mm.LdapApi (defined in Ldap0Api.scala).
  Such classes will overwrite one another on case-insensitive filesystems.

and generated classes are:

object LDAPApi {
  def apply(baseUrl: String = "http://your-mattermost-url.com") = new LDAPApi(baseUrl)
}

class LDAPApi(baseUrl: String) {
  ... operations ...

in LDAPApi.scala vs

object LdapApi {
  def apply(baseUrl: String = "http://your-mattermost-url.com") = new LdapApi(baseUrl)
}

class LdapApi(baseUrl: String) {
  ... operations ...

in Ldap0Api.scala (notice the deduplicated nickname used for file name). I've tried finding the models that describe that in
postProcessAllModels but I haven't been able to locate them. This is in api.mustache:

{{#operations}}
object {{classname}} {
  def apply(baseUrl: String = "{{{basePath}}}") = new {{classname}}(baseUrl)
}

class {{classname}}(baseUrl: String) {

@lbialy
Copy link
Author

lbialy commented Nov 1, 2024

Ok, I fixed the classname collision problem after a lengthy analysis of DefaultGenerator. There's no post-processing for the instance of OperationsMap that gets rendered to files from what I can see but with a hacky, side-effecting override of toApiName method I think I was able to generate a unique api name value per tag.

I'll take a look at ./bin/configs/scala-sttp4.yaml next.

@lbialy
Copy link
Author

lbialy commented Nov 13, 2024

This is not dead btw, I'm fine-tuning the generator by generating working libraries for
a) mattermost
b) kubeapi
c) stripe
d) github
e) spotify

a) and b) are done, stripe is a bit tricky, d) and e) are close to working.

@wing328
Copy link
Member

wing328 commented Nov 16, 2024

@lbialy thanks for the update.

let us know if you need help from us to move this forward.

@wing328
Copy link
Member

wing328 commented Nov 16, 2024

also cc @flsh86 and @chameleon82 for their feedback

do we need another generator for jsoniter-scala? or it should become the default in scala-sttp4 client generator?

@lbialy
Copy link
Author

lbialy commented Nov 17, 2024

I'm of the opinion that it should be the default for Scala 3 but I guess we can't just drop circe/json4s, people might be using that.

On the other hand scala-sttp and scala-sttp4 seem to generate broken code for all form-based stuff and I'm in the process of completely rewriting that because it wasn't even compiling. There's also the oas 2 vs oas 3 thing between collection format and style/expand that requires handling and isn't done in any of the scala generators (just oas 2 / swagger is supported). I think I also spotted an issue in specs that have multiple auth styles - all of them are required simultaneously in the generated code so I'm working on fixing that too.

@chameleon82
Copy link
Contributor

Thanks for supporting scala3 community and making those changes @lbialy !

I do believe that might be better to keep one generator scala-sttp4 but enhance openapi-generator with language version option and provide scala-3 as default value. Scala is a bit annoying in terms of backward compatibility (we don't need to support scala 2.11/2.12 as those reach eol but 2.13/3).

That sounds more complicated than having 2 generators, but I believe it will help to drop scala 2.13 support in the future without deprecating generator, while keeping better maintainance.

Regarding jsoniter-scala - it make sense to make it default for sttp4. But circe and json4s are commonly used ones, we also need to keep them for a while.

@wing328
Copy link
Member

wing328 commented Dec 10, 2024

@lbialy can you please add samples similar to ./bin/configs/scala-sttp4.yaml and add the new sample folder to the workflow (https://github.com/OpenAPITools/openapi-generator/blob/master/.github/workflows/samples-scala.yaml)?

@lbialy
Copy link
Author

lbialy commented Dec 10, 2024

@lbialy Yeah, sure.

I had to switch my attention to another task but I'm still working on this. @chameleon82 I don't think it would make sense to merge Scala 2 and Scala 3 versions exactly because of the maintenance effort required. Let me elaborate a bit. The implementations for Scala 2 are using an untyped mechanism that takes collections of Any (e.g.: Map[String, Any]) for forms and query params and then pattern match on that and stringify values with .toString(). This approach has been started with akka-based generators first from what I was able to deduce and it was something that people did about twelve to ten years ago because metaprogramming was doing its first baby steps back then. That approach is, unfortunately, not even applied consistently (at least in the sttp-based family of generators) and for some cases it just explodes in compile time or does something completely nonsensical in runtime (like stringifying a case class as a query param). What I'm doing in this provider is using metaprogramming (inlines, a Scala 3 feature, to be precise) to derive correct typeclass instances in the generated code for all params and datatypes used in argument position so that we can have generated code that is correct by construction. Now if I wanted to support Scala 2 simultaneously I'd need to add a boatload of branching based on the version of Scala used in all templates and add special files containing metaprogramming definitions (inlines for S3, macros for S2) that would also be included based on the same condition (S2 vs S3 used). This gets very brittle and hard to verify very fast.

@max-peroch
Copy link

Thanks so much for your work on this @lbialy! Sorry for the very naive question, I haven't looked at the code yet, but without making this PR about supporting simultaneously both Scala 2 & Scala 3, do you think any of your code here could benefit the Scala 2 version? Thinking specifically about oneOf support.

@lbialy
Copy link
Author

lbialy commented Feb 12, 2025

This work is being continued by my peer from SoftwareMill - @Kamil-Lontkowski who is building the missing uniform and compile-time-safe mechanism to serialize function arguments to path params and forms. That functionality was a bit borked in all previous implementations as far as I know. Is oneOf related to parameter serialization?

@max-peroch
Copy link

Oops, my bad I misread your previous comments, I thought that your rewrite would enable the support for more features given the different approach

@Kamil-Lontkowski
Copy link

It will support more features from v3 (for example regarding parameter serialization), but I did not look into inheritance and polymorphism yet so I am not able to tell if it could be supported. I will look into that but right now I don't really know.

…rations, fix file response. change serialization from Map to Seq to avoid deleting duplicate keys
@wing328
Copy link
Member

wing328 commented Feb 19, 2025

@lbialy
Copy link
Author

lbialy commented Mar 18, 2025

Ok, so this has moved along a bit, here's report from @Kamil-Lontkowski:

Here’s the English translation:


Unfortunately, this is not finished, but I think we managed to solve a few problems for sure, and also, not everything has to be implemented by this code generator.

What has been done:

  • Safe serialization of form parameters according to the specification
  • Support for Option[File]
  • Correct enum generation (at least the obvious cases)
  • Upgraded Sttp version to RC1
  • Fixed compilation errors related to the upgrade (ResponseException and asFile)
  • Serialization of query parameters
  • No codec for classes from java.time
  • Name conflicts with None
  • Auth support
  • Serialization of cookies, path parameters, and headers

What still needs to be done:

Blockers:

  • formatIdentifier formats names in such a way that duplicate cases often appear in enums – this has been partially solved, but formatIdentifier still messes up some field names in models.
  • Multipart (there are differences between 3.0 and 3.1, and it’s unclear how parameters should be serialized).
  • Enum serialization in query parameters and form data (we need to check how this works with possible jsoniter codecs – jsoniter codecs are currently used for enums).

Other issues:

  • Enums are not generated for models that only appear in parameters – they are just treated as string (not the worst case).
  • If polymorphism is disabled, in one place the isEnum flag is incorrectly set, causing the enum not to be generated.
  • It should be possible to add support for cookies with minimal effort – Done.
  • There are still some settings related to time libraries. Since Scala 3 runs on Java 8+, we should probably use java.time API and remove the rest.
  • Not sure if, when an API supports both XML and JSON, JSON is always listed first (the template assumes so, but we should verify this).
  • application/x-www-form-urlencoded theoretically supports serialization like JSON (also in multipart).
  • Serialization of LocalDate or OffsetDateTime parameters – not checked if it is correct.
  • Multipart allows defining file formats like Base64 – this is not supported yet.
  • Polymorphism considerations:
    • allOf is just inheritance; if the system can translate it into a full model, there’s no issue.
    • oneOf is an ADT; jsoniter supports this, but the question is whether it requires a discriminator.
    • anyOf generally works as "try to deserialize the listed models until one works" – not sure how to handle this.
  • It’s fairly easy to add support for parameter serialization using FromSerializable with typeclasses. If such a typeclass is in scope, it will override the default serialization attempt – this could help with all the weird cases in Stripe.
    • Typeclasses for parameter serialization have been added – they just need to be in scope for a given endpoint.

Overall, it seems that Stripe requires a closer look at what’s actually there. For example, there are probably recursive types, which prevent jsoniter from creating codecs – it keeps running until memory runs out.

Current status at commit 1a630aa:

Successfully compiles:

  • mattermost
  • petstore (v2 and v3)
  • spotify

In kubeapi, there’s a strange issue where one parameter ends up as a form parameter in the template data instead of a path parameter – this seems to be a codegen bug, and nothing can be done about it.

GitHub does not compile due to two missing enums and incorrect field names in the model (specifically, there are fields like "+1" and "-1").

In Stripe, the main issue appears to be with codecs – recursive types in jsoniter and missing serialization for cases that don’t conform to the standard.

@lbialy
Copy link
Author

lbialy commented Mar 18, 2025

@Kamil-Lontkowski mentioned that we should probably try and get this merged as it's usable for many cases already and then start fixing bugs as they show up in real use cases. I know I would still like to get more complex things like full Stripe API library working, albeit it seems that they are using out-of-openapi-spec things there that can be now handled by providing custom typeclass instances for the new serialization mechanics Kamil has built.

There are some upstream issues like the enum naming logic. Maybe we should open issues against that, I'll try to find some time to do that but definitely not in the scope of this PR.

@lbialy
Copy link
Author

lbialy commented Mar 18, 2025

Could we get this reviewed? 👓

@adamw
Copy link

adamw commented Apr 1, 2025

If there would be a possibility to get this merged it would be awesome :) We're almost there with sttp-client 4 release (the - hopefully - final RC is now published), so having a generator would be a perfect complement to the release. Also, @wing328 if there's something that we could do on our side to streamline the process, just let us know!

@chameleon82
Copy link
Contributor

@lbialy , @Kamil-Lontkowski I've rawly reviewed the code and found - there is not much difference with scala-sttp4 (mostly functions to support scala3) @xuwei-k could you help join to review this MR, maybe we can elaborate changes into single template?

@wing328 there are quite differences between Scala2 and Scala3, need your suggestion how we can continue on it? Maybe we can have AbstractScala3Codegen extends AbstractScalaCodegen? then scala3-sttp4 makes sense to me.
Another way - is to have 2 sets of mustache templates for the same scala-sttp4 with different configs.

@Kamil-Lontkowski please add examples to see if those passing tests as @wing328 suggested in the beginning.
I also concerning on why this template should be locked on jsoniter and do not support other serializators?

@adamw on a side note, looking forward for sttp4 release :) It would be also great to have otel auto-instumentation for otel feature

@adamw
Copy link

adamw commented Apr 8, 2025

@chameleon82 the release is there, no auto-instrumentation though - I still do feel that's totally not in the "spirit" of Scala's ecosystem - where you rather opt into explicitly setting up dependencies, than classpath "magic". Still, it's a matter of adding a wrapper to your backend instance in the setup of your app, so the overhead should be quite minimal :)

@lbialy
Copy link
Author

lbialy commented May 7, 2025

bump!

@dwischolek-modulon
Copy link
Contributor

May I suggest a different approach? The current sttp4 generator is not working with the final release version, anyways. I made some changes to use it in-house, which I wanted to make into a pull request later this week, but...the current sttp4 generator is a mess, after all. To get it to correctly handle different "provides" and "consumes" and some types correctly is a major rewrite. I would suggest merging this pull request as is and use this as the basis for further development. The old sttp4 generator stays in place (for now) for everyone in need of an outdated and broken generator, that nonetheless uses other json parsers. I will then commit some time in this generator here to add the other parsers (and maybe scala 2 support) in a clean way and then this generator can become the new sttp4 (or better yet sttp) generator.

@chameleon82
Copy link
Contributor

@lbialy

what about adding samples similar to ./bin/configs/scala-sttp4.yaml and add the new sample folder to the workflow (https://github.com/OpenAPITools/openapi-generator/blob/master/.github/workflows/samples-scala.yaml)?

may we have examples generated as @wing328 suggested?

@lbialy
Copy link
Author

lbialy commented May 13, 2025

uh, I thought I did that, I'll take a look today

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.

7 participants