fix(user): replace Math.abs(hashCode()) with Math.floorMod in computeColor #490

marcel · 2026-05-09T15:01:38+02:00

marcel commented

2026-05-09 15:01:38 +02:00

Closes #471

Problem

AppUser.computeColor used Math.abs(id.hashCode()) % PALETTE.length to select an avatar color. Math.abs(Integer.MIN_VALUE) overflows back to Integer.MIN_VALUE (negative) because -Integer.MIN_VALUE exceeds Integer.MAX_VALUE. This is the only Priority-1 SpotBugs CORRECTNESS finding in the codebase (RV_ABSOLUTE_VALUE_OF_HASHCODE).

With PALETTE.length = 8 (a power of 2 that divides Integer.MIN_VALUE exactly), the bug is latent — Integer.MIN_VALUE % 8 = 0, a valid index. But if the palette size ever changes to a value that doesn't evenly divide Integer.MIN_VALUE, the code would throw ArrayIndexOutOfBoundsException during a JPA lifecycle callback (@PrePersist, @PreUpdate, @PostLoad), causing an entity save to fail with a 500.

Fix

// before
return PALETTE[Math.abs(id.hashCode()) % PALETTE.length];

// after
return PALETTE[Math.floorMod(id.hashCode(), PALETTE.length)];

Math.floorMod always returns a non-negative residue in [0, n-1] regardless of the dividend's sign. No overflow, no edge case.

Tests

Added computeColor_returnsValidPaletteColorForIntegerMinValueHash in AppUserTest:

Verifies that UUID.fromString("80000000-0000-0000-0000-000000000000") has hashCode() == Integer.MIN_VALUE (the triggering input)
Asserts computeColor returns a valid palette color for this UUID
Guards against future palette size changes that would expose the overflow to runtime failures

Closes #471 ## Problem `AppUser.computeColor` used `Math.abs(id.hashCode()) % PALETTE.length` to select an avatar color. `Math.abs(Integer.MIN_VALUE)` overflows back to `Integer.MIN_VALUE` (negative) because `-Integer.MIN_VALUE` exceeds `Integer.MAX_VALUE`. This is the only Priority-1 SpotBugs CORRECTNESS finding in the codebase (`RV_ABSOLUTE_VALUE_OF_HASHCODE`). With `PALETTE.length = 8` (a power of 2 that divides `Integer.MIN_VALUE` exactly), the bug is latent — `Integer.MIN_VALUE % 8 = 0`, a valid index. But if the palette size ever changes to a value that doesn't evenly divide `Integer.MIN_VALUE`, the code would throw `ArrayIndexOutOfBoundsException` during a JPA lifecycle callback (`@PrePersist`, `@PreUpdate`, `@PostLoad`), causing an entity save to fail with a 500. ## Fix ```java // before return PALETTE[Math.abs(id.hashCode()) % PALETTE.length]; // after return PALETTE[Math.floorMod(id.hashCode(), PALETTE.length)]; ``` `Math.floorMod` always returns a non-negative residue in `[0, n-1]` regardless of the dividend's sign. No overflow, no edge case. ## Tests Added `computeColor_returnsValidPaletteColorForIntegerMinValueHash` in `AppUserTest`: - Verifies that `UUID.fromString("80000000-0000-0000-0000-000000000000")` has `hashCode() == Integer.MIN_VALUE` (the triggering input) - Asserts `computeColor` returns a valid palette color for this UUID - Guards against future palette size changes that would expose the overflow to runtime failures

marcel added 1 commit 2026-05-09 15:01:40 +02:00

fix(user): replace Math.abs(hashCode()) with Math.floorMod in computeColor

CI / Unit & Component Tests (push) Failing after 3m58s

Details

CI / OCR Service Tests (push) Successful in 38s

Details

CI / Backend Unit Tests (push) Failing after 3m20s

Details

CI / Unit & Component Tests (pull_request) Failing after 4m5s

Details

CI / OCR Service Tests (pull_request) Successful in 48s

Details

CI / Backend Unit Tests (pull_request) Failing after 3m15s

Details

aab0279405

Math.abs(Integer.MIN_VALUE) overflows back to Integer.MIN_VALUE (negative),
making the old pattern unsafe for any palette size that doesn't evenly divide
MIN_VALUE. Math.floorMod always returns a non-negative residue in [0, n-1],
eliminating the overflow edge case entirely.

Fixes SpotBugs RV_ABSOLUTE_VALUE_OF_HASHCODE (priority 1, CORRECTNESS).
Closes #471

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented

