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

[core] Refactor templating management #6357

Merged

Conversation

jimschubert
Copy link
Member

@jimschubert jimschubert commented May 18, 2020

This refactors template management to get logic out of DefaultGenerator
and to provide a cleaner API to template search and read/compile.

Deprecates MockDefaultGenerator, which is not a mock and causes
in-memory retention of file contents. Maintainers should prefer
executing a "dryRun" with new DefaultGenerator(true) or do true
mock/spies if evaluating template intermediaries is truly necessary.
Tests may read written files with lower overhead than the in-process
retention of those bytes.

This attempts to maintain some compatibility with existing templating
adapter interfaces. Any breaking change here would be unintentional but
minimal effort to retarget the new interface.

PR checklist

  • Read the contribution guidelines.
  • If contributing template-only or documentation-only changes which will change sample output, build the project before.
  • Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
  • File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
  • Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

This refactors template management to get logic out of DefaultGenerator
and to provide a cleaner API to template search and read/compile.

Deprecates MockDefaultGenerator, which is not a mock and causes
in-memory retention of file contents. Maintainers should prefer
executing a "dryRun" with new DefaultGenerator(true) or do true
mock/spies if evaluating template intermediaries is truly necessary.
Tests may read written files with lower overhead than the in-process
retention of those bytes.

This attempts to maintain some compatibility with existing templating
adapter interfaces. Any breaking change here would be unintentional but
minimal effort to retarget the new interface.
The samples scripts for Java incorrectly referenced the libraries
directories directly rather than the upper-level Java directory. This
was incorrect usage of template directories, because the generator
expects to be given the "language" directory and perform a lookup for
missing templates in the order:

* user defined libraries directory
* user defined language root
* embedded libraries directory
* embedded language root
* _common directory

This is incorrect in our samples scripts because a user or maintainer
has the expectation that any template change to files at the Java/ root
should also be honored on generation if the script specifies a custom
template directory.
@jimschubert jimschubert marked this pull request as ready for review May 22, 2020 19:08
import java.util.Scanner;
import java.util.regex.Pattern;

