Browse Source

fix(inventory): malformed rgba no longer bricks the Filaments page (#1055)

  A single legacy spool with a 7-char rgba ('FFFFFFF', missing one F)
  caused GET /api/v1/inventory/spools to 500 with a pydantic
  ResponseValidationError, leaving the reporter with a blank Filaments
  page and "Add Spool" silently failing. Root cause spans three layers:

  1. Write path: SpoolUpdate.rgba had no pattern constraint (only
     SpoolCreate did), so PATCH could plant malformed values in the DB.
  2. Frontend: ColorSection hex input's `val.length <= 6 ? 'FF' : ''`
     emitted 7-char rgba for 5-char input (XXXXX + FF = 7) and for
     7-char typed input (no alpha appended).
  3. Read path: SpoolResponse inherited the write-side pattern, so a
     single bad row 500'd the entire list endpoint instead of being
     tolerated through serialize.

  SpoolUpdate.rgba now carries the same ^[0-9A-Fa-f]{8}$ pattern as
  SpoolCreate. The hex input emits a fully-formed 8-char RRGGBBAA on
  every keystroke — 8-char paste passes through, 7-char drops the
  stray, shorter input pads RGB with '0' and appends FF alpha.
  SpoolResponse.rgba is now Optional[str] with no pattern — write-side
  validation is the right place for format rules; responses must
  tolerate historical rows.

  Tests: 16 schema tests (SpoolCreate/Update reject, SpoolResponse
  tolerate), 7 frontend tests covering every input length 0–8 plus
  non-hex strip. A user who already has a bad row in their DB now sees
  it render with a default color instead of having to hand-edit SQLite.
maziggy 1 month ago
parent
commit
bf5135cb12

File diff suppressed because it is too large
+ 0 - 0
CHANGELOG.md


+ 6 - 1
backend/app/schemas/spool.py

@@ -41,7 +41,7 @@ class SpoolUpdate(BaseModel):
     material: str | None = None
     subtype: str | None = None
     color_name: str | None = None
-    rgba: str | None = None
+    rgba: str | None = Field(None, pattern=r"^[0-9A-Fa-f]{8}$")
     brand: str | None = None
     label_weight: int | None = None
     core_weight: int | None = None
@@ -82,6 +82,11 @@ class SpoolKProfileResponse(SpoolKProfileBase):
 
 class SpoolResponse(SpoolBase):
     id: int
+    # rgba is intentionally unconstrained on the response side: the write paths
+    # (SpoolCreate, SpoolUpdate) enforce the 8-char hex pattern, but legacy rows
+    # or data sourced from AMS firmware / backups may carry malformed values.
+    # A single bad row must not 500 the entire inventory list endpoint (#1055).
+    rgba: str | None = None
     added_full: bool | None = None
     last_used: datetime | None = None
     encode_time: datetime | None = None

+ 131 - 0
backend/tests/unit/test_spool_schemas_rgba.py

@@ -0,0 +1,131 @@
+"""Schema validation tests for the spool rgba field (#1055).
+
+Three guarantees to lock in:
+1. SpoolCreate and SpoolUpdate must reject malformed rgba (short, long, non-hex)
+   on the write path — this is the "add a check" the reporter asked for.
+2. SpoolResponse must NOT validate rgba on the read path: a single legacy row
+   with a 7-char rgba (as in #1055) must not 500 the entire inventory list.
+3. Valid 8-char hex must continue to round-trip through all three schemas.
+"""
+
+import pytest
+from pydantic import ValidationError
+
+from backend.app.schemas.spool import SpoolCreate, SpoolUpdate
+
+
+class TestSpoolCreateRgbaValidation:
+    """Write-path validation on the create schema."""
+
+    def test_accepts_valid_8char_hex(self):
+        spool = SpoolCreate(material="PLA", rgba="FF00AAFF")
+        assert spool.rgba == "FF00AAFF"
+
+    def test_accepts_lowercase_hex(self):
+        spool = SpoolCreate(material="PLA", rgba="ff00aaff")
+        assert spool.rgba == "ff00aaff"
+
+    def test_accepts_null_rgba(self):
+        spool = SpoolCreate(material="PLA", rgba=None)
+        assert spool.rgba is None
+
+    def test_rejects_7char_rgba(self):
+        """#1055 repro: a 7-char 'FFFFFFF' must not be acceptable on create."""
+        with pytest.raises(ValidationError, match="rgba"):
+            SpoolCreate(material="PLA", rgba="FFFFFFF")
+
+    def test_rejects_6char_rgba(self):
+        """Plain RRGGBB without alpha must be rejected — frontend appends FF."""
+        with pytest.raises(ValidationError, match="rgba"):
+            SpoolCreate(material="PLA", rgba="FF0000")
+
+    def test_rejects_non_hex_char(self):
+        with pytest.raises(ValidationError, match="rgba"):
+            SpoolCreate(material="PLA", rgba="FFZZ00FF")
+
+
+class TestSpoolUpdateRgbaValidation:
+    """Write-path validation on the update schema — the gap that let #1055 happen.
+
+    Before the fix, SpoolUpdate.rgba was a bare `str | None` so a PATCH could
+    plant a 7-char value straight into the DB. That row then caused a 500 on
+    the next GET because SpoolResponse enforced the pattern at serialize time.
+    """
+
+    def test_accepts_valid_8char_hex(self):
+        update = SpoolUpdate(rgba="00FF00FF")
+        assert update.rgba == "00FF00FF"
+
+    def test_accepts_null_rgba(self):
+        update = SpoolUpdate(rgba=None)
+        assert update.rgba is None
+
+    def test_accepts_missing_rgba(self):
+        """Partial updates — rgba not present in payload — must still be valid."""
+        update = SpoolUpdate(material="PETG")
+        assert update.rgba is None
+
+    def test_rejects_7char_rgba(self):
+        """#1055 repro: PATCH must reject the exact pattern that bricked the reporter."""
+        with pytest.raises(ValidationError, match="rgba"):
+            SpoolUpdate(rgba="FFFFFFF")
+
+    def test_rejects_9char_rgba(self):
+        with pytest.raises(ValidationError, match="rgba"):
+            SpoolUpdate(rgba="FFFFFFFFF")
+
+    def test_rejects_non_hex_char(self):
+        with pytest.raises(ValidationError, match="rgba"):
+            SpoolUpdate(rgba="FFGG00FF")
+
+
+class TestSpoolResponseRgbaLeniency:
+    """Read-path leniency — a legacy bad row must never 500 the list endpoint.
+
+    Before the fix, SpoolResponse inherited the pattern from SpoolBase so a
+    single 7-char rgba in the DB blew up the whole inventory listing. The
+    response schema now treats rgba as an unconstrained Optional[str] — write
+    validation is where the pattern belongs; responses must tolerate whatever
+    is already persisted.
+    """
+
+    # SpoolResponse requires id + timestamps so it's easier to test via a
+    # minimal dict payload than by constructing a full instance.
+    @staticmethod
+    def _make_response_kwargs(**overrides):
+        from datetime import datetime
+
+        base = {
+            "id": 1,
+            "material": "PLA",
+            "created_at": datetime.fromisoformat("2026-01-01T00:00:00"),
+            "updated_at": datetime.fromisoformat("2026-01-01T00:00:00"),
+        }
+        base.update(overrides)
+        return base
+
+    def test_tolerates_7char_rgba_on_serialize(self):
+        """This is the #1055 bug fixed: malformed legacy rgba must serialize cleanly."""
+        from backend.app.schemas.spool import SpoolResponse
+
+        response = SpoolResponse(**self._make_response_kwargs(rgba="FFFFFFF"))
+        assert response.rgba == "FFFFFFF"
+
+    def test_tolerates_null_rgba(self):
+        from backend.app.schemas.spool import SpoolResponse
+
+        response = SpoolResponse(**self._make_response_kwargs(rgba=None))
+        assert response.rgba is None
+
+    def test_tolerates_non_hex_rgba(self):
+        """Even completely garbage rgba shouldn't crash the endpoint."""
+        from backend.app.schemas.spool import SpoolResponse
+
+        response = SpoolResponse(**self._make_response_kwargs(rgba="not-hex-at-all"))
+        assert response.rgba == "not-hex-at-all"
+
+    def test_passes_valid_rgba_through(self):
+        from backend.app.schemas.spool import SpoolResponse
+
+        response = SpoolResponse(**self._make_response_kwargs(rgba="FF00AAFF"))
+        assert response.rgba == "FF00AAFF"

+ 67 - 0
frontend/src/__tests__/components/SpoolFormModal.test.tsx

@@ -348,6 +348,73 @@ describe('SpoolFormModal weightTouched', () => {
     expect(payload).toHaveProperty('cost_per_kg', null);
   });
 
+  it('normalizes a malformed legacy rgba on edit-form load so PATCH is not rejected (#1055)', async () => {
+    // #1055 regression guard: a spool with a legacy 7-char rgba (e.g. 'FFFFFFF')
+    // was editable in the UI but any save 422'd because SpoolUpdate now enforces
+    // the 8-char pattern. The form must sanitize the loaded value to a valid
+    // default so users can edit unrelated fields without being forced to fix
+    // a color they may not even have noticed was broken.
+    const spoolWithBadRgba: InventorySpool = {
+      ...existingSpool,
+      rgba: 'FFFFFFF', // 7 chars — the exact #1055 trigger pattern
+    };
+
+    render(
+      <SpoolFormModal
+        isOpen={true}
+        onClose={vi.fn()}
+        spool={spoolWithBadRgba}
+        currencySymbol="$"
+      />
+    );
+
+    await waitFor(() => {
+      expect(screen.getByText('Edit Spool')).toBeInTheDocument();
+    });
+
+    const saveButton = screen.getByRole('button', { name: /save/i });
+    fireEvent.click(saveButton);
+
+    await waitFor(() => {
+      expect(api.updateSpool).toHaveBeenCalledTimes(1);
+    });
+
+    const [, payload] = vi.mocked(api.updateSpool).mock.calls[0];
+    // The PATCH payload must carry a valid 8-char rgba — never the raw 7-char
+    // value loaded from the stale DB row.
+    expect(payload).toHaveProperty('rgba');
+    expect(typeof (payload as { rgba: unknown }).rgba).toBe('string');
+    expect((payload as { rgba: string }).rgba).toMatch(/^[0-9A-Fa-f]{8}$/);
+  });
+
+  it('preserves a valid existing rgba on edit (no forced default)', async () => {
+    // Sanity: the normalization only kicks in for malformed values. A valid
+    // 8-char rgba must round-trip untouched so untouched edits don't quietly
+    // reset a user's chosen color.
+    render(
+      <SpoolFormModal
+        isOpen={true}
+        onClose={vi.fn()}
+        spool={existingSpool} // rgba = 'FF0000FF' (valid)
+        currencySymbol="$"
+      />
+    );
+
+    await waitFor(() => {
+      expect(screen.getByText('Edit Spool')).toBeInTheDocument();
+    });
+
+    const saveButton = screen.getByRole('button', { name: /save/i });
+    fireEvent.click(saveButton);
+
+    await waitFor(() => {
+      expect(api.updateSpool).toHaveBeenCalledTimes(1);
+    });
+
+    const [, payload] = vi.mocked(api.updateSpool).mock.calls[0];
+    expect((payload as { rgba: string }).rgba).toBe('FF0000FF');
+  });
+
   it('displays correct catalog name when duplicates exist', async () => {
     const spoolWithCatalogId: InventorySpool = {
       ...existingSpool,

+ 119 - 0
frontend/src/__tests__/components/spool-form/ColorSectionHexInput.test.tsx

@@ -0,0 +1,119 @@
+/**
+ * Regression tests for the ColorSection hex input normalization (#1055).
+ *
+ * The original bug: typing 5 hex chars on the RRGGBB field produced a 7-char
+ * rgba ("FFFFF" + "FF" alpha = 7 chars); typing 7 chars left the 7-char string
+ * unpadded. Either way the value passed frontend validation, survived a backend
+ * PATCH (SpoolUpdate had no pattern constraint), and then bricked the entire
+ * Filaments page because SpoolResponse enforced the 8-char pattern on serialize
+ * and one bad row 500'd the whole list endpoint.
+ *
+ * The input now emits a valid 8-char RRGGBBAA on every keystroke: shorter input
+ * is right-padded with '0' and given FF alpha; 7-char input drops the stray 7th
+ * char; 8-char paste passes through unchanged.
+ *
+ * These tests drive the onChange handler directly (via fireEvent.change) rather
+ * than userEvent.type so each assertion exercises a specific input length. The
+ * component itself is a controlled input whose displayed value derives from
+ * formData.rgba.substring(0, 6), so the real-world UX of typing one char at a
+ * time is quirkier than the handler contract — but the handler contract is
+ * what this regression guards.
+ */
+
+import { describe, it, expect, vi } from 'vitest';
+import { render, screen, fireEvent } from '@testing-library/react';
+import { I18nextProvider } from 'react-i18next';
+import i18n from '../../../i18n';
+import { ColorSection } from '../../../components/spool-form/ColorSection';
+import { defaultFormData } from '../../../components/spool-form/types';
+
+type UpdateField = <K extends keyof typeof defaultFormData>(
+  key: K,
+  value: (typeof defaultFormData)[K],
+) => void;
+
+function renderColorSection(overrides: Partial<typeof defaultFormData> = {}) {
+  const updateField = vi.fn() as ReturnType<typeof vi.fn> & UpdateField;
+  const formData = { ...defaultFormData, ...overrides };
+
+  render(
+    <I18nextProvider i18n={i18n}>
+      <ColorSection
+        formData={formData}
+        updateField={updateField}
+        recentColors={[]}
+        onColorUsed={vi.fn()}
+        catalogColors={[]}
+      />
+    </I18nextProvider>,
+  );
+
+  const hexInput = screen.getByPlaceholderText('RRGGBB') as HTMLInputElement;
+  return { hexInput, updateField };
+}
+
+function lastRgba(updateField: ReturnType<typeof vi.fn>): string | undefined {
+  const rgbaCalls = updateField.mock.calls.filter(([key]) => key === 'rgba');
+  return rgbaCalls.at(-1)?.[1] as string | undefined;
+}
+
+describe('ColorSection hex input normalization (#1055)', () => {
+  it('pads a 6-char RRGGBB to 8-char RRGGBBAA with FF alpha', () => {
+    const { hexInput, updateField } = renderColorSection();
+    fireEvent.change(hexInput, { target: { value: 'FF0000' } });
+    expect(lastRgba(updateField)).toBe('FF0000FF');
+  });
+
+  it('passes an 8-char RRGGBBAA paste through unchanged', () => {
+    const { hexInput, updateField } = renderColorSection();
+    fireEvent.change(hexInput, { target: { value: '00112233' } });
+    expect(lastRgba(updateField)).toBe('00112233');
+  });
+
+  it('drops the stray 7th char — the exact #1055 trigger pattern', () => {
+    const { hexInput, updateField } = renderColorSection();
+    fireEvent.change(hexInput, { target: { value: 'FFFFFFF' } });
+    // Previously emitted "FFFFFFF" (7 chars) verbatim. Must now be 8 chars.
+    const rgba = lastRgba(updateField);
+    expect(rgba).toBe('FFFFFFFF');
+    expect(rgba).toMatch(/^[0-9A-F]{8}$/);
+  });
+
+  it('pads a 5-char input to 8 chars instead of emitting a 7-char rgba', () => {
+    // 5-char + 'FF' alpha = 7 chars was the other #1055 trigger pattern.
+    // Right-pad RGB to 6 with '0' so the output is always 8 chars.
+    const { hexInput, updateField } = renderColorSection();
+    fireEvent.change(hexInput, { target: { value: 'FFFFF' } });
+    const rgba = lastRgba(updateField);
+    expect(rgba).toBe('FFFFF0FF');
+    expect(rgba).toMatch(/^[0-9A-F]{8}$/);
+  });
+
+  it('pads any partial input to exactly 8 chars — never 7', () => {
+    // The essential invariant: for every legal input length (0..8), the
+    // emitted rgba must be 8 chars. Anything else risks reintroducing #1055.
+    const { hexInput, updateField } = renderColorSection();
+    for (const input of ['', 'F', 'FF', 'FFF', 'FFFF', 'FFFFF', 'FFFFFF', 'FFFFFFF', 'FFFFFFFF']) {
+      updateField.mockClear();
+      fireEvent.change(hexInput, { target: { value: input } });
+      const rgba = lastRgba(updateField);
+      expect(rgba).toBeDefined();
+      expect(rgba!.length).toBe(8);
+      expect(rgba).toMatch(/^[0-9A-F]{8}$/);
+    }
+  });
+
+  it('ignores input past 8 chars (no updateField call)', () => {
+    const { hexInput, updateField } = renderColorSection({ rgba: 'FFFFFFFF' });
+    updateField.mockClear();
+    fireEvent.change(hexInput, { target: { value: '0011223344' } });
+    expect(updateField.mock.calls.filter(([k]) => k === 'rgba')).toHaveLength(0);
+  });
+
+  it('strips non-hex characters before normalizing', () => {
+    // '#FF00ZZ' → strip '#' and non-hex → 'FF00' (4 chars) → pad to 6 + FF alpha
+    const { hexInput, updateField } = renderColorSection();
+    fireEvent.change(hexInput, { target: { value: '#FF00ZZ' } });
+    expect(lastRgba(updateField)).toBe('FF0000FF');
+  });
+});

+ 11 - 1
frontend/src/components/SpoolFormModal.tsx

@@ -253,12 +253,22 @@ export function SpoolFormModal({
   useEffect(() => {
     if (isOpen) {
       if (spool) {
+        // Legacy rows may carry a malformed rgba (e.g. the 7-char 'FFFFFFF'
+        // from #1055 before the create/update pattern was enforced). The
+        // backend SpoolUpdate schema rejects non-8-char hex on PATCH, so
+        // re-submitting a malformed value would 422 every edit on that spool
+        // — even edits that don't touch color. Normalize on load: any value
+        // that isn't exactly 8 hex chars falls back to the default, so the
+        // user can save unrelated fields (weight, material, note) without
+        // first being forced to fix a color they may not even be aware is
+        // broken. Saving also purges the bad value from the DB.
+        const validRgba = spool.rgba && /^[0-9A-Fa-f]{8}$/.test(spool.rgba) ? spool.rgba : '808080FF';
         setFormData({
           material: spool.material || '',
           subtype: spool.subtype || '',
           brand: spool.brand || '',
           color_name: spool.color_name || '',
-          rgba: spool.rgba || '808080FF',
+          rgba: validRgba,
           label_weight: spool.label_weight || 1000,
           core_weight: spool.core_weight || 250,
           core_weight_catalog_id: spool.core_weight_catalog_id ?? null,

+ 12 - 2
frontend/src/components/spool-form/ColorSection.tsx

@@ -298,8 +298,18 @@ export function ColorSection({
                 placeholder="RRGGBB"
                 value={currentHex.toUpperCase()}
                 onChange={(e) => {
-                  const val = e.target.value.replace('#', '').replace(/[^0-9A-Fa-f]/g, '');
-                  if (val.length <= 8) updateField('rgba', val.toUpperCase() + (val.length <= 6 ? 'FF' : ''));
+                  const val = e.target.value.replace('#', '').replace(/[^0-9A-Fa-f]/g, '').toUpperCase();
+                  if (val.length > 8) return;
+                  // Normalize to a valid 8-char RRGGBBAA on every keystroke so
+                  // the backend never receives a malformed rgba (#1055). 8-char
+                  // paste passes through; 7-char drops the stray typo; anything
+                  // shorter is right-padded with '0' to a full RGB triplet and
+                  // given FF alpha. Prior logic emitted 3/5/7-char strings mid-
+                  // typing that PATCH would accept (SpoolUpdate was unchecked)
+                  // and later 500 the list endpoint on response serialization.
+                  const rgba =
+                    val.length === 8 ? val : val.length === 7 ? val.substring(0, 6) + 'FF' : val.padEnd(6, '0') + 'FF';
+                  updateField('rgba', rgba);
                 }}
               />
             </div>

File diff suppressed because it is too large
+ 0 - 0
static/assets/index-CASdUlGi.js


+ 1 - 1
static/index.html

@@ -26,7 +26,7 @@
 
     <!-- Splash screens for iOS -->
     <link rel="apple-touch-startup-image" href="/img/android-chrome-512x512.png" />
-    <script type="module" crossorigin src="/assets/index-BskdRxdi.js"></script>
+    <script type="module" crossorigin src="/assets/index-CASdUlGi.js"></script>
     <link rel="stylesheet" crossorigin href="/assets/index-CkAOuJaW.css">
   </head>
   <body>

Some files were not shown because too many files changed in this diff