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

fix(resnames): ensure determinstic code generation #865

Merged
merged 1 commit into from
Oct 28, 2021

Conversation

chanseokoh
Copy link
Contributor

@chanseokoh chanseokoh commented Oct 27, 2021

Fixes #847.

Certainly, there are other alternative approaches to fix this issue in other places, but I think this is reasonable. GapicContext will consistently return resource names in a deterministic way, no matter what the input is.

This also reverts the fix in #813 that resolved the same issue for sample code generation, which is made obsolete by this PR. Note, the previously updated golden files in #813 are not reverted, proving that this PR is enforcing the same order without #813.

@chanseokoh chanseokoh requested review from miraleung and a team as code owners October 27, 2021 20:39
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label Oct 27, 2021
@codecov
Copy link

codecov bot commented Oct 27, 2021

Codecov Report

Merging #865 (cf22ae6) into main (23e4a36) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #865      +/-   ##
==========================================
- Coverage   87.60%   87.59%   -0.01%     
==========================================
  Files         152      152              
  Lines       15988    15982       -6     
  Branches     1162     1162              
==========================================
- Hits        14006    14000       -6     
  Misses       1641     1641              
  Partials      341      341              
Impacted Files Coverage Δ
...er/samplecode/ServiceClientSampleCodeComposer.java 99.28% <ø> (-0.01%) ⬇️
...rator/gapic/protoparser/MethodSignatureParser.java 94.73% <ø> (ø)
...google/api/generator/gapic/model/GapicContext.java 95.23% <100.00%> (+0.50%) ⬆️

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 23e4a36...cf22ae6. Read the comment docs.

Copy link
Member

@meltsufin meltsufin left a comment

Choose a reason for hiding this comment

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

LGTM, but see one minor comment.

abstract GapicContext autoBuild();

public GapicContext build() {
setResourceNames(new TreeMap<>(resourceNames()));
Copy link
Member

Choose a reason for hiding this comment

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

I'm not very familiar with Autovalue, but can you just do this in the GapicContext.resourceNames() getter with @Memoized?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm totally new to AutoValue, and you may know better than I do. I tried to look up a way to achieve what I wanted to do, and this was basically the only way I could think of. I looked into @Memoized, but I think conceptually it's more appropriate to use for caching to solve a problem with an expensive operation. And the doc seems to suggest that I need another "derived method" to use @Memoized (though I am not sure). build() is called only once and new TreeMap<> is a simple thing to do, so I think storing the converted value at the beginning works and is more robust without any magic.

@chanseokoh chanseokoh merged commit 680874d into main Oct 28, 2021
@chanseokoh chanseokoh deleted the deterministic-resnames branch October 28, 2021 18:21
suztomo pushed a commit that referenced this pull request Mar 21, 2023
… to v2.16 (#865)

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
| [com.google.errorprone:error_prone_annotations](https://errorprone.info) ([source](https://togithub.com/google/error-prone)) | `2.15.0` -> `2.16` | [![age](https://badges.renovateapi.com/packages/maven/com.google.errorprone:error_prone_annotations/2.16/age-slim)](https://docs.renovatebot.com/merge-confidence/) | [![adoption](https://badges.renovateapi.com/packages/maven/com.google.errorprone:error_prone_annotations/2.16/adoption-slim)](https://docs.renovatebot.com/merge-confidence/) | [![passing](https://badges.renovateapi.com/packages/maven/com.google.errorprone:error_prone_annotations/2.16/compatibility-slim/2.15.0)](https://docs.renovatebot.com/merge-confidence/) | [![confidence](https://badges.renovateapi.com/packages/maven/com.google.errorprone:error_prone_annotations/2.16/confidence-slim/2.15.0)](https://docs.renovatebot.com/merge-confidence/) |

---

### Release Notes

<details>
<summary>google/error-prone</summary>

### [`v2.16`](https://togithub.com/google/error-prone/releases/tag/v2.16)

[Compare Source](https://togithub.com/google/error-prone/compare/v2.15.0...v2.16)

New Checkers:

-   [`ASTHelpersSuggestions`](https://errorprone.info/bugpattern/ASTHelpersSuggestions)
-   [`CanIgnoreReturnValueSuggester`](https://errorprone.info/bugpattern/CanIgnoreReturnValueSuggester)
-   [`LenientFormatStringValidation`](https://errorprone.info/bugpattern/LenientFormatStringValidation)
-   [`UnnecessarilyUsedValue`](https://errorprone.info/bugpattern/UnnecessarilyUsedValue)

Fixed issues: [#&#8203;3092](https://togithub.com/google/error-prone/issues/3092), [#&#8203;3220](https://togithub.com/google/error-prone/issues/3220), [#&#8203;3225](https://togithub.com/google/error-prone/issues/3225), [#&#8203;3267](https://togithub.com/google/error-prone/issues/3267), [#&#8203;3441](https://togithub.com/google/error-prone/issues/3441)

**Full Changelog**: https://togithub.com/google/error-prone/compare/v2.15.0...v2.16.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - 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.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox.

---

This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://app.renovatebot.com/dashboard#github/googleapis/java-shared-dependencies).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzMi4yMzIuMCIsInVwZGF0ZWRJblZlciI6IjMyLjIzOC40In0=-->
suztomo pushed a commit that referenced this pull request Mar 21, 2023
🤖 I have created a release *beep* *boop*
---


## [3.0.5](https://togithub.com/googleapis/java-shared-dependencies/compare/v3.0.4...v3.0.5) (2022-10-20)


### Dependencies

* Update dependency com.fasterxml.jackson:jackson-bom to v2.13.4.20221013 ([#868](https://togithub.com/googleapis/java-shared-dependencies/issues/868)) ([5c2a825](https://togithub.com/googleapis/java-shared-dependencies/commit/5c2a825c18af61784287dd41eba3a21be80bbe6b))
* Update dependency com.google.auth:google-auth-library-bom to v1.12.0 ([#870](https://togithub.com/googleapis/java-shared-dependencies/issues/870)) ([3e3a60d](https://togithub.com/googleapis/java-shared-dependencies/commit/3e3a60dfd45f08401ee3ac7a98007fae21d5bba6))
* Update dependency com.google.auth:google-auth-library-bom to v1.12.1 ([#871](https://togithub.com/googleapis/java-shared-dependencies/issues/871)) ([4d94c75](https://togithub.com/googleapis/java-shared-dependencies/commit/4d94c753b46d7f8c787b0efa21d7bc42b1ca1c6c))
* Update dependency com.google.cloud:grpc-gcp to v1.3.0 ([#867](https://togithub.com/googleapis/java-shared-dependencies/issues/867)) ([48ca222](https://togithub.com/googleapis/java-shared-dependencies/commit/48ca222a5e4d9f88737d4c4a4ee2a42a7145619e))
* Update dependency com.google.errorprone:error_prone_annotations to v2.16 ([#865](https://togithub.com/googleapis/java-shared-dependencies/issues/865)) ([d7a494d](https://togithub.com/googleapis/java-shared-dependencies/commit/d7a494dcd12a529121b74fd9fb9dfc679017f844))
* Update dependency com.google.protobuf:protobuf-bom to v3.21.8 ([#872](https://togithub.com/googleapis/java-shared-dependencies/issues/872)) ([ebe5d5f](https://togithub.com/googleapis/java-shared-dependencies/commit/ebe5d5f27dbe4f12c06d3a69c14c74bbf4e76dd1))
* Update dependency gcp-releasetool to v1.8.10 ([#853](https://togithub.com/googleapis/java-shared-dependencies/issues/853)) ([5c6367a](https://togithub.com/googleapis/java-shared-dependencies/commit/5c6367a643f491d2ec04be58c1ff0eca5aa10904))
* Update dependency google-api-core to v2.10.2 ([#858](https://togithub.com/googleapis/java-shared-dependencies/issues/858)) ([bc91e8d](https://togithub.com/googleapis/java-shared-dependencies/commit/bc91e8df54f9d919a9e0dc69e61d52fd855a8dbf))
* Update dependency io.grpc:grpc-bom to v1.50.0 ([#866](https://togithub.com/googleapis/java-shared-dependencies/issues/866)) ([50039f4](https://togithub.com/googleapis/java-shared-dependencies/commit/50039f41bfba37e65685c4a5b279d3cb2a92f2c5))
* Update dependency io.grpc:grpc-bom to v1.50.1 ([#873](https://togithub.com/googleapis/java-shared-dependencies/issues/873)) ([9fb1561](https://togithub.com/googleapis/java-shared-dependencies/commit/9fb15613976f83c5545e2b664c488e9811c1f185))
* Update dependency org.checkerframework:checker-qual to v3.26.0 ([#852](https://togithub.com/googleapis/java-shared-dependencies/issues/852)) ([1e8cd60](https://togithub.com/googleapis/java-shared-dependencies/commit/1e8cd609b3be0cdd748a8fea6bc0fcb15d8f4c96))
* Update dependency org.threeten:threetenbp to v1.6.3 ([#869](https://togithub.com/googleapis/java-shared-dependencies/issues/869)) ([e992190](https://togithub.com/googleapis/java-shared-dependencies/commit/e9921900ec590e281b5ae6e16ab51e7bd67c1242))
* Update dependency typing-extensions to v4.4.0 ([#854](https://togithub.com/googleapis/java-shared-dependencies/issues/854)) ([c909a13](https://togithub.com/googleapis/java-shared-dependencies/commit/c909a13fa626eb387c8ee87b7cc22607cb9cf889))
* Update dependency zipp to v3.9.0 ([#859](https://togithub.com/googleapis/java-shared-dependencies/issues/859)) ([971b84e](https://togithub.com/googleapis/java-shared-dependencies/commit/971b84eb801699b585cd35300bed8d4fb65046d8))
* Update gax.version to v2.19.4 ([#875](https://togithub.com/googleapis/java-shared-dependencies/issues/875)) ([2eb7f3d](https://togithub.com/googleapis/java-shared-dependencies/commit/2eb7f3d6cf834c474dcdc99740f8cdabf50bca51))
* Update google.core.version to v2.8.21 ([#861](https://togithub.com/googleapis/java-shared-dependencies/issues/861)) ([2fda421](https://togithub.com/googleapis/java-shared-dependencies/commit/2fda4213796df086450fdc3d69d53a0bd1d59f46))
* Update google.core.version to v2.8.22 ([#879](https://togithub.com/googleapis/java-shared-dependencies/issues/879)) ([e4f9f9a](https://togithub.com/googleapis/java-shared-dependencies/commit/e4f9f9ad6373fb52c069985ca4390663ccdacb7d))
* Update iam.version to v1.6.3 ([#857](https://togithub.com/googleapis/java-shared-dependencies/issues/857)) ([6758373](https://togithub.com/googleapis/java-shared-dependencies/commit/675837378642f39fe55c0e30b62755b9185bee3d))
* Update iam.version to v1.6.4 ([#862](https://togithub.com/googleapis/java-shared-dependencies/issues/862)) ([1e1bc34](https://togithub.com/googleapis/java-shared-dependencies/commit/1e1bc341c9dd0f8f5a2d14aa8dd52399b2ce71c1))

---
This PR was generated with [Release Please](https://togithub.com/googleapis/release-please). See [documentation](https://togithub.com/googleapis/release-please#release-please).
suztomo pushed a commit that referenced this pull request Mar 21, 2023
🤖 I have created a release *beep* *boop*
---


## [2.8.3](googleapis/java-core@v2.8.2...v2.8.3) (2022-07-26)


### Dependencies

* update dependency com.google.api-client:google-api-client-bom to v2 ([#868](googleapis/java-core#868)) ([fe7991d](googleapis/java-core@fe7991d))
* update dependency com.google.api:gax-bom to v2.18.4 ([#864](googleapis/java-core#864)) ([d4a8501](googleapis/java-core@d4a8501))
* update dependency com.google.api:gax-bom to v2.18.5 ([#876](googleapis/java-core#876)) ([e2c0c13](googleapis/java-core@e2c0c13))
* update dependency com.google.api.grpc:proto-google-common-protos to v2.9.2 ([#870](googleapis/java-core#870)) ([adf5af4](googleapis/java-core@adf5af4))
* update dependency com.google.api.grpc:proto-google-iam-v1 to v1.5.2 ([#865](googleapis/java-core#865)) ([5dad1ea](googleapis/java-core@5dad1ea))
* update dependency com.google.auth:google-auth-library-bom to v1.8.1 ([#856](googleapis/java-core#856)) ([bb58609](googleapis/java-core@bb58609))
* update dependency com.google.http-client:google-http-client-bom to v1.42.2 ([#871](googleapis/java-core#871)) ([341b04e](googleapis/java-core@341b04e))
* update dependency com.google.protobuf:protobuf-bom to v3.21.3 ([#874](googleapis/java-core#874)) ([efe8c28](googleapis/java-core@efe8c28))
* update dependency com.google.protobuf:protobuf-bom to v3.21.4 ([#877](googleapis/java-core#877)) ([25bea6c](googleapis/java-core@25bea6c))
* update dependency io.grpc:grpc-bom to v1.48.0 ([#873](googleapis/java-core#873)) ([8bfee63](googleapis/java-core@8bfee63))

---
This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

resource names are flip-flopping in generated test code
2 participants