[C++][Pistache] Simplified model template

[muttleyxd]

PR checklist

• Read the contribution guidelines.
• Ran the shell script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh, ./bin/openapi3/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in .\bin\windows\. If contributing template-only or documentation-only changes which will change sample output, be sure to build the project first.
• Filed the PR against the correct branch: master, 4.1.x, 5.0.x. Default: master.
• Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@ravinikam @stkrwork @etherealjoy @MartinDelille

Description of the PR

Well, this PR targets two issues:

• There's a bug in current generator, if you declare a structure containing an array, then it will be declared as std::vector, which is good. But also there won't be a setter, which for me is fine, since you can get it through getter. But it doesn't set isSet variable, which means to_json function doesn't do anything useful (well it puts some members into json, but not all).

class UnsettableVector {
public:
    std::vector<int32_t>& getSomeints();
    // Missing setter
    bool someintsIsSet() const;
    void unsetSomeints();
}

YAML:

openapi: 3.0.0
info:
  description: Some description
  version: 0.0.1
  title: Some title

tags:
  - name: hello

paths:
  "/there":
    get:
      operationId: helloThereGet
      tags:
        - hello
      summary: Do something
      responses:
        200:
          description: Successful operation
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/UnsettableVector"
servers:
  - url: http://localhost:8080
components:
  schemas:
    UnsettableVector:
      type: object
      properties:
        someints:
          type: array
          items:
            type: integer
        somestring:
          type: string

HelloApiImpl.cpp:

#include "UnsettableVector.h"

void HelloApiImpl::hello_there_get(Pistache::Http::ResponseWriter &response) {
    UnsettableVector vec;
    vec.setSomestring("Hello there");
    vec.getSomeints().push_back(1);
    vec.getSomeints().push_back(2);
    vec.getSomeints().push_back(3);

    nlohmann::json json;
    to_json(json, vec);
    
    response.send(Pistache::Http::Code::Ok, json.dump(4));
}

curl http://127.0.0.1:8080/there

{
    "somestring": "Hello there"
}                              

someints are missing

• Second issue may not be an issue, even it may be better design, so that's a thing to discuss. I don't come from Java, so this model looks like unneeded OOP, when this model could've been a simpler struct.
  I don't know what to do with optional fields, since I wasn't able to influence current generator to generate different code (so it could omit some parameters), so currently I don't see if it does anything.
  That's why I went with simpler struct like design, which may not be right if there may be optional fields.

Anyways, this new design allows more flexible syntax, especially that we won't need to create separate json/class objects to create one object from another type.

HelloApiImpl.cpp

#include "UnsettableVector.h"

void HelloApiImpl::hello_there_get(Pistache::Http::ResponseWriter &response) {
    response.send(Pistache::Http::Code::Ok, UnsettableVector{{1, 2, 3}, "Hello there"}.to_json().dump(4));
}

curl http://127.0.0.1:8080/there

{
    "someints": [
        1,
        2,
        3
    ],
    "somestring": "Hello there"
} 

[etherealjoy]

Here we would have a problem for native types for example we serialize everything even if not set, e.g. int being zero, string being "", default or uninitialized values
Which should not be serialized, unless if explicitly set by the application.

Additionally this is a breaking change for all existing code.

[muttleyxd]

You're right.

Anyways, I'd change three things in current model:

• Add constructor which would allow setting all members with one line
• Better from/to_json function, so we don't do C style (creating object then passing it to function)
• Fix setter for containers, but I'm not sure what should be done here, I see three ideas:
  • make getVectorMember() return const reference, and have separate set method
    this has a problem of creating unnecessary objects
  • make getVectorMember() return reference and implicitly set vectorMember_isSet to true
    this has a problem that user could receive this model, check if meaningful is inside, get empty vector then pass it further with different value than was before
  • add setVectorMemberIsSet() function, this doesn't seem intiuitive for me, but it solves two above problems

What do you think about it?

[kuzkry]

Additionally, we may think about something resembling:

template <typename... Args>
MyResponse::MyResponse(Args... args);

and if e.g. MyResponse had 5 fields and a user gave only 3, then in return the first three would be set, the rest would not.
The flaws I can see are:

