Archived
A tab icon you can actually see, and photo uploads that accept a photo
build-and-publish / build (pull_request) Successful in 2m14s
build-and-publish / build (pull_request) Successful in 2m14s
Two unrelated things the bakery hit on the same afternoon. THE FAVICON was not missing, it was unusable. The head pointed rel=icon at the 1000x1000 logo PNGs, so a browser fetched 71 KB to paint 16 square pixels, and the mark is a vine branch drawn in hairlines -- strokes thinner than one pixel at that size -- which arrives as a grey smudge. Replaced with a real icon set built from one leaf of that branch, filled rather than stroked, because at 16px a silhouette survives and an outline does not. A midrib was drawn first and cut the leaf into two pale slivers at tab size, so it went; the tilt, the two points and the stem carry the shape. The SVG answers prefers-color-scheme itself, which a .ico cannot, so the dark tab strip gets sage on bakery-900 instead of a glowing cream tile. The .ico is listed first on purpose: a browser takes the last format it understands, so reversing the two would hand Chrome the bitmap. PHOTO UPLOADS failed on anything over 1 MB, which is every photo a phone takes. The cause was an absence: nothing configured spring.servlet.multipart, so Boot's 1 MB default applied and the container rejected the file with FileSizeLimitExceededException before it reached ProductPhotoService -- the class whose entire job is turning "whatever came off a phone" into a resized webp. The pipeline could never run on the input it was written for. Now 15 MB a file and 60 MB a request, the latter because the file input is `multiple`. The failure was also ugly, and that is fixed separately: parsed eagerly, an over-sized part throws from inside Tomcat's parameter parsing where no @ExceptionHandler can reach it, so the request died as a 500 and then died again forwarding to /error, because that forward re-parsed the same too-large request (the paired "Exception Processing [ErrorPage...]" lines in the log). resolve-lazily moves the throw into argument binding, where AdminController now catches it and returns the same `problem` flash the domain's other refusals use. max-swallow-size lets the body be discarded so the browser receives that redirect rather than a connection reset. The multipart numbers are asserted rather than trusted, because a default that was never set is exactly the kind of thing that comes back silently. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
This commit is contained in:
@@ -9,9 +9,14 @@ import org.springframework.web.bind.annotation.GetMapping;
|
|||||||
import org.springframework.web.bind.annotation.PathVariable;
|
import org.springframework.web.bind.annotation.PathVariable;
|
||||||
import org.springframework.web.bind.annotation.PostMapping;
|
import org.springframework.web.bind.annotation.PostMapping;
|
||||||
import org.springframework.web.bind.annotation.RequestMapping;
|
import org.springframework.web.bind.annotation.RequestMapping;
|
||||||
|
import org.springframework.web.bind.annotation.ExceptionHandler;
|
||||||
import org.springframework.web.bind.annotation.RequestParam;
|
import org.springframework.web.bind.annotation.RequestParam;
|
||||||
|
import org.springframework.web.multipart.MaxUploadSizeExceededException;
|
||||||
import org.springframework.web.multipart.MultipartFile;
|
import org.springframework.web.multipart.MultipartFile;
|
||||||
import org.springframework.web.servlet.mvc.support.RedirectAttributes;
|
import org.springframework.web.servlet.mvc.support.RedirectAttributes;
|
||||||
|
import org.springframework.web.servlet.support.RequestContextUtils;
|
||||||
|
|
||||||
|
import jakarta.servlet.http.HttpServletRequest;
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* The catalogue, editable by the person who bakes it — as pages and form posts.
|
* The catalogue, editable by the person who bakes it — as pages and form posts.
|
||||||
@@ -170,4 +175,25 @@ public class AdminController {
|
|||||||
}
|
}
|
||||||
return "redirect:/admin";
|
return "redirect:/admin";
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A photo bigger than the configured limit, said in a sentence instead of a stack trace.
|
||||||
|
*
|
||||||
|
* <p>This is the same {@code problem} flash the refusals above use, so the editor reads it in the
|
||||||
|
* same place on the same page. Before it existed, an over-sized file escaped as the container's own
|
||||||
|
* parsing error: a 500, and then a second failure forwarding to {@code /error}, because that forward
|
||||||
|
* re-parsed the same too-large request. Reaching this handler at all depends on {@code
|
||||||
|
* spring.servlet.multipart.resolve-lazily} — parsed eagerly, the throw happens before any handler
|
||||||
|
* method is chosen and there is nothing here to catch it.
|
||||||
|
*
|
||||||
|
* <p>The flash map is written directly rather than through {@code RedirectAttributes}, which is not
|
||||||
|
* an argument Spring supplies to an {@code @ExceptionHandler}.
|
||||||
|
*/
|
||||||
|
@ExceptionHandler(MaxUploadSizeExceededException.class)
|
||||||
|
public String photoTooBig(HttpServletRequest request) {
|
||||||
|
RequestContextUtils.getOutputFlashMap(request).put("problem",
|
||||||
|
"That photo is too large. Anything up to 15 MB is fine — a photo straight off a phone "
|
||||||
|
+ "normally is — and we resize it here, so there is no need to shrink it first.");
|
||||||
|
return "redirect:/admin";
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -11,6 +11,23 @@ spring:
|
|||||||
open-in-view: false
|
open-in-view: false
|
||||||
flyway:
|
flyway:
|
||||||
enabled: true
|
enabled: true
|
||||||
|
servlet:
|
||||||
|
multipart:
|
||||||
|
# THIS BLOCK IS THE WHOLE REASON PHOTO UPLOADS FAILED. Unset, Boot defaults to a 1 MB max-file-size,
|
||||||
|
# and a photo off a phone is 3-12 MB — so every real upload died with FileSizeLimitExceededException
|
||||||
|
# before it reached ProductPhotoService, which exists precisely to resize "whatever came off a phone".
|
||||||
|
# The resizing pipeline could never run on the input it was written for.
|
||||||
|
max-file-size: 15MB
|
||||||
|
# The file input is `multiple`, so one submit can carry several photos; this bounds the whole request
|
||||||
|
# rather than each part. Four full-size photos at once is a realistic morning's worth of new stock.
|
||||||
|
max-request-size: 60MB
|
||||||
|
# Parse when the controller asks for the files, not while Tomcat is reading parameters. Eagerly, an
|
||||||
|
# oversize part throws from inside the container's parameter parsing, which no @ExceptionHandler can
|
||||||
|
# reach — the request dies as a 500 and then the forward to /error re-parses and throws again (the
|
||||||
|
# "Exception Processing [ErrorPage...]" pairs in the log). Lazily, it surfaces as a
|
||||||
|
# MaxUploadSizeExceededException during argument binding, where AdminController can catch it.
|
||||||
|
resolve-lazily: true
|
||||||
|
|
||||||
mail:
|
mail:
|
||||||
host: ${SMTP_SERVER:localhost}
|
host: ${SMTP_SERVER:localhost}
|
||||||
port: ${SMTP_PORT:25}
|
port: ${SMTP_PORT:25}
|
||||||
@@ -29,6 +46,13 @@ spring:
|
|||||||
ssl:
|
ssl:
|
||||||
trust: ${SMTP_SERVER:localhost}
|
trust: ${SMTP_SERVER:localhost}
|
||||||
|
|
||||||
|
server:
|
||||||
|
tomcat:
|
||||||
|
# Read and discard the rest of an over-sized body instead of resetting the connection, so the browser
|
||||||
|
# actually receives the redirect and the message rather than "connection reset". Only reachable now for
|
||||||
|
# a genuinely enormous file, but that is exactly when a clear answer matters.
|
||||||
|
max-swallow-size: -1
|
||||||
|
|
||||||
platform:
|
platform:
|
||||||
web:
|
web:
|
||||||
spa:
|
spa:
|
||||||
|
|||||||
Binary file not shown.
|
After Width: | Height: | Size: 4.8 KiB |
Binary file not shown.
|
After Width: | Height: | Size: 15 KiB |
@@ -0,0 +1,37 @@
|
|||||||
|
<svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 64 64" role="img" aria-label="The Vine">
|
||||||
|
<!--
|
||||||
|
ONE LEAF, NOT THE LOGO. The wordmark's mark is a six-leaf vine branch drawn in hairlines, and at the
|
||||||
|
16px a browser tab actually paints it collapses into a grey smudge, because the strokes are thinner
|
||||||
|
than a pixel. So this takes the single recognisable unit of that branch, a lanceolate leaf, and draws
|
||||||
|
it filled rather than stroked: at this size a silhouette survives and an outline does not.
|
||||||
|
|
||||||
|
NO MIDRIB, deliberately. A vein was drawn first and rendered at 16px it split the leaf into two pale
|
||||||
|
slivers, which is the same legibility problem the outline had. It looked better at 32px and worse
|
||||||
|
where it counts, so it went. What identifies the shape is the two pointed ends, the tilt and the stem.
|
||||||
|
|
||||||
|
Sage on cream, both from the site's own palette (bakery-600 on bakery-50). An SVG favicon can answer
|
||||||
|
the browser's colour scheme, which a .ico cannot, so the dark variant swaps to bakery-400 on
|
||||||
|
bakery-900 rather than leaving a cream tile glowing in a dark tab strip.
|
||||||
|
-->
|
||||||
|
<style>
|
||||||
|
.ground { fill: #faf7f0; }
|
||||||
|
.leaf { fill: #5f6f52; }
|
||||||
|
.stem { stroke: #5f6f52; }
|
||||||
|
@media (prefers-color-scheme: dark) {
|
||||||
|
.ground { fill: #232b1e; }
|
||||||
|
.leaf { fill: #a2ae8b; }
|
||||||
|
.stem { stroke: #a2ae8b; }
|
||||||
|
}
|
||||||
|
</style>
|
||||||
|
|
||||||
|
<rect class="ground" width="64" height="64" rx="14"/>
|
||||||
|
|
||||||
|
<!-- Tilted so it reads as growing rather than floating. The stem's tip lands inside the tile after the
|
||||||
|
rotation: at -30 degrees it sits at roughly (46, 56) of 64. -->
|
||||||
|
<g transform="rotate(-30 32 32)">
|
||||||
|
<!-- Pointed at both ends: two mirrored curves from tip to tip, which is what makes it a leaf and
|
||||||
|
not an eye. -->
|
||||||
|
<path class="leaf" d="M32 7 C47 22 47 41 32 53 C17 41 17 22 32 7 Z"/>
|
||||||
|
<path class="stem" fill="none" stroke-width="3.4" stroke-linecap="round" d="M32 53 L32 60"/>
|
||||||
|
</g>
|
||||||
|
</svg>
|
||||||
|
After Width: | Height: | Size: 1.9 KiB |
@@ -13,8 +13,19 @@
|
|||||||
<meta charset="UTF-8">
|
<meta charset="UTF-8">
|
||||||
<meta name="viewport" content="width=device-width, initial-scale=1.0">
|
<meta name="viewport" content="width=device-width, initial-scale=1.0">
|
||||||
|
|
||||||
<link rel="icon" media="(prefers-color-scheme: light)" href="/images/resources/logo_L.png">
|
<!--/*
|
||||||
<link rel="icon" media="(prefers-color-scheme: dark)" href="/images/resources/logo_dark.png">
|
These pointed at the 1000x1000 logo PNGs, which is why the tab looked empty: 71 KB downloaded to
|
||||||
|
paint 16 square pixels, and the mark is a hairline vine branch whose strokes are thinner than one
|
||||||
|
pixel at that size, so it arrived as a grey smudge. favicon.svg is one leaf from that branch, filled
|
||||||
|
rather than stroked, and it answers prefers-color-scheme itself — so no media queries here.
|
||||||
|
|
||||||
|
Three lines is the whole modern set. The .ico is only for browsers that will not take an SVG icon
|
||||||
|
(Safari), and it is listed FIRST because a browser takes the last format it understands: reverse
|
||||||
|
these two and Chrome would settle for the bitmap.
|
||||||
|
*/-->
|
||||||
|
<link rel="icon" href="/favicon.ico" sizes="32x32">
|
||||||
|
<link rel="icon" href="/favicon.svg" type="image/svg+xml">
|
||||||
|
<link rel="apple-touch-icon" href="/apple-touch-icon.png">
|
||||||
|
|
||||||
<!--/* Open the connection to the photo bucket while the head is still parsing. */-->
|
<!--/* Open the connection to the photo bucket while the head is still parsing. */-->
|
||||||
<link rel="preconnect" th:href="${assetOrigin}" crossorigin>
|
<link rel="preconnect" th:href="${assetOrigin}" crossorigin>
|
||||||
|
|||||||
@@ -1,5 +1,6 @@
|
|||||||
package com.itsthevine.web;
|
package com.itsthevine.web;
|
||||||
|
|
||||||
|
import static org.assertj.core.api.Assertions.assertThat;
|
||||||
import static org.hamcrest.Matchers.containsString;
|
import static org.hamcrest.Matchers.containsString;
|
||||||
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
|
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.csrf;
|
||||||
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user;
|
import static org.springframework.security.test.web.servlet.request.SecurityMockMvcRequestPostProcessors.user;
|
||||||
@@ -13,7 +14,9 @@ import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.
|
|||||||
import org.junit.jupiter.api.BeforeEach;
|
import org.junit.jupiter.api.BeforeEach;
|
||||||
import org.junit.jupiter.api.Test;
|
import org.junit.jupiter.api.Test;
|
||||||
import org.springframework.beans.factory.annotation.Autowired;
|
import org.springframework.beans.factory.annotation.Autowired;
|
||||||
|
import org.springframework.boot.servlet.autoconfigure.MultipartProperties;
|
||||||
import org.springframework.boot.test.context.SpringBootTest;
|
import org.springframework.boot.test.context.SpringBootTest;
|
||||||
|
import org.springframework.util.unit.DataSize;
|
||||||
import org.springframework.boot.testcontainers.service.connection.ServiceConnection;
|
import org.springframework.boot.testcontainers.service.connection.ServiceConnection;
|
||||||
import org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers;
|
import org.springframework.security.test.web.servlet.setup.SecurityMockMvcConfigurers;
|
||||||
import org.springframework.test.web.servlet.MockMvc;
|
import org.springframework.test.web.servlet.MockMvc;
|
||||||
@@ -79,6 +82,25 @@ class AdminPagesTest {
|
|||||||
.build();
|
.build();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The bug this guards was an ABSENCE: nothing configured multipart, so Boot's 1 MB default applied and
|
||||||
|
* every photo off a phone was rejected by the container before {@code ProductPhotoService} — the class
|
||||||
|
* whose whole job is resizing phone photos — could see it. A default is exactly the kind of thing that
|
||||||
|
* comes back silently, so the numbers are asserted rather than trusted.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void photosOffAPhoneFitInsideTheUploadLimits() {
|
||||||
|
MultipartProperties multipart = context.getBean(MultipartProperties.class);
|
||||||
|
|
||||||
|
assertThat(multipart.getMaxFileSize()).isEqualTo(DataSize.ofMegabytes(15));
|
||||||
|
assertThat(multipart.getMaxRequestSize()).isEqualTo(DataSize.ofMegabytes(60));
|
||||||
|
// Without this the throw happens inside the container's parameter parsing, where no
|
||||||
|
// @ExceptionHandler can reach it — which is what made an over-sized photo a 500.
|
||||||
|
assertThat(multipart.isResolveLazily()).isTrue();
|
||||||
|
// The default is 1 MB. If this ever passes, the fix has been undone.
|
||||||
|
assertThat(multipart.getMaxFileSize()).isNotEqualTo(DataSize.ofMegabytes(1));
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void theCatalogueScreenShowsWhatIsOnThePageWithItsPhotos() throws Exception {
|
void theCatalogueScreenShowsWhatIsOnThePageWithItsPhotos() throws Exception {
|
||||||
mvc.perform(get("/admin").with(user("morissa")))
|
mvc.perform(get("/admin").with(user("morissa")))
|
||||||
|
|||||||
@@ -0,0 +1,56 @@
|
|||||||
|
package com.itsthevine.web;
|
||||||
|
|
||||||
|
import static org.hamcrest.Matchers.containsString;
|
||||||
|
import static org.mockito.ArgumentMatchers.any;
|
||||||
|
import static org.mockito.ArgumentMatchers.eq;
|
||||||
|
import static org.mockito.Mockito.doThrow;
|
||||||
|
import static org.mockito.Mockito.mock;
|
||||||
|
import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.multipart;
|
||||||
|
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.flash;
|
||||||
|
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.redirectedUrl;
|
||||||
|
import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;
|
||||||
|
|
||||||
|
import org.junit.jupiter.api.Test;
|
||||||
|
import org.springframework.mock.web.MockMultipartFile;
|
||||||
|
import org.springframework.test.web.servlet.MockMvc;
|
||||||
|
import org.springframework.test.web.servlet.setup.MockMvcBuilders;
|
||||||
|
import org.springframework.web.multipart.MaxUploadSizeExceededException;
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A photo too big for the limit comes back as a sentence, not a stack trace.
|
||||||
|
*
|
||||||
|
* <p>Photo uploads used to fail at 1 MB — no multipart limits were configured, so Boot's default applied
|
||||||
|
* (see {@code AdminPagesTest#photosOffAPhoneFitInsideTheUploadLimits}, which pins the numbers). Raising
|
||||||
|
* them fixes the everyday case; this covers what the editor sees when a file really is too large, because
|
||||||
|
* what happened before was a 500 followed by a second failure forwarding to {@code /error} — that forward
|
||||||
|
* re-parsed the same over-sized request and threw again.
|
||||||
|
*
|
||||||
|
* <p>Standalone rather than a booted context, and the throw is staged from the service rather than from a
|
||||||
|
* genuinely huge upload, because MockMvc does not enforce the container's multipart limits — there is no
|
||||||
|
* way to provoke the real parse failure here. What that leaves worth asserting is the wiring: that the
|
||||||
|
* handler catches this exception type, writes the same {@code problem} flash the other refusals use, and
|
||||||
|
* redirects instead of rendering an error page. Whether the exception can reach a handler at all is a
|
||||||
|
* property of {@code resolve-lazily}, which is asserted separately.
|
||||||
|
*/
|
||||||
|
class AdminUploadRefusalTest {
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void anOversizePhotoIsRefusedOnThePageRatherThanAsAStackTrace() throws Exception {
|
||||||
|
Catalogue catalogue = mock(Catalogue.class);
|
||||||
|
doThrow(new MaxUploadSizeExceededException(15_728_640L))
|
||||||
|
.when(catalogue).addPhotos(eq(7L), any());
|
||||||
|
|
||||||
|
MockMvc mvc = MockMvcBuilders.standaloneSetup(new AdminController(catalogue)).build();
|
||||||
|
|
||||||
|
mvc.perform(multipart("/admin/items/7/photos")
|
||||||
|
.file(new MockMultipartFile("photos", "cake.jpg", "image/jpeg", new byte[] {1, 2, 3})))
|
||||||
|
.andExpect(status().is3xxRedirection())
|
||||||
|
.andExpect(redirectedUrl("/admin"))
|
||||||
|
// The same flash key the domain's own refusals use, so it lands in the same place on the
|
||||||
|
// page, and it names the limit rather than saying "invalid".
|
||||||
|
.andExpect(flash().attribute("problem", containsString("too large")))
|
||||||
|
.andExpect(flash().attribute("problem", containsString("15 MB")))
|
||||||
|
// It should not tell the baker to go and shrink the photo: resizing is this app's job.
|
||||||
|
.andExpect(flash().attribute("problem", containsString("resize it here")));
|
||||||
|
}
|
||||||
|
}
|
||||||
@@ -60,6 +60,34 @@ class SiteControllerTest {
|
|||||||
mvc = MockMvcBuilders.webAppContextSetup(context).build();
|
mvc = MockMvcBuilders.webAppContextSetup(context).build();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* The tab icon used to be the 1000x1000 logo PNG, which is 71 KB fetched to paint 16 pixels and, being
|
||||||
|
* a hairline drawing, arrived as a grey smudge at that size. The order matters and is the reason this
|
||||||
|
* asserts position: a browser takes the LAST icon format it understands, so the .ico has to come first
|
||||||
|
* or Chrome settles for the bitmap instead of the SVG.
|
||||||
|
*/
|
||||||
|
@Test
|
||||||
|
void theTabIconIsAnIconRatherThanTheFullLogo() throws Exception {
|
||||||
|
String head = mvc.perform(get("/")).andExpect(status().isOk())
|
||||||
|
.andReturn().getResponse().getContentAsString();
|
||||||
|
|
||||||
|
assertThat(head).contains("<link rel=\"icon\" href=\"/favicon.ico\" sizes=\"32x32\">");
|
||||||
|
assertThat(head).contains("<link rel=\"icon\" href=\"/favicon.svg\" type=\"image/svg+xml\">");
|
||||||
|
assertThat(head).contains("<link rel=\"apple-touch-icon\" href=\"/apple-touch-icon.png\">");
|
||||||
|
assertThat(head.indexOf("/favicon.ico")).isLessThan(head.indexOf("/favicon.svg"));
|
||||||
|
// The 1000x1000 logos are no longer offered as icons anywhere.
|
||||||
|
assertThat(head).doesNotContain("rel=\"icon\" media=");
|
||||||
|
assertThat(head).doesNotContain("logo_dark.png");
|
||||||
|
}
|
||||||
|
|
||||||
|
@Test
|
||||||
|
void theIconFilesAreActuallyServed() throws Exception {
|
||||||
|
// A link to a 404 is worse than no link: the browser shows its default and caches the miss.
|
||||||
|
for (String icon : new String[] {"/favicon.svg", "/favicon.ico", "/apple-touch-icon.png"}) {
|
||||||
|
mvc.perform(get(icon)).andExpect(status().isOk());
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
void everyPageStatesItsOwnTitleAndDescription() throws Exception {
|
void everyPageStatesItsOwnTitleAndDescription() throws Exception {
|
||||||
// One generic shell for every page was the SPA's problem, and the reason a controller used to
|
// One generic shell for every page was the SPA's problem, and the reason a controller used to
|
||||||
|
|||||||
Reference in New Issue
Block a user