From 34bb471d3899feef67e149b66e9f5a9b265362de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Dandelion=20Man=C3=A9?= Date: Wed, 13 May 2020 11:44:23 -0700 Subject: [PATCH] Make TimestampMs validation optional (#1789) Currently, the Timestamp module requires that all TimestampMs types be validated via the `fromNumber` method. However, as discussed in #1788, this creates marginal benefit (just knowing that a TimestampMs is an integer does not give great confidence that it's a meaningful and correct timestamp for any use case), and creates a lot of overhead, especially in test code. It makes adopting the TimestampMs type across the codebase an arduous and mostly thankless task that would involve re-writing dozens or hundreds of test instances to add zero actual safety. Therefore, I've made the type non-opaque, and renamed `fromNumber` to `validate`. I then changed existing test code to not use the validator, since we don't need any manual validation of our test cases, and `123` is not a meaningful timestamp in any case :) Test plan: `yarn test` --- src/analysis/output.js | 5 +--- src/plugins/initiatives/createGraph.test.js | 7 +++--- src/plugins/initiatives/edgeSpec.test.js | 11 ++++---- src/plugins/initiatives/nodeEntry.test.js | 16 ++++++------ src/util/timestamp.js | 13 +++++----- src/util/timestamp.test.js | 28 ++++++++++----------- 6 files changed, 36 insertions(+), 44 deletions(-) diff --git a/src/analysis/output.js b/src/analysis/output.js index 57f1012..844df93 100644 --- a/src/analysis/output.js +++ b/src/analysis/output.js @@ -9,7 +9,6 @@ import {NodeAddress} from "../core/graph"; import type {Alias} from "../plugins/identity/alias"; import type {PluginDeclaration} from "./pluginDeclaration"; import type {TimestampMs} from "../util/timestamp"; -import * as Timestamp from "../util/timestamp"; import {TimelineCred} from "./timeline/timelineCred"; import {nodeWeightEvaluator} from "../core/algorithm/weightEvaluator"; @@ -66,14 +65,12 @@ export function fromTimelineCredAndPlugins( // know what period to mint it in. // When we transition to CredRank, we should remove this check. const minted = timestampMs == null ? 0 : nodeEvaluator(address); - const timestamp = - timestampMs == null ? null : Timestamp.fromNumber(timestampMs); return { address: NodeAddress.toParts(address), cred, minted, description, - timestamp, + timestamp: timestampMs, }; } ); diff --git a/src/plugins/initiatives/createGraph.test.js b/src/plugins/initiatives/createGraph.test.js index c22d139..7ac0408 100644 --- a/src/plugins/initiatives/createGraph.test.js +++ b/src/plugins/initiatives/createGraph.test.js @@ -7,7 +7,6 @@ import { NodeAddress, } from "../../core/graph"; import * as Weights from "../../core/weights"; -import * as Timestamp from "../../util/timestamp"; import sortBy from "../../util/sortBy"; import type {ReferenceDetector, URL} from "../../core/references"; import {createWeightedGraph, initiativeWeight} from "./createGraph"; @@ -30,7 +29,7 @@ import { const exampleEntry = (overrides: $Shape): NodeEntry => ({ key: overrides.title ? _titleSlug(overrides.title) : "sample-title", title: "Sample title", - timestampMs: Timestamp.fromNumber(123), + timestampMs: 123, contributors: [], weight: 456, ...overrides, @@ -40,7 +39,7 @@ function _createInitiative(overrides?: $Shape): Initiative { return { id: createId("UNSET_SUBTYPE", "42"), title: "Unset test initiative", - timestampMs: Timestamp.fromNumber(123), + timestampMs: 123, completed: false, contributions: {urls: [], entries: []}, dependencies: {urls: [], entries: []}, @@ -66,7 +65,7 @@ class MockInitiativeRepository implements InitiativeRepository { const initiative = _createInitiative({ id: createId("TEST_SUBTYPE", String(num)), title: `Example Initiative ${num}`, - timestampMs: Timestamp.fromNumber(400 + num), + timestampMs: 400 + num, ...shape, }); diff --git a/src/plugins/initiatives/edgeSpec.test.js b/src/plugins/initiatives/edgeSpec.test.js index 7900007..5a98ade 100644 --- a/src/plugins/initiatives/edgeSpec.test.js +++ b/src/plugins/initiatives/edgeSpec.test.js @@ -1,7 +1,6 @@ // @flow import {type TimestampMs} from "../../util/timestamp"; -import * as Timestamp from "../../util/timestamp"; import {type NodeEntry} from "./nodeEntry"; import { type EdgeSpec, @@ -14,7 +13,7 @@ import { const exampleEntry = (overrides: $Shape): NodeEntry => ({ key: "sample-title", title: "Sample title", - timestampMs: Timestamp.fromNumber(123), + timestampMs: 123, contributors: [], weight: 456, ...overrides, @@ -23,7 +22,7 @@ const exampleEntry = (overrides: $Shape): NodeEntry => ({ describe("plugins/initiatives/edgeSpec", () => { describe("normalizeEdgeSpec", () => { it("should add empty arrays when missing", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(321); + const timestampMs: TimestampMs = 321; const json: EdgeSpecJson = {}; const actual = normalizeEdgeSpec(json, timestampMs); @@ -31,7 +30,7 @@ describe("plugins/initiatives/edgeSpec", () => { }); it("should preserve urls when present", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(321); + const timestampMs: TimestampMs = 321; const json: EdgeSpecJson = { urls: ["https://foo.bar/a", "https://foo.bar/b"], }; @@ -41,7 +40,7 @@ describe("plugins/initiatives/edgeSpec", () => { }); it("should normalize any entries from json format", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(321); + const timestampMs: TimestampMs = 321; const json: EdgeSpecJson = { entries: [{title: "Minimal example"}], }; @@ -62,7 +61,7 @@ describe("plugins/initiatives/edgeSpec", () => { }); it("should throw on entries with the same key", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(321); + const timestampMs: TimestampMs = 321; const json: EdgeSpecJson = { entries: [ {key: "1", title: "first"}, diff --git a/src/plugins/initiatives/nodeEntry.test.js b/src/plugins/initiatives/nodeEntry.test.js index e3bd0ea..e20f543 100644 --- a/src/plugins/initiatives/nodeEntry.test.js +++ b/src/plugins/initiatives/nodeEntry.test.js @@ -52,14 +52,14 @@ describe("plugins/initiatives/nodeEntry", () => { describe("normalizeNodeEntry", () => { it("should throw without a title", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(123); + const timestampMs: TimestampMs = 123; const entry: NodeEntryJson = {key: "no-title"}; const f = () => normalizeNodeEntry(entry, timestampMs); expect(f).toThrow(TypeError); }); it("should handle a minimal entry", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(123); + const timestampMs = 123; const entry: NodeEntryJson = {title: "Most minimal"}; const expected: NodeEntry = { title: "Most minimal", @@ -72,7 +72,7 @@ describe("plugins/initiatives/nodeEntry", () => { }); it("should handle an entry with weights", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(123); + const timestampMs: TimestampMs = 123; const entry: NodeEntryJson = {title: "Include weight", weight: 42}; const expected: NodeEntry = { title: "Include weight", @@ -85,7 +85,7 @@ describe("plugins/initiatives/nodeEntry", () => { }); it("should handle an entry with contributors", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(123); + const timestampMs: TimestampMs = 123; const entry: NodeEntryJson = { title: "Include contributors", contributors: ["https://foo.bar/u/abc"], @@ -101,7 +101,7 @@ describe("plugins/initiatives/nodeEntry", () => { }); it("should handle an entry with timestamp", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(123); + const timestampMs: TimestampMs = 123; const entry: NodeEntryJson = { title: "Include timestamp", timestampIso: Timestamp.toISO(Date.parse("2018-02-03T12:34:56.789Z")), @@ -110,16 +110,14 @@ describe("plugins/initiatives/nodeEntry", () => { title: "Include timestamp", key: "include-timestamp", contributors: [], - timestampMs: Timestamp.fromNumber( - Date.parse("2018-02-03T12:34:56.789Z") - ), + timestampMs: Date.parse("2018-02-03T12:34:56.789Z"), weight: null, }; expect(normalizeNodeEntry(entry, timestampMs)).toEqual(expected); }); it("should handle an entry with key", () => { - const timestampMs: TimestampMs = Timestamp.fromNumber(123); + const timestampMs: TimestampMs = 123; const entry: NodeEntryJson = { title: "Include key", key: "much-different-key", diff --git a/src/util/timestamp.js b/src/util/timestamp.js index 527492b..963fb48 100644 --- a/src/util/timestamp.js +++ b/src/util/timestamp.js @@ -9,7 +9,7 @@ */ // A timestamp representation in ms since epoch. -export opaque type TimestampMs: number = number; +export type TimestampMs = number; // A timestamp representation in ISO 8601 format. export opaque type TimestampISO: string = string; @@ -22,7 +22,7 @@ export opaque type TimestampISO: string = string; * than a forced refactor across the codebase. */ export function toISO(timestampLike: TimestampMs | number): TimestampISO { - const timestampMs: TimestampMs = fromNumber(timestampLike); + const timestampMs: TimestampMs = validate(timestampLike); return new Date(timestampMs).toISOString(); } @@ -47,13 +47,12 @@ export function fromISO(timestampISO: TimestampISO): TimestampMs { } /** - * Creates a TimestampMs from a number input. + * Validate that a number is potentially a valid timestamp. * - * Since much of the previous types have used `number` as a type instead of - * TimestampMs. Accepting `number` will give an easier upgrade path, rather - * than a forced refactor across the codebase. + * This checks that the number is a finite integer, which avoids some potential + * numbers that are not valid timestamps. */ -export function fromNumber(timestampMs: number): TimestampMs { +export function validate(timestampMs: number): TimestampMs { const asNumber = Number(timestampMs); if ( timestampMs === null || diff --git a/src/util/timestamp.test.js b/src/util/timestamp.test.js index edfc7cb..2ac4814 100644 --- a/src/util/timestamp.test.js +++ b/src/util/timestamp.test.js @@ -1,7 +1,7 @@ // @flow import type {TimestampMs, TimestampISO} from "./timestamp"; -import {fromISO, toISO, fromNumber} from "./timestamp"; +import {fromISO, toISO, validate} from "./timestamp"; /** * Helper function to write readable "toThrow" tests. This intentionally @@ -33,14 +33,14 @@ describe("util/timestamp", () => { describe("roundtrips", () => { it("should handle number roundtrips", () => { const origin: number = fullExample.ms; - expect(fromNumber(origin)).toBe(origin); + expect(validate(origin)).toBe(origin); expect(fromISO(toISO(origin))).toBe(origin); - expect(fromNumber(fromISO(toISO(origin)))).toBe(origin); + expect(validate(fromISO(toISO(origin)))).toBe(origin); }); it("should handle ISO roundtrips", () => { const origin: TimestampISO = fullExample.ISO; expect(toISO(fromISO(origin))).toBe(origin); - expect(toISO(fromNumber(fromISO(origin)))).toBe(origin); + expect(toISO(validate(fromISO(origin)))).toBe(origin); }); }); @@ -96,34 +96,34 @@ describe("util/timestamp", () => { }); }); - describe("fromNumber", () => { + describe("validate", () => { it("should throw on null", () => { - given(null).expect(fromNumber).toThrow(TypeError); + given(null).expect(validate).toThrow(TypeError); }); it("should throw on undefined", () => { - given(undefined).expect(fromNumber).toThrow(TypeError); + given(undefined).expect(validate).toThrow(TypeError); }); it("should throw on NaN", () => { - given(NaN).expect(fromNumber).toThrow(TypeError); + given(NaN).expect(validate).toThrow(TypeError); }); it("should throw on Infinity", () => { - given(Infinity).expect(fromNumber).toThrow(TypeError); + given(Infinity).expect(validate).toThrow(TypeError); }); it("should throw on floating point numbers", () => { given(1 / 3) - .expect(fromNumber) + .expect(validate) .toThrow(TypeError); }); it("should handle 0 correctly", () => { - expect(fromNumber(0)).toBe(0); + expect(validate(0)).toBe(0); }); it("should handle Date.now correctly", () => { const now = Date.now(); - expect(fromNumber(now)).toBe(now); + expect(validate(now)).toBe(now); }); it("should handle examples correctly", () => { - expect(fromNumber(fullExample.ms)).toBe(fullExample.ms); - expect(fromNumber(partialExample.ms)).toBe(partialExample.ms); + expect(validate(fullExample.ms)).toBe(fullExample.ms); + expect(validate(partialExample.ms)).toBe(partialExample.ms); }); }); });