Skip to content

Commit

Permalink
[cli] Add --global-property for -D replacement (#5687)
Browse files Browse the repository at this point in the history
-D option has been deprecated as it was previously used to:

* Pass "system properties"
* Pass additional properties

This was confusing because we already have --additional-properties and
because Java System Properties are passed as -D before program
arguments.

Confusion around the -D option had existed for some time, but when we
introduced the thread-safe GlobalSettings to avoid overwriting Java
System Properties, we created a hard break from Java System Properties
in the generator. This also disconnected the previous "system
properties" from accepting additional properties.

Once these newly deprecated methods are removed, we will have a clear
separation of concerns between:

* Java System Properties
* Global generator properties (used as workflow context)
* Additional properties (used as generator options)

This commit marks multiple places for cleanup in 5.0. These will be
breaking changes, and lower effort to break in 5.0 with deprecation
warnings now rather than adding sibling properties throughout the code
and potentially introducing logic errors.
  • Loading branch information
jimschubert committed Apr 2, 2020
1 parent e14e5fc commit 2957dd4
Show file tree
Hide file tree
Showing 6 changed files with 58 additions and 12 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -71,12 +71,13 @@ public class Generate extends OpenApiGeneratorCommand {
+ "Pass in a URL-encoded string of name:header with a comma separating multiple values")
private String auth;

// TODO: Remove -D short option in 5.0
@Option(
name = {"-D"},
title = "system properties",
description = "sets specified system properties in "
name = {"-D", "--global-property"},
title = "global properties",
description = "sets specified global properties (previously called 'system properties') in "
+ "the format of name=value,name=value (or multiple options, each with name=value)")
private List<String> systemProperties = new ArrayList<>();
private List<String> globalProperties = new ArrayList<>();

@Option(
name = {"-c", "--config"},
Expand Down Expand Up @@ -403,9 +404,9 @@ public void execute() {
configurator.setStrictSpecBehavior(strictSpecBehavior);
}

if (systemProperties != null && !systemProperties.isEmpty()) {
System.err.println("[DEPRECATED] -D arguments after 'generate' are application arguments and not Java System Properties, please consider changing to -p, or apply your options to JAVA_OPTS, or move the -D arguments before the jar option.");
applySystemPropertiesKvpList(systemProperties, configurator);
if (globalProperties != null && !globalProperties.isEmpty()) {
System.err.println("[DEPRECATED] -D arguments after 'generate' are application arguments and not Java System Properties, please consider changing to --global-property, apply your system properties to JAVA_OPTS, or move the -D arguments before the jar option.");
applyGlobalPropertiesKvpList(globalProperties, configurator);
}
applyInstantiationTypesKvpList(instantiationTypes, configurator);
applyImportMappingsKvpList(importMappings, configurator);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,13 +28,14 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Objects;
import java.util.concurrent.atomic.AtomicLong;

/**
* Represents those settings applied to a generation workflow.
*/
@SuppressWarnings("WeakerAccess")
public class WorkflowSettings {

private static final AtomicLong lastWarning = new AtomicLong(0);
private static final Logger LOGGER = LoggerFactory.getLogger(WorkflowSettings.class);
public static final String DEFAULT_OUTPUT_DIR = ".";
public static final boolean DEFAULT_VERBOSE = false;
Expand Down Expand Up @@ -77,7 +78,15 @@ private WorkflowSettings(Builder builder) {
this.templateDir = builder.templateDir;
this.templatingEngineName = builder.templatingEngineName;
this.ignoreFileOverride = builder.ignoreFileOverride;
// TODO: rename to globalProperties for 5.0
this.systemProperties = ImmutableMap.copyOf(builder.systemProperties);
if (this.systemProperties.size() > 0) {
// write no more than every 5s. This is temporary until version 5.0 as once(Logger) is not accessible here.
// thread contention may cause this to write more than once, but this is just an attempt to reduce noise
if (System.currentTimeMillis() - lastWarning.getAndUpdate(x -> System.currentTimeMillis()) > 5000) {
LOGGER.warn("systemProperties will be renamed to globalProperties in version 5.0");
}
}
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,7 @@ open class GenerateTask : DefaultTask() {
}

if (systemProperties.isPresent) {
// TODO: rename to globalProperties in 5.0
systemProperties.get().forEach { entry ->
configurator.addSystemProperty(entry.key, entry.value)
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ public class CodeGenMojo extends AbstractMojo {
@Parameter(defaultValue = "true", property = "openapi.generator.maven.plugin.addCompileSourceRoot")
private boolean addCompileSourceRoot = true;

// TODO: Rename to global properties in version 5.0
@Parameter
protected Map<String, String> environmentVariables = new HashMap<>();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ public CodegenConfigurator addLanguageSpecificPrimitive(String value) {
return this;
}

// TODO: rename this and other references to "global property" rather than "system property"
public CodegenConfigurator addSystemProperty(String key, String value) {
this.systemProperties.put(key, value);
workflowSettingsBuilder.withSystemProperty(key, value);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,47 @@
*/
public final class CodegenConfiguratorUtils {

/**
* Applies "system" properties to the configurator as global properties.
*
* @deprecated
* This method is deprecated due to confusion around the tool's use of system properties. We called these system properties
* in the past and accepted them via CLI option -D. This lead to confusion between true Java System Properties and generator-specific
* "system level properties". They've since been renamed as "Global Properties". Please use {@link CodegenConfiguratorUtils#applyGlobalPropertiesKvpList(List, CodegenConfigurator)}.
*
* @param systemProperties List of properties to be globally available throughout the generator execution.
* @param configurator The {@link CodegenConfigurator} instance to configure.
*/
@Deprecated
public static void applySystemPropertiesKvpList(List<String> systemProperties, CodegenConfigurator configurator) {
for(String propString : systemProperties) {
applySystemPropertiesKvp(propString, configurator);
// TODO: Remove in 5.0
applyGlobalPropertiesKvpList(systemProperties, configurator);
}

/**
* Applies a key-value pair of strings as "system" properties to the configurator as global properties.
*
* @deprecated
* This method is deprecated due to confusing between Java Sytsem Properties and generator-specific "system-level properties".
* They've since been renamed as "Global Properties". Please use {@link CodegenConfiguratorUtils#applyGlobalPropertiesKvp(String, CodegenConfigurator)}.
*
* @param systemProperties List of properties to be globally available throughout the generator execution.
* @param configurator The {@link CodegenConfigurator} instance to configure.
*/
@Deprecated
public static void applySystemPropertiesKvp(String systemProperties, CodegenConfigurator configurator) {
// TODO: Remove in 5.0
applyGlobalPropertiesKvp(systemProperties, configurator);
}

public static void applyGlobalPropertiesKvpList(List<String> globalProperties, CodegenConfigurator configurator) {
for(String propString : globalProperties) {
applyGlobalPropertiesKvp(propString, configurator);
}
}

public static void applySystemPropertiesKvp(String systemProperties, CodegenConfigurator configurator) {
final Map<String, String> map = createMapFromKeyValuePairs(systemProperties);
public static void applyGlobalPropertiesKvp(String globalProperties, CodegenConfigurator configurator) {
final Map<String, String> map = createMapFromKeyValuePairs(globalProperties);
for (Map.Entry<String, String> entry : map.entrySet()) {
configurator.addSystemProperty(entry.getKey(), entry.getValue());
}
Expand Down

0 comments on commit 2957dd4

Please sign in to comment.