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] Fixed #2643 #2653

Merged
merged 5 commits into from
Apr 19, 2019
Merged

Conversation

SalDiAngelus
Copy link
Contributor

Refactored to/from json functions to use universal object serialization method.

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\.
  • Filed the PR against the correct branch: master, 3.4.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@ravinikam, @stkrwork, @fvarose, @etherealjoy, @MartinDelille

Description of the PR

Purpose is to fix issue #2643. While I was in there, I saw the mustache file for serialization was more complex than it needed to be so I simplified it by using the to_json and from_json functions that nlohmann/json uses to find custom types to serialize. Removed the ModelBase class because the helper functions weren't needed anymore.

Refactored to/from json functions to use universal object serialization method.
@wing328
Copy link
Member

wing328 commented Apr 13, 2019

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/OpenAPITools/openapi-generator/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/OpenAPITools/openapi-generator/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

@SalDiAngelus
Copy link
Contributor Author

@stkrwork Can I ask what the purpose of the helpers class is? It doesn't seems to be used for anything in generated APIs.

@stkrwork
Copy link
Contributor

stkrwork commented Apr 15, 2019

Their purpose was to convert strings, or array in string to the correct datatype. with your new system, they should be redundant as well now.

@SalDiAngelus
Copy link
Contributor Author

Should I delete it then?

@stkrwork
Copy link
Contributor

stkrwork commented Apr 15, 2019

if they aren't referenced anywhere in the code, yes

@stkrwork
Copy link
Contributor

stkrwork commented Apr 15, 2019

@etherealjoy as you made the helpers, can you check this as well?

@etherealjoy
Copy link
Contributor

etherealjoy commented Apr 15, 2019

@SalDiAngelus
I will try this PR with nested arrays/maps.
Did you try a spec with nested arrays and maps by any chance

@etherealjoy
Copy link
Contributor

Code does not compile anymore
https://github.com/OpenAPITools/openapi-generator/pull/2653/files#diff-d3400a21fe4566d943fead7d6a483aebL90

