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.

Sign up for GitHub

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

Jump to bottom

[cpp-pistache] Various fixes for Pistache #497

Merged
merged 4 commits into from
Jul 20, 2018
Merged

[cpp-pistache] Various fixes for Pistache #497

merged 4 commits into from
Jul 20, 2018

Conversation

etherealjoy
Copy link
Contributor

@etherealjoy etherealjoy commented Jul 7, 2018 •
edited
Loading

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 and ./bin/security/{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.1.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

  • Add Map support
  • Add Array support
  • Add ByteArray support
  • Fixed function signatures

Fixes #482
Fixes #446
Fixes #414
Fixes #407
Fixes #258

@stkrwork @MartinDelille @ravinikam

This was referenced Jul 7, 2018
Closed
Closed
Closed
@etherealjoy etherealjoy mentioned this pull request Jul 7, 2018
Closed
@etherealjoy
Copy link
Contributor Author

PetStore samples can be tested after build together with this change #495

@wing328 wing328 added this to the 3.1.1 milestone Jul 8, 2018
@CyrilleBenard
Copy link

I just checked all the concerned issues (listed above) and I agree that they are all fixed (#407 - cpprestsdk - too)
Another one is also fixed : #258

General remark : The fixes don't implement the std::shared_ptr usage so that all objects are copied "instead of passing their content". That means a lack of performances.
Anyway, I suppose the use of std::shared_ptr would have took a lot of time to implement it.

Good job @etherealjoy 👍

@etherealjoy
Copy link
Contributor Author

etherealjoy commented Jul 10, 2018 •
edited
Loading

@CyrilleBenard
We actually moved from pointer to objects because people were terrified of memory leakages.
Also when reference object is used copying is not done.

@etherealjoy etherealjoy mentioned this pull request Jul 10, 2018
Closed
stkrwork
stkrwork requested changes Jul 13, 2018
View reviewed changes
modules/openapi-generator/src/main/resources/cpp-rest-sdk-client/model-source.mustache
@@ -245,26 +245,29 @@ void {{classname}}::fromJson(web::json::value& val)
{{#isString}}
{{setter}}(ModelBase::stringFromJson(val[utility::conversions::to_string_t("{{baseName}}")]));
{{/isString}}
{{#isByteArray}}{{setter}}(ModelBase::stringFromJson(val[utility::conversions::to_string_t("{{baseName}}")]));{{/isByteArray}}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stringFromJson does not exist, according to your header

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check.

Copy link
Contributor Author

@etherealjoy etherealjoy Jul 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stkrwork
I checked it and it is there. Actually I generated and tested this code
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/cpp-rest-sdk-client/modelbase-header.mustache#L52
Fixes for cpp-rest-sdk are in this PR also actually.

Copy link
Contributor Author

@etherealjoy etherealjoy Jul 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested using the spec from #407 for cpp-rest-sdk
See also comment by @CyrilleBenard above.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked this, and this is the modelbase header from your pr:
https://github.com/etherealjoy/openapi-generator/blob/4098c7ae299bb8e0e0b31a6beb37fbcb08700a81/modules/openapi-generator/src/main/resources/cpp-pistache-server/modelbase-header.mustache

i cant seem to find the function there.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stkrwork
This change is in cpp-rest-sdk-client
I simply combined both the fixes in one.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nvm, i didn't see the cpp-rest-sdk-client in the path, my bad

Copy link
Contributor

@stkrwork stkrwork Jul 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Next time, please separate it into two pull requests, or change the title, because i was not expecting cpprestsdk code in the pr

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, sorry for the confusion.

modules/openapi-generator/src/main/resources/cpp-rest-sdk-client/model-source.mustache
{{/isString}}
{{^isString}}
{{/isString}}{{#isByteArray}}
{{setter}}(ModelBase::stringFromJson(val[utility::conversions::to_string_t("{{baseName}}")]));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will check.

Copy link
Contributor Author

@etherealjoy etherealjoy Jul 14, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See above. Also tested using spec from #407
Confirmed by @CyrilleBenard above.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm surprised with this suspicious use of the undefined ModelBase::stringFromJson because I generated a cpprestsdk client with a heavy YAML input file and the build + partial link (lib generation) succeeded. May be I missed something ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@CyrilleBenard
:)
ModelBase::stringFromJson is there
https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/cpp-rest-sdk-client/modelbase-header.mustache#L52

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@etherealjoy Yes sure but so, what's the @stkrwork remark about ?

stringFromJson does not exist, according to your header

I'm a little lost :

  • I checked your current PR and I found no compilation issue
  • @stkrwork said : stringFromJson does not exist

😨

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i didnt see that that file was already part of the cpp rest sdk. i thought he was purely working on pistache in the pr, so that is why i was confused, because i checked the pistache file for modelbase-header

@jmini jmini modified the milestones: 3.1.1, 3.1.2 Jul 18, 2018
@etherealjoy
Copy link
Contributor Author

@wing328
Let's merge this this. We can solve many bugs with this.

@wing328 wing328 merged commit 97d6b71 into OpenAPITools:master Jul 20, 2018
@wing328
Copy link
Member

wing328 commented Jul 20, 2018

@etherealjoy 👌 PR merged into master. Have a nice weekend.

@CyrilleBenard
Copy link

Nice 👍

@wing328
Copy link
Member

wing328 commented Jul 20, 2018

I'll give it a shot this weekend to setup CI for samples/server/petstore/cpp-pistache/

My understanding is that I will need to build https://github.com/oktal/pistache manually.

@etherealjoy etherealjoy deleted the cpp-pistache-fix-nocompilation branch July 21, 2018 02:21
@wing328 wing328 changed the title [cpp-pistache] Fix compilation of petstore for Pistache [cpp-pistache] Various fixes for Pistache Jul 25, 2018
@etherealjoy etherealjoy mentioned this pull request Aug 6, 2018
Closed
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
@etherealjoy @wing328
[cpp-pistache] Fix compilation of petstore for Pistache (OpenAPITools…
651de59 
…#497)

* Fix compilation of petstore for Pistache
Add Map support

* Add support for ByteArray

* Add Support for ByteArray in cpprest

* Implement TODOs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stkrwork stkrwork stkrwork approved these changes

@CyrilleBenard CyrilleBenard CyrilleBenard left review comments

Assignees
No one assigned
Labels
Issue: Bug Server: C++
Projects
None yet
Milestone
3.1.2
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@etherealjoy @CyrilleBenard @wing328 @stkrwork @jmini