-
-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
[core] Refactor templating management #6357
Conversation
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.
066f916
to
d5307f3
Compare
import java.util.Scanner; | ||
import java.util.regex.Pattern; | ||
|
||
public abstract class AbstractGenerator implements TemplatingGenerator { |
There was a problem hiding this comment.
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"; |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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.
Note on breaking change without fallback… removal of |
* 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)
@OpenAPITools/generator-core-team |
There was a problem hiding this 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
@spacether would you like to take a look before merge? cc @Fjolnir-Dvorak (who's waiting on this work) |
@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:
|
@@ -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", |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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")); |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
This PR completes my targeted refactor work for the "Template Engine" component defined in #845 |
* 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)
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)
* 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) { |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
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
./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).master
,4.3.x
,5.0.x
. Default:master
.