[C++] Simplify forward declaration detection #11490

dschmidt · 2022-02-01T20:20:48Z

This makes sure all model classes are added to the forward declarations which makes it possible to create templates without any model includes.
This helps to resolve circular inclusion issues.

This adds more forward declarations of which a lot are not strictly needed by the current templates, but they don't harm either. Trying to detect in which case declarations are needed or not or making it configurable does not seem to be worth the effort and would add a lot of complexity.

If you have a spec like this:

openapi: 3.0.1
info:
  title: Libre Graph API
  version: v0.9.0
  license:
    name: Apache 2.0
    url: https://www.apache.org/licenses/LICENSE-2.0.html
servers:
  - url: https://ocis.ocis-traefik.latest.owncloud.works/
    description: ownCloud Infinite Scale Latest
paths: {}
components:
  schemas:
    drive:
      description: Storage Space. Read-only.
      readOnly: true
      type: object
      properties:
        root:
          $ref: '#/components/schemas/driveItem'
    user:
      description: Represents an Active Directory user object.
      type: object
      properties:
        drive:
          $ref: '#/components/schemas/drive'
    driveItem:
      description: Reprensents a resource inside a drive. Read-only.
      readOnly: true
      type: object
      properties:
        lastModifiedByUser:
          $ref: '#/components/schemas/user'
          description: Identity of the user who last modified the item. Read-only.
          readOnly: true

(excerpt from https://github.com/owncloud/libre-graph-api/blob/main/api/openapi-spec/v0.0.yaml)

You get issues with circular #includes: Drive requires DriveItem requires User requires Drive 💥
To solve this issue a template may only use forward declarations and must use pointers to model objects or a pimple implementation.

I'm working on templates based on the cpp-qt-client, but with the current implementation (without this PR) required forward declarations are missing: e.g., Drive only has a User forward declaration, the DriveItem forward declaration is missing. With this PR all models used in the header are added to the forward declarations.

I found the old code hard to reason about so i'm not 100% sure this does not remove any forward declarations in some edge case, but it works for me for the original spec that my excerpt is from and it's not completely trivial.

Attention: This change alone is not enough to make the generator produce compilable code for the spec above, but it's all that's needed to create working templates. Templates are still work in progress because I've still got runtime issues to resolve.

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.
File the PR against the correct branch: master (5.3.0), 6.0.x
If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

dschmidt · 2022-02-01T20:25:58Z

The PR template said I should @ mention the language technical commitee but the link to more information was dead ... and I could not find out how to mention the cpp technical commitee. If anyone knows the name to mention reads this, please add it :)

dschmidt · 2022-02-02T08:24:56Z

Maybe CC @wing328

dschmidt · 2022-02-02T13:30:14Z

...s/openapi-generator/src/main/java/org/openapitools/codegen/languages/AbstractCppCodegen.java

