jdaugherty commented on code in PR #15828:
URL: https://github.com/apache/grails-core/pull/15828#discussion_r3538013254
##########
grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/DigestUtils.groovy:
##########
@@ -18,30 +18,69 @@
*/
package org.grails.plugins.codecs
+import java.lang.reflect.Array
import java.nio.charset.StandardCharsets
import java.security.MessageDigest
+import groovy.transform.CompileStatic
+
+@CompileStatic
abstract class DigestUtils {
// Digest byte[], any list/array or string into a byte[]
- static digest(String algorithm, data) {
+ static Object digest(String algorithm, Object data) {
if (data == null) {
return null
}
- def md = MessageDigest.getInstance(algorithm)
- def src
- if (data instanceof Byte[] || data instanceof byte[]) {
- src = data
+ MessageDigest md = MessageDigest.getInstance(algorithm)
+ byte[] src = toByteArray(data)
+ md.update(src) // This probably needs to use the thread's Locale
encoding
+ return md.digest()
+ }
+
+ protected static byte[] toByteArray(Object data) {
+ if (data instanceof byte[]) {
+ return (byte[]) data
}
- else if (data instanceof List || data.getClass().isArray()) {
- src = new byte[data.size()]
- data.eachWithIndex { v, i -> src[i] = v }
+ if (data instanceof Byte[]) {
+ return toByteArrayFromWrapper((Byte[]) data)
}
- else {
- src = data.toString().getBytes(StandardCharsets.UTF_8)
+ if (data instanceof List) {
+ return toByteArrayFromList((List<?>) data)
}
- md.update(src) // This probably needs to use the thread's Locale
encoding
- return md.digest()
+ if (data.getClass().isArray()) {
+ return toByteArrayFromArray(data, Array.getLength(data))
+ }
+
+ return data.toString().getBytes(StandardCharsets.UTF_8)
+ }
+
+ private static byte[] toByteArrayFromWrapper(Byte[] data) {
+ byte[] result = new byte[data.length]
+ for (int i = 0; i < data.length; i++) {
+ result[i] = data[i].byteValue()
+ }
+ return result
+ }
+
+ private static byte[] toByteArrayFromList(List<?> data) {
+ byte[] result = new byte[data.size()]
+ for (int i = 0; i < data.size(); i++) {
+ result[i] = toByte(data.get(i))
+ }
+ return result
+ }
+
+ private static byte[] toByteArrayFromArray(Object data, int length) {
+ byte[] result = new byte[length]
+ for (int i = 0; i < length; i++) {
+ result[i] = toByte(Array.get(data, i))
+ }
+ return result
+ }
+
+ private static byte toByte(Object value) {
+ return ((Number) value).byteValue()
Review Comment:
This narrows the accepted element types compared to the old dynamic code.
Previously `src[i] = v` went through Groovy's `DefaultTypeTransformation`
coercion, which also accepted `Character` elements (and `char[]` arrays via the
array path); now any non-`Number` element throws `ClassCastException`. If
preserving the old tolerance matters,
`DefaultTypeTransformation.castToNumber(value).byteValue()` keeps the coercion
while staying statically compiled. If the narrowing is intentional, it's
probably worth a line in the PR compatibility notes.
##########
grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/DigestUtils.groovy:
##########
@@ -18,30 +18,69 @@
*/
package org.grails.plugins.codecs
+import java.lang.reflect.Array
import java.nio.charset.StandardCharsets
import java.security.MessageDigest
+import groovy.transform.CompileStatic
+
+@CompileStatic
abstract class DigestUtils {
// Digest byte[], any list/array or string into a byte[]
- static digest(String algorithm, data) {
+ static Object digest(String algorithm, Object data) {
if (data == null) {
return null
}
- def md = MessageDigest.getInstance(algorithm)
- def src
- if (data instanceof Byte[] || data instanceof byte[]) {
- src = data
+ MessageDigest md = MessageDigest.getInstance(algorithm)
+ byte[] src = toByteArray(data)
+ md.update(src) // This probably needs to use the thread's Locale
encoding
+ return md.digest()
+ }
+
+ protected static byte[] toByteArray(Object data) {
Review Comment:
Nit: `protected` reads as "for subclasses", but the actual consumers
(`Base64CodecExtensionMethods`, `HexCodecExtensionMethods`) rely on
`protected`'s package-access side effect. `@PackageScope` (or a brief comment)
would make the intent clearer.
##########
grails-codecs-core/src/test/groovy/org/grails/web/codecs/HexCodecTests.groovy:
##########
@@ -48,6 +49,13 @@ class HexCodecTests {
assertIterableEquals(new Byte[] {65, 32, 66, 32, 67, 32, 68, 32,
69}.toList(), result.toList())
//make sure decoding null returns null
assertEquals(null.decodeHex(), null)
+
+ //make sure decoding Groovy-falsy values returns null
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(''))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(0))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(false))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex([]))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(new byte[0]))
Review Comment:
Per the repo rule to test via public APIs, these could use the
extension-method form users actually call — `''.decodeHex()`, `0.decodeHex()`,
`false.decodeHex()`, `[].decodeHex()`, `new byte[0].decodeHex()` — which also
exercises the extension-module registration path rather than the static class
directly (matching the existing `null.decodeHex()` assertion above).
##########
grails-codecs-core/src/test/groovy/org/grails/web/codecs/HexCodecTests.groovy:
##########
@@ -48,6 +49,13 @@ class HexCodecTests {
assertIterableEquals(new Byte[] {65, 32, 66, 32, 67, 32, 68, 32,
69}.toList(), result.toList())
//make sure decoding null returns null
assertEquals(null.decodeHex(), null)
+
+ //make sure decoding Groovy-falsy values returns null
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(''))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(0))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(false))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex([]))
+ assertEquals(null, HexCodecExtensionMethods.decodeHex(new byte[0]))
}
void testRoundtrip() {
Review Comment:
Pre-existing, but since this file is being touched: `testRoundtrip` has no
`@Test` annotation, so it never runs — and it's exactly the round-trip coverage
the new static implementations want. Note that enabling it as-is will likely
fail on the first assertion: `assertIterableEquals` compares `Integer` expected
values against the `Byte` elements from `decodeHex(...).toList()`, and
`Integer(65).equals(Byte(65))` is false — the expectations need a byte-typed
list.
##########
grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/MD5CodecExtensionMethods.groovy:
##########
@@ -18,15 +18,19 @@
*/
package org.grails.plugins.codecs
+import groovy.transform.CompileStatic
+
+@CompileStatic
class MD5CodecExtensionMethods {
// Returns the byte[] of the digest, taken from UTF-8 of the string
representation
// or the raw data coerced to bytes
- static encodeAsMD5(theTarget) {
- theTarget.encodeAsMD5Bytes()?.encodeAsHex()
+ static Object encodeAsMD5(Object theTarget) {
+ byte[] digest = (byte[])
MD5BytesCodecExtensionMethods.encodeAsMD5Bytes(theTarget)
Review Comment:
Just confirming this is intended: the old
`theTarget.encodeAsMD5Bytes()?.encodeAsHex()` dispatched dynamically, so a
runtime metaclass override of `encodeAsMD5Bytes` (e.g. an application-supplied
codec replacing the default) would also change what `encodeAsMD5` produced. The
direct static call hardwires the default implementation. Same applies to the
SHA-1/SHA-256 variants. Probably fine — it's the whole point of removing
dynamic dispatch — but worth a note in the compatibility section.
##########
grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/HexCodecExtensionMethods.groovy:
##########
@@ -21,51 +21,47 @@ package org.grails.plugins.codecs
import java.nio.charset.StandardCharsets
import org.codehaus.groovy.runtime.NullObject
+import org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation
+import groovy.transform.CompileStatic
+
+@CompileStatic
class HexCodecExtensionMethods {
- static HEXDIGITS = '0123456789abcdef'
+ static Object HEXDIGITS = '0123456789abcdef'
// Expects an array/list of numbers
- static encodeAsHex(theTarget) {
+ static Object encodeAsHex(Object theTarget) {
if (theTarget == null || theTarget instanceof NullObject) {
return null
}
- def result = new StringBuilder()
- if (theTarget instanceof String) {
- theTarget = theTarget.getBytes(StandardCharsets.UTF_8)
- }
- theTarget.each() {
- result << HexCodecExtensionMethods.HEXDIGITS[(it & 0xF0) >> 4]
- result << HexCodecExtensionMethods.HEXDIGITS[it & 0x0F]
+ byte[] bytes = theTarget instanceof String ? ((String)
theTarget).getBytes(StandardCharsets.UTF_8) : DigestUtils.toByteArray(theTarget)
+ StringBuilder result = new StringBuilder(bytes.length * 2)
+ String hexDigits = (String) HEXDIGITS
+ for (byte value : bytes) {
+ int unsignedValue = value & 0xFF
+ result.append(hexDigits.charAt((unsignedValue & 0xF0) >> 4))
+ result.append(hexDigits.charAt(unsignedValue & 0x0F))
}
return result.toString()
}
- static decodeHex(theTarget) {
- if (!theTarget) return null
+ static Object decodeHex(Object theTarget) {
+ if (theTarget == null || theTarget instanceof NullObject ||
!DefaultTypeTransformation.castToBoolean(theTarget)) return null
- def output = []
-
- def str = theTarget.toString().toLowerCase()
- if (str.size() % 2) {
+ String str = theTarget.toString().toLowerCase()
+ if (str.size() % 2 != 0) {
throw new UnsupportedOperationException('Decode of hex strings
requires strings of even length')
}
- def currentByte
- str.eachWithIndex { val, idx ->
- if (!(idx % 2)) {
- currentByte = HEXDIGITS.indexOf(val) << 4
- }
- else {
- output << (currentByte | HEXDIGITS.indexOf(val))
- currentByte = 0
- }
+ byte[] result = new byte[str.size().intdiv(2)]
+ String hexDigits = (String) HEXDIGITS
+ for (int i = 0; i < str.size(); i += 2) {
+ int high = hexDigits.indexOf((int) str.charAt(i))
+ int low = hexDigits.indexOf((int) str.charAt(i + 1))
+ result[i.intdiv(2)] = (byte) ((high << 4) | low)
Review Comment:
Nit for a perf-focused PR: `i.intdiv(2)` boxes on every iteration; `i >> 1`
(and `str.length() >> 1` on line 58) avoids it. Same for `str.size()` vs plain
`str.length()`. Also, on line 51 the explicit `null`/`NullObject` checks are
redundant — `DefaultTypeTransformation.castToBoolean` already returns `false`
for both.
##########
grails-codecs-core/src/main/groovy/org/grails/plugins/codecs/HexCodecExtensionMethods.groovy:
##########
@@ -21,51 +21,47 @@ package org.grails.plugins.codecs
import java.nio.charset.StandardCharsets
import org.codehaus.groovy.runtime.NullObject
+import org.codehaus.groovy.runtime.typehandling.DefaultTypeTransformation
+import groovy.transform.CompileStatic
+
+@CompileStatic
class HexCodecExtensionMethods {
- static HEXDIGITS = '0123456789abcdef'
+ static Object HEXDIGITS = '0123456789abcdef'
// Expects an array/list of numbers
- static encodeAsHex(theTarget) {
+ static Object encodeAsHex(Object theTarget) {
if (theTarget == null || theTarget instanceof NullObject) {
return null
}
- def result = new StringBuilder()
- if (theTarget instanceof String) {
- theTarget = theTarget.getBytes(StandardCharsets.UTF_8)
- }
- theTarget.each() {
- result << HexCodecExtensionMethods.HEXDIGITS[(it & 0xF0) >> 4]
- result << HexCodecExtensionMethods.HEXDIGITS[it & 0x0F]
+ byte[] bytes = theTarget instanceof String ? ((String)
theTarget).getBytes(StandardCharsets.UTF_8) : DigestUtils.toByteArray(theTarget)
Review Comment:
Behavior change for iterable targets that aren't `List`s or arrays (e.g. a
`Set<Integer>` or an `Iterator`): the old code element-wise hex-encoded
anything via `theTarget.each { ... }`, but `DigestUtils.toByteArray` only
special-cases `byte[]`/`Byte[]`/`List`/arrays and otherwise falls through to
`toString().getBytes(UTF_8)` — so a `Set` of numbers now silently encodes its
string representation instead of its elements. Consider handling
`Collection`/`Iterable` in `toByteArray` (or at least calling this out in the
compatibility notes, since the failure mode is silent wrong output rather than
an exception).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