PetApi.cpp: In member function ‘void 
org::openapitools::server::api::PetApi::find_pets_by_status_handler(const Pistache::Rest::Request&, 
Pistache::Http::ResponseWriter)’:
PetApi.cpp:85:12: error: ‘fromStringValue’ was not declared in this scope
         if(fromStringValue(statusQuery.get(), value)){

@SalDiAngelus
Copy link
Contributor Author

I've tried nested arrays with non default types, but I haven't used maps.

@etherealjoy etherealjoy changed the title Fixed #2643 [C++][Pistache] Fixed #2643 Apr 15, 2019
@etherealjoy
Copy link
Contributor

ok, it compiles now
I will test it with nested maps

@etherealjoy
Copy link
Contributor

I used this spec

---
# This is a sample Swagger spec, describing a simple API as a starting point.
swagger: "2.0"
info:
  description: Template Description
  version: 1.0.0
  title: Testspec


host: 0.0.0.0:8080
basePath: /api
schemes:
- http

paths:
  /v1.0/test0:
    get:
      description: ""
      operationId: test0
      responses:
        200:
          description: "Successful response"
          schema:
            $ref: '#/definitions/obj'
        400:
          description: "The request was not successfully executed."

  /v1.0/test1:
    get:
      description: ""
      operationId: test1
      responses:
        200:
          description: "Successful response"
          schema:
            $ref: '#/definitions/arrofarr'
        400:
          description: "The request was not successfully executed."
     
  /v1.0/test2:
    get:
      description: ""
      operationId: test2
      responses:
        200:
          description: "Successful response"
          schema:
            $ref: '#/definitions/mapofmap'
        400:
          description: "The request was not successfully executed."

  /v1.0/test3:
    get:
      description: ""
      operationId: test3
      responses:
        200:
          description: "Successful response"
          schema:
            $ref: '#/definitions/arrofmap'
        400:
          description: "The request was not successfully executed."
     
  /v1.0/test4:
    get:
      description: ""
      operationId: test4
      responses:
        200:
          description: "Successful response"
          schema:
            $ref: '#/definitions/mapofarr'
        400:
          description: "The request was not successfully executed."      
  /v1.0/test5:
    get:
      description: ""
      operationId: test5
      responses:
        200:
          description: "Successful response"
          schema:
            $ref: '#/definitions/cplxobj'
        400:
          description: "The request was not successfully executed."            

definitions:

  obj:
    description: "simple object"
    type: object
    properties:
      simpleitem:
        type: string

  arrofarr:
    description: "array of array"
    type: array
    items:
      type: array
      items: 
        $ref: '#/definitions/obj'

  mapofmap:
    description: "map of map"
    type: object
    additionalProperties:
      type: object
      additionalProperties:
        $ref: '#/definitions/obj'

  arrofmap:
    description: "array of map"
    type: array
    items:
      type: object
      additionalProperties:
        $ref: '#/definitions/obj'

  mapofarr:
    description: "map of array"
    type: object
    additionalProperties:
      type: array
      items: 
        $ref: '#/definitions/obj' 
        
  cplxobj:
    description: "complex object"
    type: object
    properties:
      item1:
        $ref: '#/definitions/arrofarr'  
      item2:
        $ref: '#/definitions/mapofmap'  
      item3:
        $ref: '#/definitions/arrofmap'  
      item4:
        $ref: '#/definitions/mapofarr'          

@etherealjoy
Copy link
Contributor

This is up for discussion, but the deserialiation of array and map aliases is a little more complicated due to missing helpers.

Just for discussion i dont have preferences.

@SalDiAngelus
Copy link
Contributor Author

This is up for discussion, but the deserialiation of array and map aliases is a little more complicated due to missing helpers.

Just for discussion i dont have preferences.

Nlohmann json had std container types built in. Using the yaml file you provided, the code below generates the json below.

Cplxobj obj;
Obj obj2;
obj2.setSimpleitem("value");
obj.m_Item1IsSet = true;
obj.m_Item2IsSet = true;
obj.m_Item3IsSet = true;
obj.m_Item4IsSet = true;
obj.m_Item1 = { {obj2,obj2}, {obj2, obj2, obj2} };
obj.m_Item2 = { {"a", {{"1", obj2}, {"2", obj2}}}, {"b", { { { "3", obj2 }, {"4", obj2}, {"5", obj2} } }} };
obj.m_Item3 = { {{"1", obj2}, {"2", obj2}}, { {"3", obj2}, {"4", obj2}, {"5", obj2}} };
obj.m_Item4 = { {"a", {obj2,obj2} }, {"b", {obj2, obj2, obj2}} };
string str = nlohmann::json(obj).dump();
{
  "item1":[
    [
      {
        "simpleitem":"value"
      },
      {
        "simpleitem":"value"
      }
    ],
    [
      {
        "simpleitem":"value"
      },
      {
        "simpleitem":"value"
      },
      {
        "simpleitem":"value"
      }
    ]
  ],
  "item2":{
    "a":{
      "1":{
        "simpleitem":"value"
      },
      "2":{
        "simpleitem":"value"
      }
    },
    "b":{
      "3":{
        "simpleitem":"value"
      },
      "4":{
        "simpleitem":"value"
      },
      "5":{
        "simpleitem":"value"
      }
    }
  },
  "item3":[
    {
      "1":{
        "simpleitem":"value"
      },
      "2":{
        "simpleitem":"value"
      }
    },
    {
      "3":{
        "simpleitem":"value"
      },
      "4":{
        "simpleitem":"value"
      },
      "5":{
        "simpleitem":"value"
      }
    }
  ],
  "item4":{
    "a":[
      {
        "simpleitem":"value"
      },
      {
        "simpleitem":"value"
      }
    ],
    "b":[
      {
        "simpleitem":"value"
      },
      {
        "simpleitem":"value"
      },
      {
        "simpleitem":"value"
      }
    ]
  }
}

@etherealjoy
Copy link
Contributor

etherealjoy commented Apr 18, 2019

Ok, good.
One improvement we can do in the future is somethign like the Qt5 Server it create handler with correct signature directly
Or at least create a local objects which are deserialized from the request for the steps you mentioned above

@etherealjoy etherealjoy added this to the 4.0.0 milestone Apr 19, 2019
@etherealjoy etherealjoy merged commit 89eb603 into OpenAPITools:master Apr 19, 2019
jimschubert added a commit that referenced this pull request Apr 25, 2019
* master: (40 commits)
  Remove quotation marks around {{paramName}} for header params in api-body.mustache (#2727)
  Add FiNC Technologies (#2728)
  fix missing parenthesis for http bearer auth (#2723)
  Add missing closing parenthesis (#2720)
  update perl test with correct body parameter (#2717)
  [Java][Spring] Fix template for reactive implementation with `interfaceOnly` parameter (#2437)
  Bugfix(Perl): Support nested primitive types in ARRARY or HASH for basic object (#2713)
  Remove `-XX:MaxPermSize` (#2712)
  Remove setting generateAliasAsModel in rust server generator (#2714)
  update rust server samples
  Revert "update rust samples"
  update rust samples
  update samples
  [Rust Server] Improve XML support (#2504)
  Improve CONTRIBUTING.md (#2699)
  [PHP][Lumen] Rename template folder (#2707)
  [aspnetcore] Support async tasks and some code cleanups (#2629)
  [C++][Pistache] Fixed #2643 (#2653)
  update petstore samples (#2697)
  [JAVA][Webclient]fix select body for url encoded media type. (#2686)
  ...
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.

4 participants