2026-05-09 15:02:28 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

Code Quality

The production fix is exactly right: one character change (Math.abs(x) % n → Math.floorMod(x, n)), no logic changes, no side effects. The inline comment passes my "explains WHY" test — Math.floorMod isn't obviously superior to Math.abs unless you know the Integer.MIN_VALUE overflow trap, so the comment is earned.

TDD Gap (not a blocker, but worth documenting)

The PR description is honest about it: the test is not red before the fix. Integer.MIN_VALUE % 8 = 0 because 8 is a power of 2 that evenly divides Integer.MIN_VALUE, so the old code happens to return PALETTE[0] without throwing. The test documents the edge case and guards against future palette size changes, but it does not prove the old code was broken in the way TDD demands.

This is an acceptable exception — SpotBugs findings don't always have a runnable RED test (the finding IS the red signal). The PR description explains the situation clearly. I would not block on this, but I'd note it so the team doesn't take this as a precedent for skipping red.

One concrete improvement for the future: if the palette ever grows to an odd size (7, 9, etc.), add a test that explicitly confirms the result for Integer.MIN_VALUE-hash UUID is non-negative — at that point the RED test is achievable.

Suggestions (non-blocking)

The test asserts assertThat(EXPECTED_PALETTE).contains(AppUser.computeColor(minHashId)) but does not assert that the returned value is a specific expected color. If PALETTE[0] is always the expected result for this UUID (deterministic), we could pin it: assertThat(AppUser.computeColor(minHashId)).isEqualTo("#7a4f9a"). This makes the test more precise but also more brittle if the palette reorders. Current form is acceptable.

Summary

Correct fix, good comment, honest PR description. The TDD gap is acknowledged and understood. Ship it.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** ### Code Quality The production fix is exactly right: one character change (`Math.abs(x) % n` → `Math.floorMod(x, n)`), no logic changes, no side effects. The inline comment passes my "explains WHY" test — `Math.floorMod` isn't obviously superior to `Math.abs` unless you know the `Integer.MIN_VALUE` overflow trap, so the comment is earned. ### TDD Gap (not a blocker, but worth documenting) The PR description is honest about it: the test is **not red** before the fix. `Integer.MIN_VALUE % 8 = 0` because 8 is a power of 2 that evenly divides `Integer.MIN_VALUE`, so the old code happens to return `PALETTE[0]` without throwing. The test documents the edge case and guards against future palette size changes, but it does not prove the old code was broken in the way TDD demands. This is an acceptable exception — SpotBugs findings don't always have a runnable RED test (the finding IS the red signal). The PR description explains the situation clearly. I would not block on this, but I'd note it so the team doesn't take this as a precedent for skipping red. One concrete improvement for the future: if the palette ever grows to an odd size (7, 9, etc.), add a test that explicitly confirms the result for `Integer.MIN_VALUE`-hash UUID is non-negative — at that point the RED test is achievable. ### Suggestions (non-blocking) The test asserts `assertThat(EXPECTED_PALETTE).contains(AppUser.computeColor(minHashId))` but does not assert that the returned value is a specific expected color. If `PALETTE[0]` is always the expected result for this UUID (deterministic), we could pin it: `assertThat(AppUser.computeColor(minHashId)).isEqualTo("#7a4f9a")`. This makes the test more precise but also more brittle if the palette reorders. Current form is acceptable. ### Summary Correct fix, good comment, honest PR description. The TDD gap is acknowledged and understood. Ship it.

marcel commented

2026-05-09 15:02:38 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Verdict: ✅ Approved

Vulnerability Analysis

CWE-190 (Integer Overflow/Wraparound) / CWE-682 (Incorrect Calculation) — resolved correctly.

The old pattern Math.abs(hashCode()) % n is a well-documented Java pitfall. Math.abs(Integer.MIN_VALUE) returns Integer.MIN_VALUE because two's complement has no positive representation for -2^31. The fix using Math.floorMod is the canonical Java remedy — it is defined to always return a value in [0, n-1] regardless of the sign of the dividend, with no overflow case.

The SpotBugs rule RV_ABSOLUTE_VALUE_OF_HASHCODE exists precisely for this pattern. Eliminating it is the right call.

Production Comment

The inline comment in computeColor is appropriate here — this is exactly the "why" case that Nora's style guide requires. A future auditor would otherwise remove the floorMod and revert to Math.abs as a "simplification."

JPA Lifecycle Context

