Skip to content

Commit cecf84f

Browse files
committed
implement suggested fixes from CR. fix some general nitpicks.
1 parent d343433 commit cecf84f

5 files changed

Lines changed: 117 additions & 83 deletions

File tree

modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java

Lines changed: 20 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -31,6 +31,7 @@
3131
import org.apache.commons.io.IOCase;
3232
import org.apache.commons.lang3.ObjectUtils;
3333
import org.apache.commons.lang3.StringUtils;
34+
import org.apache.commons.lang3.Strings;
3435
import org.openapitools.codegen.api.*;
3536
import org.openapitools.codegen.config.GlobalSettings;
3637
import org.openapitools.codegen.ignore.CodegenIgnoreProcessor;
@@ -60,7 +61,6 @@
6061
import java.util.function.Supplier;
6162
import java.util.stream.Collectors;
6263

63-
import static org.apache.commons.lang3.StringUtils.removeStart;
6464
import static org.openapitools.codegen.CodegenConstants.X_INTERNAL;
6565
import static org.openapitools.codegen.utils.OnceLogger.once;
6666

@@ -86,16 +86,17 @@ public class DefaultGenerator implements Generator {
8686
private String basePath;
8787
private String basePathWithoutHost;
8888
private String contextPath;
89-
private Map<String, String> generatorPropertyDefaults = new HashMap<>();
89+
private final Map<String, String> generatorPropertyDefaults = new HashMap<>();
9090
/**
9191
* Retrieves an instance to the configured template processor, available after user-defined options are
9292
* applied via
9393
*/
94-
@Getter protected TemplateProcessor templateProcessor = null;
94+
@Getter
95+
protected TemplateProcessor templateProcessor = null;
9596

9697
private List<TemplateDefinition> userDefinedTemplates = new ArrayList<>();
97-
private String generatorCheck = "spring";
98-
private String templateCheck = "apiController.mustache";
98+
private final String generatorCheck = "spring";
99+
private final String templateCheck = "apiController.mustache";
99100

100101

101102
public DefaultGenerator() {
@@ -266,8 +267,7 @@ void configureGeneratorProperties() {
266267
openapiNormalizer.normalize();
267268
}
268269
} catch (Exception e) {
269-
LOGGER.error("An exception occurred in OpenAPI Normalizer. Please report the issue via https://github.com/openapitools/openapi-generator/issues/new/: ");
270-
e.printStackTrace();
270+
LOGGER.error("An exception occurred in OpenAPI Normalizer. Please report the issue via https://github.com/openapitools/openapi-generator/issues/new/: ", e);
271271
}
272272

273273
// resolve inline models
@@ -607,10 +607,10 @@ private void generateModelsForVariable(List<File> files, List<ModelMap> allModel
607607
if (!processedModels.contains(key) && allSchemas.containsKey(key)) {
608608
generateModels(files, allModels, unusedModels, aliasModels, processedModels, () -> Set.of(key));
609609
} else {
610-
LOGGER.info("Type " + variable.getComplexType() + " of variable " + variable.getName() + " could not be resolve because it is not declared as a model.");
610+
LOGGER.info("Type {} of variable {} could not be resolve because it is not declared as a model.", variable.getComplexType(), variable.getName());
611611
}
612612
} else {
613-
LOGGER.info("Type " + variable.getOpenApiType() + " of variable " + variable.getName() + " could not be resolve because it is not declared as a model.");
613+
LOGGER.info("Type {} of variable {} could not be resolve because it is not declared as a model.", variable.getOpenApiType(), variable.getName());
614614
}
615615
}
616616

@@ -639,8 +639,8 @@ private Set<String> getPropertyAsSet(String propertyName) {
639639
}
640640

641641
return Arrays.stream(propertyRaw.split(","))
642-
.map(String::trim)
643-
.collect(Collectors.toSet());
642+
.map(String::trim)
643+
.collect(Collectors.toSet());
644644
}
645645

