-
Notifications
You must be signed in to change notification settings - Fork 280
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
Move 2 engineDoFinal methods from OpenSSLCipher.java to its subclasses. #1264
Conversation
Awesome, thanks! LGTM but I'll take another look with fresh eyes in the morning. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're lacking some tests around zero length inputs but overall LGTM apart from one untested branch noted below.
|
||
final int bytesWritten; | ||
if (inputLen > 0) { | ||
bytesWritten = updateInternal(input, inputOffset, inputLen, output, outputOffset, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No tests seem to hit this branch, could you add one, please.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. The Cipher interface has many similar methods, and depending on how exactly it is called, a different method of the implementation is executed. I've now changed the tests of Cipher so that it tries a range of different call patterns. I think this should give a better coverage.
} | ||
|
||
/** Concatenates the given arrays into a single array.*/ | ||
byte[] concatArrays(byte[]... arrays) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We already have ArrayUtils.concat for the two array case. Maybe move this method into ArrayUtils for the other cases (it's fairly small).
byte[] concatArrays(byte[]... arrays) { | ||
int length = 0; | ||
for (byte[] array : arrays) { | ||
if (array == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe throw on null? But you can certainly skip zero length arrays.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually don't worry about fixing the nits :)
Each of the three subclasses needs a slightly different implementation of these methods. So it's better to have the implementation here.