mirror of
https://github.com/MBombeck/HealthLog.git
synced 2026-11-14 22:50:51 +00:00
fix(measurements): harden the aggregated HR bucket validation
Two data-integrity gaps found in review of the v1.30.7 contract: - The bucket externalId guard accepted a non-canonical timestamp that V8 silently rolls over (T24:00, 04-31, non-leap 02-29). Such a row would key under a literal string the canonical client never emits, so the two would never collapse on the overwrite key, and it would plot on the wrong local day. Require canonical round-trip (toISOString() === suffix) in both the guard and the bucket-start parser. - The per-bucket min/max bypassed the plausibility-range guard that value passes; now that they drive the DAY min/max band and the intraday envelope, an out-of-range or mis-ordered spread poisoned the aggregate. Persist the spread only when both extremes are in range and ordered around value; otherwise drop to null and keep the average.
This commit is contained in:
1 parent
9a6a05a0ef
commit
6af0e36c88
4 files changed
+76
-10
No files matched your search
@@ -172,6 +172,39 @@ describe("POST /api/measurements/batch — aggregated HR wire contract (iOS #34)
|
||||
expect(rows[0].valueMax).toBe(96);
|
||||
});
|
||||
|
||||
it.each([
|
||||
// out-of-plausible-range spread (a sensor glitch / spurious discreteMax)
|
||||
{ valueMin: 58, valueMax: 99999 },
|
||||
{ valueMin: -9999, valueMax: 96 },
|
||||
// mis-ordered: valueMin above the average, or valueMax below it
|
||||
{ valueMin: 80, valueMax: 96 },
|
||||
{ valueMin: 58, valueMax: 70 },
|
||||
])(
|
||||
"v1.30.8 — drops an out-of-range or mis-ordered spread to null but keeps the average %o",
|
||||
async (spread) => {
|
||||
vi.mocked(prisma.measurement.createMany).mockResolvedValue({ count: 1 });
|
||||
const res = await POST(
|
||||
makeRequest({ entries: [hrBucketEntry(HR_BUCKET_ID, 72, spread)] }),
|
||||
);
|
||||
expect(res.status).toBe(200);
|
||||
const createArg = vi.mocked(prisma.measurement.createMany).mock
|
||||
.calls[0][0];
|
||||
const rows = (
|
||||
createArg as {
|
||||
data: {
|
||||
value: number;
|
||||
valueMin: number | null;
|
||||
valueMax: number | null;
|
||||
}[];
|
||||
}
|
||||
).data;
|
||||
// The trustworthy average survives; the invalid spread is dropped.
|
||||
expect(rows[0].value).toBe(72);
|
||||
expect(rows[0].valueMin).toBeNull();
|
||||
expect(rows[0].valueMax).toBeNull();
|
||||
},
|
||||
);
|
||||
|
||||
it("overwrites min/max alongside the average on a re-post", async () => {
|
||||
vi.mocked(prisma.measurement.findMany).mockResolvedValue([
|
||||
{ type: "PULSE", source: "APPLE_HEALTH", externalId: HR_BUCKET_ID },
|
||||
|
||||
@@ -318,15 +318,25 @@ async function postBatch(request: NextRequest): Promise<Response> {
|
||||
// per-day cumulative `stats:` total, or a manual entry never carries a
|
||||
// spread, so we pin them to null there even if a client mis-sends the
|
||||
// fields — `value` stays the single source of truth for those rows.
|
||||
//
|
||||
// v1.30.8 — validate the spread the same way `value` is guarded (above):
|
||||
// it now drives the DAY min/max band + the intraday envelope, so an
|
||||
// out-of-range or mis-ordered spread (a spurious `discreteMin` of 0, a
|
||||
// sensor glitch) would poison the aggregate exactly like a bogus `value`
|
||||
// would. Persist the spread ONLY when both extremes are present, in range,
|
||||
// and correctly ordered around `value`; otherwise drop to null and keep the
|
||||
// trustworthy average. Partial or invalid spread → avg-only, still valid.
|
||||
const isHrBucket = isAggregatedBucketExternalId(entry.externalId);
|
||||
const valueMin =
|
||||
isHrBucket && typeof mapped.valueMin === "number"
|
||||
? mapped.valueMin
|
||||
: null;
|
||||
const valueMax =
|
||||
isHrBucket && typeof mapped.valueMax === "number"
|
||||
? mapped.valueMax
|
||||
: null;
|
||||
const spreadValid =
|
||||
isHrBucket &&
|
||||
typeof mapped.valueMin === "number" &&
|
||||
typeof mapped.valueMax === "number" &&
|
||||
validateMeasurementRange(mapped.type, mapped.valueMin) === null &&
|
||||
validateMeasurementRange(mapped.type, mapped.valueMax) === null &&
|
||||
mapped.valueMin <= mapped.value &&
|
||||
mapped.value <= mapped.valueMax;
|
||||
const valueMin = spreadValid ? mapped.valueMin : null;
|
||||
const valueMax = spreadValid ? mapped.valueMax : null;
|
||||
|
||||
prepared.push({
|
||||
index,
|
||||
|
||||
@@ -90,6 +90,19 @@ describe("isAggregatedBucketExternalId", () => {
|
||||
])("returns false for a non-bucket externalId %s", (id) => {
|
||||
expect(isAggregatedBucketExternalId(id as string | null)).toBe(false);
|
||||
});
|
||||
|
||||
it.each([
|
||||
// v1.30.8 — these PARSE via V8's silent rollover but are NOT canonical, so
|
||||
// they'd key a row the canonical (toISOString) client never emits → an
|
||||
// un-mergeable duplicate + wrong-day placement. Must be rejected.
|
||||
"stats:HKQuantityTypeIdentifierHeartRate:2026-07-18T24:00:00.000Z", // → 07-19T00
|
||||
"stats:HKQuantityTypeIdentifierHeartRate:2026-04-31T14:30:00.000Z", // → 05-01
|
||||
"stats:HKQuantityTypeIdentifierHeartRate:2026-02-29T14:30:00.000Z", // non-leap → 03-01
|
||||
"stats:HKQuantityTypeIdentifierHeartRate:2026-12-31T24:00:00.000Z", // → 2027-01-01
|
||||
])("rejects a non-canonical rollover instant %s", (id) => {
|
||||
expect(isAggregatedBucketExternalId(id)).toBe(false);
|
||||
expect(parseAggregatedBucketStart(id)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
describe("targetsAggregatedBucket", () => {
|
||||
|
||||
@@ -897,7 +897,14 @@ export function isAggregatedBucketExternalId(
|
||||
const suffix = externalId.slice(prefix.length);
|
||||
if (!AGGREGATED_BUCKET_SUFFIX_RE.test(suffix)) return false;
|
||||
const parsed = new Date(suffix);
|
||||
return !Number.isNaN(parsed.getTime());
|
||||
if (Number.isNaN(parsed.getTime())) return false;
|
||||
// Reject a non-canonical instant that PARSES via V8's silent rollover
|
||||
// (`T24:00`, `2026-04-31`, a non-leap `02-29`) — it would key a row under
|
||||
// a literal string the canonical client (`heartRateBucketExternalId`, which
|
||||
// mints via `toISOString()`) never emits, so the two would never collapse
|
||||
// on the overwrite key. Round-trip equality pins the suffix to the one
|
||||
// canonical form.
|
||||
return parsed.toISOString() === suffix;
|
||||
}
|
||||
return false;
|
||||
}
|
||||
@@ -937,7 +944,10 @@ export function parseAggregatedBucketStart(
|
||||
const suffix = externalId.slice(prefix.length);
|
||||
if (!AGGREGATED_BUCKET_SUFFIX_RE.test(suffix)) return null;
|
||||
const parsed = new Date(suffix);
|
||||
return Number.isNaN(parsed.getTime()) ? null : parsed;
|
||||
if (Number.isNaN(parsed.getTime())) return null;
|
||||
// Canonical-form gate (mirrors `isAggregatedBucketExternalId`): a rolled-over
|
||||
// instant would place the bucket on the wrong local day.
|
||||
return parsed.toISOString() === suffix ? parsed : null;
|
||||
}
|
||||
return null;
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user