garydgregory commented on code in PR #618:
URL: https://github.com/apache/commons-compress/pull/618#discussion_r1863594455
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java:
##########
@@ -298,6 +298,18 @@ public int type() {
private int bufferSize = 512;
private int deflateStrategy = Deflater.DEFAULT_STRATEGY;
+ private void checkEncodedStringHasNoZero(String text) {
Review Comment:
- Call the method requireNonNulByte(), in the same style as
`Objects.requireNonNull()`
- Use `final` for the parameter.
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java:
##########
@@ -298,6 +298,18 @@ public int type() {
private int bufferSize = 512;
private int deflateStrategy = Deflater.DEFAULT_STRATEGY;
+ private void checkEncodedStringHasNoZero(String text) {
+ if (text != null && text.length() > 0) {
Review Comment:
Just call `org.apache.commons.lang3.ArrayUtils.contains(byte[], byte)`.
##########
src/test/java/org/apache/commons/compress/compressors/gzip/GzipParametersTest.java:
##########
@@ -46,4 +50,33 @@ public void testToString() {
gzipParameters.setOS(GzipParameters.OS.Z_SYSTEM);
assertTrue(gzipParameters.toString().contains("Z_SYSTEM"));
}
+
+ @ParameterizedTest
+ @CsvSource({ //@formatter:off
+ " , helloworld, true",
+ " , hello�world, true",
+ " , hello\0world, false",
+ "ISO-8859-1, helloworld, true",
+ "ISO-8859-1, hello�world, true",
+ "ISO-8859-1, hello\0world, false",
+ "UTF-8 , helloworld, true",
+ "UTF-8 , hello�world, true",
+ "UTF-8 , hello\0world, false",
+ "UTF-16BE , helloworld, false",
+ })//@formatter:on
+ public void testIllegalCommentOrFileName(String charset, String text,
boolean shouldPass) {
Review Comment:
Use `final` where you can.
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java:
##########
@@ -298,6 +298,18 @@ public int type() {
private int bufferSize = 512;
private int deflateStrategy = Deflater.DEFAULT_STRATEGY;
+ private void checkEncodedStringHasNoZero(String text) {
+ if (text != null && text.length() > 0) {
+ byte[] ba = text.getBytes(fileNameCharset);
+ for (int i = 0; i < ba.length; i++) {
+ if (ba[i] == 0) {
+ throw new IllegalArgumentException(
+ "String in charset '" + fileNameCharset + "'
contains the byte 0 (at encoded position " + i + ") which is not supported in
gzip. ");
+ }
+ }
+ }
+ }
Review Comment:
Return the input String and use the result in the assignment in call-sites,
like you would with `Objects.requireNonNull()`
##########
src/main/java/org/apache/commons/compress/compressors/gzip/GzipParameters.java:
##########
@@ -298,6 +298,18 @@ public int type() {
private int bufferSize = 512;
private int deflateStrategy = Deflater.DEFAULT_STRATEGY;
+ private void checkEncodedStringHasNoZero(String text) {
+ if (text != null && text.length() > 0) {
+ byte[] ba = text.getBytes(fileNameCharset);
+ for (int i = 0; i < ba.length; i++) {
+ if (ba[i] == 0) {
+ throw new IllegalArgumentException(
+ "String in charset '" + fileNameCharset + "'
contains the byte 0 (at encoded position " + i + ") which is not supported in
gzip. ");
Review Comment:
- "String in charset" -> "String encoded with Charset"
- No space after the "." at the end of the String.
##########
src/test/java/org/apache/commons/compress/compressors/gzip/GzipParametersTest.java:
##########
@@ -46,4 +50,33 @@ public void testToString() {
gzipParameters.setOS(GzipParameters.OS.Z_SYSTEM);
assertTrue(gzipParameters.toString().contains("Z_SYSTEM"));
}
+
+ @ParameterizedTest
+ @CsvSource({ //@formatter:off
Review Comment:
Split this test in 2:
- One for legal values.
- One for illegal values.
--
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]
