feat: Add DIREGAPIC-specific pagination #767

vam-google · 2021-06-14T08:51:19Z

No description provided.

…or from dumped file. For debugging puproses.

codecov · 2021-06-14T09:01:21Z

Codecov Report

Merging #767 (7fd8258) into master (bc6eb85) will decrease coverage by 0.46%.
The diff coverage is 48.02%.

@@            Coverage Diff             @@
##           master     #767      +/-   ##
==========================================
- Coverage   91.71%   91.25%   -0.47%     
==========================================
  Files         140      140              
  Lines       14795    14884      +89     
  Branches     1052     1061       +9     
==========================================
+ Hits        13569    13582      +13     
- Misses        942     1011      +69     
- Partials      284      291       +7

Impacted Files	Coverage Δ
.../com/google/api/generator/gapic/model/Message.java	`62.16% <18.18%> (-18.61%)`	⬇️
...ic/composer/defaultvalue/DefaultValueComposer.java	`88.88% <42.85%> (-5.04%)`	⬇️
...common/AbstractServiceClientTestClassComposer.java	`84.26% <46.51%> (-3.75%)`	⬇️
...mmon/AbstractServiceStubSettingsClassComposer.java	`97.16% <50.00%> (-1.90%)`	⬇️
...google/api/generator/gapic/protoparser/Parser.java	`43.71% <60.00%> (+0.50%)`	⬆️
...ic/composer/common/ServiceClientClassComposer.java	`98.45% <100.00%> (ø)`
...er/samplecode/ServiceClientSampleCodeComposer.java	`99.28% <100.00%> (ø)`
...a/com/google/api/generator/gapic/model/Method.java	`91.66% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bc6eb85...7fd8258. Read the comment docs.

miraleung

Could we base this PR on #765 if possible?

BUILD.bazel

src/main/java/com/google/api/generator/Main.java

src/main/java/com/google/api/generator/debug/CodeGeneratorRequestDumper.java

src/main/java/com/google/api/generator/debug/MainFromFile.java

src/main/java/com/google/api/generator/debug/CodeGeneratorRequestDumper.java

src/main/java/com/google/api/generator/gapic/composer/defaultvalue/DefaultValueComposer.java

src/main/java/com/google/api/generator/gapic/model/Message.java

src/main/java/com/google/api/generator/gapic/model/Method.java

...com/google/api/generator/gapic/composer/common/AbstractServiceStubSettingsClassComposer.java

miraleung · 2021-06-14T18:47:13Z

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

+      List<String> fieldNames = new ArrayList<>();
+      fieldNames.add("page_size");
+      if (transport == Transport.REST) {
+        fieldNames.add("max_results");


Do we need to do something to check that either max_results xor page_size are present, and not both?

No. This logic explicitly prefers page_size over max_results if both are present, for safer backward compatibility (i.e. to not let DIREGAPIC-speficif logic mess with normal GAPICS, if somebody for whatever reason decides to add max_results field into a regular paginated gapic method.

SGTM, could we add a one-line comment to that effect, so that this behavior is clear to future readers who may be unfamiliar with this codebase?

vam-google · 2021-06-14T23:36:19Z

@miraleung There was a merge commit with conflicts in between, so rebasing was painful. I just cleaned up the other pr form this one manually, since it was easier (just 6 files with straightforward changes). I also migrated the relevant comments to the #765, please continue there.

src/main/java/com/google/api/generator/gapic/model/Method.java

miraleung · 2021-06-15T18:45:27Z

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

-    return inputMessage.fieldMap().containsKey("page_size")
-        && inputMessage.fieldMap().containsKey("page_token")
-        && outputMessage.fieldMap().containsKey("next_page_token");
+    String pagedFieldName = null;


As per the comment thread above, can we remove this null-assignment?

This is a legit null assignment, as there is no else block. I.e. if there are not signs of field being paginated, null is a legit value, representing non-paginated case.

miraleung · 2021-06-15T18:46:17Z

src/main/java/com/google/api/generator/gapic/protoparser/Parser.java

+      List<String> fieldNames = new ArrayList<>();
+      fieldNames.add("page_size");
+      if (transport == Transport.REST) {
+        fieldNames.add("max_results");


SGTM, could we add a one-line comment to that effect, so that this behavior is clear to future readers who may be unfamiliar with this codebase?

vam-google · 2021-06-16T07:44:52Z

@miraleung PTAL

🤖 I have created a release *beep* *boop* --- ## [3.0.1](googleapis/java-shared-dependencies@v3.0.0...v3.0.1) (2022-08-02) ### Dependencies * update dependency com.google.code.gson:gson to v2.9.1 ([#766](googleapis/java-shared-dependencies#766)) ([b0d531d](googleapis/java-shared-dependencies@b0d531d)) * update gax.version to v2.18.7 ([#767](googleapis/java-shared-dependencies#767)) ([02e98d5](googleapis/java-shared-dependencies@02e98d5)) * update google.core.version to v2.8.6 ([#770](googleapis/java-shared-dependencies#770)) ([3acea4b](googleapis/java-shared-dependencies@3acea4b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).

[![WhiteSource Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Change | Age | Adoption | Passing | Confidence | |---|---|---|---|---|---| | [org.threeten:threetenbp](https://www.threeten.org/threetenbp) ([source](https://github.com/ThreeTen/threetenbp)) | `1.5.2` -> `1.6.0` | [![age](https://badges.renovateapi.com/packages/maven/org.threeten:threetenbp/1.6.0/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/org.threeten:threetenbp/1.6.0/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/org.threeten:threetenbp/1.6.0/compatibility-slim/1.5.2)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/org.threeten:threetenbp/1.6.0/confidence-slim/1.5.2)](https://docs.renovatebot.com/merge-confidence/) | --- ### Release Notes <details> <summary>ThreeTen/threetenbp</summary> ### [`v1.6.0`](https://github.com/ThreeTen/threetenbp/releases/v1.6.0) [Compare Source](https://github.com/ThreeTen/threetenbp/compare/v1.5.2...v1.6.0) See the [change notes](https://www.threeten.org/threetenbp/changes-report.html) for more information. </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [WhiteSource Renovate](https://renovate.whitesourcesoftware.com). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-core).

vam-google added 5 commits June 1, 2021 23:16

feat: implement DIREGAPIC

8983e23

Merge branch 'diregapic'

83e004f

feat: Add code request generator dumper and an ability to run generat…

a978fff

…or from dumped file. For debugging puproses.

reformat code

310737e

feat: Add DIREGAPIC-specific pagination

58685cd

vam-google requested review from miraleung and a team as code owners June 14, 2021 08:51

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Jun 14, 2021

miraleung suggested changes Jun 14, 2021

View reviewed changes

vam-google mentioned this pull request Jun 14, 2021

feat: Add code request generator dumper and an ability to run generator from dumped file #765

Merged

remove the googleapis#765 parts

65b88e2

address PR feedback

0cb8593

vam-google requested a review from miraleung June 15, 2021 01:14

miraleung suggested changes Jun 15, 2021

View reviewed changes

vam-google added 2 commits June 15, 2021 23:26

Merge remote-tracking branch 'upstream/master' into diregapic_pagination

4893747

address PR feedback.

6d33ad1

vam-google requested a review from miraleung June 16, 2021 07:44

miraleung approved these changes Jun 16, 2021

View reviewed changes

Merge remote-tracking branch 'upstream/master' into diregapic_pagination

7fd8258

vam-google merged commit 1294c29 into googleapis:master Jun 17, 2021

suztomo pushed a commit that referenced this pull request Mar 21, 2023

deps: update gax.version to v2.18.7 (#767)

02e98d5

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add DIREGAPIC-specific pagination #767

feat: Add DIREGAPIC-specific pagination #767

vam-google commented Jun 14, 2021

codecov bot commented Jun 14, 2021 •

edited

Loading

miraleung left a comment •

edited by vam-google

Loading

miraleung Jun 14, 2021

vam-google Jun 15, 2021

miraleung Jun 15, 2021

vam-google Jun 16, 2021

vam-google commented Jun 14, 2021 •

edited

Loading

miraleung Jun 15, 2021

vam-google Jun 16, 2021 •

edited

Loading

miraleung Jun 15, 2021

vam-google commented Jun 16, 2021

feat: Add DIREGAPIC-specific pagination #767

feat: Add DIREGAPIC-specific pagination #767

Conversation

vam-google commented Jun 14, 2021

codecov bot commented Jun 14, 2021 • edited Loading

Codecov Report

miraleung left a comment • edited by vam-google Loading

Choose a reason for hiding this comment

miraleung Jun 14, 2021

Choose a reason for hiding this comment

vam-google Jun 15, 2021

Choose a reason for hiding this comment

miraleung Jun 15, 2021

Choose a reason for hiding this comment

vam-google Jun 16, 2021

Choose a reason for hiding this comment

vam-google commented Jun 14, 2021 • edited Loading

miraleung Jun 15, 2021

Choose a reason for hiding this comment

vam-google Jun 16, 2021 • edited Loading

Choose a reason for hiding this comment

miraleung Jun 15, 2021

Choose a reason for hiding this comment

vam-google commented Jun 16, 2021

codecov bot commented Jun 14, 2021 •

edited

Loading

miraleung left a comment •

edited by vam-google

Loading

vam-google commented Jun 14, 2021 •

edited

Loading

vam-google Jun 16, 2021 •

edited

Loading