From 2eb74d80e37cc91ada4d160b1e98a7a7360929ab Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Thu, 12 Jun 2025 05:08:42 +0000 Subject: [PATCH] fix: Improve ICO decoding and UI consistency This commit addresses issues related to ICO file decoding and refines UI aspects for better consistency. Changelog: Backend: - Integrated Apache Commons Imaging library (`1.0.0-alpha6`) for more robust parsing of ICO files, particularly for logos. - Implemented a fallback mechanism to `ImageIO` if Commons Imaging fails to decode an ICO. - Enhanced stream handling in `ColorExtractionService` to allow multiple parsing attempts on logo image streams (using `ByteArrayInputStream` to ensure `mark`/`reset` support). - Improved logging around logo image decoding to provide more clarity on success/failure of different parsing attempts. - Added comprehensive unit tests in `ColorExtractionServiceTest.java` to cover various scenarios of ICO and non-ICO logo decoding, including success and failure paths for both Commons Imaging and ImageIO. Frontend: - Corrected a CSS selector in `color-input-display.component.css` from `.color-info p` to `.color-details p` (and related `strong` tag selector) to ensure styles for color item details are correctly applied as per the HTML structure. - Verified that color names (defaulting to hex if no friendly name is available via `ColorNamer`) and sources are consistently passed from the backend and displayed in the UI. These changes aim to improve the reliability of logo color extraction, especially for websites using ICO files as favicons/logos, and ensure that UI styles are applied as intended. --- .../color-input-display.component.css | 4 +- pom.xml | 5 + .../service/ColorExtractionService.java | 93 +++++++++-- .../service/ColorExtractionServiceTest.java | 146 +++++++++++++++++- 4 files changed, 231 insertions(+), 17 deletions(-) diff --git a/color-extractor-ui/src/app/color-input-display/color-input-display.component.css b/color-extractor-ui/src/app/color-input-display/color-input-display.component.css index 23386a3..2a2e931 100644 --- a/color-extractor-ui/src/app/color-input-display/color-input-display.component.css +++ b/color-extractor-ui/src/app/color-input-display/color-input-display.component.css @@ -109,12 +109,12 @@ h2 { margin-bottom: 10px; } -.color-info p { +.color-details p { margin: 5px 0; font-size: 14px; word-break: break-all; } -.color-info p strong { +.color-details p strong { color: #555; } diff --git a/pom.xml b/pom.xml index fcbbd45..b601177 100644 --- a/pom.xml +++ b/pom.xml @@ -26,6 +26,11 @@ jsoup 1.15.3 + + org.apache.commons + commons-imaging + 1.0.0-alpha6 + org.projectlombok lombok diff --git a/src/main/java/com/example/colorschemeidentifier/service/ColorExtractionService.java b/src/main/java/com/example/colorschemeidentifier/service/ColorExtractionService.java index 52d2bf7..f6cacc9 100644 --- a/src/main/java/com/example/colorschemeidentifier/service/ColorExtractionService.java +++ b/src/main/java/com/example/colorschemeidentifier/service/ColorExtractionService.java @@ -10,9 +10,13 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.stereotype.Service; +import org.apache.commons.imaging.Imaging; +import org.apache.commons.imaging.ImageReadException; + import javax.imageio.ImageIO; import java.awt.image.BufferedImage; +import java.io.ByteArrayInputStream; // import java.io.ByteArrayOutputStream; // No longer strictly needed after refactor import java.io.IOException; import java.io.InputStream; @@ -448,30 +452,91 @@ private void extractColorsFromLogoImage(Set logoUrlSet, Map 0) { + dataUriMimeType = logoUrl.substring(5, mimeEnd).toLowerCase(); + } + } + boolean isIcoByMime = "image/x-icon".equals(dataUriMimeType) || "image/vnd.microsoft.icon".equals(dataUriMimeType); + + imageStream = logoUrl.startsWith("data:image") + ? processDataUriGetStream(logoUrl, sourceIdentifier) // This returns ByteArrayInputStream + : openConnectionAndGetStream(logoUrl); // This returns HttpInputStream if (imageStream == null) { logger.warn("Could not get input stream for logo URL: {}", logoUrl); return; } - try (InputStream in = imageStream) { // Ensure stream is closed - BufferedImage image = ImageIO.read(in); - if (image != null) { - logger.info("Processing logo image: {}", sourceIdentifier); - // Calls the internal method, setting isLogoColor to true - extractDominantColorsFromImageInternal(image, colorFrequencies, sourceIdentifier, true); - } else { - logger.warn("Could not decode logo image from source: {}", sourceIdentifier); + // For ICO, we need to buffer the stream if it's not already a ByteArrayInputStream, + // because Apache Commons Imaging might need to read it multiple times or it might not support mark/reset on HttpInputStream. + // ImageIO.read also benefits from a resettable stream for some formats. + if (!imageStream.markSupported()) { + imageStream = new ByteArrayInputStream(imageStream.readAllBytes()); + } + imageStream.mark(Integer.MAX_VALUE); // Mark the beginning of the stream + + + if (isIcoByExtension || isIcoByMime) { + logger.info("Attempting to decode ICO logo with Apache Commons Imaging: {}", sourceIdentifier); + try { + image = Imaging.getBufferedImage(imageStream); + logger.info("Successfully decoded ICO logo with Apache Commons Imaging: {}", sourceIdentifier); + } catch (ImageReadException | IOException imagingEx) { + logger.warn("Apache Commons Imaging failed to decode logo ({}): {}. Attempting fallback to ImageIO.", sourceIdentifier, imagingEx.getMessage()); + imageStream.reset(); // Reset stream for ImageIO + try { + image = ImageIO.read(imageStream); + if (image != null) { + logger.info("Successfully decoded ICO logo with ImageIO fallback: {}", sourceIdentifier); + } else { + logger.warn("ImageIO fallback also failed to decode ICO logo: {}", sourceIdentifier); + } + } catch (IOException imageIoEx) { + logger.warn("ImageIO fallback failed with IOException for ICO logo ({}): {}", sourceIdentifier, imageIoEx.getMessage()); + } + } + } else { + logger.info("Attempting to decode non-ICO logo with ImageIO: {}", sourceIdentifier); + try { + image = ImageIO.read(imageStream); + if (image != null) { + logger.info("Successfully decoded non-ICO logo with ImageIO: {}", sourceIdentifier); + } else { + // This case is important: ImageIO.read can return null for unsupported formats without throwing an exception. + logger.warn("ImageIO.read returned null for logo (likely unsupported format or corrupt image): {}", sourceIdentifier); + } + } catch (IOException imageIoEx) { + logger.warn("ImageIO failed to decode non-ICO logo ({}) with IOException: {}", sourceIdentifier, imageIoEx.getMessage()); } } - } catch (IOException e) { - logger.error("Error reading logo image stream for {}: {}", sourceIdentifier, e.getMessage()); - } catch (Exception e) { + + if (image != null) { + extractDominantColorsFromImageInternal(image, colorFrequencies, sourceIdentifier, true); + } else { + logger.warn("Could not decode logo image from source (all attempts failed): {}", sourceIdentifier); + } + + } catch (IOException e) { // Catches IO errors from stream opening or readAllBytes + logger.error("IOException during logo image processing for {}: {}", sourceIdentifier, e.getMessage()); + } catch (Exception e) { // Catches other unexpected errors logger.error("An unexpected error occurred while processing logo image {}: {}", sourceIdentifier, e.getMessage(), e); + } finally { + if (imageStream != null) { + try { + imageStream.close(); + } catch (IOException e) { + logger.error("Failed to close image stream for {}: {}", sourceIdentifier, e.getMessage()); + } + } } } diff --git a/src/test/java/com/example/colorschemeidentifier/service/ColorExtractionServiceTest.java b/src/test/java/com/example/colorschemeidentifier/service/ColorExtractionServiceTest.java index 2847db3..567f8c7 100644 --- a/src/test/java/com/example/colorschemeidentifier/service/ColorExtractionServiceTest.java +++ b/src/test/java/com/example/colorschemeidentifier/service/ColorExtractionServiceTest.java @@ -588,7 +588,7 @@ void extractColorsFromUrl_withLogoProcessing() throws IOException { // Renamed to reflect it tests the internal algorithm if made accessible @Test void testExtractDominantColorsDirectlyInternal_logic() throws Exception { - BufferedImage image = new BufferedImage(3, 1, BufferedImage.TYPE_INT_ARGB); + BufferedImage image = new BufferedImage(3, 1, BufferedImage.TYPE_INT_ARGB); // A sample image image.setRGB(0, 0, new java.awt.Color(255, 0, 0).getRGB()); // Red image.setRGB(1, 0, new java.awt.Color(0, 255, 0).getRGB()); // Green image.setRGB(2, 0, new java.awt.Color(255, 0, 0).getRGB()); // Red @@ -628,6 +628,150 @@ void testExtractDominantColorsDirectlyInternal_logic() throws Exception { assertEquals("logo:logo_image.png", colorFrequencies.get("#20E020").getColorInfo().getSource()); } + // Helper method to create a dummy BufferedImage + private BufferedImage createDummyImage(int width, int height, int color) { + BufferedImage img = new BufferedImage(width, height, BufferedImage.TYPE_INT_RGB); + for (int x = 0; x < width; x++) { + for (int y = 0; y < height; y++) { + img.setRGB(x, y, color); + } + } + return img; + } + + private void setupMockDocumentForLogoUrl(Document mockDoc, String logoUrlToReturn) { + when(mockDoc.select("link[rel=icon], link[rel~=(?i)shortcut icon]")).thenAnswer(inv -> { + Elements mockIconLinks = new Elements(); + Element mockLinkElement = mock(Element.class); + when(mockLinkElement.absUrl("href")).thenReturn(logoUrlToReturn); + mockIconLinks.add(mockLinkElement); + return mockIconLinks; + }); + when(mockDoc.baseUri()).thenReturn(TEST_BASE_URL); + // Prevent other extractions by returning empty elements for other selectors + when(mockDoc.select(not(eq("link[rel=icon], link[rel~=(?i)shortcut icon]")))).thenReturn(new Elements()); + } + + @Test + void icoLogo_DecodedByCommonsImaging_Success() throws IOException { + String logoUrl = TEST_BASE_URL + "logo.ico"; + Document mockDoc = mock(Document.class); + setupMockDocumentForLogoUrl(mockDoc, logoUrl); + BufferedImage mockIcoImage = createDummyImage(1,1, java.awt.Color.GREEN.getRGB()); // Green -> #20E020 + + try (MockedStatic mockedJsoup = Mockito.mockStatic(Jsoup.class); + MockedStatic mockedImaging = Mockito.mockStatic(org.apache.commons.imaging.Imaging.class); + MockedStatic mockedImageIO = Mockito.mockStatic(ImageIO.class)) { + + Connection mockMainConnection = mock(Connection.class); + mockedJsoup.when(() -> Jsoup.connect(TEST_URL)).thenReturn(mockMainConnection); + when(mockMainConnection.timeout(anyInt())).thenReturn(mockMainConnection); + when(mockMainConnection.get()).thenReturn(mockDoc); + + mockedImaging.when(() -> org.apache.commons.imaging.Imaging.getBufferedImage(any(InputStream.class))) + .thenReturn(mockIcoImage); + mockedImageIO.when(() -> ImageIO.read(any(InputStream.class))).thenThrow(new AssertionError("ImageIO.read should not be called")); + + List result = colorExtractionService.extractColorsFromUrl(TEST_URL); + + assertEquals(1, result.size()); + assertEquals("#20E020", result.get(0).getHexValue()); // Quantized Green + assertTrue(result.get(0).isLogoColor()); + assertEquals("logo:" + logoUrl, result.get(0).getSource()); + mockedImaging.verify(() -> org.apache.commons.imaging.Imaging.getBufferedImage(any(InputStream.class)), times(1)); + mockedImageIO.verify(() -> ImageIO.read(any(InputStream.class)), never()); + } + } + + @Test + void icoLogo_CommonsImagingFails_ImageIOSucceeds() throws IOException { + String logoUrl = TEST_BASE_URL + "favicon.ico"; + Document mockDoc = mock(Document.class); + setupMockDocumentForLogoUrl(mockDoc, logoUrl); + BufferedImage mockFallbackImage = createDummyImage(1,1, java.awt.Color.RED.getRGB()); // Red -> #E02020 + + + try (MockedStatic mockedJsoup = Mockito.mockStatic(Jsoup.class); + MockedStatic mockedImaging = Mockito.mockStatic(org.apache.commons.imaging.Imaging.class); + MockedStatic mockedImageIO = Mockito.mockStatic(ImageIO.class)) { + + Connection mockMainConnection = mock(Connection.class); + mockedJsoup.when(() -> Jsoup.connect(TEST_URL)).thenReturn(mockMainConnection); + when(mockMainConnection.timeout(anyInt())).thenReturn(mockMainConnection); + when(mockMainConnection.get()).thenReturn(mockDoc); + + mockedImaging.when(() -> org.apache.commons.imaging.Imaging.getBufferedImage(any(InputStream.class))) + .thenThrow(new org.apache.commons.imaging.ImageReadException("Commons Imaging test error")); + mockedImageIO.when(() -> ImageIO.read(any(InputStream.class))) + .thenReturn(mockFallbackImage); + + List result = colorExtractionService.extractColorsFromUrl(TEST_URL); + + assertEquals(1, result.size()); + assertEquals("#E02020", result.get(0).getHexValue()); // Quantized Red + assertTrue(result.get(0).isLogoColor()); + mockedImaging.verify(() -> org.apache.commons.imaging.Imaging.getBufferedImage(any(InputStream.class)), times(1)); + mockedImageIO.verify(() -> ImageIO.read(any(InputStream.class)), times(1)); + } + } + + @Test + void icoLogo_CommonsImagingFails_ImageIOFails() throws IOException { + String logoUrl = TEST_BASE_URL + "another.ico"; + Document mockDoc = mock(Document.class); + setupMockDocumentForLogoUrl(mockDoc, logoUrl); + + try (MockedStatic mockedJsoup = Mockito.mockStatic(Jsoup.class); + MockedStatic mockedImaging = Mockito.mockStatic(org.apache.commons.imaging.Imaging.class); + MockedStatic mockedImageIO = Mockito.mockStatic(ImageIO.class)) { + + Connection mockMainConnection = mock(Connection.class); + mockedJsoup.when(() -> Jsoup.connect(TEST_URL)).thenReturn(mockMainConnection); + when(mockMainConnection.timeout(anyInt())).thenReturn(mockMainConnection); + when(mockMainConnection.get()).thenReturn(mockDoc); + + mockedImaging.when(() -> org.apache.commons.imaging.Imaging.getBufferedImage(any(InputStream.class))) + .thenThrow(new org.apache.commons.imaging.ImageReadException("Commons Imaging test error")); + mockedImageIO.when(() -> ImageIO.read(any(InputStream.class))) + .thenReturn(null); // ImageIO.read returns null for failure + + List result = colorExtractionService.extractColorsFromUrl(TEST_URL); + + assertTrue(result.isEmpty(), "No colors should be extracted if logo decoding fails completely."); + mockedImaging.verify(() -> org.apache.commons.imaging.Imaging.getBufferedImage(any(InputStream.class)), times(1)); + mockedImageIO.verify(() -> ImageIO.read(any(InputStream.class)), times(1)); + } + } + + @Test + void pngLogo_DecodedByImageIO_CommonsImagingSkipped() throws IOException { + String logoUrl = TEST_BASE_URL + "logo.png"; // Non-ICO + Document mockDoc = mock(Document.class); + setupMockDocumentForLogoUrl(mockDoc, logoUrl); + BufferedImage mockPngImage = createDummyImage(1,1, java.awt.Color.CYAN.getRGB()); // Cyan -> #20E0E0 + + try (MockedStatic mockedJsoup = Mockito.mockStatic(Jsoup.class); + MockedStatic mockedImaging = Mockito.mockStatic(org.apache.commons.imaging.Imaging.class); + MockedStatic mockedImageIO = Mockito.mockStatic(ImageIO.class)) { + + Connection mockMainConnection = mock(Connection.class); + mockedJsoup.when(() -> Jsoup.connect(TEST_URL)).thenReturn(mockMainConnection); + when(mockMainConnection.timeout(anyInt())).thenReturn(mockMainConnection); + when(mockMainConnection.get()).thenReturn(mockDoc); + + mockedImageIO.when(() -> ImageIO.read(any(InputStream.class))) + .thenReturn(mockPngImage); + + List result = colorExtractionService.extractColorsFromUrl(TEST_URL); + + assertEquals(1, result.size()); + assertEquals("#20E0E0", result.get(0).getHexValue()); // Quantized Cyan + assertTrue(result.get(0).isLogoColor()); + // Commons Imaging should not be called for a .png + mockedImaging.verify(() -> org.apache.commons.imaging.Imaging.getBufferedImage(any(InputStream.class)), never()); + mockedImageIO.verify(() -> ImageIO.read(any(InputStream.class)), times(1)); + } + } @Test void testFrequencyCountingAndTopN() throws IOException {