Fix/issue216 and 217

[dz-1]

Please review the changes for #216 and #217.

This change takes the simple approach. More specifically, all .yml files in the src/main/resources/config/ folder will be re-written as per the requirement. Currently, I copied the following files to this folder. Users can add more if they want. Unit test OpenApiGeneratorTest.testGeneratorYaml() can be used to generate the config files.
-metrics.yml
-correlation.yml
-traceability.yml
-audit.yml
-body.yml
-sanitizer.yml
-openapi-validator.yml
-health.yml
-info.yml

Array and Map are processed as per @jiachen1120's suggestion. But the processing of values.yml is not coded. I assume that's already in place.

[ddobrin]

Hi @dz-1 :
[1] could you please add a default to the openAPIGenerator class, for the generation of the environment variables:
...
boolean generateEnvVars = false; // I would suggest false as a default, to preserve the existing behaviour for developers
...
// if developers wish to use it, they set it in the config.json and your code makes use of it
boolean generateEnvVars = config.toBoolean("generateEnvVars");
....

[2] after the feature is merged, could we also update the document in the doc.networknt.com site?

[dz-1]

@ddobrin
ok. But generateEnvVars = false means that the config files are copied as is (that is, not parameterized). Is that what you want?
Or, you want add another flag to enable or disable this feature?

[ddobrin]

@dz-1 I might understand it incorrectly, however, if we wish to generate environment variables, we set it in the config.json and set it to true.

In the case of it being set to false, whether by default, or by explicitly adding it to config.json, it is set to false and the files are copied as is. We agreed that in the interest of time, this is good for this release.

If it would be only for my current client, I would set a default to true, as I know that this is what they'll use and what they asked for, however wouldn't wish to impose this on the community at large.

Let's leave that part as-is then, no issue.

[dz-1]

@ddobrin
I changed the code to make generateEnvVars optional. The feature is enabled only when generateEnvVars is explicitly set.

[ddobrin]

@dz-1 could you please contact me directly, I'd like to provide some feedback, and ask a question, as I have encountered a different behaviour than expected

[ddobrin]

@dz-1 and I had a chat and looked into some behaviour directly

[ddobrin]

Looking at issue #216 and the discussions within, I am afraid that we might have to add an additional flag: "replace lists/maps" or not, as values.yml is not auto-generated and would not get these values automatically.

I'll look more into it later

[ddobrin]

@dz-1 :
Please allow me to make some recommendations which might be helpful and will address these fixes for this release. I leave this up to you to address.

1. You have added a class and method called: YAMLFileParameterizer.rewrite(), as static. this is fine.

2. Instead of adding those 9 YML files, which the code doesn't use actually, you can add 9 rocker.raw files and render them from within the code, as is, unchanged.
   ex:
   L.177: transfer(targetPath, ("src.main.resources.config").replace(".", separator), "mask.yml", templates.rest.maskYml.template());

You can add for example:
an auditYml.rocker.raw files and add the line

transfer(targetPath, ("src.main.resources.config").replace(".", separator), "audit.yml", templates.rest.auditYml.template());

3. with these changes, if a developer doesn't wish envVars, the developer gets all files, as listed in handler.yml and he/she can work on it as they see fit

4. if we wish now to add functionality to add env Vars, we won't do it for every file, for example handler.yml, and we have full control, as follows:

• each line listing a config file to which we wish to add envVars can be adapted to include the call to YAMLFileParameterizer.rewrite(), retaining individualized control