• use of templates (for some ;)) and workarounds for the lack of fold expressions in C++11
• forced order of member setting (I mean, the user couldn't tell to initialize last 3 elements)

[etherealjoy]

@muttleyxd @kuzkry
Thanks for the follow up

Add constructor which would allow setting all members with one line

For objects with lots of members, which is fairly common in REST, it is not intuitive, and default values cannot help when the members of interest are not in order

Better from/to_json function, so we don't do C style (creating object then passing it to function)

This is a good idea

Fix setter for containers, but I'm not sure what should be done here, I see three ideas:
make getVectorMember() return const reference, and have separate set method
this has a problem of creating unnecessary objects
make getVectorMember() return reference and implicitly set vectorMember_isSet to true
this has a problem that user could receive this model, check if meaningful is inside, get empty vector then pass it further with different value than was before
add setVectorMemberIsSet() function, this doesn't seem intiuitive for me, but it solves two above problems

You could take a look at the other C++ generators for a hint, Qt5 client/server generator for example.
In general how people use it now is get the vector and start adding items. During serialization if the vector is empy then it is skipped. We could improve here.

In general, please keep in mind drastic changes which involves rework of user code are to be kept as less as possible. Inside the classes is not a problem, just we have to be careful what the user code will access.

[muttleyxd]

Closing & Reopening to retrigger CI

[muttleyxd]

I've updated PR, now it targets 5.0.x branch, as it contains breaking changes.
Non-required fields are wrapped in Pistache::Optional.

@etherealjoy @wing328

struct UnsettableVector
{
    Pistache::Optional<std::vector<int32_t>> someints;
    Pistache::Optional<std::string> somestring;

    nlohmann::json to_json();
    static nlohmann::json to_json(const UnsettableVector& o);
    static UnsettableVector from_json(const nlohmann::json& j);
};

response.send(Pistache::Http::Code::Ok, UnsettableVector
{
  Pistache::Some(std::vector<int32_t>{1, 2, 3}), 
  Pistache::Some(std::string("Hello there"))
}.to_json().dump(4));

curl http://127.0.0.1:8080/there
{
    "someints": [
        1,
        2,
        3
    ],
    "somestring": "Hello there"
}

[etherealjoy]

@muttleyxd
We can target the PR for 4.x by renaming it and then when 5.x arrives we will deprecate the current one.
Does it makes sense ?

[muttleyxd]

@etherealjoy
Let me make sure I understood you correctly:

• you'd want to add simplified models as new files enabled with an 'optional' flag or something?
  lets' say model-simplified-source.mustache and model-simplified-header.mustache
  for 4.x

and when 5.x arrives, we'd swap them around?

Fine by me, if that's what you meant, since I'm not sure what are your intentions with renaming it

[wing328]

and when 5.x arrives, we'd swap them around?

We can make yours as the default in 5.x release but my guess is that there will be developers who prefer the current implementation so we need to keep it as an option.

For models in other generators (e.g. C#, Java), there are various options to customize the model so as to meet different requirement/preference.

[muttleyxd]

and when 5.x arrives, we'd swap them around?

We can make yours as the default in 5.x release but my guess is that there will be developers who prefer the current implementation so we need to keep it as an option.

For models in other generators (e.g. C#, Java), there are various options to customize the model so as to meet different requirement/preference.

Thank you for your tips.

Some changes to models & Java code. Please check if they're ok (I don't usually write Java, so I might have done something wrong).

So now model consists of:

struct struct_name {
  Pistache::Optional<type> optional_field;
  type required_field;

  nlohmann::json to_json();
  static struct_name from_json(nlohmann::json);
};

void to_json(nlohmann::json &j, struct_name const& o);
void from_json(nlohmann::json const &j, struct_name &o);

The last two methods are required for nlohmann::json.get_to() method to work, hence they are not members of model.
I've tested code it generates, it seems to work both when request is a model or when request is parsed/outputted from/to nlohmann::json.

Calling @etherealjoy since I've done changes I wanted.

[etherealjoy]

Interesting, It looks fine for me.
I will try it and give some feedback

[muttleyxd]

I've updated the PR, I removed some unnecessary code from to_json and from_json methods (as we don't need specific code for model members).

@etherealjoy

Interesting, It looks fine for me.
I will try it and give some feedback

You can play around with UnsettableVector (name comes from old & fixed bug in Pistache's generator), it's a nice and short showcase (well, it lacks required arguments)

openapi: 3.0.0
info:
  description: Some description
  version: 0.0.1
  title: Some title

tags:
  - name: hello

paths:
  "/there":
    get:
      operationId: helloThereGet
      tags:
        - hello
      summary: Do something
      responses:
        200:
          description: Successful operation
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/UnsettableVector"
    post:
      requestBody:
        description: json to deserialize then serialize
        required: true
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/UnsettableVector"
      operationId: helloTherePost
      tags:
        - hello
      summary: Do something (receive)
      responses:
        200:
          description: Successful operation
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/UnsettableVector"
servers:
  - url: http://localhost:8080
components:
  schemas:
    UnsettableVector:
      type: object
      properties:
        someints:
          type: array
          items:
            type: integer
        member:
          $ref: '#/components/schemas/VectorMember'
        strmem:
          type: string
    VectorMember:
      type: object
      properties:
        number:
          type: integer
        text:
          type: string

handlers (HelloApiImpl.cpp)

void HelloApiImpl::hello_there_get(Pistache::Http::ResponseWriter &response) {
    UnsettableVector vec
    {
        Pistache::Some(std::vector<int32_t>{1, 2, 3}),
        Pistache::Some(VectorMember
        {
            Pistache::Some(123),
            Pistache::None()
        }),
        Pistache::Some(std::string("hello there"))
    };
    response.send(Pistache::Http::Code::Ok, vec.to_json().dump());
}
void HelloApiImpl::hello_there_post(const UnsettableVector &unsettableVector, Pistache::Http::ResponseWriter &response) {
    response.send(Pistache::Http::Code::Ok, unsettableVector.to_json().dump());
}

curl -s http://127.0.0.1:8080/there
curl -s http://127.0.0.1:8080/there -X POST -d '{"member":{"number":123},"someints":[1,2,3],"strmem":"hello there"}'

it nicely outputs and parses incoming UnsettableVector.

[etherealjoy]

This new change is very interesting. Makes it a lot simpler when handling objects.
Did you have a chance to check out this issue #2769
I have not been able to not wrap my around to solving it. I think with this approach it could be solvable.

[muttleyxd]

@etherealjoy
I'll take a look at mentioned issue on Monday.

[wing328]

@muttleyxd thanks for the PR, which has been included in the v4.1.3 release: https://twitter.com/oas_generator/status/1180123829626003456