[Core] Add warning on headers with underscores

[ybelenko]

PR checklist

• Read the contribution guidelines.
• If contributing template-only or documentation-only changes which will change sample output, build the project before.
• Run the shell script(s) under ./bin/ (or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).
• File the PR against the correct branch: master, 4.3.x, 5.0.x. Default: master.
• Copy the technical committee to review the pull request if your PR is targeting a particular programming language.

Closes #4217

I didn't add any tests, because underscore search is pretty basic in current PR. I can add tests if necessary.

Examples of new warnings:

[main] WARN  o.o.codegen.DefaultCodegen - Security schema 'api_key' mapped to 
underscored header 'api_key' which is forbidden in Apache and Nginx webservers 
by default. 
https://stackoverflow.com/questions/22856136/why-http-servers-forbid-underscores-in-http-header-names/22856867#22856867

[main] WARN  o.o.codegen.DefaultCodegen - Request header parameter name 
'enum_header_string_array' contains underscore which is forbidden in Apache 
and Nginx webservers by default. 
https://stackoverflow.com/questions/22856136/why-http-servers-forbid-underscores-in-http-header-names/22856867#22856867

[main] WARN  o.o.codegen.DefaultCodegen - Response header name 'X_Rate_Limit' 
contains underscore which is forbidden in Apache and Nginx webservers by 
default. 
https://stackoverflow.com/questions/22856136/why-http-servers-forbid-underscores-in-http-header-names/22856867#22856867

cc @OpenAPITools/generator-core-team

[wing328]

Thanks for the PR. Instead of adding the warning in the default codegen, what about adding it to the PHP Slim 4 generator to start with?

If I'm just using the documentation generator, I don't think the warning is relevant to me.

[ybelenko]

@wing328 Hmmm, maybe you right. I've chosen default codegen because it's an api design problem, not framework. Wanted to hear other opinions, but your argument seems reasonable.

I would add it to all server generators, but there is no AbstractServerCodegen.

What do you think about log message? Do you like it? Maybe I should remove stackoverflow link from it?

[ybelenko]

Warning triggers only with server generators from now.

[jimschubert]

I think this would be better at spec validation, since users can skip those warnings. This is excellent quality of life improvement for the community.

[ybelenko]

@jimschubert Do we have our own spec validator module/package? I used swagger-cli npm package for validation before. Btw I've never seen a warning from swagger-cli validator before, only errors.

[jimschubert]

@ybelenko our validations are done ad hoc in the generate and validate tasks. See for example:

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

Line 54 in de162f7

if (Boolean.TRUE.equals(recommend)) {

I created a simple validation framework in #3183 which I plan to consume in work done in the PR linked in that PR. I think I can move your validations when I do the validation extraction.

[jimschubert]

Mind if we remove the security label on this PR and related issue? We should only use this for issues related to security of using the tool (e.g. CVE for Jackson Databind, see https://github.com/OpenAPITools/openapi-generator/issues?q=label%3A%22Issue%3A+Security%22+is%3Aclosed). This isn't related to issues with authorization defined in spec.

[ybelenko]

@jimschubert So, what with this PR? Should I close it without merge?

Btw, personally me, I usually skip warnings because they looks like debug logs right now. There is no color difference between info and warn in terminal output. Can we enhance logging to use colors?

[jimschubert]

@ybelenko Just reviewed the PR more in depth…

Would it be possible to add a flag and display this message only once? Consider if someone has 100 endpoints with 3-4 headers per endpoint, all following this format which is valid according to RFC. That would be a ton of noise in the console, and would be frustrating for anyone not deploying to Apache/Nginx (e.g. standalone Spring Boot server). Other than that concern, I think this looks great and I appreciate the detailed message.

There is no color difference between info and warn in terminal output. Can we enhance logging to use colors?

I think this would be somewhat low priority, but definitely something we could evaluate.

[ybelenko]

That would be a ton of noise in the console, and would be frustrating for anyone not deploying to Apache/Nginx

I would add an option to turn this validation off completely. I filed this issue because I encountered this problem twice(every time like the first time, I didn't remember about Apache specifics and been frustrated). This PR heals my personal pain 😄

The other thing that drives me crazy is that Apache doesn't allow dashes or underscores in domain names... well I already forget which one, see.

[jimschubert]

@ybelenko following up, what about William's suggestion to move this to PHP generators only, or my suggestion to move this out of code which will run for every operation to something like the custom validation? As the PR stands at the moment I think this will add too much noise and I'd like to avoid doing this since it's done similarly in many different places. For example, see these from ModelUtils and DefaultCodegen:

[ybelenko]

@jimschubert I think this is up to you and @wing328.

my suggestion to move this out of code which will run for every operation to something like the custom validation?

I've never used that validator before, but it sounds like a good idea to me.

[jimschubert]

I opened #4979 to move the recommendations from our validate command into new validator types, which will allow us to provide recommendations on OpenAPI Specification docs up front from each of our interfaces rather than just the CLI.

Please check it out and let me know what you think.

[jimschubert]

Included in #4979