Skip to content

fix: normalize null grunt_type for invasion alarms#141

Merged
hokiepokedad2 merged 2 commits into
mainfrom
fix/139-quickpick-invasion-null-grunt-type
Apr 6, 2026
Merged

fix: normalize null grunt_type for invasion alarms#141
hokiepokedad2 merged 2 commits into
mainfrom
fix/139-quickpick-invasion-null-grunt-type

Conversation

@hokiepokedad2

Copy link
Copy Markdown
Contributor

Summary

  • Fixes quick pick "all invasions" sending grunt_type: null to PoracleNG, which rejects it with 400 Bad Request
  • Normalizes null"" (empty string) in QuickPickService.ApplyInvasionAsync at the point of origin
  • Adds defensive GruntType ??= "" in InvasionService.CreateAsync, UpdateAsync, and BulkCreateAsync to guard all entry points

Test plan

  • Added CreateAsyncNormalizesNullGruntType and UpdateAsyncNormalizesNullGruntType tests
  • All 12 InvasionServiceTests pass

Closes #139

)

Quick pick 'all invasions' sent grunt_type: null to PoracleNG, which
returned 400. PoracleNG expects empty string for "any grunt type".

Normalize null to "" in QuickPickService.ApplyInvasionAsync and
defensively in InvasionService create/update/bulk-create methods.
@hokiepokedad2

Copy link
Copy Markdown
Contributor Author

PR Review: fix: normalize null grunt_type for invasion alarms

Executive Summary

Metric Assessment
Overall Grade A
Code Quality Excellent
Requirements Satisfied Yes
Architecture Fit Consistent
Risk Level Low

Requirements Traceability

Requirement (#139) Implementation Status
Quick pick "all invasions" sends grunt_type: null causing 400 QuickPickService.cs:494invasion.GruntType ??= "" ✅ Satisfied
Normalize null to empty string before proxy call InvasionService.cs:28,42,125 — defensive ??= "" in all write paths ✅ Satisfied

Code Review Findings

Critical Issues: None
Major Issues: None
Minor Issues: None

Analysis

Root cause fix (QuickPickService.cs:494): Correctly normalizes at the point of origin — ApplyInvasionAsync deserializes from the quick pick definition JSON, which may omit grunt_type entirely, leaving the C# string? property as null. The ??= "" coalesces this to empty string before handing to InvasionService.

Defense-in-depth (InvasionService.cs:28,42,125): All three write paths (CreateAsync, UpdateAsync, BulkCreateAsync) normalize GruntType before serialization. This is the right pattern — InvasionService is the system boundary to PoracleNG, and PoracleNG rejects null with 400. Protecting at the boundary ensures no future caller can hit this bug.

Note: UpdateDistanceByUserAsync and UpdateDistanceByUidsAsync (fetch-mutate-POST pattern) don't need normalization because they deserialize from PoracleNG responses where grunt_type is already non-null.

Test coverage: Two new tests (CreateAsyncNormalizesNullGruntType, UpdateAsyncNormalizesNullGruntType) directly verify the null→empty-string normalization. The existing BulkCreateAsyncSetsUserIds test already included a GruntType = null model at line 143, which now implicitly tests BulkCreateAsync normalization too.

CHANGELOG: Entry added under [Unreleased] > Fixed. Also fixes a pre-existing duplicate [2.1.3] header.

Architecture Fit

The ??= "" pattern is consistent with how the codebase handles null-to-empty normalization at system boundaries (per CLAUDE.md: "PoracleNG handles field defaults" but expects non-null strings). The fix is placed at exactly the right layers — origin (QuickPickService) and boundary (InvasionService).

Risk Assessment

Risk Level Notes
Breaking Change None Only adds null coalescing
Data Integrity None Empty string is the correct "any grunt" semantic
Performance None Single null check per call
Security None No auth/input changes
Rollback Complexity Trivial Revert one commit

Verdict: APPROVED

Clean, minimal, well-tested fix. Addresses the root cause and hardens the boundary layer.

@hokiepokedad2 hokiepokedad2 merged commit 686d86a into main Apr 6, 2026
4 checks passed
@hokiepokedad2 hokiepokedad2 deleted the fix/139-quickpick-invasion-null-grunt-type branch April 6, 2026 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

bug: quick pick 'all invasions' fails — sends null grunt_type

1 participant