Skip to content

Commit dbd0a19

Browse files
Rylan-cgiclaude
andcommitted
fix(schedule10): clear the remaining SonarCloud findings
Follow-up to the code review. Every reported item is fixed; none was dismissed as a false positive. DANGLING JAVADOC (Critical). PERCENTAGE_CONVERTER had been inserted between the CONVERTER_KEYS_BY_TARGET javadoc and its field, orphaning the doc block. Both converter constants now sit above it. VOLUME_CONVERTER is extracted at the same time, which also covers the third usage in the type-fallback branch. JOIN THESE MULTIPLE ASSERTIONS (8). Seven came from the previous commit's containsEntry conversion, which left runs of consecutive single-entry assertions on the same subject; they are now chained. The eighth, in Schedule10CheckStatusIT, was missed by that pass because the seven assertions there are separated by comments -- now one chain with each link's reasoning kept beside it. No consecutive same-subject runs remain in any Schedule 10 test. TEST THE EMPTINESS OF THE LIST (2 Bugs). doesNotContain(...) passes on an empty list, so "End Haul is checked nowhere" and "method N skips the gated rules" would both have passed even if the rule engine returned nothing at all. Each now asserts isEmpty() -- the actual claim -- plus a CONTROL that proves the engine ran: one drives an unrelated field out of range and expects exactly that field, the other asserts the same empty stabilizing block under method C does produce issues. Suppression is now demonstrably the gate rather than a dead rule set. METHOD HAS 8 PARAMETERS. requireRange's bounds and format are bundled into a Band record, taking it to six. The band(lower, format, upper) factory preserves the legacy argument ORDER at all 30 call sites, so the rewrite was a pure textual wrap and the reordering happens once inside the factory -- no chance of a silent transposition changing a reported bound. The emission-order and bound-formatting tests, which assert exact rendered bounds, pass unchanged. CLASS HAS 6 PARENTS. InvalidBecClassificationException now extends BusinessException directly. The subtype relationship bought nothing: nothing catches either type, both map to 400, and the only thing it required was a protected constructor on the parent existing solely to pass this class's message key through. That constructor is removed as dead. MONSTER CLASS (21 dependencies). Resolved in the previous commit by deleting the persist(Runnable) overload, which became unreachable once every call site carried a resource-specific message key. While fixing the Critical dangling javadoc, the identical mistake was made in Schedule10CheckStatus -- the new Band record landed between requireRange's javadoc and its method. Checkstyle caught it along with two over-long lines from the same edit. Verification: 1151 unit + 881 integration tests, 0 failures; 0 checkstyle violations in schedule10 under the newly bumped 13.10.0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent f7a9718 commit dbd0a19

9 files changed

Lines changed: 202 additions & 121 deletions