@@ -418,13 +418,10 @@ private void addForwardDeclarations(CodegenModel parentModel, Map<String, Object
                if( !childPropertyType.equals(childModel.classname) || childPropertyType.equals(parentModel.classname) || !childModel.hasVars ){


What is the !childModel.hasVars check good for?
To me it looks like we might want to get rid of that too

dschmidt · 2022-02-02T13:31:47Z

Found the commitee info: @ravinikam (2017/07) @stkrwork (2017/07) @etherealjoy (2018/02) @MartinDelille (2018/03) @muttleyxd (2019/08)

wing328 · 2022-02-12T07:27:03Z

samples/client/petstore/cpp-restsdk/client/model/Pet.h

@@ -31,6 +31,8 @@ namespace openapitools {
 namespace client {
 namespace model {

+class Category;
+class Tag;


@dschmidt thanks for the PR. Are these changes expected?

Thanks for coming back to me!

Well, yes. In this specific case they are not strictly neccessary obviously, but they help in the general case. It's what this PR is all about: make it possible to create template which don't need model includes and avoid circular inclusion problems that way.

I think having forward declarations that are not strictly needed for everyone and every template is still much better than trying to be supersmart and to add complicated logic to determine whether they are needed or better than adding an option for it

... and actually that should be the case for cpp-restsdk already as it uses shared_ptr for storing data members.

I. e. in theory we should be able to move model includes to the cpp files already

dschmidt · 2022-02-22T13:51:33Z

Can I do anything to make this happen?

muttleyxd · 2022-02-22T13:59:20Z

Can I do anything to make this happen?

I remember looking at this few days ago, but forgot to press approve. This looks fine to me, from C++ side, I didn't work much with cpp-restsdk though.

dschmidt · 2022-02-22T14:01:34Z

Yeah, this should not change anything for cpp-restsdk (except for fixing circular include issues).
We could possibly remove includes as well, but that's beyond the scope of this PR.

dschmidt · 2022-02-22T14:07:01Z

btw here's what we're doing with that now: https://github.com/owncloud/libre-graph-api/tree/main/templates/cpp-qt-client

We have pimpl'ed the model classes and made it compile despite the circular deps in our spec file. We could have gone for shared pointers in the main class as well but while at it .... 😁

No idea if that would be interesting for upstream, as it's most likely a breaking change if anyone relies on the internal implementation of the model classes.

edit:
... and here's the generated code: https://github.com/owncloud/libre-graph-api-cpp-qt-client

P.S.: Thanks for reviewing and approving

wing328 · 2022-02-24T06:36:41Z

@dschmidt can you please resolve the merge conflicts when you've time as I've just merged #11487 ?

dschmidt · 2022-02-24T09:12:12Z

Sure, will take care

This makes sure all model classes are added to the forward declarations which makes it possible to create templates without any model includes which helps to resolve circular inclusion issues.

dschmidt · 2022-02-24T19:37:10Z

After my rebase the CI started failing, but I don't see how this change would be able to introduce any runtime issue... is this an issue in master?

wing328 · 2022-02-26T06:56:38Z

Tested locally and the result is good.

dschmidt commented Feb 2, 2022

View reviewed changes

wing328 reviewed Feb 12, 2022

View reviewed changes

muttleyxd approved these changes Feb 22, 2022

View reviewed changes

dschmidt force-pushed the more_extensive_forward_declares branch 2 times, most recently from 2998e3e to 6f3a5a0 Compare February 24, 2022 09:49

Simplify forward declaration detection

91beb7b

This makes sure all model classes are added to the forward declarations which makes it possible to create templates without any model includes which helps to resolve circular inclusion issues.

dschmidt force-pushed the more_extensive_forward_declares branch from 6f3a5a0 to 91beb7b Compare February 24, 2022 18:21

wing328 added Client: C++ Enhancement: General labels Feb 26, 2022

wing328 added this to the 6.0.0 milestone Feb 26, 2022

wing328 merged commit e35a127 into OpenAPITools:master Feb 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[C++] Simplify forward declaration detection #11490

[C++] Simplify forward declaration detection #11490

dschmidt commented Feb 1, 2022 •

edited

dschmidt commented Feb 1, 2022

dschmidt commented Feb 2, 2022

dschmidt Feb 2, 2022

dschmidt commented Feb 2, 2022

wing328 Feb 12, 2022

dschmidt Feb 12, 2022

dschmidt Feb 12, 2022

dschmidt commented Feb 22, 2022

muttleyxd commented Feb 22, 2022

dschmidt commented Feb 22, 2022

dschmidt commented Feb 22, 2022 •

edited

wing328 commented Feb 24, 2022

dschmidt commented Feb 24, 2022

dschmidt commented Feb 24, 2022

wing328 commented Feb 26, 2022

		@@ -418,13 +418,10 @@ private void addForwardDeclarations(CodegenModel parentModel, Map<String, Object
		if( !childPropertyType.equals(childModel.classname) \|\| childPropertyType.equals(parentModel.classname) \|\| !childModel.hasVars ){

[C++] Simplify forward declaration detection #11490

[C++] Simplify forward declaration detection #11490

Conversation

dschmidt commented Feb 1, 2022 • edited

PR checklist

dschmidt commented Feb 1, 2022

dschmidt commented Feb 2, 2022

dschmidt Feb 2, 2022

Choose a reason for hiding this comment

dschmidt commented Feb 2, 2022

wing328 Feb 12, 2022

Choose a reason for hiding this comment

dschmidt Feb 12, 2022

Choose a reason for hiding this comment

dschmidt Feb 12, 2022

Choose a reason for hiding this comment

dschmidt commented Feb 22, 2022

muttleyxd commented Feb 22, 2022

dschmidt commented Feb 22, 2022

dschmidt commented Feb 22, 2022 • edited

wing328 commented Feb 24, 2022

dschmidt commented Feb 24, 2022

dschmidt commented Feb 24, 2022

wing328 commented Feb 26, 2022

dschmidt commented Feb 1, 2022 •

edited

dschmidt commented Feb 22, 2022 •

edited