CodegenConfigurator systemProperties are reused between code generation invokations

[JLLeitschuh]

I'm using the swagger CodegenConfigurator in a gradle build where the swagger codegen project is loaded as a library and is being directly passed into the DefaultGenerator.

This usage in this gradle plugin is very similar to my usage.

If I configure one CodegenConfigurator object with systemProperties being set, these settings seem to be being stored somewhere statically.

I'm observing that if I use a new DefaultGenerator again in the same JVM I end up having files not get generated because of input from a previous run of the generator.

I know that the two CodegenConfigurator objects that I'm passing in are different because I've tested logging the systemProperties variable just before passing it in and for the first call it is populated and the second time it is empty.

[JLLeitschuh]

It seems that these lines are the one to blame in CodegenConfigurator :

    private void setSystemProperties() {
        for (Map.Entry<String, String> entry : systemProperties.entrySet()) {
            System.setProperty(entry.getKey(), entry.getValue());
        }
    }

swagger-codegen/modules/swagger-codegen/src/main/java/io/swagger/codegen/config/CodegenConfigurator.java

Line 466 in 029728d

private void setSystemProperties() {

[otrosien]

@JLLeitschuh are you working on that subject?

[ePaul]

This is a general problem of using Java system properties – a kind of global variables – for this thing.
We should try to avoid this in general, and pass the parameters over other channels.

[otrosien]

@ePaul agree, do you have an idea? Beware that current users are relying on this feature.

[ePaul]

Beware that current users are relying on this feature.

On which part of the feature do they rely? Do they actually need system properties being set (like file.separator), or just need a way to pass custom parameters to parts of the code?

I guess we need to clean up all locations in the code which are using System.getProperty() to use other ways of passing parameters instead (like the additionalProperties map, or maybe even have a "systemProperties" map being passed to the Codegens). The Configurator should put the data from the systemProperties parameter into the new map instead of setting system properties.

[JLLeitschuh]

I'm not. I just use the ugly synchronization hack you've seen in my codegen plugin.

[otrosien]

@ePaul do you want to go ahead and tackle this issue?

[ePaul]

@otrosien I would like to, but I can't promise I'll have time for this anytime soon. (I still have #4898 on my plate, which I would like to finish first, and my time for Swagger-codegen is limited.)

[wing328]

@ePaul if you want, please feel free to work on this issue instead and leverage the community to work on #4898

[otrosien]

@ePaul I'm not too deep into the system property thing, but I assume that they were used to pass custom properties from the command line.

[ePaul]

@otrosien Yeah, looks like it.

I did a short search for System.getProperty() over the codebase.

Property	used in	meaning
swagger.codegen. inflector.apipackage	JavaInflectorServerCodegen	package name for API
swagger.codegen. inflector.modelpackage	JavaInflectorServerCodegen	package name for models
noservice	NodeJSServerCodegen	if given, don't generate service.js from service.mustache.
swagger.codegen. undertow.apipackage	UndertowCodegen	package name for API
swagger.codegen. undertow.modelpackage	UndertowCodegen	package name for models
debugParser	DefaultCodegen	if given, output the parsed parameter structure given to fromParameter. (Was likely meant to be used in many more places instead of just this one.)
apis	DefaultGenerator	List of API names to generate (comma-separated)
models	DefaultGenerator	List of model names to generate (comma-separated)
supportingFiles	DefaultGenerator	List of names of supporting files to generate (comma-separated)
modelTests	DefaultGenerator	whether to generate tests for models (boolean, default true)
modelDocs	DefaultGenerator	whether to generate docs for models (boolean, default true)
apiTests	DefaultGenerator	whether to generate tests for APIs (boolean, default true)
apiDocs	DefaultGenerator	whether to generate docs for APIs (boolean, default true)
debugSwagger	DefaultGenerator	if set, output the Swagger object (as pretty-printed JSON) before starting the code generation.
debugModels	DefaultGenerator	if set, output the list of models (as pretty-printed JSON) at the end of generateModels
debugOperations	DefaultGenerator	if set, output the list of operations (as pretty-printed JSON) at the end of generateApis. Also, output some logging information at the start of processOperation.
debugSupportingFiles	DefaultGenerator	if set, output the created map (as pretty-printed JSON) at the end of buildSupportFileBundle

System.setProperty is used only in CodegenConfigurator – once in setVerboseFlags for verbose mode (setting the last four debug* properties), and in setSystemProperties for copying the argument map.

So we basically have two usages here:

• Some (3) language specific Codegens use system properties as a mechanism to transfer parameters. I think for the package names we have another mechanism elsewhere, which should be used instead. noservice potentially too.
• The others are generic parameters for the generator. Those should be passed the same way as every other parameters.

[ePaul]

So, the standard way of passing the model (and API) package names is via the additionalProperties' modelPackage (or apiPackage) entries. Those are taken in DefaultCodegen.processOpts into setModelPackage(setApiPackage). (And CodegenConfigurator sets them into additionalProperties.)

This should work for JavaInflector and Undertow too. The system properties given just set default values which will be overwritten if a package name is given in the normal way.

We could implement a compatibility layer somewhere, if this is deemed needed: If this system property is set, we are using this generator and there is no model/API package given in a different location, use the value of the system property. Though I'm not sure this is needed. Maybe just output a warning for some time?