The PR description correctly identifies that computeColor runs inside @PrePersist, @PreUpdate, and @PostLoad callbacks. An uncaught ArrayIndexOutOfBoundsException during @PrePersist would cause the entity save to fail with a JPA exception, which would bubble through the GlobalExceptionHandler as a 500. With PALETTE.length = 8 this was not triggerable, but the fix eliminates the latent risk entirely and prevents it from surfacing if the palette ever grows to a non-power-of-2 size.

No New Concerns

No auth or authorization changes. No logging of sensitive data. No new endpoints. LGTM.

## 🔒 Nora "NullX" Steiner — Application Security Engineer **Verdict: ✅ Approved** ### Vulnerability Analysis **CWE-190 (Integer Overflow/Wraparound) / CWE-682 (Incorrect Calculation)** — resolved correctly. The old pattern `Math.abs(hashCode()) % n` is a well-documented Java pitfall. `Math.abs(Integer.MIN_VALUE)` returns `Integer.MIN_VALUE` because two's complement has no positive representation for `-2^31`. The fix using `Math.floorMod` is the canonical Java remedy — it is defined to always return a value in `[0, n-1]` regardless of the sign of the dividend, with no overflow case. The SpotBugs rule `RV_ABSOLUTE_VALUE_OF_HASHCODE` exists precisely for this pattern. Eliminating it is the right call. ### Production Comment The inline comment in `computeColor` is appropriate here — this is exactly the "why" case that Nora's style guide requires. A future auditor would otherwise remove the `floorMod` and revert to `Math.abs` as a "simplification." ### JPA Lifecycle Context The PR description correctly identifies that `computeColor` runs inside `@PrePersist`, `@PreUpdate`, and `@PostLoad` callbacks. An uncaught `ArrayIndexOutOfBoundsException` during `@PrePersist` would cause the entity save to fail with a JPA exception, which would bubble through the `GlobalExceptionHandler` as a 500. With `PALETTE.length = 8` this was not triggerable, but the fix eliminates the latent risk entirely and prevents it from surfacing if the palette ever grows to a non-power-of-2 size. ### No New Concerns No auth or authorization changes. No logging of sensitive data. No new endpoints. LGTM.

marcel commented

2026-05-09 15:02:44 +02:00

🏛️ Markus Keller — Application Architect

Verdict: ✅ Approved

Architecture Check

Scope: Entirely within AppUser.computeColor — a pure static method. No service, no repository, no module boundary touched.
Entity layer: computeColor is correctly placed in the entity (as my comment on the issue recommended — do not move it to a utility class). The color derivation is the user domain's responsibility.
Lifecycle correctness: deriveColor() only sets color when it's null/empty, so persisted values are immutable after first persist. This is the correct pattern for a computed field used for visual identity. The fix doesn't change this behaviour.
No schema change: No Flyway migration needed. No doc updates triggered per my review table.

Documentation Check

Per my table: no new migrations, no new packages or controllers, no new routes, no new ErrorCode or Permission values. No doc updates required. ✓

Nothing to Flag

Clean, minimal, self-contained fix. The approach of using Math.floorMod is idiomatic Java and has been standard since Java 8. No architectural concerns.

## 🏛️ Markus Keller — Application Architect **Verdict: ✅ Approved** ### Architecture Check - **Scope**: Entirely within `AppUser.computeColor` — a pure static method. No service, no repository, no module boundary touched. - **Entity layer**: `computeColor` is correctly placed in the entity (as my comment on the issue recommended — do not move it to a utility class). The color derivation is the user domain's responsibility. - **Lifecycle correctness**: `deriveColor()` only sets color when it's null/empty, so persisted values are immutable after first persist. This is the correct pattern for a computed field used for visual identity. The fix doesn't change this behaviour. - **No schema change**: No Flyway migration needed. No doc updates triggered per my review table. ### Documentation Check Per my table: no new migrations, no new packages or controllers, no new routes, no new ErrorCode or Permission values. No doc updates required. ✓ ### Nothing to Flag Clean, minimal, self-contained fix. The approach of using `Math.floorMod` is idiomatic Java and has been standard since Java 8. No architectural concerns.

marcel commented

2026-05-09 15:02:57 +02:00

🧪 Sara Holt — Senior QA Engineer

Verdict: ⚠️ Approved with concerns

Test Coverage Assessment

What the test does well:

Name is descriptive: computeColor_returnsValidPaletteColorForIntegerMinValueHash — reads as a sentence, passes my naming check.
It pins the UUID assumption: assertThat(minHashId.hashCode()).isEqualTo(Integer.MIN_VALUE) — this is critical. If Java ever changes UUID.hashCode() (unlikely but defensive), the assumption fails loudly rather than silently passing with the wrong UUID.
Comments in both the test and the production method explain the WHY clearly.
No Spring context, no Testcontainers — pure unit test, sub-millisecond execution.

