System properties do not make it down to dependencies

[jdomizio]

Bug Report Checklist

• [n/a] Have you provided a full/minimal spec to reproduce the issue?
• Have you validated the input using an OpenAPI validator (example)?
• What's the version of OpenAPI Generator used?
• Have you search for related issues/PRs?
• What's the actual output vs expected output?
• [Optional] Bounty to sponsor the fix (example)

Description

Option to set system property -Dio.swagger.v3.parser.util.RemoteUrl.trustAll=true in openapi-generator-cli does not actually set a system property that can be read by System.getProperty() call in io.swagger.v3.parser.util.RemoteUrl

openapi-generator version

v4.0.0-beta3

OpenAPI declaration file content or url

n/a

Command line used for generation

docker run --rm -v ${PWD}:/local \
    openapitools/openapi-generator-cli:v4.0.0-beta3 generate \
    -Dio.swagger.v3.parser.util.RemoteUrl.trustAll=true \
    -i https://xxx.dev.yyy.local/api/v1/swagger.json \
    -g csharp-netcore \
    -o /local/xxx

Unfortunately the url I am testing against is on a closed network, and I don't have an example that is internet-accessible.

The actual output I am seeing is here
(https://gist.github.com/jdomizio/a21471b8a1b65b8b5904aad2e9276b44)

the expected output is the normal output for code generation.

Steps to reproduce

Run the above command against a swagger document hosted under https with a self-signed certificate. After pulling the v4.0.0-beta3 tag and modifying org.openapitools.codegen.config.CodegenConfigurator.setSystemProperties

to

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

the expected system property gets set and I can successfully run the cli tool against self-signed endpoints again.

Related issues/PRs

(#845 (comment))

Suggest a fix

Looks like previously a decision was made to remove use of System.(set|get|clear)Property. A dependency of openapi-generator: swagger-parser still depends on properties being set such that System.getProperty() can retrieve them. Is there a plan to update or replace those dependencies such that they rely on the same mechanisms with the v4.0.0 release?

(98ae7a8#diff-9981d70179c9125460299c24c3efa319)

[wing328]

@jdomizio thanks for reporting the issue.

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

Please file a PR with the suggested fix when you've time.

[jimschubert]

@jdomizio This option is confusing. When you pass -D as an argument to generate, you're not using a Java system property but a custom argument for the generate command. See

openapi-generator/modules/openapi-generator-cli/src/main/java/org/openapitools/codegen/cmd/Generate.java

Line 85 in e85e1f1

name = {"-D"},

If you move this option to a JAVA_OPTS environment variable, it will become a Java system property and should therefore work.

Example:

docker run --rm \
    -e JAVA_OPTS='-Dio.swagger.v3.parser.util.RemoteUrl.trustAll=true'  \
    -v ${PWD}:/local \
    openapitools/openapi-generator-cli:v4.0.0-beta3 \
    generate \
    -i https://xxx.dev.yyy.local/api/v1/swagger.json \
    -g csharp-netcore \
    -o /local/xxx

This difference between Java system properties -D and the generator argument -D confused me quite a bit when I first began contributing to the project.

@wing328 the suggested fix of setting properties at the time of setting GeneratorProperties will potentially introduce race conditions in CI (the reason GeneratorProperties was introduced). We may want to consider renaming this "System Properties" argument from -D to something else as part of #2556. These are more "global properties" or maybe even "workflow properties". I'm working on a branch to refactor underlying settings to make them immutable and separate some concerns. If you think it's a good idea, I can make a name and option change on that branch to target 4.1. It'd be a breaking change, but the fallback would be targeting the new option/property name.

[jdomizio]

@jimschubert Thank you for the clarification! Would you like me to close this or keep it open to track your discussion around your refactor?

[jimschubert]

@jdomizio thanks for checking. I've introduced a deprecation in a new PR and I'll update #2556 to explain the concerns and the new deprecation message.