-
Notifications
You must be signed in to change notification settings - Fork 6k
[Java] Templates to support google-api-client library #6838
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
[Java] Templates to support google-api-client library #6838
Conversation
...ing API instances
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick check. Look goods to me. I just have some points that needs clarification. Thanks for the PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this should go in the GoogleApiClientCodegen.java since it's only useful for your generator.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I'll do that right now, sounds like a plan.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI this is following the existing pattern. There actually aren't any subclasses of JavaClientCodegen; it uses if-statements on the library throughout to determine the configuration of each library. I'd have to figure out how to support a new subclass, it looks like it uses reflection to instantiate the JavaClientCodegen with the classes listed in modules/swagger-codegen/src/main/resources/META-INF/services/io.swagger.codegen.CodegenConfig
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh I see other library has done it too... @wing328, is this normal? Since we are using objects, we should'nt have any "if" in the JavaClientCodegen... It should all be in specific file of each generator...
Or there is a good reason for this ??
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it appears there are no subclasses of JavaClientCodegen, see above comment, @wing328 probably has a more complete explanation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. It's weird because for example, for the server generator "java play framework" which I maintain, there is:
JavaPlayFrameworkCodegen -> AbstractJavaCodegen -> DefaultCodegen.
This prevent having "if" everywhere.
Same thing for Typescript jQuery which I maintain too:
TypeScriptJqueryClientCodegen -> AbstractTypeScriptClientCodegen -> DefaultCodegen
I wonder why the JavaClientCodegen are all mixed in one file.
For the moment, don't change anything. We will see with @wing328 answers :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Yeah, a refactor into subclasses probably belongs in a separate pull request, but I'm game to do whatever you suggest.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is ok for now and belong in another pull request like you said. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we added support for other HTTP libraries, we didn't foresee we can support so many libraries (9 HTTP libraries support so far thanks to the awesome community). I agree a separate PR to refactor into subclasses would be ideal.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense. Is this waiting on any additional reviews? Let me know if any more changes are needed here. Thanks! This will really help out our company in our migration to use Swagger clients for all of our service-to-service communication. (And possibly we could use this for our Android app, too).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@charlescapps I'll take another look later today and merge accordingly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to put CodegenParameter in the ArrayList to be able to compile swagger-codegen in Java < 1.7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do right now! Yes, makes sense to support 1.6.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm surprised this is needed. Most generator use the params array and loop with "isRequired" to build almost everything. Is there any specific reason to add this to the "generic code"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you can check {{isRequired}} but that's not sufficient, because we use {{hasNext}} to see if there's a subsequent element, and that isn't aware of isRequired, so it was generating code with 2 commas in a row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok makes sense!
@charlescapps thanks for the PR, which looks good to me and my test results are good.
cc @lwander
UPDATE: Added the new Java API client to CircleCI via cf5eba8
Thanks for being so responsive. I'm excited to use this new client, now we won't need to build from a fork.
@charlescapps FYI. I've filed #6885 to skip the check for body parameter, which can be optional
Ref: #6878
@charlescapps looks like #6885 introduces an issue. In the latest master, I ran ./bin/java-petstore-google-api-client.sh to update the Java (google-api-client) Petstore sample and notice some changes. Here is the diff for one of them:
return apiClient.getHttpRequestFactory().buildRequest(HttpMethods.POST, genericUrl, content).execute();
}
- public HttpResponse fakeOuterBooleanSerializeForHttpResponse(Boolean body, Map<String, Object> params) throws IOException {
+ public HttpResponse fakeOuterBooleanSerializeForHttpResponse(Map<String, Object> params) throws IOException {
Object postBody = body;
UriBuilder uriBuilder = UriBuilder.fromUri(apiClient.getBasePath() + "/fake/outer/boolean");```
May I know if you've time to take a look?
Uh oh!
There was an error while loading. Please reload this page.
PR checklist
./bin/to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.shand./bin/security/{LANG}-petstore.shif 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\.3.0.0branch for changes related to OpenAPI spec 3.0. Default:master.@bbdouglas (2017/07) @JFCote (2017/08) @sreeshas (2017/08) @jfiala (2017/08) @lukoyanov (2017/09) @cbornet (2017/09)
Description of the PR
See issue #6825
This PR adds support for generating client code for the Google APIs Client for Java. This HTTP client is not just for Google's APIs, as you can read in the description of their page:
Motivation
The
google-api-clientis really standard in GCP for making HTTP requests. It works great with authentication in GCP (or outside GCP), by automatically handling refresh tokens with the googleCredentialclass. Furthermore, this provides a clear upgrade path for anyone on Google Cloud Endpoints who wants to migrate to a better REST framework, since Cloud Endpoints uses thegoogle-api-client.Changes in this PR
google-api-clientmodules/swagger-codegen/src/main/resources/Java/libraries/google-api-clientDefaultCodegento have a list of required/body params, so the client can generate overloaded methods that use aMap<String, Object>for query params.google-api-client, and verified they compile, etc.Features of the generated Google ApiClient
apiClient.myFooApi().doSomeRequest(params...)Map<String, Object>, and methods ending inforHttpResponsethat return the rawHttpResponserather than deserializing as a POJO.basePath,HttpTransport,HttpRequestInitializer(used for auth), andObjectMapper.ObjectMapperInstead of using the problematic Google-custom-Jackson (e.g.com.google.api.client.json.jackson2.JacksonFactory)authpackage, because I'm not sure what that would gain us; it's really straightforward to use anHttpRequestInitializerto provide and access token, e.g. you can usecom.google.api.client.auth.oauth2.Credentialand it can also do token refreshes automatically (or just use an access token if that's all you have).cc: @drd