New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Server][Spring] fix support interface and implementation classes for API controllers #16945
Conversation
[Server][Spring] support interface and implementation classes for API controllers
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks ok
@martin-mfg, As all test cases are passed and I also have added changes suggested by you in forked repo(kapilguptavz#1 (review)), could you please merger the PR in master branch. |
I don't have the rights to merge PRs. Please wait for the project maintainer |
Thanks! @martin-mfg. @wing328, We are trying our first contribution on behalf of Verizon. Kindly review the PR. |
Agreed with you that it shouldn't overwrite both files. Only interface files (e.g. PetsApi.java) should be overwritten. |
tested locally and confirmed implementation file is not overwritten (only interface file is overwritten) |
modules/openapi-generator/src/main/java/org/openapitools/codegen/DefaultGenerator.java
Show resolved
Hide resolved
Added Log messages
@kapilguptavz btw, are you aware of the following option?
which skip the generation of the implementation files. Looks like it's trying to address the issue you encountered. |
Not aware of this option as I followed the problem statement. If anything needs to be done from our end?, if not you can merger the PR. |
I'll merge it over the weekend if no question/feedback from anyone else. Have a nice weekend. |
Thanks! same to you! |
And in addition to above, same logic can be added for other java servers(like java play framework and all), we can make generatorCheck and templateCheck as configurable input. |
// checking if apiController file is already existed for spring generator | ||
private boolean apiFilePreCheck(String filename, String generator, String templateName, String apiControllerTemplate){ | ||
File apiFile = new File(filename); | ||
if(apiFile.exists() && config.getName().equals(generator) && templateName.equals(apiControllerTemplate)){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can directly return the boolean value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MelleD , have removed else and directly returned Boolean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better :-)
But you can also remove completely the whole if, right?
return apiFile.exists() || config.getName().equals(generator) ||templateName.equals(apiControllerTemplate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
below logic I am applying here:
if file is already existed && generator is spring && template is controller, then only skip creating the file , other wise we have to create files if any of the conditions holds false.
but in above suggestion it will create file if any of the condition holds true.
If I remove if I will have to add as below:
return apiFile.exists() && config.getName().equals(generator) && templateName.equals(apiControllerTemplate)
additionally "!" I have to add where I m using this function.
Please let me know if it is needed ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes or set brackets !(condition).
Up to you but to make a if and directly return a Boolean looks redundant :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pushed the suggested changes 👍
@wing328 so far looks good for me |
added enhancement for java spring asked in #426 (comment)
Reproduction Steps for java spring:
updated sample.yaml (paths details with post method added)
Result: In step 1, it will create PetsApi.java(interface ), PetsApiController.java(implementation class) as new and in 2nd step
it overwrites both file.
Enhancement with result:
added checks for file creation for java spring server for controller class.
Unit Test Case: Tested for multiple endpoints
sample.yaml
Result: generated server stubs multiple times. It only overwrites interfaces (pets.api and pots.api). Implementation classes(PetsApiController.java and PotsApiController.java) are created once.
@cachescrubber @welshm @MelleD @atextor @manedev79 @javisst @borsch @Zomzog @martin-mfg @wing328
PR checklist
This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
These must match the expectations made by your contribution.
You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example
./bin/generate-samples.sh bin/configs/java*
.For Windows users, please run the script in Git BASH.
master
(upcoming 7.1.0 minor release - breaking changes with fallbacks),8.0.x
(breaking changes without fallbacks)