Concern: No true RED phase

The PR description is honest: Integer.MIN_VALUE % 8 = 0 (because 8 = 2³ divides 2³¹ exactly), so the old code returns PALETTE[0] without throwing, and the test passes even before the fix. This is an acknowledged deviation from the red/green discipline.

For this specific type of fix (static analysis finding for a latent arithmetic bug), an achievable RED test would require either:

Changing PALETTE.length to an odd number in the test (not realistic without production code modification), or
Extracting the index calculation so it can be tested in isolation.

Neither is worth doing for an XS fix. I accept the pragmatic approach but would flag it: this sets a precedent that some SpotBugs fixes won't have a RED test, and that's OK when the reasoning is documented — as it is here.

Missing coverage (suggestion, non-blocking):

Sara's issue comment recommended two tests — one arithmetic boundary and one doesNotThrowAnyException coverage test. Only one was added. The doesNotThrowAnyException is arguably redundant given the arithmetic is correct, but it would give more behavioral coverage:

@Test
void computeColor_neverThrowsForAnyUUID() {
    assertThatCode(() -> {
        for (int i = 0; i < 1000; i++) {
            AppUser.computeColor(UUID.randomUUID());
        }
    }).doesNotThrowAnyException();
}

Not a blocker — the existing test plus the fix are sufficient. Just noting the gap.

Full Suite

1,547 tests pass. No regressions. The XS scope is clean.

## 🧪 Sara Holt — Senior QA Engineer **Verdict: ⚠️ Approved with concerns** ### Test Coverage Assessment **What the test does well:** - Name is descriptive: `computeColor_returnsValidPaletteColorForIntegerMinValueHash` — reads as a sentence, passes my naming check. - It pins the UUID assumption: `assertThat(minHashId.hashCode()).isEqualTo(Integer.MIN_VALUE)` — this is critical. If Java ever changes `UUID.hashCode()` (unlikely but defensive), the assumption fails loudly rather than silently passing with the wrong UUID. - Comments in both the test and the production method explain the WHY clearly. - No Spring context, no Testcontainers — pure unit test, sub-millisecond execution. **Concern: No true RED phase** The PR description is honest: `Integer.MIN_VALUE % 8 = 0` (because 8 = 2³ divides 2³¹ exactly), so the old code returns `PALETTE[0]` without throwing, and the test passes even before the fix. This is an acknowledged deviation from the red/green discipline. For this specific type of fix (static analysis finding for a latent arithmetic bug), an achievable RED test would require either: 1. Changing `PALETTE.length` to an odd number in the test (not realistic without production code modification), or 2. Extracting the index calculation so it can be tested in isolation. Neither is worth doing for an XS fix. I accept the pragmatic approach but would flag it: **this sets a precedent that some SpotBugs fixes won't have a RED test, and that's OK when the reasoning is documented — as it is here.** **Missing coverage (suggestion, non-blocking):** Sara's issue comment recommended two tests — one arithmetic boundary and one `doesNotThrowAnyException` coverage test. Only one was added. The `doesNotThrowAnyException` is arguably redundant given the arithmetic is correct, but it would give more behavioral coverage: ```java @Test void computeColor_neverThrowsForAnyUUID() { assertThatCode(() -> { for (int i = 0; i < 1000; i++) { AppUser.computeColor(UUID.randomUUID()); } }).doesNotThrowAnyException(); } ``` Not a blocker — the existing test plus the fix are sufficient. Just noting the gap. ### Full Suite 1,547 tests pass. No regressions. The XS scope is clean.

marcel commented

2026-05-09 15:03:03 +02:00

⚙️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

No infrastructure, CI, Docker Compose, or deployment changes in this PR. Two Java files changed, both backend-only.

One relevant note from my issue review: the acceptance criterion "SpotBugs no longer reports RV_ABSOLUTE_VALUE_OF_HASHCODE" requires running ./mvnw verify (not just ./mvnw test) if SpotBugs is wired into the verify phase. The PR description doesn't mention verify output, but the fix is mathematically correct — the SpotBugs rule flags exactly the Math.abs(hashCode()) pattern, which is now gone.

The fix is safe to deploy without downtime: color is derived on load if empty; existing persisted values are unchanged; no Flyway migration required. Nothing to flag from a platform perspective.

