Copilot commented on code in PR #812:
URL: https://github.com/apache/fesod/pull/812#discussion_r2709180492
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/FileTypeUtils.java:
##########
@@ -56,12 +57,21 @@ public static int getImageTypeFormat(byte[] image) {
}
public static ImageData.ImageType getImageType(byte[] image) {
- if (image == null || image.length <= IMAGE_TYPE_MARK_LENGTH) {
+ if (image == null || image.length < IMAGE_TYPE_MARK_MIN_LENGTH) {
return null;
}
- byte[] typeMarkByte = new byte[IMAGE_TYPE_MARK_LENGTH];
- System.arraycopy(image, 0, typeMarkByte, 0, IMAGE_TYPE_MARK_LENGTH);
- return FILE_TYPE_MAP.get(encodeHexStr(typeMarkByte));
+ int lengthToCopy = Math.min(image.length, IMAGE_TYPE_MARK_LENGTH);
+ byte[] typeMarkByte = new byte[lengthToCopy];
+ System.arraycopy(image, 0, typeMarkByte, 0, lengthToCopy);
+
+ String hexString = encodeHexStr(typeMarkByte);
+ for (Map.Entry<String, ImageData.ImageType> entry :
FILE_TYPE_MAP.entrySet()) {
+ String magicNumber = entry.getKey();
+ if (hexString.startsWith(magicNumber)) {
+ return entry.getValue();
+ }
+ }
Review Comment:
The iteration order of HashMap entries is not guaranteed. While not an issue
with the current two magic numbers, this could cause incorrect matches if
future image types are added with overlapping prefixes. Consider using
LinkedHashMap to maintain insertion order or implementing explicit ordering
logic (e.g., longest prefix first) to ensure deterministic and correct matching
behavior.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/FileTypeUtils.java:
##########
@@ -56,12 +57,21 @@ public static int getImageTypeFormat(byte[] image) {
}
public static ImageData.ImageType getImageType(byte[] image) {
- if (image == null || image.length <= IMAGE_TYPE_MARK_LENGTH) {
+ if (image == null || image.length < IMAGE_TYPE_MARK_MIN_LENGTH) {
return null;
}
- byte[] typeMarkByte = new byte[IMAGE_TYPE_MARK_LENGTH];
- System.arraycopy(image, 0, typeMarkByte, 0, IMAGE_TYPE_MARK_LENGTH);
- return FILE_TYPE_MAP.get(encodeHexStr(typeMarkByte));
+ int lengthToCopy = Math.min(image.length, IMAGE_TYPE_MARK_LENGTH);
+ byte[] typeMarkByte = new byte[lengthToCopy];
+ System.arraycopy(image, 0, typeMarkByte, 0, lengthToCopy);
+
+ String hexString = encodeHexStr(typeMarkByte);
+ for (Map.Entry<String, ImageData.ImageType> entry :
FILE_TYPE_MAP.entrySet()) {
+ String magicNumber = entry.getKey();
+ if (hexString.startsWith(magicNumber)) {
+ return entry.getValue();
+ }
+ }
Review Comment:
The iteration-based approach with startsWith() has O(n) time complexity for
each lookup, compared to O(1) for the previous HashMap.get() approach. For
better performance with the new logic, consider using a more efficient data
structure like a Trie or maintaining a sorted list by magic number length for
early exit optimization.
##########
fesod-sheet/src/main/java/org/apache/fesod/sheet/util/FileTypeUtils.java:
##########
@@ -33,6 +33,7 @@ public class FileTypeUtils {
private static final char[] DIGITS = {'0', '1', '2', '3', '4', '5', '6',
'7', '8', '9', 'a', 'b', 'c', 'd', 'e', 'f'
};
private static final int IMAGE_TYPE_MARK_LENGTH = 28;
+ private static final int IMAGE_TYPE_MARK_MIN_LENGTH = 4;
Review Comment:
The IMAGE_TYPE_MARK_MIN_LENGTH is set to 4 bytes, but JPEG's magic number
"ffd8ff" only requires 3 bytes. The test test_getImageType_tooShort expects
null for a 3-byte JPEG magic number, which would incorrectly reject valid JPEG
headers. Either IMAGE_TYPE_MARK_MIN_LENGTH should be 3 to accommodate JPEG, or
the JPEG magic number definition should be extended to require 4 bytes.
```suggestion
// Must be at least the length (in bytes) of the shortest magic number
(currently JPEG: "ffd8ff" = 3 bytes).
private static final int IMAGE_TYPE_MARK_MIN_LENGTH = 3;
```
##########
fesod-sheet/src/test/java/org/apache/fesod/sheet/util/FileTypeUtilsTest.java:
##########
@@ -0,0 +1,113 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.fesod.sheet.util;
+
+import org.apache.fesod.sheet.metadata.data.ImageData;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.NullAndEmptySource;
+
+/**
+ * Tests {@link FileTypeUtils}
+ */
+class FileTypeUtilsTest {
+
+ @ParameterizedTest
+ @NullAndEmptySource
+ void test_getImageType_NullOrEmpty(byte[] input) {
+ Assertions.assertNull(FileTypeUtils.getImageType(input));
+ }
+
+ @Test
+ void test_getImageType_tooShort() {
+ byte[] input = new byte[] {(byte) 0xFF, (byte) 0xD8, (byte) 0xFF};
Review Comment:
This test creates a 3-byte array with valid JPEG magic bytes (0xFF, 0xD8,
0xFF) and expects null, but JPEG's magic number is "ffd8ff" which is exactly 3
bytes. This test appears to validate incorrect behavior where valid JPEG images
would be rejected. The test should either expect PICTURE_TYPE_JPEG or use a
different magic number that genuinely requires 4 bytes.
```suggestion
byte[] input = new byte[] {(byte) 0x00, (byte) 0x01};
```
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
