[go: nahoru, domu]
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.

Sign up for GitHub

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

Jump to bottom

NetHttpTransport.trustCertificates ignores result of SslUtils.initSslContext #1710

Closed
Capstan opened this issue Aug 19, 2022 · 3 comments · Fixed by #1716
Closed

NetHttpTransport.trustCertificates ignores result of SslUtils.initSslContext #1710

Capstan opened this issue Aug 19, 2022 · 3 comments · Fixed by #1716
Assignees
@suztomo
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.

Comments

@Capstan
Copy link
Contributor
Capstan commented Aug 19, 2022

https://github.com/googleapis/google-http-java-client/blob/main/google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java#L317

In the monorepo, this fails to compile.

google-http-client/src/main/java/com/google/api/client/http/javanet/NetHttpTransport.java:317: error: [CheckReturnValue] The result of `initSslContext(...)` must be used
      SslUtils.initSslContext(
                             ^
  If you really don't want to use the result, then assign it to a variable: `var unused = ...`.
  
  If callers of `initSslContext(...)` shouldn't be required to use its result, then annotate it with `@CanIgnoreReturnValue`.

This is yet another case where if error prone was used and enforced in the external repo, we would not have import issues.

Does this code simply rely on the side effects on the passed in sslContext? OTOH, why return an SSLContext that was passed in as a variable? Chaining? Does initSslContext guarantee that the returned context is always the passed in context?
@suztomo suztomo self-assigned this Aug 25, 2022
@suztomo
Copy link
Member
suztomo commented Aug 25, 2022 •
edited
Loading

The Javadoc doesn't explain the return value. The implementation does return the instance sslContext passed as argument.

google-http-java-client/google-http-client/src/main/java/com/google/api/client/util/SslUtils.java

Line 130 in 17bbfc1

public static SSLContext initSslContext(

Annotating the initSslContext with @CanIgnoreReturnValue seems a good option to me. Would you be willing to create a pull request? Is initSslContext is the only place you got the error?

Note that this library must work with Java 7 as per README.md. -> error_prone_annotations artifact version 2.15.0 is compiled targeting Java 7 (pom.xml). Good.

Related: http://yaqs/1089314756940005376

@lqiu96 lqiu96 added the priority: p2 Moderately-important priority. Fix may not be included in next release. label Aug 29, 2022
@suztomo suztomo added the type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design. label Aug 29, 2022
@cpovirk
Copy link
cpovirk commented Aug 29, 2022

This is yet another case where if error prone was used and enforced in the external repo, we would not have import issues.

I am all in favor of running Error Prone in more places :) But FYI, this particular check runs in a stricter mode in the monorepo than it does externally. So you wouldn't see this error externally even if you were running Error Prone there :\

@cpovirk
Copy link
cpovirk commented Aug 29, 2022

(Also: You could consider disabling this check (or any other check) for your directory tree in the monorepo. Of course, the hope is that such checks are normally, well, useful. But I can see how the CheckReturnValue check in particular could lead to difficulties as we make it more strict in the monorepo. Still, I'm cautiously optimistic that it will be OK for you to continue to run there.)

Capstan added a commit to Capstan/google-http-java-client that referenced this issue Sep 1, 2022
@Capstan
Add @CanIgnoreReturnValue to avoid errorprone errors.
2d7152b 
In the monorepo, this blocks the build from succeeding.

Fixes googleapis#1710.
@Capstan Capstan mentioned this issue Sep 1, 2022
Merged
Capstan added a commit to Capstan/google-http-java-client that referenced this issue Sep 1, 2022
@Capstan
Add @CanIgnoreReturnValue to avoid errorprone errors.
6d42922 
In the monorepo, this blocks the build from succeeding.

Fixes googleapis#1710.
Capstan added a commit to Capstan/google-http-java-client that referenced this issue Sep 1, 2022
@Capstan
Add @CanIgnoreReturnValue to avoid errorprone errors.
51107d1 
In the monorepo, this blocks the build from succeeding.

Fixes googleapis#1710.
suztomo pushed a commit that referenced this issue Sep 1, 2022
@Capstan
fix: add @CanIgnoreReturnValue to avoid errorprone errors (#1716)
cba2f82 
In the Google monorepo, this was blocking the build from succeeding.

Fixes #1710.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@suztomo suztomo

Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: feature request ‘Nice-to-have’ improvement, new feature or different behavior or design.
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: add @CanIgnoreReturnValue to avoid errorprone errors. Capstan/google-http-java-client
4 participants
@suztomo @Capstan @cpovirk @lqiu96