diff --git a/src/__tests__/provider-rounding-guard.test.ts b/src/__tests__/provider-rounding-guard.test.ts index 10fe0b655..6c4b279c2 100644 --- a/src/__tests__/provider-rounding-guard.test.ts +++ b/src/__tests__/provider-rounding-guard.test.ts @@ -1,11 +1,14 @@ /** - * Structural guard: a provider client rounds an imported value through - * `importValue` (`src/lib/integrations/import-value.ts`) and nothing else. + * Structural guard: a provider client or a file importer rounds an imported + * value through `importValue` (`src/lib/integrations/import-value.ts`) and + * nothing else. * * Each client used to keep its own `parseFloat(n.toFixed(2))`. Five copies * of one rule drift apart, and `toFixed` mis-rounds halves on the double - * below the decimal. A `toFixed` or a private `round2` in a provider - * directory is the class coming back. + * below the decimal. The Health Connect importer had the other spelling, + * `Math.round(x * 1000) / 1000`, in three places and stored its unit + * factors' noise unrounded everywhere else. A `toFixed`, a private `round2` + * or a scaled `Math.round` in these directories is the class coming back. */ import { readFileSync } from "node:fs"; import { join } from "node:path"; @@ -25,8 +28,18 @@ const PROVIDER_DIRS = [ "strava", "whoop", "withings", + "import", ]; -const ROUNDING = /\.toFixed\s*\(|\b(?:function|const|let)\s+round2\b/; +const ROUNDING = + /\.toFixed\s*\(|\b(?:function|const|let)\s+round2\b|Math\.round\s*\([^;]*\*\s*(10+)\s*\)\s*\/\s*\1\b/; + +/** Files that round for display, not on the way in, and say so. */ +const ALLOWED: Record = { + "dawarich/movement.ts": + "Formats a stored distance to one decimal in the account's unit for the day view; nothing is stored.", + "import/unzip-export-xml.ts": + "Prints a compression ratio into an error message.", +}; function strip(text: string): string { return text @@ -47,8 +60,10 @@ describe("provider clients round through importValue", () => { }); it("finds no rounding of their own", () => { - const offenders = files.filter((f) => - ROUNDING.test(strip(readFileSync(join(LIB, f), "utf8"))), + const offenders = files.filter( + (f) => + !(f in ALLOWED) && + ROUNDING.test(strip(readFileSync(join(LIB, f), "utf8"))), ); expect(offenders).toEqual([]); }); @@ -57,6 +72,12 @@ describe("provider clients round through importValue", () => { expect(ROUNDING.test("const v = parseFloat(n.toFixed(2));")).toBe(true); expect(ROUNDING.test("function round2(n: number) {}")).toBe(true); expect(ROUNDING.test("const round2 = (n: number) => n;")).toBe(true); + expect(ROUNDING.test("const a = Math.round(e.amount * 1000) / 1000;")).toBe( + true, + ); + expect(ROUNDING.test("value: Math.round(mean * 10) / 10,")).toBe(true); expect(ROUNDING.test("value: importValue(n)")).toBe(false); + expect(ROUNDING.test("Math.round((end - start) / 1000)")).toBe(false); + expect(ROUNDING.test("Math.round(mmol * MGDL_PER_MMOL)")).toBe(false); }); }); diff --git a/src/lib/import/health-connect/import.ts b/src/lib/import/health-connect/import.ts index a36b8e20f..a35f8d481 100644 --- a/src/lib/import/health-connect/import.ts +++ b/src/lib/import/health-connect/import.ts @@ -44,6 +44,7 @@ * Privacy: the result carries counts only, per type and per app. No value, * no timestamp and nothing a person typed (notes, titles) leaves this module. */ +import { importValue } from "@/lib/integrations/import-value"; import type { FlowLevel, GlucoseContext, @@ -547,7 +548,7 @@ class ImportRun { this.stat("PULSE").read += 1; await sink.push({ type: "PULSE", - value: Math.round(mean * 10) / 10, + value: importValue(mean, 1), valueMin: hcNumber(best.lo), valueMax: hcNumber(best.hi), // The middle of the local hour, in the record's own zone. @@ -889,7 +890,10 @@ class ImportRun { const days: Array<{ dayKey: string; value: number }> = []; for (const day of this.dailyGroups(sql, HC_CATEGORY.ACTIVITY)) { this.stat(spec.type).read += 1; - days.push({ dayKey: day.dayKey, value: day.total * spec.factor }); + days.push({ + dayKey: day.dayKey, + value: importValue(day.total * spec.factor), + }); if (days.length >= WRITE_BATCH) { await this.writeDayTotals(spec.type, days.splice(0)); } @@ -1136,7 +1140,7 @@ class ImportRun { ): Promise { if (entries.length === 0) return; const valid = entries.filter((e) => { - const amount = Math.round(e.amount * 1000) / 1000; + const amount = importValue(e.amount, 3); if (amount > NUTRIENT_CATALOG[e.code].plausibleDailyMax) { this.nutrients.outOfRange += 1; this.skip("out_of_range", 1, `nutrient:${e.code}`); @@ -1160,7 +1164,7 @@ class ImportRun { ); const fresh: Prisma.NutrientIntakeDayCreateManyInput[] = []; for (const e of valid) { - const amount = Math.round(e.amount * 1000) / 1000; + const amount = importValue(e.amount, 3); const prior = existing.get(`${e.day}|${e.code}`); if (prior === undefined) { fresh.push({ @@ -1597,8 +1601,19 @@ class MeasurementSink { private readonly hourlyPulse = false, ) {} + /** + * Every measurement value this import writes passes here (or through + * `writeDayTotals`), so this is where it takes the one import rounding: + * a mapper's unit factor (grams to kilograms, millis to minutes) leaves + * binary noise that would otherwise be stored and compared as is. + */ async push(row: PendingRow): Promise { - this.rows.push(row); + this.rows.push({ + ...row, + value: importValue(row.value), + valueMin: row.valueMin == null ? row.valueMin : importValue(row.valueMin), + valueMax: row.valueMax == null ? row.valueMax : importValue(row.valueMax), + }); if (this.rows.length >= WRITE_BATCH) await this.flush(); } diff --git a/src/lib/integrations/__tests__/import-value.test.ts b/src/lib/integrations/__tests__/import-value.test.ts index ff83ad1bf..73aa3faf1 100644 --- a/src/lib/integrations/__tests__/import-value.test.ts +++ b/src/lib/integrations/__tests__/import-value.test.ts @@ -26,4 +26,12 @@ describe("importValue", () => { expect(importValue(1e-7)).toBe(0); expect(importValue(1e22)).toBe(1e22); }); + + it("rounds to another precision on request, halves away from zero", () => { + expect(importValue(72.34999, 1)).toBe(72.3); + expect(importValue(1.0005, 3)).toBe(1.001); + expect(importValue(-2.5, 0)).toBe(-3); + expect(importValue(0.30000000000000004, 3)).toBe(0.3); + expect(() => importValue(1, 7)).toThrow(RangeError); + }); }); diff --git a/src/lib/integrations/import-value.ts b/src/lib/integrations/import-value.ts index a3de3d846..616b8126c 100644 --- a/src/lib/integrations/import-value.ts +++ b/src/lib/integrations/import-value.ts @@ -9,20 +9,27 @@ * also mis-rounds on halves (`(1.005).toFixed(2)` is `"1.00"`, because the * double below 1.005 is what gets rounded). * - * Two decimals, rounded half away from zero on the decimal the value prints - * as, so the result is the double nearest to a two-place decimal and prints - * cleanly. Two places is the precision every provider client already stored; - * this keeps it. `src/__tests__/provider-rounding-guard.test.ts` keeps - * `toFixed` out of the provider directories. + * Two decimals by default, rounded half away from zero on the decimal the + * value prints as, so the result is the double nearest to a short decimal + * and prints cleanly. Two places is the precision every provider client + * already stored; this keeps it. A source whose values need another + * precision (a nutrient in grams to the milligram) passes `places` rather + * than rounding privately. `src/__tests__/provider-rounding-guard.test.ts` + * keeps `toFixed` and hand-rolled `Math.round(x * 10^n) / 10^n` out of the + * provider and import directories. */ -export function importValue(n: number): number { +export function importValue(n: number, places = 2): number { if (!Number.isFinite(n)) return n; + if (!Number.isInteger(places) || places < 0 || places > 6) { + throw new RangeError(`importValue: places must be 0..6, got ${places}`); + } const magnitude = Math.abs(n); const text = String(magnitude); + const scale = 10 ** places; // A value printed in exponent form is below a millionth or beyond 1e21; // shifting its text would not parse, and plain arithmetic is exact enough. - const hundredths = text.includes("e") - ? Math.round(magnitude * 100) - : Math.round(Number(`${text}e2`)); - return (Math.sign(n) * hundredths) / 100 || 0; + const units = text.includes("e") + ? Math.round(magnitude * scale) + : Math.round(Number(`${text}e${places}`)); + return (Math.sign(n) * units) / scale || 0; } diff --git a/tests/integration/health-connect-import.test.ts b/tests/integration/health-connect-import.test.ts index 87cd33fd9..e7dceb5bd 100644 --- a/tests/integration/health-connect-import.test.ts +++ b/tests/integration/health-connect-import.test.ts @@ -20,6 +20,7 @@ import { rmSync, writeFileSync, } from "node:fs"; +import { DatabaseSync } from "node:sqlite"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterAll, beforeEach, describe, expect, it, vi } from "vitest"; @@ -233,6 +234,29 @@ describe("Health Connect import, mapping", () => { expect(JSON.stringify(result)).not.toContain("Evening run"); }); + it("stores a scaled value with the import rounding, not its unit factor's noise", async () => { + const prisma = getPrismaClient(); + const user = await createUser("hc-round"); + const { path } = fixtureDb("round", { days: 1, endUtc: today }); + // Grams to kilograms: 80 345.678 g is 80.345678 kg before rounding. + const db = new DatabaseSync(path); + db.exec("UPDATE weight_record_table SET weight = 80345.678"); + db.close(); + await importHealthConnectExport({ + prisma, + dbPath: path, + userId: user.id, + userTimezone: "Europe/Berlin", + connectedIntegrations: [], + }); + const weights = await prisma.measurement.findMany({ + where: { userId: user.id, type: "WEIGHT", source: "HEALTH_CONNECT" }, + select: { value: true }, + }); + expect(weights.length).toBeGreaterThan(0); + for (const w of weights) expect(w.value).toBe(80.35); + }); + it("writes nothing the second time the same export comes in", async () => { const prisma = getPrismaClient(); const user = await createUser("hc-twice");