646646
private Set<String> modelKeys() {
@@ -665,7 +665,6 @@ private Set<String> modelKeys() {
665665
return modelKeys;
666666
}
667667

668-
@SuppressWarnings("unchecked")
669668
void generateApis(List<File> files, List<OperationsMap> allOperations, List<ModelMap> allModels) {
670669
if (!generateApis) {
671670
// TODO: Process these anyway and present info via dryRun?
@@ -1006,7 +1005,7 @@ private void generateOpenapiGeneratorIgnoreFile() {
10061005
File ignoreFile = new File(ignoreFileNameTarget);
10071006
// use the entries provided by the users to pre-populate .openapi-generator-ignore
10081007
try {
1009-
LOGGER.info("Writing file " + ignoreFileNameTarget + " (which is always overwritten when the option `openapiGeneratorIgnoreFile` is enabled.)");
1008+
LOGGER.info("Writing file {} (which is always overwritten when the option `openapiGeneratorIgnoreFile` is enabled.)", ignoreFileNameTarget);
10101009
new File(config.outputFolder()).mkdirs();
10111010
if (!ignoreFile.createNewFile()) {
10121011
// file may already exist, do nothing
@@ -1393,7 +1392,7 @@ private void processUserDefinedTemplates() {
13931392
// hack: destination filename in this scenario might be a suffix like Impl.java
13941393
templateExt = userDefinedTemplate.getDestinationFilename();
13951394
} else {
1396-
templateExt = StringUtils.prependIfMissing(templateExt, ".");
1395+
templateExt = Strings.CS.prependIfMissing(templateExt, ".");
13971396
}
13981397
String templateOutputFolder = userDefinedTemplate.getFolder();
13991398
if (!templateOutputFolder.isEmpty()) {
@@ -1430,7 +1429,9 @@ protected File processTemplateToFile(Map<String, Object> templateData, String te
14301429
return processTemplateToFile(templateData, templateName, outputFilename, shouldGenerate, skippedByOption, this.config.getOutputDir());
14311430
}
14321431

1433-
/** Stores lowercased absolute paths for O(1) case-insensitive duplicate detection. */
1432+
/**
1433+
* Stores lowercased absolute paths for O(1) case-insensitive duplicate detection.
1434+
*/
14341435
private final Set<String> seenFilesLower = new HashSet<>();
14351436

14361437
private File processTemplateToFile(Map<String, Object> templateData, String templateName, String outputFilename, boolean shouldGenerate, String skippedByOption, String intendedOutputDir) throws IOException {
@@ -1993,7 +1994,7 @@ private void generateFilesMetadata(List<File> files) {
19931994
// Some implementations make the output ./c/d which seems to mix the logic
19941995
// as documented for symlinks. So we need to trim any / or ./ from the start,
19951996
// as nobody should be generating into system root and our expectation is no ./
1996-
String relativePath = removeStart(removeStart(f.toString(), "." + File.separator), File.separator);
1997+
String relativePath = Strings.CS.removeStart(Strings.CS.removeStart(f.toString(), "." + File.separator), File.separator);
19971998
if (File.separator.equals("\\")) {
19981999
// ensure that windows outputs same FILES format
19992000
relativePath = relativePath.replace(File.separator, "/");
@@ -2003,10 +2004,8 @@ private void generateFilesMetadata(List<File> files) {
20032004
}
20042005
});
20052006

2006-
Collections.sort(relativePaths, (a, b) -> IOCase.SENSITIVE.checkCompareTo(a, b));
2007-
relativePaths.forEach(relativePath -> {
2008-
sb.append(relativePath).append(System.lineSeparator());
2009-
});
2007+
relativePaths.sort(IOCase.SENSITIVE::checkCompareTo);
2008+
relativePaths.forEach(relativePath -> sb.append(relativePath).append(System.lineSeparator()));
20102009

20112010
String targetFile = config.outputFolder() + File.separator + METADATA_DIR + File.separator + config.getFilesMetadataFilename();
20122011

@@ -2021,7 +2020,7 @@ private void generateFilesMetadata(List<File> files) {
20212020
}
20222021

20232022
private String removeTrailingSlash(String value) {
2024-
return StringUtils.removeEnd(value, "/");
2023+
return Strings.CS.removeEnd(value, "/");
20252024
}
20262025

20272026
}

modules/openapi-generator/src/main/java/org/openapitools/codegen/TemplateManager.java

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -94,15 +94,22 @@ public Path getFullTemplatePath(String name) {
9494
return Paths.get(getFullTemplateFile(name));
9595
}
9696

97+
/**
98+
* Pre-compiled pattern for replacing the OS file separator with '/' in classpath resource paths.
99+
* Only non-null on operating systems where {@link File#separator} is not already '/'.
100+
*/
101+
private static final Pattern FILE_SEP_PATTERN =
102+
"/".equals(File.separator) ? null : Pattern.compile(Pattern.quote(File.separator));
103+
97104
/**
98105
* Gets a normalized classpath resource location according to OS-specific file separator
99106
*
100107
* @param name The name of the resource file/directory to find
101108
* @return A normalized string according to OS-specific file separator
102109
*/
103110
public static String getCPResourcePath(final String name) {
104-
if (!"/".equals(File.separator)) {
105-
return name.replaceAll(Pattern.quote(File.separator), "/");
111+
if (FILE_SEP_PATTERN != null) {
112+
return FILE_SEP_PATTERN.matcher(name).replaceAll("/");
106113
}
107114
return name;
108115
}

modules/openapi-generator/src/main/java/org/openapitools/codegen/templating/GeneratorTemplateContentLocator.java

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,13 +7,22 @@
77

88
import java.io.File;
99
import java.nio.file.Paths;
10+
import java.util.Optional;
11+
import java.util.concurrent.ConcurrentHashMap;
1012

1113
/**
1214
* Locates templates according to {@link CodegenConfig} settings.
1315
*/
1416
public class GeneratorTemplateContentLocator implements TemplatePathLocator {
1517
private final CodegenConfig codegenConfig;
1618

19+
/**
20+
* Cache of relativeTemplateFile -> resolved full path (or empty Optional when the template does not exist).
21+
* The filesystem/classpath existence probes inside resolveFullTemplatePath are expensive on repeated calls
22+
* for the same template name, so we memoize the result for the lifetime of this locator instance.
23+
*/
24+
private final ConcurrentHashMap<String, Optional<String>> templatePathCache = new ConcurrentHashMap<>();
25+
1726
/**
1827
* Constructs a new instance of {@link GeneratorTemplateContentLocator} for the provided {@link CodegenConfig}
1928
*
@@ -51,12 +60,25 @@ private boolean classpathTemplateExists(String name) {
5160
* 4) (embedded template dir)
5261
* <p>
5362
* Where "template dir" may be user defined and "embedded template dir" are the built-in templates for the given generator.
63+
* <p>
64+
* Results are cached per {@code relativeTemplateFile} name because the filesystem/classpath probes are expensive
65+
* and the outcome is constant for the lifetime of this locator instance.
5466
*
5567
* @param relativeTemplateFile Template file
56-
* @return String Full template file path
68+
* @return String Full template file path, or {@code null} if the template does not exist in any location
5769
*/
5870
@Override
5971
public String getFullTemplatePath(String relativeTemplateFile) {
72+
return templatePathCache
73+
.computeIfAbsent(relativeTemplateFile, key -> Optional.ofNullable(resolveFullTemplatePath(key)))
74+
.orElse(null);
75+
}
76+
77+
/**
78+
* Performs the actual filesystem/classpath probes to find the full template path.
79+
* Called at most once per unique {@code relativeTemplateFile} value; all subsequent lookups use the cache.
80+
*/
81+
private String resolveFullTemplatePath(String relativeTemplateFile) {
6082
CodegenConfig config = this.codegenConfig;
6183

6284
//check the supplied template library folder for the file

modules/openapi-generator/src/main/java/org/openapitools/codegen/templating/HandlebarsEngineAdapter.java

Lines changed: 44 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838

3939
import java.io.IOException;
4040
import java.util.Arrays;
41-
import java.util.Locale;
4241
import java.util.Map;
4342
import java.util.concurrent.ConcurrentHashMap;
4443

@@ -49,20 +48,24 @@ public class HandlebarsEngineAdapter extends AbstractTemplatingEngineAdapter {
4948
// We use this as a simple lookup for valid file name extensions. This adapter will inspect .mustache (built-in) and infer the relevant handlebars filename
5049
private final String[] canCompileFromExtensions = {".handlebars", ".hbs", ".mustache"};
5150
private boolean infiniteLoops = false;
52-
@Setter private boolean prettyPrint = false;
51+
@Setter
52+
private boolean prettyPrint = false;
5353

5454
/**
55-
* Cache of (templateFile -> compiled Template). Compiled Handlebars templates are stateless after
56-
* compilation and safe to reuse across multiple data-bundle invocations within the same run.
55+
* Per-executor cache of fully-configured {@link Handlebars} engine instances.
56+
* Each executor gets its own engine because the engine's {@link TemplateLoader} closes over the
57+
* executor; sharing an engine across executors would silently resolve templates from the wrong source.
58+
* {@link ConcurrentHashMap#computeIfAbsent} ensures the engine is built at most once per executor.
5759
*/
58-
private final Map<String, Template> compiledTemplateCache = new ConcurrentHashMap<>();
60+
private final ConcurrentHashMap<TemplatingExecutor, Handlebars> engineCache = new ConcurrentHashMap<>();
5961

6062
/**
61-
* Cached Handlebars engine instance with all helpers pre-registered.
62-
* Re-created lazily whenever the TemplatingExecutor changes (different template paths).
63+
* Per-executor cache of compiled {@link Template} objects.
64+
* Keying on the executor instance eliminates the non-atomic check-clear-update invalidation
65+
* that the previous single-cache approach required; no state ever needs to be cleared.
6366
*/
64-
private volatile Handlebars cachedHandlebars;
65-
private volatile TemplatingExecutor cachedExecutor;
67+
private final ConcurrentHashMap<TemplatingExecutor, ConcurrentHashMap<String, Template>> templateCaches =
68+
new ConcurrentHashMap<>();
6669

6770
/**
6871
* Provides an identifier used to load the adapter. This could be a name, uuid, or any other string.
@@ -86,40 +89,44 @@ public String compileTemplate(TemplatingExecutor executor,
8689
AccessAwareFieldValueResolver.INSTANCE)
8790
.build();
8891

89-
// Reuse the Handlebars engine when the executor hasn't changed; rebuild otherwise.
90-
if (cachedHandlebars == null || cachedExecutor != executor) {
91-
TemplateLoader loader = new AbstractTemplateLoader() {
92-
@Override
93-
public TemplateSource sourceAt(String location) {
94-
return findTemplate(executor, location);
95-
}
96-
};
97-
Handlebars handlebars = new Handlebars(loader);
98-
handlebars.registerHelperMissing((obj, options) -> {
99-
LOGGER.warn(String.format(Locale.ROOT, "Unregistered helper name '%s', processing template:%n%s", options.helperName, options.fn.text()));
100-
return "";
101-
});
102-
handlebars.registerHelper("json", Jackson2Helper.INSTANCE);
103-
StringHelpers.register(handlebars);
104-
handlebars.registerHelpers(ConditionalHelpers.class);
105-
handlebars.registerHelpers(org.openapitools.codegen.templating.handlebars.StringHelpers.class);
106-
handlebars.setInfiniteLoops(infiniteLoops);
107-
handlebars.setPrettyPrint(prettyPrint);
108-
// Changing the executor means template content may differ; clear stale entries.
109-
compiledTemplateCache.clear();
110-
cachedHandlebars = handlebars;
111-
cachedExecutor = executor;
112-
}
92+
// Each executor gets its own Handlebars engine (the loader closes over the executor) and its
93+
// own compiled-template cache. computeIfAbsent is atomic, so concurrent calls with the same
94+
// executor share one engine/cache rather than racing to create duplicates.
95+
Handlebars handlebars = engineCache.computeIfAbsent(executor, this::buildHandlebars);
96+
ConcurrentHashMap<String, Template> cache =
97+
templateCaches.computeIfAbsent(executor, k -> new ConcurrentHashMap<>());
11398

114-
Template tmpl = compiledTemplateCache.get(templateFile);
99+
// Manual get → compile → put so IOException propagates naturally.
100+
Template tmpl = cache.get(templateFile);
115101
if (tmpl == null) {
116-
// compile() declares throws IOException — propagate it directly without wrapping.
117-
tmpl = cachedHandlebars.compile(templateFile);
118-
compiledTemplateCache.put(templateFile, tmpl);
102+
tmpl = handlebars.compile(templateFile);
103+
cache.put(templateFile, tmpl);
119104
}
120105
return tmpl.apply(context);
121106
}
122107

108+
/** Constructs and fully configures a {@link Handlebars} engine for the given executor. */
109+
private Handlebars buildHandlebars(TemplatingExecutor executor) {
110+
TemplateLoader loader = new AbstractTemplateLoader() {
111+
@Override
112+
public TemplateSource sourceAt(String location) {
113+
return findTemplate(executor, location);
114+
}
115+
};
116+
Handlebars handlebars = new Handlebars(loader);
117+
handlebars.registerHelperMissing((obj, options) -> {
118+
LOGGER.warn("Unregistered helper name '{}', processing template:\n{}", options.helperName, options.fn.text());
119+
return "";
120+
});
121+
handlebars.registerHelper("json", Jackson2Helper.INSTANCE);
122+
StringHelpers.register(handlebars);
123+
handlebars.registerHelpers(ConditionalHelpers.class);
124+
handlebars.registerHelpers(org.openapitools.codegen.templating.handlebars.StringHelpers.class);
125+
handlebars.setInfiniteLoops(infiniteLoops);
126+
handlebars.setPrettyPrint(prettyPrint);
127+
return handlebars;
128+
}
129+
123130
@SuppressWarnings("java:S108")
124131
public TemplateSource findTemplate(TemplatingExecutor generator, String templateFile) {
125132
String[] possibilities = getModifiedFileLocation(templateFile);

0 commit comments

Comments
 (0)