public abstract class AbstractGenerator implements TemplatingGenerator {
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI breaking change, but we should really try sticking to interfaces and move away from abstract base types.

@@ -163,7 +163,6 @@ apiTemplateFiles are for API outputs only (controllers/handlers).
protected Map<String, String> reservedWordsMappings = new HashMap<String, String>();
protected String templateDir;
protected String embeddedTemplateDir;
protected String commonTemplateDir = "_common";
Copy link
Member Author

Choose a reason for hiding this comment

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

this is now handled by template loading functionality, as that is not the responsibility of DefaultCodegen.

* @param filename filename under root
*
* @return a {@link WrittenTemplateBasedFile}
* @deprecated Since 5.0. Please avoid this method and usage of {@link MockDefaultGenerator}, prefer {@link DefaultGenerator#DefaultGenerator(Boolean)} with dryRun=true.
Copy link
Member Author

Choose a reason for hiding this comment

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

MockDefaultGenerator duplicates all written bytes. getTemplateBasedFile is used to "inspect" data passed to templates, but this should be tested differently in the future.

* master:
  [samples] Regenerate python-experimental
  [core][general] Add metadata file tracking to aid in "Golden Tests" regeneration (OpenAPITools#6325)
  [python-experimental] Add support for pep 3134, attach cause of exception (OpenAPITools#6388)
  [Java-jersey2] Add new ApiClient constructor with auth objects (OpenAPITools#6393)
HandlebarseEngineAdapter previously didn't handle files without
extensions in the same was as the MustacheEngineAdapter. This now allows
for files without extension (or dotfiles) to lookup in the same
location.

Meta tasks are cleaned up to use template manager only, rather than
attempting to create an "empty" generator to use the previous templating
specific methods.
@jimschubert
Copy link
Member Author

Note on breaking change without fallback… removal of AbstractGenerator has no fallback abstract class and requires template management construction similar to what is done in this PR.

* master:
  update java jersey2 samples
  [Java] Fix mustache tag in pom template for HTTP signature (OpenAPITools#6404)
  [Python-experimental] Rename from_server variable to json_variable_naming (OpenAPITools#6390)
  Add a link to medium blog post (OpenAPITools#6403)
  Clean up debug in test (OpenAPITools#6398)
  readding bin/swift5-petstore-readonlyProperties.json
  remove ./bin/swift5-petstore-readonlyProperties.json
* master:
  decomission nodejs server generator (OpenAPITools#6406)
  [Java] Generate valid code if no Authentication implementations present (OpenAPITools#5788)
@jimschubert jimschubert added this to the 5.0.0 milestone May 23, 2020
@jimschubert
Copy link
Member Author

@OpenAPITools/generator-core-team

Copy link
Member

@wing328 wing328 left a comment

Choose a reason for hiding this comment

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

The following tests are good:

  • used -t
  • used -t with --library option
  • removed template files and got an exception
  • removed customized template files and fell back to the default
  • wrong template folder (not exist) resulted in an exception

@jimschubert
Copy link
Member Author

@spacether would you like to take a look before merge?

cc @Fjolnir-Dvorak (who's waiting on this work)

@jimschubert
Copy link
Member Author

@wing328 I'd created a GitHub Workflow to cross-compile in linux and Windows, then run a smoke test for the gradle build of the go client which was failing on AppVeyor. This allowed me to finish up more quickly as I was seeing 1.5 hours in queue and 30-45 minutes on AppVeyor. We may want to consider a cross-compile job as part of our PRs for quicker feedback.

Here's the workflow:

name: Build
on:
  push:
    branches:  
      - master
  pull_request:
    branches:
      - master

jobs:
  build:
    name: Build on JDK ${{ matrix.java }} and ${{ matrix.os }}
    runs-on: ${{ matrix.os }}
    strategy:
      matrix:
        java: [8, 11]
        os: [ubuntu-latest, windows-latest]
        
    steps:
      - name: Check out code
        uses: actions/checkout@v2

      - name: Set up JDK ${{ matrix.java }}
        uses: actions/setup-java@v1 
        with:
          java-version: ${{ matrix.java }}
          
      - uses: actions/cache@v1
        with:
          path: ~/.m2/repository
          key: ${{ runner.os }}-maven-${{ hashFiles('**/pom.xml') }}
          restore-keys: |
            ${{ runner.os }}-maven-
        
      - name: Build with Maven
        shell: bash
        run: mvn -nsu -B --quiet -Djacoco.skip=true -Dorg.slf4j.simpleLogger.defaultLogLevel=error --no-transfer-progress clean install --file pom.xml ${{ matrix.flags }}
        
      - name: Test gradle
        shell: bash
        run: gradle -b modules/openapi-generator-gradle-plugin/samples/local-spec/build.gradle buildGoSdk --stacktrace

image

@@ -3,7 +3,7 @@
"generatorName": "java",
"inputSpec": "modules/openapi-generator/src/test/resources/2_0/petstore-with-fake-endpoints-models-for-testing.yaml",
"outputDir": "samples/client/petstore/java/feign",
"templateDir": "modules/openapi-generator/src/main/resources/Java/libraries/feign",
Copy link
Member Author

@jimschubert jimschubert May 25, 2020

Choose a reason for hiding this comment

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

Note these removals of libraries/<library name> in the Java sample configs and scripts.

While this syntax works if the entirety of the library code is in the named folder, this would cause fallbacks to fail if anyone were to remove something like model.mustache intending to fallback to resources/Java/model.mustache. We should avoid this possible confusion especially when these subdirectories are structured to follow our library lookup logic:

  • User customized library path (e.g. custom_template/libraries/feign/model.mustache)
  • User customized generator top-level path (e.g. custom_template/model.mustache)
  • Embedded library path (e.g. resources/Java/libraries/feign/model.mustache)
  • Embedded top-level path (e.g. resources/Java/model.mustache)
  • Common embedded path (e.g. resources/_common/model.mustache)
  • throw TemplateNotFoundException

@@ -50,7 +50,7 @@ build_script:
# install openapi-generator locally
- mvn clean install --quiet -Dorg.slf4j.simpleLogger.defaultLogLevel=error
# run the locally installed openapi-generator-gradle-plugin
- gradle -b modules\openapi-generator-gradle-plugin\samples\local-spec\build.gradle buildGoSdk --info
- gradle -b modules\openapi-generator-gradle-plugin\samples\local-spec\build.gradle buildGoSdk --stacktrace
Copy link
Member Author

Choose a reason for hiding this comment

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

I'd like to leave --stacktrace here rather than --info. It was taking ~2 hours to get the feedback from AppVeyor after commit and I was unable to repro the issue in Windows locally.


String formatted = template;

Mustache.TemplateLoader loader = name -> templateProcessor.getTemplateReader(name.concat(MUSTACHE_EXTENSION));
Copy link
Member Author

Choose a reason for hiding this comment

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

Notice regarding the design… template processor no longer requires that we pass that logic through a DefaultGenerator instance, which holds logic for OpenAPI input generation to templated output.

We can now generate without tight coupling to OpenAPI documents. This is work directed toward #841 and somewhat related to #843.

@@ -218,7 +218,7 @@ public void processOpts() {
// supportingFiles.add(new SupportingFile("LICENSE", "", "LICENSE"));

// all PHP codegens requires Composer, it means that we need to exclude from SVN at least vendor folder
supportingFiles.add(new SupportingFile(".gitignore", "", ".gitignore"));
Copy link
Member Author

Choose a reason for hiding this comment

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

I found that .gitignore files sometimes don't load via this.getClass().getClassLoader().getResource(path).

For example, this works: this.getClass().getClassLoader().getResource("_common/.openapi-generator-ignore") while this did not: this.getClass().getClassLoader().getResource("graphql-nodejs-express-server/.gitignore"). This only seems to affect PHP and the GraphQL Express servers as everything else is either gitignore or gitignore.mustache (the formats documented as acceptable on Java documentation).

throw new TemplateNotFoundException(templateFile);

// allow lookup of files without extension modification (such as .openapi-generator-ignore, README.md, etc)
try {
Copy link
Member Author

Choose a reason for hiding this comment

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

This is required because filenames without extensions would become .hbs or .handlebars in the above loop. A file which isn't intended to be handled like REAMDE.md would become README.hbs or README.handlebars and would result in template not found without this as-is attempt.

#docs/*.md
# Then explicitly reverse the ignore rule for a single file:
#!docs/README.md
gradle/wrapper/gradle-wrapper.jar
Copy link
Member Author

Choose a reason for hiding this comment

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

I had issues executing this test locally with Bash 5.0. Had to update gradle in the example so the bash sourcing would succeed. I've only updated the generated output rather than the embedded files so we're not forcing users to update to a potentially invalid Gradle version as they regenerate the kotlin multiplatform generate.

@jimschubert
Copy link
Member Author

This PR completes my targeted refactor work for the "Template Engine" component defined in #845

image

* master:
  [kotlin][client] add support for coroutines with OkHttp (OpenAPITools#6362)
  update package-json.lock (OpenAPITools#6430)
  fix hardcoded match type (OpenAPITools#6431)
  java jersey2 enhance anyOf (OpenAPITools#6420)
  [Java][jersey2] minor improvement to jersey2 tests (OpenAPITools#6418)
  ps minor style change (OpenAPITools#6424)
  [JS] mark ES5 as deprecated (OpenAPITools#6408)
  [ci] Execute maven and verify with no-snapshot-updates (OpenAPITools#6415)
  add new file in ts axios samples
  Migrate all scala generators to use OAS3 (OpenAPITools#6407)
  migrate ruby samples to oas3 (OpenAPITools#6414)
  [PS] check if JSON properties is defined (OpenAPITools#6419)
  Add C++ UE4  client generator (OpenAPITools#6399)
  Add a link to the article in dev.to (OpenAPITools#6421)
  typescript-axios anytype is not defined (OpenAPITools#6335)
  [Java][jersey2] Make (de)serialization work for oneOf models, add convenience and comparison methods (OpenAPITools#6323)
  Migrate OCaml petstore to use OAS v3 spec (OpenAPITools#6348)
  [Python-experimental] Fix type error if oneof/anyof child schema is null type (OpenAPITools#6387)
  [Python-server] Fix blueplanet 'file not found' error  (OpenAPITools#6411)
  [nodejs] Fix deprecation notice when running sample nodejs script (OpenAPITools#6412)
@jimschubert
Copy link
Member Author

I'll merge this tomorrow or Friday if there's no additional feedback.

* master:
  Fix ruby deprecation error (OpenAPITools#6450)
  [Java][Feign] decommission 9.x support (OpenAPITools#6445)
  fix struct export in rust reqwest (OpenAPITools#6453)
  [Rust][reqwest] add tests to CI (OpenAPITools#6454)
  set pester version (OpenAPITools#6448)
  update datadog logo
  update groovy petstore samples
  add datadog as sponsor (OpenAPITools#6444)
  Migrate Groovy samples to oas3 (OpenAPITools#6435)
  [samples] Renerate Kotlin coroutines
* master:
  [PS] Refactor the http signing auth with ecdsa support (OpenAPITools#6397)
  [Rust Server] Hyper 0.13 + Async/Await support (OpenAPITools#6244)
  [Rust] set supportAsync to true as the default (OpenAPITools#6480)
  [php-symfony] Set required PHP version ^7.1.3 (OpenAPITools#6181)
  update doc
  [csharp] Rename netstandard to netstandard1.3 (OpenAPITools#6460)
  feat: support deprecated parameters for typescript-axios generator (OpenAPITools#6475)
  [REQ][typescript-axios] useSingleRequestParameter should mark parameter optional if all properties are optional (OpenAPITools#6477)
  better struct alias in rust (OpenAPITools#6470)
  Migrate Go server samples to OAS 3 only (OpenAPITools#6471)
  [Rust][reqwest] add async support (OpenAPITools#6464)
  [codegen][python-experimental] Composed schema with additionalProperties  (OpenAPITools#6290)
  [Java] Decommission Retrofit 1.x support (OpenAPITools#6447)
  remove scala oas3 samples (OpenAPITools#6446)
  [Java][jersey2] Fix RuntimeException when HTTP signature parameters are not configured (OpenAPITools#6457)
  [Java][jersey2] Improve http signature code comments (OpenAPITools#6463)
  [typescript-angular] drop support of angular below 6.0.0 (OpenAPITools#6360)
  [cli] new 'author template' command (OpenAPITools#6441)
  python-experimental updates ancestor + adds descendant discriminator tests (OpenAPITools#6417)
@jimschubert jimschubert merged commit a47e522 into OpenAPITools:master May 30, 2020
@jimschubert jimschubert deleted the cleanup-template-management branch May 30, 2020 04:19
jimschubert added a commit that referenced this pull request May 30, 2020
* master: (36 commits)
  Improve handling spaces in example command (#6482)
  fix maven plugin snapshot version
  comment out erlang server test (#6499)
  Migrate Perl samples to use OAS v3 spec (#6490)
  [core] Refactor templating management (#6357)
  migrate apex samples to use oas3 spec (#6488)
  add new file in php-symfony sample
  [PS] Refactor the http signing auth with ecdsa support (#6397)
  [Rust Server] Hyper 0.13 + Async/Await support (#6244)
  [Rust] set supportAsync to true as the default (#6480)
  [php-symfony] Set required PHP version ^7.1.3 (#6181)
  update doc
  [csharp] Rename netstandard to netstandard1.3 (#6460)
  feat: support deprecated parameters for typescript-axios generator (#6475)
  [REQ][typescript-axios] useSingleRequestParameter should mark parameter optional if all properties are optional (#6477)
  better struct alias in rust (#6470)
  Migrate Go server samples to OAS 3 only (#6471)
  [Rust][reqwest] add async support (#6464)
  [codegen][python-experimental] Composed schema with additionalProperties  (#6290)
  [Java] Decommission Retrofit 1.x support (#6447)
  ...
if (StringUtils.isNotEmpty(library)) {
//look for the file in the library subfolder of the supplied template
final String libTemplateFile = buildLibraryFilePath(config.templateDir(), library, relativeTemplateFile);
if (new File(libTemplateFile).exists() || this.getClass().getClassLoader().getResource(libTemplateFile) != null) {

Choose a reason for hiding this comment

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

JAR classpath resource path requires '/' as file separator always, must not be OS dependent, e.g. '\' under Windows won't work!
Suggestion: use TemplateManager.getCPResourcePath(libTemplateFile), may extract to similar method like embeddedTemplateExists(String)

Copy link
Member Author

Choose a reason for hiding this comment

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

@DenisKnoepfle are you suggesting this based on code quality or based on an error you've experienced?

This method falls back to embedded templates in the last two conditions, which allows for Windows paths evaluation using exactly what you've mentioned above.

The methods as written here allow us to check for the embedded file in operating systems without windows style paths without incurring the performance overhead of doing that adjustment.

Maybe this method's comment would be better to document it as this to avoid confusion?

     * 1) (template dir)/libraries/(library) [Windows + *nix]
     * 2) (embedded template dir)/libraries/(library) [*nix]
     * 2) (template dir) [Windows + *nix]
     * 3) (embedded template dir) [*nix]
     * 4) (embedded template dir)/libraries/(library) [Windows only]
     * 4) (embedded template dir) [Windows only]

And since we use File.separator in buildLibraryFilePath, the file check works for both Windows and non-Windows separators consistently throughout.

The alternative that you suggest would require transforming via the regular expression in TemplateManager.getCPResourcePath for all operating systems, and we really shouldn't have to pay that price on non-Windows.

Choose a reason for hiding this comment

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

Sorry for the late response, I had been on holidays.
@jimschubert I'm suggesting this based on an error I've experienced under Windows using the template path feature (resolve from classpath).

And since we use File.separator in buildLibraryFilePath, the file check works for both Windows and non-Windows separators consistently throughout.

This is what's breaking it. You must not use backslashes ('\') in a java classpath-path, it won't work, not even under Windows. Thus it must always be a forward slash ('/') when forming a classpath-path.
Is it clear now how I meant it? Did you test it under Windows trying to find a resource on a classpath that was not in the root of a jar, e.g. some.jar/some/deep/folder/JavaSpring/pojo.mustache? In my case it always resorted to the embedded dir instead of using my custom mustache template.

Copy link
Member Author

Choose a reason for hiding this comment

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

@DenisKnoepfle thanks for the detailed response. The issue was indeed that I was testing only with a directory structure which mirrors the resources directory in our project. However, I was also testing under Git Bash which I assume is not using the same file separators. I've never done Java development under windows, so I'm not entirely sure how to attach a remote debugger to find out.

If you don't mind, I'd like to tag you in the PR I'll open if you're able to test.

Choose a reason for hiding this comment

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

Sorry, I couldn't test it sooner, but I confirmed that it's working properly now for me under Windows (v5.0.0-beta3). Thank you!


//check the supplied template main folder for the file
final String template = config.templateDir() + File.separator + relativeTemplateFile;
if (new File(template).exists() || this.getClass().getClassLoader().getResource(template) != null) {

Choose a reason for hiding this comment

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

Same issue with '\' in resource path under windows, see line 63.

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