backend/src/main/java/ca/bc/gov/nrs/ilcr/exception/GlobalExceptionHandler.java

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -258,7 +258,7 @@ public ResponseEntity<ProblemDetail> handleNotReadable(
258258
String key = converterKeyForField(causeMessage);
259259
if (key == null) {
260260
if (causeMessage.contains("java.math.BigDecimal")) {
261-
key = "volumeConverterErrorMsg";
261+
key = VOLUME_CONVERTER;
262262
} else if (causeMessage.contains("java.lang.Integer")
263263
|| causeMessage.contains("java.lang.Long")) {
264264
key = "costConverterErrorMsg";
@@ -298,6 +298,12 @@ private static String converterKeyForField(String causeMessage) {
298298
.orElse(null);
299299
}
300300

301+
/** Shared by the five Schedule 10 material percentages, which all fail identically. */
302+
private static final String PERCENTAGE_CONVERTER = "percentageConverterErrorMsg";
303+
304+
/** Shared by every volume field, Schedule 10's two haul volumes included. */
305+
private static final String VOLUME_CONVERTER = "volumeConverterErrorMsg";
306+
301307
/**
302308
* Converter message keys scoped to {@code DeclaringType["property"]}, matched against the
303309
* reference
@@ -320,9 +326,6 @@ private static String converterKeyForField(String causeMessage) {
320326
* collection
321327
* hops ({@code CulvertSaveAllRequest["culverts"]->…->CulvertRequest["spanSize"]}).
322328
*/
323-
/** Shared by all five Schedule 10 material percentages, which fail identically. */
324-
private static final String PERCENTAGE_CONVERTER = "percentageConverterErrorMsg";
325-
326329
private static final Map<String, String> CONVERTER_KEYS_BY_TARGET = Map.ofEntries(
327330
Map.entry("CulvertRequest[\"spanSize\"]", "culvertSpanConverterErrorMsg"),
328331
Map.entry("CulvertRequest[\"riseSize\"]", "culvertRiseConverterErrorMsg"),
@@ -331,8 +334,8 @@ private static String converterKeyForField(String causeMessage) {
331334
// detail — a percentage or a haul volume — fell through to the type default and told the
332335
// reporter their COST was invalid.
333336
Map.entry("RoadDetailRequest[\"sideSlopePct\"]", "sideSlopePercentageConverterErrorMsg"),
334-
Map.entry("RoadDetailRequest[\"endHaulVolume\"]", "volumeConverterErrorMsg"),
335-
Map.entry("RoadDetailRequest[\"overlandVolume\"]", "volumeConverterErrorMsg"),
337+
Map.entry("RoadDetailRequest[\"endHaulVolume\"]", VOLUME_CONVERTER),
338+
Map.entry("RoadDetailRequest[\"overlandVolume\"]", VOLUME_CONVERTER),
336339
Map.entry("MaterialCompositionRequest[\"solidRockPct\"]", PERCENTAGE_CONVERTER),
337340
Map.entry("MaterialCompositionRequest[\"rippableRockPct\"]", PERCENTAGE_CONVERTER),
338341
Map.entry("MaterialCompositionRequest[\"coarsePct\"]", PERCENTAGE_CONVERTER),

backend/src/main/java/ca/bc/gov/nrs/ilcr/schedule10/InvalidBecClassificationException.java

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
package ca.bc.gov.nrs.ilcr.schedule10;
22

3+
import ca.bc.gov.nrs.ilcr.exception.BusinessException;
4+
import org.springframework.http.HttpStatus;
5+
36
/**
47
* Raised when a road-detail write names a BEC classification the cross-reference does not offer, or
58
* when that classification plus the RSMR class resolves to no moisture-code pair at all. Maps to
@@ -13,10 +16,17 @@
1316
* derived from this classification and the RSMR class, and both target columns are {@code NOT NULL}
1417
* with enabled foreign keys. Failing here with a clear 400 is the alternative to letting the insert
1518
* reach Oracle and return an opaque constraint violation.
19+
*
20+
* <p>Extends {@link BusinessException} directly rather than {@code
21+
* InvalidClassificationCodeException}, even though a BEC code IS a kind of classification code. The
22+
* subtype relationship bought nothing — nothing catches either type, both map to 400, and the only
23+
* thing it required was a {@code protected} constructor on the parent existing solely to pass this
24+
* class's message key through. It also pushed the inheritance depth to six. Flattened at code
25+
* review follow-up 2026-08-18.
1626
*/
17-
public class InvalidBecClassificationException extends InvalidClassificationCodeException {
27+
public class InvalidBecClassificationException extends BusinessException {
1828

1929
public InvalidBecClassificationException() {
20-
super("invalidBiogeoCode");
30+
super(HttpStatus.BAD_REQUEST, "invalidBiogeoCode");
2131
}
2232
}

backend/src/main/java/ca/bc/gov/nrs/ilcr/schedule10/InvalidClassificationCodeException.java

Lines changed: 0 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -20,14 +20,4 @@ public class InvalidClassificationCodeException extends BusinessException {
2020
public InvalidClassificationCodeException() {
2121
super(HttpStatus.BAD_REQUEST, "invalidCodeValueErrorMsg");
2222
}
23-
24-
/**
25-
* For the BEC-classification case, which carries its own legacy message rather than the generic
26-
* code-list one.
27-
*
28-
* @param messageKey the legacy bundle key to resolve
29-
*/
30-
protected InvalidClassificationCodeException(String messageKey) {
31-
super(HttpStatus.BAD_REQUEST, messageKey);
32-
}
3323
}

backend/src/main/java/ca/bc/gov/nrs/ilcr/schedule10/Schedule10CheckStatus.java

Lines changed: 85 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -196,76 +196,93 @@ static DetailOutcome evaluateRoadDetail(
196196
requirePresent(
197197
issues, "relSoilMoistRgmClsCode", prefix + " RSMR Class", detail.relSoilMoistRgmClsCode());
198198
requireRange(issues, "sideSlopePct", prefix + " Side Slope (%)",
199-
value(detail.sideSlopePct()), ZERO, FMT_INT, PCT_MAX, true);
199+
value(detail.sideSlopePct()),
200+
band(ZERO, FMT_INT, PCT_MAX), true);
200201

201202
MaterialComposition material = detail.materialComposition();
202203
requireRange(issues, "solidRockPct", prefix + " Solid (Hard) Rock (%)",
203-
value(material == null ? null : material.solidRockPct()), ZERO, FMT_INT, PCT_MAX, false);
204+
value(material == null ? null : material.solidRockPct()),
205+
band(ZERO, FMT_INT, PCT_MAX), false);
204206
requireRange(issues, "rippableRockPct", prefix + " Ripple Rock (%)",
205-
value(material == null ? null : material.rippableRockPct()), ZERO, FMT_INT, PCT_MAX, false);
207+
value(material == null ? null : material.rippableRockPct()),
208+
band(ZERO, FMT_INT, PCT_MAX), false);
206209
requireRange(issues, "coarsePct", prefix + " Coarse (%)",
207-
value(material == null ? null : material.coarsePct()), ZERO, FMT_INT, PCT_MAX, false);
210+
value(material == null ? null : material.coarsePct()),
211+
band(ZERO, FMT_INT, PCT_MAX), false);
208212
requireRange(issues, "finePct", prefix + " Fine (%)",
209-
value(material == null ? null : material.finePct()), ZERO, FMT_INT, PCT_MAX, false);
213+
value(material == null ? null : material.finePct()),
214+
band(ZERO, FMT_INT, PCT_MAX), false);
210215
requireRange(issues, "organicPct", prefix + " Organic (%)",
211-
value(material == null ? null : material.organicPct()), ZERO, FMT_INT, PCT_MAX, false);
216+
value(material == null ? null : material.organicPct()),
217+
band(ZERO, FMT_INT, PCT_MAX), false);
212218
// Both bounds are 100, which selects the must-equal message. The legacy total coerces nulls to
213219
// zero and is never absent, so an untouched material breakdown reports 0 against 100.
214220
requireRange(issues, "materialTypeTotal", prefix + " Material Type Total (%)",
215-
value(material == null ? null : material.totalPct()), PCT_MAX, FMT_INT, PCT_MAX, false);
221+
value(material == null ? null : material.totalPct()),
222+
band(PCT_MAX, FMT_INT, PCT_MAX), false);
216223

