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

4117: Added Md5String() method and updated tests. #4397

Merged
merged 13 commits into from
Feb 11, 2019

Conversation

andrey-qlogic
Copy link

Fixes #4117

@andrey-qlogic andrey-qlogic requested a review from a team as a code owner January 21, 2019 12:39
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 21, 2019
@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 21, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 21, 2019
@andrey-qlogic
Copy link
Author

@frankyn, would you please take a look at this PR.

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

@andrey-qlogic adding this feature to the client sg. I recommend adding the same for crc32c as well.

@andrey-qlogic
Copy link
Author

Added Crc32cString() method and updated tests. Added to snippets.

@ajaaym ajaaym added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 23, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 23, 2019
@pmakani pmakani added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 24, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Jan 24, 2019
@frankyn
Copy link
Member

frankyn commented Jan 24, 2019

Hi @andrey-qlogic, Thanks for the update. I chatted with the GCS team about this design. It sounds good to them but suggested adding a Hex Hash String to GCS base64 encoded string for setCrc32c() method to the Blob class.

The naming convention would help: @googleapis/yoshi-java do you have input on Java community method names?

WDYT about the following names?

getMd5ToHexString()
setMd5FromHexString()

getCrc32cToHexString()
setCrc32cFromHexString()

@andrey-qlogic
Copy link
Author

andrey-qlogic commented Jan 25, 2019

Renamed new methods using naming convention getMd5ToHexString(), getCrc32cToHexString().
I believe that setter methods are not required as those new methods return a string representation md5 and cr32 that set in another methods.

@andrey-qlogic andrey-qlogic self-assigned this Jan 28, 2019
@frankyn
Copy link
Member

frankyn commented Jan 31, 2019

The other method setMd5 for example expects the base64 encoded string of an Md5 hash. It would be nice to have a similar helper method to getting the md5 hash as a base64 decoded md5 hex string.

@andrey-qlogic
Copy link
Author

@frankyn, that is unclear, are you suggesting to add a new setter method to class BuilderImpl
like
public Builder setMd5FromHexString(String md5HexString)
that will do something like
byte[] decoded = BaseEncoding.base64().decode(md5HexString);

and assign it to md5 ?

Otherwise the method getMd5ToHexString is implepented for BlobInfo class in 85588a3

I would appreciate for your input

@frankyn
Copy link
Member

frankyn commented Feb 1, 2019

I'm expecting to see public Builder setMd5FromHexString(String md5HexString) do the following:

// pseudo code
setMd5FromHexString(String md5HexString) {
   // convert md5HexString back to its binary representation e.g. reverse binary to human readable hex.
   // base64 encode the binary representation
  // set the value of Md5 in BlobInfo to the base64 encoded version.
}

@andrey-qlogic
Copy link
Author

Added methods to set Md2 and Crc32s from hex strings.

@andrey-qlogic andrey-qlogic added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 4, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 4, 2019
@andrey-qlogic
Copy link
Author

there are code format issues that will be fixed with #4437

@andrey-qlogic andrey-qlogic added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 5, 2019
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Feb 5, 2019
@andrey-qlogic
Copy link
Author

@frankyn, I realized that Md5 sometimes used with 0x prefix that can be easily removed at BlobInfo class.

@frankyn
Copy link
Member

frankyn commented Feb 7, 2019

For this PR, @andrey-qlogic could you skip supporting the removal of "0x" for now?
I'm leaning towards supporting that incrementally instead of in the first iteration.

Thank you, apologies for constant delays on these reviews.

@JustinBeckwith JustinBeckwith added the 🚨 This issue needs some love. label Feb 7, 2019
@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Feb 7, 2019
@andrey-qlogic
Copy link
Author

@frankyn, do you want me to drop the whole last commit 7f19e33 ?

@frankyn
Copy link
Member

frankyn commented Feb 7, 2019

Yes except the integration test.

@andrey-qlogic
Copy link
Author

Reverted back the commit 7f19e33 except integration tests.

@andrey-qlogic
Copy link
Author

@frankyn, do you mean the change for both Md5 and Crc32c, correct
What about unit tests values?

@andrey-qlogic
Copy link
Author

andrey-qlogic commented Feb 7, 2019

@frankyn
Removed code that removes 0x from hex string.
Removed code that adds prepending 0x.
Removed OmitPadding, it looks like it is not required.
Added MD_NO_PREFIX, CRC32C_NO_PREFIX constant for testing.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019
Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

I think this is the last nits. Thanks for your patience @andrey-qlogic.

@andrey-qlogic
Copy link
Author

@frankyn , I realized that new BigInteger(md5HexString, 16).toByteArray() is not safe with leading zeros. I changed to DatatypeConverter.parseHexBinary(md5HexString). It works fine. If needed we can make an utility method instead.

Copy link
Member

@frankyn frankyn left a comment

Choose a reason for hiding this comment

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

LGTM, I notice you dropped, DatatypeConverter.parseHexBinary(md5HexString) due to supportability. Thanks for verifying that!

@andrey-qlogic andrey-qlogic merged commit a4246de into googleapis:master Feb 11, 2019
@andrey-qlogic
Copy link
Author

thanks for the approval

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. 🚨 This issue needs some love.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants