[Java][okhttp-gson] Add option to employ builders for API requests #1341

Kiran-Sivakumar · 2018-10-30T01:41:03Z

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.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.

@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger

Description of the PR

fix #1217
This PR (filed against branch 3.4.x) adds the option to generate APIs of the form shown in the proposed solution in the above issue. This makes generated okhttp-gson clients much nicer to use, especially in cases involving unused optional parameters. The hope is to implement a similar solution for all Java generators in the future, so that generated Java clients aren't frustrating to use.

Changes:

JavaClientCodegen.java modified to add a new CLI option "useBuildersForApiRequests" (default: false)
okhttp-gson/api.mustache
- Optionally generate API-request builders
- Change original methods from "public" to "private" if builders are generated, since user will want to use the public builder methods instead (the builder methods simply wrap around the original methods that take all parameters as input)
- Generate comments
okhttp-gson/api_test.mustache added to generate tests that use the builders if the "useBuildersForApiRequests" option is enabled
bin/java-petstore-okhttp-gson-builders.sh added to generate petstore samples with API-request builders (added one for Windows as well)
samples/client/petstore/java/okhttp-gson-requestBuilders/: new petstore samples generated with the above script
docs/generators/java.md updated with new config option

I also ran the original shell scripts under ./bin/ to ensure that absolutely nothing changes in the existing petstore samples when the "useBuildersForApiRequests" option is disabled.

cbornet · 2018-10-30T06:50:06Z

docs/generators/java.md

@@ -146,6 +146,9 @@ CONFIG OPTIONS for java
 	feignVersion
 	    Version of OpenFeign: '10.x', '9.x' (default) (Default: false)

+	useBuildersForApiRequests
+	    Generate API that does not require specifying unused optional parameters when making requests with the okhttp-gson library (Default: false)


I would set it to true by default

cbornet · 2018-10-30T10:04:15Z

...es/openapi-generator/src/main/java/org/openapitools/codegen/languages/JavaClientCodegen.java

@@ -63,6 +63,7 @@
    public static final String FEIGN_VERSION = "feignVersion";
    public static final String PARCELABLE_MODEL = "parcelableModel";
    public static final String USE_RUNTIME_EXCEPTION = "useRuntimeException";
+    public static final String USE_BUILDERS_FOR_API_REQUESTS = "useBuildersForApiRequests";


Too many options 😢

cbornet · 2018-10-30T10:13:28Z

As always, I'm reluctant to have too many options. So I would prefer to always generate the builder, keep the constructors public and have a way to encourage users to move to the builder style (eg. setting the constructor @Deprecated and adding a comment that future version will set it as private. Then in a minor release or 2, we can completely switch to private.

Kiran-Sivakumar · 2018-10-30T16:01:02Z

@cbornet Thanks for your feedback. I agree that too many options can be undesirable. Perhaps the option can be removed in the next major release (to only generate the builder style)? Otherwise, I can incorporate your proposed changes. This would require the public builder method name to be changed, however, to not conflict with one of the current method names.

wing328 · 2018-10-30T16:12:08Z

@Kiran-Sivakumar @cbornet FYI. next major version 4.x (with breaking changes) will be released before year-end so we can clean up the options as part of the 4.x release later.

Kiran-Sivakumar · 2018-10-31T05:39:31Z

Ok, so should I keep it as an option for this PR, changing it to be true by default, and file another PR against 4.x in a few weeks that removes the option entirely?

wing328 · 2018-10-31T07:29:57Z

@Kiran-Sivakumar I've other ideas. Let me review and get back to you by Thursday.

wing328 · 2018-11-01T09:40:51Z

@Kiran-Sivakumar instead of using useBuildersForApiRequests in the templates, what about using x-group-parameters vendor extensions like what I did for PHP in https://github.com/OpenAPITools/openapi-generator/pull/1337/files#diff-076573ccfdac2ab5af4dc1813e5ccee3R92?

This will avoid another option and make it possible to enable group parameters per operation.

Kiran-Sivakumar · 2018-11-01T23:31:26Z

I see. What if you wanted to enable builders for all operations for a cohesive API? Is there an easy way to apply group parameters to all of them?

wing328 · 2018-11-02T02:49:49Z

@Kiran-Sivakumar similar to what we did for consumes, produces (global vs local), if x-group-parameters is defined in the top level of the spec, we will add x-group-parameters to all operations in postProcessOperationsAndModel (only if x-group-parameters is not defined in the operation level).

Does it sound good to you?

Kiran-Sivakumar · 2018-11-02T17:37:18Z

Oh great. Yes, this sounds good to me. I'll incorporate your feedback.

wing328 · 2018-11-09T02:56:40Z

@Kiran-Sivakumar I've filed #1405 to add top level x-group-parameters support.

Kiran-Sivakumar · 2018-11-14T22:30:08Z

@cbornet @wing328 I have incorporated your feedback.

wing328 · 2018-12-04T10:21:53Z

@Kiran-Sivakumar thanks for the PR, which has been included in the v3.3.4 release: https://twitter.com/oas_generator/status/1068772409795207168

…penAPITools#1341) * Generate APIs that use the builder pattern * Add option to use builders for API requests * Fix template spacing * Add new sample-generation script and generated samples * Update docs * Add new sample-generation script for Windows * Replace config option with vendor extension * Remove useBuildersForApiRequests config option * Remove builders sample-generation scripts * Replace config option with vendor extension in api_test template * Remove okhttp-gson-requestBuilders sample * Rename x-builders-for-api-requests to x-group-parameters * Add modified api_doc.mustache under okhttp-gson resources * Add modified README.mustache under okhttp-gson resources * Update petstore samples * Fix FakeApiTest.java in petstore samples * Add whitespace to rerun checks * Remove whitespace to rerun tests * Fix FakeApiTest.java in parcelable petstore sample * Update versions in samples * Update versions in samples

Kiran-Sivakumar changed the title ~~[Java][okhttp-gson] Add option to employ request builder for much nicer APIs~~ [Java][okhttp-gson] Add option to employ builders for API requests Oct 30, 2018

cbornet reviewed Oct 30, 2018

View reviewed changes

wing328 added Client: Java Enhancement: Feature labels Nov 2, 2018

wing328 added this to the 3.3.3 milestone Nov 2, 2018

Kiran-Sivakumar force-pushed the api-request-builders branch from fe4d53a to 43e6c70 Compare November 9, 2018 00:45

Kiran-Sivakumar changed the base branch from 3.4.x to master November 9, 2018 19:29

Kiran-Sivakumar changed the base branch from master to 3.4.x November 9, 2018 19:30

Kiran-Sivakumar added 11 commits November 9, 2018 12:02

Generate APIs that use the builder pattern

cb6215b

Add option to use builders for API requests

f1d012c

Fix template spacing

13791a1

Add new sample-generation script and generated samples

2faea90

Update docs

dcb9d34

Add new sample-generation script for Windows

b907067

Replace config option with vendor extension

f8bfe66

Remove useBuildersForApiRequests config option

51cd5fd

Remove builders sample-generation scripts

a141220

Replace config option with vendor extension in api_test template

b88f74e

Remove okhttp-gson-requestBuilders sample

f6b3f68

Kiran-Sivakumar force-pushed the api-request-builders branch from 43e6c70 to f6b3f68 Compare November 9, 2018 20:02

Kiran-Sivakumar changed the base branch from 3.4.x to master November 9, 2018 20:03

wing328 modified the milestones: 3.3.3, 3.3.4 Nov 14, 2018

Kiran-Sivakumar added 5 commits November 14, 2018 13:25

Rename x-builders-for-api-requests to x-group-parameters

ceca104

Add modified api_doc.mustache under okhttp-gson resources

f9c17a7

Add modified README.mustache under okhttp-gson resources

4602ab0

Merge remote-tracking branch 'upstream/master' into api-request-builders

eacca0b

Update petstore samples

d5e9f01

Kiran-Sivakumar mentioned this pull request Nov 15, 2018

Feature: [TypeScript] "CollectParameters" to accept params as option-bag #865

Open

Kiran-Sivakumar and others added 7 commits November 14, 2018 16:38

Fix FakeApiTest.java in petstore samples

67999e9

Add whitespace to rerun checks

3ef11fd

Remove whitespace to rerun tests

e06c9b1

Merge branch 'master' into api-request-builders

36b5109

Fix FakeApiTest.java in parcelable petstore sample

bd8b08d

Update versions in samples

e9f6698

Update versions in samples

f01e9da

wing328 merged commit 9df7079 into OpenAPITools:master Nov 28, 2018

This was referenced Nov 29, 2019

Use builder pattern for requests DataDog/datadog-api-client-java#45

Merged

[Java][jersey2] Use builder pattern for requests #4666

Merged

arielshaqed mentioned this pull request Nov 15, 2021

Update lakeFS API SDK clients so applications can upgrade their client version treeverse/lakeFS#2704

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Java][okhttp-gson] Add option to employ builders for API requests #1341

[Java][okhttp-gson] Add option to employ builders for API requests #1341

Kiran-Sivakumar commented Oct 30, 2018 •

edited

Loading

cbornet Oct 30, 2018

cbornet Oct 30, 2018 •

edited

Loading

cbornet commented Oct 30, 2018

Kiran-Sivakumar commented Oct 30, 2018 •

edited

Loading

wing328 commented Oct 30, 2018

Kiran-Sivakumar commented Oct 31, 2018

wing328 commented Oct 31, 2018

wing328 commented Nov 1, 2018

Kiran-Sivakumar commented Nov 1, 2018 •

edited

Loading

wing328 commented Nov 2, 2018

Kiran-Sivakumar commented Nov 2, 2018

wing328 commented Nov 9, 2018

Kiran-Sivakumar commented Nov 14, 2018

wing328 commented Dec 4, 2018

[Java][okhttp-gson] Add option to employ builders for API requests #1341

[Java][okhttp-gson] Add option to employ builders for API requests #1341

Conversation

Kiran-Sivakumar commented Oct 30, 2018 • edited Loading

PR checklist

Description of the PR

cbornet Oct 30, 2018

Choose a reason for hiding this comment

cbornet Oct 30, 2018 • edited Loading

Choose a reason for hiding this comment

cbornet commented Oct 30, 2018

Kiran-Sivakumar commented Oct 30, 2018 • edited Loading

wing328 commented Oct 30, 2018

Kiran-Sivakumar commented Oct 31, 2018

wing328 commented Oct 31, 2018

wing328 commented Nov 1, 2018

Kiran-Sivakumar commented Nov 1, 2018 • edited Loading

wing328 commented Nov 2, 2018

Kiran-Sivakumar commented Nov 2, 2018

wing328 commented Nov 9, 2018

Kiran-Sivakumar commented Nov 14, 2018

wing328 commented Dec 4, 2018

Kiran-Sivakumar commented Oct 30, 2018 •

edited

Loading

cbornet Oct 30, 2018 •

edited

Loading

Kiran-Sivakumar commented Oct 30, 2018 •

edited

Loading

Kiran-Sivakumar commented Nov 1, 2018 •

edited

Loading