217224
SubGrade subGrade = detail.subGrade();
218225
requireRange(issues, "subGradeLength", prefix + " Sub-Grade: Length (km)",
219-
field(subGrade, SubGrade::length), ZERO, FMT_3DP, PCT_MAX, false);
226+
field(subGrade, SubGrade::length),
227+
band(ZERO, FMT_3DP, PCT_MAX), false);
220228
requireRange(issues, "subGradeSurfaceWidth", prefix + " Sub-Grade: Surface Width (m)",
221-
field(subGrade, SubGrade::surfaceWidth), ZERO, FMT_1DP, WIDTH_MAX, false);
229+
field(subGrade, SubGrade::surfaceWidth),
230+
band(ZERO, FMT_1DP, WIDTH_MAX), false);
222231
requireRange(issues, "subGradeActualCost", prefix + " Sub-Grade: Actual Cost ($)",
223-
field(subGrade, SubGrade::actualCost), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
232+
field(subGrade, SubGrade::actualCost),
233+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), false);
224234
requireRange(issues, "subGradeTtTransfer", prefix + " Sub-Grade: TtT Transfer ($)",
225-
field(subGrade, SubGrade::ttTransfer), SEVEN_DIGITS.negate(), FMT_MONEY, SEVEN_DIGITS,
226-
false);
235+
field(subGrade, SubGrade::ttTransfer),
236+
band(SEVEN_DIGITS.negate(), FMT_MONEY, SEVEN_DIGITS), false);
227237
requireRange(issues, "subGradeOtherTransfer", prefix + " Sub-Grade: Other Transfer ($)",
228-
field(subGrade, SubGrade::otherTransfer), SEVEN_DIGITS.negate(), FMT_MONEY, SEVEN_DIGITS,
229-
false);
238+
field(subGrade, SubGrade::otherTransfer),
239+
band(SEVEN_DIGITS.negate(), FMT_MONEY, SEVEN_DIGITS), false);
230240
requireRange(issues, "subGradeTotalCosts", prefix + " Sub-Grade: Total Costs ($)",
231-
field(subGrade, SubGrade::totalCosts), EIGHT_DIGITS.negate(), FMT_MONEY, EIGHT_DIGITS,
232-
false);
241+
field(subGrade, SubGrade::totalCosts),
242+
band(EIGHT_DIGITS.negate(), FMT_MONEY, EIGHT_DIGITS), false);
233243
requireRange(issues, "lessBridges", prefix + " Sub-Grade: Less Bridges ($)",
234-
field(subGrade, SubGrade::lessBridges), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
244+
field(subGrade, SubGrade::lessBridges),
245+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), false);
235246
requireRange(issues, "lessCulverts", prefix + " Sub-Grade: Less Culverts ($)",
236-
field(subGrade, SubGrade::lessCulverts), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
247+
field(subGrade, SubGrade::lessCulverts),
248+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), false);
237249
requireRange(issues, "lessLandings", prefix + " Sub-Grade: Less Landing ($)",
238-
field(subGrade, SubGrade::lessLandings), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
250+
field(subGrade, SubGrade::lessLandings),
251+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), false);
239252
requireRange(issues, "lessEndHaul", prefix + " Sub-Grade: Less End Haul ($)",
240-
field(subGrade, SubGrade::lessEndHaul), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
253+
field(subGrade, SubGrade::lessEndHaul),
254+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), false);
241255
requireRange(issues, "lessOverland", prefix + " Sub-Grade: Less Overland ($)",
242-
field(subGrade, SubGrade::lessOverland), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
256+
field(subGrade, SubGrade::lessOverland),
257+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), false);
243258
requireRange(issues, "lessOtherEng", prefix + " Sub-Grade: Less Other Eng ($)",
244-
field(subGrade, SubGrade::lessOtherEng), ZERO, FMT_MONEY, SEVEN_DIGITS, false);
259+
field(subGrade, SubGrade::lessOtherEng),
260+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), false);
245261
requireRange(issues, "subGradeTotal", prefix + " Sub-Grade: Total ($)",
246-
field(subGrade, SubGrade::total), EIGHT_DIGITS.negate(), FMT_MONEY_2DP, EIGHT_DIGITS,
247-
false);
262+
field(subGrade, SubGrade::total),
263+
band(EIGHT_DIGITS.negate(), FMT_MONEY_2DP, EIGHT_DIGITS), false);
248264
requireRange(issues, "subGradeCostPerLength", prefix + " Sub-Grade: $/km",
249-
field(subGrade, SubGrade::costPerLength), EIGHT_DIGITS.negate(), FMT_MONEY_2DP,
250-
EIGHT_DIGITS, false);
265+
field(subGrade, SubGrade::costPerLength),
266+
band(EIGHT_DIGITS.negate(), FMT_MONEY_2DP, EIGHT_DIGITS), false);
251267