5. The line:
   if (config.keys().contains(GENERATE_ENV_VARS)) {
   YAMLFileParameterizer.rewriteAll(..)
   is not required anymore, as we actually do not wish to replace all YML files, only those YML files which can make good use of env vras, as in : server, service, datasource, audit, correlationid, etc

6. In the next release, we make make this list of files generic, pass it in, etc, however that's next release

[dz-1]

@ddobrin
I don't want to use rocker because I think it's not necessary to maintain two copies of the yml files. Ideally, we should directly read yml file from jar files in the classpath without any editing.
I prefer to put all the files in a folder and plan to use the class path folder handlerconfig/ in the classpath (for example, it can be in src/main/resources/handlerconfig of a project). Do you want this folder to be configurable? We can add a configuration item to let user specify the location of handler config files.
I'll add a flag to control skipping of Array and Map.

[ddobrin]

@dz-1 While I do not know why we would have two copies of the YML files, I leave this up to you to propose an implementation.

As this is at code generation time, do we need to add src/main/resources/handlerconfig when we have src/main/resources/config? Is it our intention to separate them?

[dz-1]

@ddobrin
Yes, I think it's necessary to separate them. Because the folder src/main/resources/config can have any configuration files. For example, codegen-cli has a file service.yml in src/main/resources/config. We don't want to rewrite this file as it is not for handlers.
Using another folder, the users can control what needs to be re-written.

[dz-1]

But this folder is just for the temporary solution. It won't be needed if we decide to load configuration files from classpath.

[dz-1]

@ddobrin
I fixed the defect you reported yesterday and tested it using codegen-cli. By default, the 9 yml files as mentioned in the PR are generated in src/main/resources/config.
If you want to re-write more files, you can put them in src/main/resources/handlerconfig in codegen-cli (or in the folder handlerconfig anywhere in the classpath).

I have also changed the config item generateEnvVars to the format below. All the three items are boolean and are considered false if not provided.

  "generateEnvVars": {
  	"generate": true,
  	"skipArray": true,
  	"skipMap": true
  }

[jiachen1120]

@dz-1
Hey, I just pulled your branch to test. Some suggestion provided here:

[1] The yml format would be invalid since lack of space after the colon
For example, info.yml generated by your feature is

enableServerInfo:${info.enableServerInfo:true}

But it should be

enableServerInfo: ${info.enableServerInfo:true}

Otherwise, the server wouldn't work properly and an exception is thrown

Exception in thread "main" java.lang.ExceptionInInitializerError
	at java.lang.Class.forName0(Native Method)
	at java.lang.Class.forName(Class.java:264)
	at com.networknt.handler.Handler.splitClassAndName(Handler.java:481)
	at com.networknt.handler.Handler.initStringDefinedHandler(Handler.java:391)
	at com.networknt.handler.Handler.initHandlers(Handler.java:73)
	at com.networknt.handler.Handler.init(Handler.java:54)
	at com.networknt.server.Server.start(Server.java:156)
	at com.networknt.server.Server.init(Server.java:128)
	at com.networknt.server.Server.main(Server.java:115)
Caused by: expected '<document start>', but found Scalar
 in 'reader', line 8, column 1:
    influxdbProtocol:${metrics.influ ... 
    ^

	at org.yaml.snakeyaml.parser.ParserImpl$ParseDocumentStart.produce(ParserImpl.java:226)
	at org.yaml.snakeyaml.parser.ParserImpl.peekEvent(ParserImpl.java:158)
	at org.yaml.snakeyaml.parser.ParserImpl.checkEvent(ParserImpl.java:148)
	at org.yaml.snakeyaml.composer.Composer.getSingleNode(Composer.java:109)
	at org.yaml.snakeyaml.constructor.BaseConstructor.getSingleData(BaseConstructor.java:140)
	at org.yaml.snakeyaml.Yaml.loadFromReader(Yaml.java:524)
	at org.yaml.snakeyaml.Yaml.load(Yaml.java:452)
	at com.networknt.config.Config$FileConfigImpl.loadSpecificConfigFileAsObject(Config.java:221)
	at com.networknt.config.Config$FileConfigImpl.loadObjectConfig(Config.java:234)
	at com.networknt.config.Config$FileConfigImpl.getJsonObjectConfig(Config.java:152)
	at com.networknt.metrics.MetricsHandler.<clinit>(MetricsHandler.java:64)
	... 9 more

[2] Only the 9 new files can be rewritten, but previous config file(server.yml, service.yml...) cannot be rewritten since they are generated from rocker instead from YAMLFileParameterizer.

[3] If do not provide "generateEnvVars" in config.json. These 9 new config files would not be generated. In my opinion, generate these config files and rewrite them are separate issue, they maybe shouldn't affect each other. So, I think no matter whether "generateEnvVars" is in config.json or not. These files should be generated.

[dz-1]

@jiachen1120
I'll fix the formatting issue.
For [2] and [3], I think @ddobrin needs to confirm the requirement. If confirmed, we need to remove openapi-validator.yml from the list too. Because it's currently generated by rocker too.

[dz-1]

@jiachen1120 Could you verify the formatting now?

[jiachen1120]

@dz-1 Yes, the format is correct now.

[ddobrin]

@jiachen1120 : @dz-1 and me met yesterday and went over the aspects you were inquiring about

[dz-1]

@ddobrin
I have made changes based on our discussion yesterday. Please verify.

1. the 8 files are copied to the output folder src/main/resources/config regardlessly.
   -metrics.yml
   -correlation.yml
   -traceability.yml
   -audit.yml
   -body.yml
   -sanitizer.yml
   -health.yml
   -info.yml

2. all yml files in src/main/resources/config are re-written if generateEnvVars is set and enabled.
   Users can choose to exclude files from re-writing. The config item is generateEnvVars.exclude. Please see the example below.

  "generateEnvVars": {
  	"generate": true,
  	"skipArray": true,
  	"skipMap": true,
  	"exclude": [
  		"handerl.yml",
  		"values.yml"
  	]
  }

[DSchrupert]

Hi @dz-1, could we have a pr to the light-docs repo about this change linked here?

[dz-1]

@NicholasAzar done.

[DSchrupert]

Linking: networknt/light-doc@e9d37fd