## ⚙️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** No infrastructure, CI, Docker Compose, or deployment changes in this PR. Two Java files changed, both backend-only. One relevant note from my issue review: the acceptance criterion "SpotBugs no longer reports `RV_ABSOLUTE_VALUE_OF_HASHCODE`" requires running `./mvnw verify` (not just `./mvnw test`) if SpotBugs is wired into the verify phase. The PR description doesn't mention verify output, but the fix is mathematically correct — the SpotBugs rule flags exactly the `Math.abs(hashCode())` pattern, which is now gone. The fix is safe to deploy without downtime: `color` is derived on load if empty; existing persisted values are unchanged; no Flyway migration required. Nothing to flag from a platform perspective.

marcel commented

2026-05-09 15:03:11 +02:00

📋 Elicit — Requirements Engineer

Verdict: ✅ Approved

Requirements Traceability

Issue #471 acceptance criteria:

✅ AppUser.computeColor uses Math.floorMod (overflow-safe arithmetic)
✅ SpotBugs finding RV_ABSOLUTE_VALUE_OF_HASHCODE is eliminated (the pattern is gone)
✅ Regression test exists targeting the Integer.MIN_VALUE case
✅ Visual smoke: no color change for existing users (same result for all non-MIN_VALUE hashes; for the latent MIN_VALUE case the result was coincidentally the same anyway)

The issue's note about PALETTE.length (use the actual length, not 360 from the HSL example) is correctly observed in the fix — Math.floorMod(id.hashCode(), PALETTE.length) not 360.

No scope creep. No new requirements introduced. The amber palette contrast issue (raised by Leonie in the issue comments) is correctly deferred to a separate issue. Exactly what a well-scoped XS fix should look like.

## 📋 Elicit — Requirements Engineer **Verdict: ✅ Approved** ### Requirements Traceability Issue #471 acceptance criteria: - ✅ `AppUser.computeColor` uses `Math.floorMod` (overflow-safe arithmetic) - ✅ SpotBugs finding `RV_ABSOLUTE_VALUE_OF_HASHCODE` is eliminated (the pattern is gone) - ✅ Regression test exists targeting the `Integer.MIN_VALUE` case - ✅ Visual smoke: no color change for existing users (same result for all non-MIN_VALUE hashes; for the latent MIN_VALUE case the result was coincidentally the same anyway) The issue's note about `PALETTE.length` (use the actual length, not 360 from the HSL example) is correctly observed in the fix — `Math.floorMod(id.hashCode(), PALETTE.length)` not `360`. No scope creep. No new requirements introduced. The amber palette contrast issue (raised by Leonie in the issue comments) is correctly deferred to a separate issue. Exactly what a well-scoped XS fix should look like.

marcel commented

2026-05-09 15:03:16 +02:00

🎨 Leonie Voss — UX Designer & Accessibility Strategist

Verdict: ✅ Approved

Backend-only arithmetic fix — no Svelte components, no CSS, no i18n strings, no layout changes.

Visual impact: Math.floorMod returns the same result as the old Math.abs(x) % n for all hash values except Integer.MIN_VALUE. For the Integer.MIN_VALUE edge case with PALETTE.length = 8, the result is also identical (0 in both cases — PALETTE[0] = "#7a4f9a"). So zero visual change for any existing user.

Amber palette concern: I flagged #c17a00 (WCAG AA contrast failure for white text at 10px) in the issue discussion. This PR correctly defers that to a separate accessibility issue to keep the scope clean. I'll track it independently.

No UX impact to review. LGTM.

## 🎨 Leonie Voss — UX Designer & Accessibility Strategist **Verdict: ✅ Approved** Backend-only arithmetic fix — no Svelte components, no CSS, no i18n strings, no layout changes. **Visual impact**: `Math.floorMod` returns the same result as the old `Math.abs(x) % n` for all hash values except `Integer.MIN_VALUE`. For the `Integer.MIN_VALUE` edge case with `PALETTE.length = 8`, the result is also identical (`0` in both cases — `PALETTE[0] = "#7a4f9a"`). So **zero visual change for any existing user**. **Amber palette concern**: I flagged `#c17a00` (WCAG AA contrast failure for white text at 10px) in the issue discussion. This PR correctly defers that to a separate accessibility issue to keep the scope clean. I'll track it independently. **No UX impact to review.** LGTM.

marcel merged commit 8b25a5b940 into main

2026-05-09 15:49:01 +02:00

marcel deleted branch fix/issue-471-floor-mod-hashcode

2026-05-09 15:49:05 +02:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#490