252268
Stabilizing stabilizing = detail.stabilizing();
253269
boolean crushed =
254270
stabilizing != null && BALLAST_CRUSHED.equals(stabilizing.ballastMethodCode());
255271

256272
requireRange(issues, "stabilizingLength", prefix + " Additional Stabilizing: Length (km)",
257-
field(stabilizing, Stabilizing::length), ZERO, FMT_3DP, new BigDecimal("999.999"), crushed);
273+
field(stabilizing, Stabilizing::length),
274+
band(ZERO, FMT_3DP, new BigDecimal("999.999")), crushed);
258275
requireRange(issues, "stabilizingSurfaceWidth",
259276
prefix + " Additional Stabilizing: Surface Width (m)",
260-
field(stabilizing, Stabilizing::surfaceWidth), ZERO, FMT_1DP, WIDTH_MAX,
261-
crushed);
277+
field(stabilizing, Stabilizing::surfaceWidth),
278+
band(ZERO, FMT_1DP, WIDTH_MAX), crushed);
262279
requireRange(issues, "stabilizingDepth", prefix + " Additional Stabilizing: Depth (m)",
263-
field(stabilizing, Stabilizing::depth), ZERO, FMT_2DP_SMALL, new BigDecimal("99.9"),
264-
crushed);
280+
field(stabilizing, Stabilizing::depth),
281+
band(ZERO, FMT_2DP_SMALL, new BigDecimal("99.9")), crushed);
265282
requireRange(issues, "stabilizingDistanceToSource",
266283
prefix + " Additional Stabilizing: Distance to Source (km)",
267-
field(stabilizing, Stabilizing::distanceToSource), ZERO, FMT_1DP, WIDTH_MAX,
268-
crushed);
284+
field(stabilizing, Stabilizing::distanceToSource),
285+
band(ZERO, FMT_1DP, WIDTH_MAX), crushed);
269286

270287
if (!crushed) {
271288
return new DetailOutcome(
@@ -276,19 +293,24 @@ static DetailOutcome evaluateRoadDetail(
276293
stabilizing.ballastMaterialCode());
277294
requireRange(issues, "stabilizingActualCost",
278295
prefix + " Additional Stabilizing: Actual Cost ($)",
279-
stabilizing.actualCost(), ZERO, FMT_MONEY, SEVEN_DIGITS, true);
296+
stabilizing.actualCost(),
297+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), true);
280298
// Floor of ZERO, while the entry form accepts down to -9,999,999 for both transfers. A value
281299
// the form allowed is therefore reported here. Legacy carries the same disagreement.
282300
requireRange(issues, "stabilizingTtTransfer",
283301
prefix + " Additional Stabilizing: TtT Transfer ($)",
284-
stabilizing.ttTransfer(), ZERO, FMT_MONEY, SEVEN_DIGITS, true);
302+
stabilizing.ttTransfer(),
303+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), true);
285304
requireRange(issues, "stabilizingOtherTransfer",
286305
prefix + " Additional Stabilizing: Other Transfer ($)",
287-
stabilizing.otherTransfer(), ZERO, FMT_MONEY, SEVEN_DIGITS, true);
306+
stabilizing.otherTransfer(),
307+
band(ZERO, FMT_MONEY, SEVEN_DIGITS), true);
288308
requireRange(issues, "stabilizingTotal", prefix + " Additional Stabilizing: total ($)",
289-
stabilizing.total(), new BigDecimal("-999999"), FMT_MONEY_NARROW, SEVEN_DIGITS, false);
309+
stabilizing.total(),
310+
band(new BigDecimal("-999999"), FMT_MONEY_NARROW, SEVEN_DIGITS), false);
290311
requireRange(issues, "stabilizingCostPerLength", prefix + " Additional Stabilizing: $/km",
291-
stabilizing.costPerLength(), ZERO, FMT_MONEY_WIDE, EIGHT_DIGITS, false);
312+
stabilizing.costPerLength(),
313+
band(ZERO, FMT_MONEY_WIDE, EIGHT_DIGITS), false);
292314

293315
return new DetailOutcome(
294316
detail.roadDetailId(), detail.rowNumber(), detail.roadDetailLabel(), issues);
@@ -301,6 +323,26 @@ private static void requirePresent(
301323
}
302324
}
303325

326+
/**
327+
* One rule's inclusive bounds and the pattern both are rendered with.
328+
*
329+
* <p>Bundled into a record so {@link #requireRange} takes six parameters rather than eight (Sonar
330+
* brain-overload, 2026-08-18). The three genuinely travel together: legacy formats BOTH bounds
331+
* with the LOWER bound's pattern, which is why one format field serves both and why they cannot
332+
* sensibly be separated.
333+
*
334+
* @param lower the inclusive lower bound
335+
* @param upper the inclusive upper bound; equal to {@code lower} selects the must-equal message
336+
* @param format the legacy number pattern both bounds are rendered with
337+
*/
338+
private record Band(BigDecimal lower, BigDecimal upper, String format) {
339+
}
340+
341+
/** Reads as a bound pair at the call site, in the legacy lower/format/upper order. */
342+
private static Band band(BigDecimal lower, String format, BigDecimal upper) {
343+
return new Band(lower, upper, format);
344+
}
345+
304346
/**
305347
* The legacy numeric rule: an absent optional value passes, an absent required value is reported
306348
* as missing, and otherwise the value must sit inside the inclusive range.
@@ -311,8 +353,11 @@ private static void requirePresent(
311353
* instead, which agrees for every rule in this schedule and does not depend on that accident.
312354
*/
313355
private static void requireRange(
314-
List<Issue> issues, String field, String label, BigDecimal value,
315-
BigDecimal lower, String lowerFormat, BigDecimal upper, boolean required) {
356+
List<Issue> issues, String field, String label, BigDecimal value, Band band,
357+
boolean required) {
358+
BigDecimal lower = band.lower();
359+
BigDecimal upper = band.upper();
360+
String lowerFormat = band.format();
316361
if (value == null) {
317362
if (required) {
318363
issues.add(new Issue(field, label, MSG_REQUIRED, List.of()));

0 commit comments

Comments
 (0)