Skip to content
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

New generator - Scala Play Framework #2421

Merged
merged 16 commits into from
Mar 26, 2019

Conversation

adigerber
Copy link
Contributor

@adigerber adigerber commented Mar 15, 2019

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, ./bin/openapi3/{LANG}-petstore.sh, ./bin/security/{LANG}-petstore.sh and ./bin/openapi3/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.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This is work in progress!

This PR introduces a new generator definition - Scala Play Framework.

Features

  • Models (including JSON formats)
    • Basic properties (primitives)
    • Container properties (map/list)
    • Marking properties as required (by default properties are generated using Scala's Option)
    • Objects with additionalProperties
    • Definitions declared with type: array
    • Date/time properties
    • File properties (might skip in this PR) Edit: operations support files, models do not
  • API
    • Base traits
    • Stubs
    • Controllers - Done, but needs to be tested in runtime for correctness
    • Default module (for DI)
    • Routes
    • Error handler
  • Support files
    • Minimal SBT
    • README
    • LICENSE removed
    • Logging
    • Application configuration (including Play configuration)
    • API docs
  • Generation flags/additional properties
    • Generate Skip generation of stubs
    • Generate async API
    • Generate API docs
    • Skip routes generation Edit: since routes are essential for Play apps I chose not to implement this.
    • Set routes file name
    • Anything else I might've forgotten
  • Tests

CC (technical committee): @clasnake, @jimschubert, @shijinkui, @ramzimaalej

Migrated from swagger-codegen

Note this was originally written for swagger-codegen so there may be some remnants around in the code before this is fully complete.

Some issues that need attention:

  • Default values for stubs - check if given schema/property is required
  • When generating against modules/openapi-generator/src/test/resources/3_0/petstore.yaml, models InlineObject and InlineObject1 are generated but are not used anywhere at the moment Edit: removed these files by running the generator with -DskipFormModel=true
  • Enums are generated inside their parent object (e.g. Pet.Status), but there is no context of the parent object in the API operations so enumName and datatypeWithEnum do not reference the enum (e.g. Status instead of Pet.Status). Couldn't find a way to inject the parent model name anywhere. Edit: since java-play-framework does not convert enum params in operations this will not be done here either.
  • Friendly output of JsResultException (New generator - Scala Play Framework #2421 (comment))

Copy link
Member

@jimschubert jimschubert left a comment

Choose a reason for hiding this comment

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

Thanks for working on this new generator! I've left a few comments. I'll check back when the PR is no longer draft or if you are looking for additional feedback during implementation.

@@ -0,0 +1,7 @@
{{! Keep indentation at 4 spaces for proper formatting of the output. }}
Copy link
Member

Choose a reason for hiding this comment

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

I've found mustache to be inconsistent with indents in some situations. I don't remember the specifics, but I think indents may be dropped in files at an import depth greater than 1?

To resolve the issue I had created a lambda that you can reuse if the indents become an issue. See the kotlin-server generator. Example usage of the lambda: https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/kotlin-server/libraries/ktor/_api_body.mustache

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! I'll try experimenting more with the indents and if nothing works I'll give this method a shot.


public class ScalaPlayFrameworkServerCodegen extends AbstractScalaCodegen implements CodegenConfig {
public static final String TITLE = "title";
public static final String IMPL_STUBS = "implStubs";
Copy link
Member

Choose a reason for hiding this comment

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

I think this name may be confusing, as it indicates generation of stubbed implementations... but this is a default behavior in most generators that support interfaces.

I'm wondering if this is the same option in the Java Play generator, controllerOnly:

Whether to generate only API interface stubs without the server files.

If so, we may want to align the option name to avoid confusion for those evaluating the Play generation with different languages. This wouldn't be necessary in this PR, though. I just wanted to document the thought.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

implStubs is similar to controllerOnly in that they both affect whether stubs are generated, but controllerOnly also affects the interface in java-play-framework, while in this generator I wish to allow generating the trait regardless of the stubs. Perhaps I should make both configurable?

Also in the spirit of keeping things similar, I'll rename apiFutures to supportAsync.

@@ -0,0 +1,8 @@
This software is licensed under the Apache 2 license, quoted below.
Copy link
Member

Choose a reason for hiding this comment

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

We probably don't want to enforce a LICENSE in the template.

val {{.}} = Value{{/values}}{{/allowableValues}}

type {{enumName}} = Value
implicit lazy val {{enumName}}JsonFormat: Format[Value] = Format(Reads.enumNameReads(this), Writes.enumNameWrites[this.type])
Copy link
Member

Choose a reason for hiding this comment

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

May want to import Format and other Json related types at the object level here and/or above line 7, otherwise this assumes they exist in the including mustache file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a partial that's only used in model.mustache which has this import. I split the model into three main partials (arrayCaseClass.mustache, caseClass.mustache and caseObject.mustache) in an attempt to keep it somewhat cleaner.

@wing328
Copy link
Member

wing328 commented Mar 25, 2019

From the list, looks like many items have been completed. Great work @adigerber !

What about adding the auto-generated README and then we'll release a beta version to collect feedback from the community?

@wing328 wing328 added this to the 4.0.0 milestone Mar 25, 2019
@adigerber
Copy link
Contributor Author

@wing328 Sure, will do later today.

@adigerber
Copy link
Contributor Author

@wing328 I added the README, but I think there's more work to be done before even beta release, specifically - error handling (i.e. converting exceptions to suitable HTTP responses).
I've already started working on that a few days ago but haven't had the time to finish. I'll see if I can get it done within the next couple hours.

- Fix Indentation using mustache lambdas
- Option to set routes file name (default: routes) - allows uninterrupted manual maintenance of main routes file, which may include a subroute to the generated routes file
- Move supporting file classes to a package and update application.conf generation accordingly
- Option to generate custom exceptions (default: true) which are used in the controller to differentiate between API call exceptions and validation exceptions
- Generate error handler with basic exception mapping
- Option to generate API docs under /api route
- Reorder routes file so parameter-less paths are given priority over parameterized paths. Prevents case like /v2/user/:username activating before /v2/user/login (thus shadowing the login route completely) as observed using v3 petstore.yaml
- Option to set base package name (default: org.openapitools) to allow placing supporting files under a different package
@adigerber
Copy link
Contributor Author

I've finished most of the code generation I wanted to do.
I generated a clean sample (using bin/scala-play-framework-petstore.sh), which builds successfully and runs seemingly without problems (tested JSON body, query string and form parameters including file uploads).

One small fix I have planned is to correctly format JsResultException - when sending a request to an endpoint which expects a JSON object with required properties, if the requirements aren't met a JsResultException is thrown by Play's JSON library. This is caught in the generated error handler, which in turn responds with HTTP 400, but the output is not friendly:

JsResultException(errors:List((/photoUrls,List(JsonValidationError(List(error.path.missing),WrappedArray()))), (/name,List(JsonValidationError(List(error.path.missing),WrappedArray())))))

Apart from tests and the above fix this generator should be ready :-]

@wing328 wing328 marked this pull request as ready for review March 26, 2019 01:28
@wing328
Copy link
Member

wing328 commented Mar 26, 2019

@adigerber thanks. I'll take a look and merge it into master later today if no question from me.

@adigerber
Copy link
Contributor Author

@wing328 Sure. Something is preventing Circle CI from approving the PR though, so I'll see what I can do about it.

CC (technical committee; for final review): @clasnake, @jimschubert, @shijinkui, @ramzimaalej

@wing328
Copy link
Member

wing328 commented Mar 26, 2019

Tested locally and the result looks good.

@wing328
Copy link
Member

wing328 commented Mar 26, 2019

I'll update the doc and make some other minor enhancements in a separate PR

@wing328 wing328 merged commit 28ae33c into OpenAPITools:master Mar 26, 2019
@wing328
Copy link
Member

wing328 commented Mar 27, 2019

#2512 merged into master with some minor enhancements made to the Scala play generator.

@wing328
Copy link
Member

wing328 commented Mar 27, 2019

Tweet to promote the new generator: https://twitter.com/oas_generator/status/1110821644774178816

Reddit: https://www.reddit.com/r/scala/comments/b5zb53/generate_scala_play_server_stub_using_openapi/

Show HN: https://news.ycombinator.com/item?id=19497940

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