Skip to content
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

[C++][Pistache] Generate API generalization interface #15279

Merged
merged 11 commits into from
Apr 26, 2023

Conversation

CTerasa-ep
Copy link
Contributor

@CTerasa-ep CTerasa-ep commented Apr 21, 2023

The code generated for main() contains self-similar construction of, and calls to API implementation classes.

Example:

    PetApiImpl PetApiserver(router);
    PetApiserver.init();
    StoreApiImpl StoreApiserver(router);
    StoreApiserver.init();
    UserApiImpl UserApiserver(router);
    UserApiserver.init();

This sceams for OO generalization.

We could then store all *server objects inside one vector of API implementations and call init() on all of them.

    auto apiImpls = std::vector<std::shared_ptr<ApiBase>>();

    apiImpls.push_back(std::make_shared<PetApiImpl>(router));
    apiImpls.push_back(std::make_shared<StoreApiImpl>(router));
    apiImpls.push_back(std::make_shared<UserApiImpl>(router));

    for (auto api : apiImpls) {
        api->init();
    }

This patch set refactors some Pistache generator code and introduces a base class for APIs called ApiBase and adds tehe above construction code.

Additionally do some refactoring work in the Pistache generator code, that leverage some functional stream() methods that may make some intentions more explicit.

Technical Committee C++:

PR checklist

  • Read the contribution guidelines.
  • Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
  • Run the following to build the project and update samples:
    ./mvnw clean package 
    ./bin/generate-samples.sh
    ./bin/utils/export_docs_generators.sh
    
    Commit all changed files.
    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.
  • In case you are adding a new generator, run the following additional script :
    ./bin/utils/ensure-up-to-date.sh
    
    Commit all changed files.
  • File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
  • If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

@CTerasa-ep CTerasa-ep changed the title Cpp pistache add api base [C++][Pistache] Add API generalization interface Apr 21, 2023
@CTerasa-ep CTerasa-ep changed the title [C++][Pistache] Add API generalization interface [C++][Pistache] Generate API generalization interface Apr 21, 2023
Supporting files are set up in the CppPistacheServerCodegen()
constructor as well as in processOpts(). Refactor the code and extract a
method setupSupportingFiles().
Both branches of the if/else do the same steps. Refactor this out and
invert logic.
Both branches of the if/else if do the similar steps and are dependent
on the suffix. Make this obvious by introducing a new method
injectImplInFilename(String result, String suffix).
We do not need the separatorChar index to inject the "Impl" string.
Simply truncate the whole string.

Also rename the parameter from 'result' to' filename'.
Pull out the post-processing for a single operation, and also pull out
post-processing for parameters.

Introduce boolean expressions for supported parsing per parameter, and
consumption of JSON.

Reorder code to make locality more explicit i.e. how consumeJSON and
isParsingSupported is generated and used.
Functional matching like anyMatch() directly state what boolean value is
 searched.

 However, the Predicates deserve to heave names themselves.
Looking at the generated main-api-server.cpp code it gets obvious that
the API classes are self similar with a similar interface.
Only the construction and teh initialization is called in the main()
function. Leverage this fact to create a generalization ApiBase.

Introduce ApiBase as a pure virtual base class to the concrete API
classes and declare init() as virtual.

Pull the route member into the base class.

With this change we could have a container hold all the ApiImpl
objects later and call init() on all of them using a for_each loop.
Refactor the main-api-server template to use a vector for ApiImpl
storage instead of separate objects. This leverages the previously
added ApiBase generalization.

 We push all concrete ApiImpl objects into a vector and call init() on
  each of them.
Due to teh addition of ApiBase class update the generated sample.
@CTerasa-ep CTerasa-ep marked this pull request as ready for review April 24, 2023 14:44
While writing the comment, I realized that the method name could be more
precise. Thus rename injectImplInFilename to implFilenameFromApiFilename
and add comment.
@muttleyxd
Copy link
Contributor

C++ and template code looks good to me

@wing328 wing328 merged commit b847140 into OpenAPITools:master Apr 26, 2023
@wing328 wing328 added this to the 6.6.0 milestone Apr 26, 2023
@CTerasa-ep
Copy link
Contributor Author

Great! Thank you for the feedback and your effort!

sjoubert added a commit to sjoubert/openapi-generator that referenced this pull request May 10, 2023
This is following OpenAPITools#15279
Marking those methods 'override' should avoid producing
'-Winconsistent-missing-override' warnings or similar.
wing328 pushed a commit that referenced this pull request May 16, 2023
This is following #15279
Marking those methods 'override' should avoid producing
'-Winconsistent-missing-override' warnings or similar.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants