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

[CPPREST][CPP] Several Compilation Errors, setTags method missing #5862

Closed
solvingj opened this issue Jun 17, 2017 · 50 comments
Closed

[CPPREST][CPP] Several Compilation Errors, setTags method missing #5862

solvingj opened this issue Jun 17, 2017 · 50 comments

Comments

@solvingj
Copy link

Description

There are several methods which are causing compilation errors.
fromJson
fromMultipart
setTags

While I am able to find fromJson and fromMultipart, I cannot find the method setTags method anywhere. Interestingly, I generated Petstore and Petstore on Heroku samples and this method was not referenced anywhere in that code. Perhaps those specs don't have custom tags and my spec does.

Swagger-codegen version

http://editor.swagger.io
http://editor2.swagger.io

Swagger declaration file content or url

Here is the JSON Swagger spec:
https://raw.githubusercontent.com/Azure/azure-rest-api-specs/master/arm-iothub/2017-01-19/swagger/iothub.json

Command line used for generation

http://editor.swagger.io
http://editor2.swagger.io

Both produce this method setTags()

Steps to reproduce
  1. Generate the code from the provided swagger spec using the web editor.
  2. Open file model\Resource.cpp
  3. Navigate to line 166 and find :

setTags( newItem );

This method does not exist anywhere in any version of the the Microsoft CPPREST library:
https:/Microsoft/cpprestsdk
Nor does it exist in the generated sources.

Related issues

I could find no previous github issue referencing the setTags method.

Suggest a Fix

I noticed that the QT5 sample for petstore also references the setTags method. In researching, I saw that the CPPREST generator was probably based off the QT5 generator to some extent. It seems possible that the generation of setTags() was copied from the QT5 generator but without the code that generates an implementation of the method. Meanwhile, perhaps none of the sample specs used for testing happen to feature custom tags, so it never got generated during testing and never discovered. All very speculative, I'm really not sure.

@solvingj
Copy link
Author

solvingj commented Jun 17, 2017

I have posted the Azure IOTHub SDK with the issues here:

https:/solvingJ/azure-iothub-sdk-cpprest

@wing328
Copy link
Contributor

wing328 commented Jun 17, 2017

@solvingj I would suggest you to try the latest master to see if you can still repeat the issue as there're 9 PRs for CppRest since last stable release (which is used by the Swagger Editors)

@solvingj
Copy link
Author

I saw that but I wasn't sure which branch/version the swagger editors were built from. Furthermore, I only ever generated from the web interface. I'm not really an expert developer, so I don't know that I'm capable of compiling and building the generator and running it locally (or at least, not without spending a day on it and asking someone for help). When do you think the new generator code will be live on online editor?

@wing328
Copy link
Contributor

wing328 commented Jun 18, 2017

@solvingj what about using the pre-built Swagger Codegen CLI Docker image: https:/swagger-api/swagger-codegen#public-pre-built-docker-images?

When do you think the new generator code will be live on online editor?

We hope to do it by the end of July (it was already postponed from May 31st)

@solvingj
Copy link
Author

solvingj commented Jun 18, 2017

I actually did use the docker pre-built Swagger Codegen CLI image last night, and was able to generate. It still has the same issues. Will try to get post specific errors soon.

Here is the branch with the latest:
https:/solvingJ/azure-iothub-sdk-cpprest/blob/codegen-2_2_3-from-snapshot-via-docker

@klemens-morgenstern
Copy link

I have the same issue, I tried to generate the Swagger spec of the Azure IoT Hub and it generates a lot of errors where it tries to call fromMultiPart on an std::map. Not only in the 2.2 but also in the current 2.2.3 version:

azure-iothub-sdk-cpprest\model\IotHubProperties.cpp(337): error C2039: "toMultipart": Ist kein Element von "std::map<utility::string_t,std::shared_ptr<io::swagger::client::model::MessagingEndpointProperties>,std:: less<_Kty>,std::allocator<std::pair<const _Kty,_Ty>>>" [build\libs\azure-iothub-sdk-cpprest\CppRestSwaggerClient.vcxproj]

It seems to me like it's generating the wrong type there.

@wing328
Copy link
Contributor

wing328 commented Jun 18, 2017

It seems possible that the generation of setTags() was copied from the QT5 generator but without the code that generates an implementation of the method.

Yup, I think likely that's the case so we'll need to remove the code copied from the QT5 templates.

The cpprest templates can be found in https:/swagger-api/swagger-codegen/tree/master/modules/swagger-codegen/src/main/resources/cpprest

@solvingj @klemens-morgenstern May I know if you've time to contribute a PR to clean up the auto-generated code for CPPREST generator?

@solvingj
Copy link
Author

solvingj commented Jun 18, 2017

I'm a not experienced enough to be helpful. I barely know any C++, and don't understand how the code-generation in swagger codegen works really. It seems like this will require someone who understands both .
Sorry.

@wing328
Copy link
Contributor

wing328 commented Jun 19, 2017

I digged into this issue a little bit deeper and it turns out I was wrong about copying the code from QT5 templates. The issue was due to missing "setter" for properties that are "array":
https:/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/main/resources/cpprest/model-source.mustache#L405-L411

Using the Petstore store example, the Pet model has the property tags, which is an array of string: https:/swagger-api/swagger-codegen/blob/master/samples/client/petstore/cpprest/model/Pet.cpp#L296-L309

My question is what would the setter look like so that I can add it back to the model template for cpprest?

@solvingj
Copy link
Author

#5877 Fixed the setTags method.
Per @stkrwork

I added my changes to the cpprest template, which should fix the settag problem, but there is still the issue with the toMultipart, toJson, and fromJson function. The cpprest template will require some repairs.

Still looking for help implementing these.

solvingj referenced this issue Jun 19, 2017
…e as specified in contract (#5791)

* fixed map to use value instead of mapentry while doing fromJson.

* cpprest models now use inheritance properly instead of always extending from ModelBase

* cpprest models now use inheritance properly instead of always extending from ModelBase

* removed a sysout used for debugging

* toJson() and fromJson() now leverages parent class's corresponding methods

* virtual is not needed as override essentially does the same thing.

* added docstring for getModelByName

* corrected the javadoc

* fixed @param issue in javadoc

* fixed @param uncapitalized P in param in javadoc
@wing328
Copy link
Contributor

wing328 commented Jun 19, 2017

@solvingj I've merged @stkrwork PR into master. Can you please pull the latest docker image to give it another try and list out what still needs to be fixed?

@solvingj
Copy link
Author

solvingj commented Jun 19, 2017

@stkrwork already tested cpprest for us, check my comments above, looks like still broken are:
toMultipart, toJson, and fromJson

@wing328
Copy link
Contributor

wing328 commented Jun 19, 2017

Can you share a bit more details? Did you get compilation errors? If yes, please provide them. The steps you provided previously are very helpful

1. Generate the code from the provided swagger spec using the web editor.
2. Open file model\Resource.cpp
3. Navigate to line 166 and find :
setTags( newItem );

@solvingj
Copy link
Author

FYI, I have been working on this for the past hour. Will continue.

@solvingj
Copy link
Author

Here are some examples of the errors which still exist

azure-iothub-sdk-cpprest\model\EventHubConsumerGroupInfo.cpp(46): error C2664: 'web::json::value io::swagger::client::model::ModelBase::toJson(bool)': cannot convert argument 1 from 'const std::map<utility::string_t,utility::string_t,std::less<_Kty>,std::allocator<std::pair<const _Kty,_Ty>>>' to 'const utility::string_t &'

azure-iothub-sdk-cpprest\model\EventHubConsumerGroupInfo.cpp(67): error C2227: left of '->fromJson' must point to class/struct/union/generic type

azure-iothub-sdk-cpprest\model\EventHubConsumerGroupInfo.cpp(93): error C2819: type 'std::map<utility::string_t,utility::string_t,std::less<_Kty>,std::allocator<std::pair<const _Kty,_Ty>>>' does not have an overloaded member 'operator ->'

azure-iothub-sdk-cpprest\model\Resource.cpp(122): error C2039: 'toMultipart': is not a member of
'std::map<utility::string_t,utility::string_t,std::less<_Kty>,std::allocator<std::pair<const _Kty,_Ty>>>'

@stkrwork
Copy link
Contributor

I might look into this today

@stkrwork
Copy link
Contributor

stkrwork commented Jun 20, 2017

The template is only recognizing the isListContainer and isPrimitiveType when generating, and a default generation template. As tags in this case is a map, the isMapContainer needs to be checked. I am implementing that change currently, which means it should work then. The other complexTypes are handled ok with the default, I think.

The petstore example models don't have a map datatype, so therefore this error could never be seen in the samples.

The question is, how do you want to represent the map in the json? In form of this?
[ { "key":"testkey", "value":"testval" } ]

Or would another format be better?

@wing328
Copy link
Contributor

wing328 commented Jun 20, 2017

The petstore example models don't have a map datatype, so therefore this error could never be seen in the samples.

Right, the cpprest petstore samples (based on petstore.yaml) only covers the basic. We've created petstore-with-fake-endpoints-models-for-testing.yaml to cover a lot more edge cases such as map datatype and map of map as well.

@stkrwork
Copy link
Contributor

For a first step, I am implementing a way for using maps and not maps of maps, as maps of maps will require refactoring of the modelbase as well, as everything needs to be moved into new functions for lists and maps.

@wing328
Copy link
Contributor

wing328 commented Jun 20, 2017

For a first step, I am implementing a way for using maps and not maps of maps, as maps of maps will require refactoring of the modelbase as well, as everything needs to be moved into new functions for lists and maps.

Yup, that's totally fine (I'm not saying you need to fix all the edge cases covered by petstore-with-fake-endpoints-models-for-testing.yaml)

@stkrwork
Copy link
Contributor

Can you look through the template @wing328, because according to my changes, it should generate the map components correctly, but when using it with the spec provided in this issue, it still generates it the wrong way, so I am a little confused.

@stkrwork
Copy link
Contributor

#5884

@wing328
Copy link
Contributor

wing328 commented Jun 20, 2017

UPDATE: #5884 has been merged into master.

@solvingj can you please give it a try when you've time?

@solvingj
Copy link
Author

solvingj commented Jun 20, 2017

Gave it a try, now up to 82 errors currently.

@stkrwork do you get the same number of errors?

Here's what mine generated, maybe @stkrwork can double check that I did get the latest changes from #5884.

https:/solvingJ/azure-iothub-sdk-cpprest/tree/Swagger_codegen_2_2_3-snapshot_6_20

Steps to reproduce on windows 10:

conan install
mkdir build
cd build
cmake -G "Visual Studio 15 2017 Win64" ..
msbuild ALL_BUILD.vcxproj /p:Configuration=Release

I've put my build log in the following file which includes every source file that has an error, and tells what line number it is. Sorry, error logs are kinda hard to read:

https:/solvingJ/azure-iothub-sdk-cpprest/blob/Swagger_codegen_2_2_3-snapshot_6_20/MSBuild%20Log%206-20-17.txt

@stkrwork
Copy link
Contributor

I'll review it tomorrow.

@stkrwork
Copy link
Contributor

It now generates and compiles your spec correctly. I used Visual Studio 17 for it. I did not use conan, but it should not make a major difference.

There were some issues with Syntax and not escaping some signs correctly. but that should be fixed now.

@stkrwork
Copy link
Contributor

#5893 is pr

@solvingj
Copy link
Author

@stkrwork Can't wait to check this when i get into the office, thank you so much! @wing328 do you think you'll be able to push to docker any time soon?

wing328 pushed a commit that referenced this issue Jun 22, 2017
* - Added Restbed Generator

* - Added Json processing functions to model
- Removed unnused code from restbed codegen class
- Added response header processing to api template

* Changed it to respect alphabetical order

* Made the string joining java 7 compatible

* Added samples

* First step in fixing the cpp rest template
regenerated new samples

TODO: Fix the other functions

* Updated samples

* Added isMapContainer check

* Fixed item selection in json object for MapContainer

* - Fixed Syntax error in C++
- Fixed Syntax error in Mustache, that escaped characters
@wing328
Copy link
Contributor

wing328 commented Jun 22, 2017

#5893 merged into master. Please give it a try.

@solvingj
Copy link
Author

@wing328 FYI I'm trying to get setup to build this project from source today. I'm using windows/docker for windows and a novice with docker, so i'm just working through adjusting the scripts to work on my machine.

@wing328
Copy link
Contributor

wing328 commented Jun 22, 2017

If you're using Windows, you can also build the project locally by installing Maven (mvn command)

@solvingj
Copy link
Author

@wing328
Was already half way down that road. I think I just got it working.

@solvingj
Copy link
Author

@stkrwork
Good news, down to 4 errors, and they might be my problems.

Build FAILED.

"C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\build\ALL_BUILD.vcxproj" (default target) (1) ->
"C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\build\CppRestSwaggerClient.vcxproj" (default target) (3) ->
(ClCompile target) ->
  C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\ModelBase.cpp(259): error C2440: '<function-style-cast>': cannot conver
t from 'const wchar_t [28]' to 'web::json::json_exception' [C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\build\CppRest
SwaggerClient.vcxproj]
  C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\ModelBase.cpp(264): error C2440: '<function-style-cast>': cannot conver
t from 'const wchar_t [32]' to 'web::json::json_exception' [C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\build\CppRest
SwaggerClient.vcxproj]
  C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\ModelBase.cpp(292): error C2446: ':': no conversion from 'const wchar_t
 *' to 'const std::string' [C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\build\CppRestSwaggerClient.vcxproj]
  C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\ModelBase.cpp(297): error C2664: 'utility::datetime utility::datetime::
from_string(const utility::string_t &,utility::datetime::date_format)': cannot convert argument 1 from 'const std::stri
ng' to 'const utility::string_t &' [C:\Users\jerry\git-c\azure-iothub-sdk-cpprest\build\CppRestSwaggerClient.vcxproj]

    0 Warning(s)
    4 Error(s)

Time Elapsed 00:01:58.96

@stkrwork
Copy link
Contributor

I let it run through again, and it looks fine to me. there seems to be no build errors when i compile it.

@solvingj
Copy link
Author

Is there any difference with my ModelBase.cpp (visible here);
https:/solvingJ/azure-iothub-sdk-cpprest/tree/6-22-17_strkwork_says_working

If you look at my errors, do you have any idea what would cause them?

ModelBase.cpp(297)

'utility::datetime utility::datetime::from_string(const utility::string_t &,utility::datetime::date_format)': 
cannot convert argument 1 
from 'const std::string' 
to 'const utility::string_t &'

@solvingj
Copy link
Author

@stkrwork
Copy link
Contributor

utility::string_t is nothing else but a std::string.

I ran your code from your repo as well, and there were no errors for me when i used it with Visual Studio 15. I included cpprestsdk and boost with the NuGet package manager in this case.

@stkrwork
Copy link
Contributor

What do you use when building it?

@solvingj
Copy link
Author

Msbuild "Visual Studio 15 2017 Win64"

@stkrwork
Copy link
Contributor

stkrwork commented Jun 22, 2017

Ok. Thats quite similar to my setup right now.

I used the versions from your conan file as well for cpprestsdk and boost, and it still compiled fine for me. I again used the Nuget Package Manager.

@solvingj
Copy link
Author

I'm on Boost 1.5.9. I'll try upgrading when I get home.

I don't know if any of the following is relevant, but here's what I found:

The following weird statements print in the msbuild log, but don't seem to be errors. I googled, which indicated it might be related to boost.

  MultipartFormData.cpp
  Unknown compiler version - please run the configure tests and report the results
  DELETEApi.cpp
  Unknown compiler version - please run the configure tests and report the results
  GETApi.cpp
  Unknown compiler version - please run the configure tests and report the results
  POSTApi.cpp
  Unknown compiler version - please run the configure tests and report the results
  PUTApi.cpp
  Unknown compiler version - please run the configure tests and report the results

The two main issues are lines 292 and 297, which I can easily fix by replacing utival.as_string() with utility::conversions::to_string_t(val.as_string()).

The other two errors are about throw statements, which I don't know how to fix because I don't understand this "U()" thing. However, I can easily comment those out for now.
throw web::json::json_exception( U( "Invalid Padding in Base 64!" ) );

I also have a warning:

ModelBase.cpp(172): warning C4244: 'initializing': conversion from 'std::streamoff' to '::size_t', possible loss of data

Regarding your comment about strings, I wanted to clarify something. The stack overflow post had the following exchange:

If you see the documentation for C++ REST SDK from github, you'll find a typedef
C++ Rest SDK - utility Namespace Reference
typedef std::string string_t;
So no need to convert it. Both are same types

That seems similar to your comment, and someone replied:

This only applies on non-Windows. On Windows, this typedef looks like typedef std::wstring string_t;

If it was something like this, it doesn't really explain why it works for you with cmake -G "Visual Studio 14 2015 Win64". Also, I don't know why, but when I run that cmake command, it says:

cmake -G "Visual Studio 14 2015 Win64" ..
-- The C compiler identification is unknown
-- The CXX compiler identification is unknown
CMake Error at CMakeLists.txt:12 (project):
  No CMAKE_C_COMPILER could be found.

@solvingj
Copy link
Author

So, found I had uninstalled VS 2015.. guess that was obvious. I re-installed, and sure enough the library builds fine under 2015. Thank you so much!

I'm curious what this means for support on VS2017. Should the template be modified so that it works under 2017?

@stkrwork
Copy link
Contributor

i built it with visual studio 2017 as well, directly in the IDE, and it built fine there as well.

@solvingj
Copy link
Author

Must have to do with the versions of CPPRest and/or Conan I'm using. Thanks for helping me figure this out, and fixing the code.

@stkrwork
Copy link
Contributor

No problem

@wing328
Copy link
Contributor

wing328 commented Jun 26, 2017

@solvingj @stkrwork when you've time I wonder if you guys can share the steps for installation and usage of the cpprest API client. We want to put that into an auto-generated README (similar to what we've done for other languages)

@solvingj
Copy link
Author

solvingj commented Jun 26, 2017

Adding this to my todo list. FYI, we've still not been able to test with the latest CPPRest (It's surprisingly hard to build). Will soon though.

@wing328
Copy link
Contributor

wing328 commented Jun 28, 2017

@solvingj thanks. I'll close this issue, which has been resolved, and open a new one for tracking instead.

@wing328 wing328 closed this as completed Jun 28, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants