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

Using %d instead of %s for formating in checkArgument #159

Closed
runevalle-cognite opened this issue Mar 3, 2020 · 2 comments · Fixed by #163
Closed

Using %d instead of %s for formating in checkArgument #159

runevalle-cognite opened this issue Mar 3, 2020 · 2 comments · Fixed by #163
Assignees
Labels
api: storage Issues related to the googleapis/java-storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@runevalle-cognite
Copy link

There are currently being used %d instead of %s in template string to checkArguments in
https://github.com/googleapis/java-storage/blob/master/google-cloud-storage/src/main/java/com/google/cloud/storage/spi/v1/HttpStorageRpc.java#L692

This makes the output from precondition failure come out awkwardly.

I guess someone with more routine in getting changes into this codebase can make this fix faster than me.

@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/java-storage API. label Mar 3, 2020
@athakor athakor self-assigned this Mar 4, 2020
@athakor
Copy link
Contributor

athakor commented Mar 4, 2020

@runevalle-cognite i dont think so we need to add %s instead of %d, because with the %d format specifier, you can use an argument of all integral types including byte, short, int, long and BigInteger. you can see here argument position is long.

@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Mar 4, 2020
@runevalle-cognite
Copy link
Author

runevalle-cognite commented Mar 4, 2020

@athakor
Yes, if it had been the standard formater that had been used, %d would have worked, however that is not the case in guava Preconditions.

Part of stack trace that lead to the discovery of this bug:
com.google.cloud.storage.StorageException: java.lang.IllegalArgumentException: Position should be non-negative, is %d [-1] at com.google.cloud.storage.StorageException.translateAndThrow(StorageException.java:74) at com.google.cloud.storage.BlobReadChannel.read(BlobReadChannel.java:141)

Outline of to reproduce:

  • Establish a channel to a google storage bucket.
  • channel.seek(-1);
  • channel.read(buffer)

Documentation for guava:
https://guava.dev/releases/19.0/api/docs/com/google/common/base/Preconditions.html#checkArgument(boolean,%20java.lang.String,%20java.lang.Object...)

Relevant source code line in the formater that is actually used:
https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Strings.java#L276

@athakor athakor added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. and removed triage me I really want to be triaged. labels Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/java-storage API. priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants