Bug 1710827 - Fail validation of counter and quantity if value is not int (#319)

Author: Beatriz Rizental

CHANGELOG.md

@@ -3,6 +3,7 @@
 [Full changelog](https://github.com/mozilla/glean.js/compare/v0.12.0...main)
 
 * [#313](https://github.com/mozilla/glean.js/pull/313): Implement error recording mechanism and error checking testing API.
+* [#319](https://github.com/mozilla/glean.js/pull/319): BUGFIX: Do not allow recording floats with the quantity and counter metric types.
 
 # v0.12.0 (2021-05-11)
 

glean/src/core/error/index.ts

@@ -4,8 +4,8 @@
 
 import type { MetricType } from "../metrics";
 import type { ErrorType } from "./error_type.js";
-import CounterMetricType from "../metrics/types/counter";
-import { combineIdentifierAndLabel, stripLabel } from "../metrics/types/labeled";
+import CounterMetricType from "../metrics/types/counter.js";
+import { combineIdentifierAndLabel, stripLabel } from "../metrics/types/labeled.js";
 
 /**
  * For a given metric, get the metric in which to record errors.

glean/src/core/metrics/types/counter.ts

@@ -5,7 +5,7 @@
 import type { CommonMetricData } from "../index.js";
 import type { JSONValue } from "../../utils.js";
 import { MetricType } from "../index.js";
-import { isUndefined, isNumber } from "../../utils.js";
+import { isUndefined, isInteger } from "../../utils.js";
 import { Context } from "../../context.js";
 import { Metric } from "../metric.js";
 import { ErrorType } from "../../error/error_type.js";
@@ -16,7 +16,7 @@ export class CounterMetric extends Metric<number, number> {
   }
 
   validate(v: unknown): v is number {
-    if (!isNumber(v)) {
+    if (!isInteger(v)) {
       return false;
     }
 

glean/src/core/metrics/types/quantity.ts

@@ -4,7 +4,7 @@
 
 import type { CommonMetricData } from "../index.js";
 import { MetricType } from "../index.js";
-import { isNumber } from "../../utils.js";
+import { isInteger } from "../../utils.js";
 import { Context } from "../../context.js";
 import { Metric } from "../metric.js";
 import { ErrorType } from "../../error/error_type.js";
@@ -15,7 +15,7 @@ export class QuantityMetric extends Metric<number, number> {
   }
 
   validate(v: unknown): v is number {
-    if (!isNumber(v)) {
+    if (!isInteger(v)) {
       return false;
     }
 

glean/src/core/utils.ts

@@ -75,7 +75,7 @@ export function isUndefined(v: unknown): v is undefined {
  *          stating whether `v` is a string.
  */
 export function isString(v: unknown): v is string {
-  return (typeof v === "string" || (typeof v === "object" && v !== null && v.constructor === String));
+  return typeof v === "string";
 }
 
 /**
@@ -87,7 +87,7 @@ export function isString(v: unknown): v is string {
  *          stating whether `v` is a boolean.
  */
 export function isBoolean(v: unknown): v is boolean {
-  return (typeof v === "boolean" || (typeof v === "object" && v !== null && v.constructor === Boolean));
+  return typeof v === "boolean";
 }
 
 /**
@@ -99,7 +99,19 @@ export function isBoolean(v: unknown): v is boolean {
  *          stating whether `v` is a number.
  */
 export function isNumber(v: unknown): v is number {
-  return ((typeof v === "number" || (typeof v === "object" && v !== null && v.constructor === Number)) && !isNaN(v));
+  return typeof v === "number" && !isNaN(v);
+}
+
+/**
+ * Checks whether or not `v` is an integer.
+ *
+ * @param v The value to verify.
+ *
+ * @returns A special Typescript value (which compiles down to a boolean)
+ *          stating whether `v` is a number.
+ */
+export function isInteger(v: unknown): v is number {
+  return isNumber(v) && Number.isInteger(v);
 }
 
 /**

glean/tests/core/utils.spec.ts

@@ -50,7 +50,6 @@ describe("utils", function() {
     assert.strictEqual(utils.isString(""), true);
     assert.strictEqual(utils.isString("something else"), true);
     assert.strictEqual(utils.isString(obj["test"]), true);
-    assert.strictEqual(utils.isString(new String("check")), true);
   });
 
   it("isBoolean validates correctly", function() {
@@ -62,7 +61,6 @@ describe("utils", function() {
     // Valid values
     assert.strictEqual(utils.isBoolean(true), true);
     assert.strictEqual(utils.isBoolean(false), true);
-    assert.strictEqual(utils.isBoolean(new Boolean(true)), true);
     assert.strictEqual(utils.isBoolean(!!"something else"), true);
   });
 
@@ -76,7 +74,31 @@ describe("utils", function() {
     // Valid values
     assert.strictEqual(utils.isNumber(10), true);
     assert.strictEqual(utils.isNumber(-10), true);
-    assert.strictEqual(utils.isNumber(new Number(10)), true);
+  });
+
+  it("isInteger validates correctly", function() {
+    // Invalid values
+    assert.strictEqual(utils.isInteger(undefined), false);
+    assert.strictEqual(utils.isInteger("10"), false);
+    assert.strictEqual(utils.isInteger({}), false);
+    assert.strictEqual(utils.isInteger(NaN), false);
+    assert.strictEqual(utils.isInteger(0.1), false);
+    assert.strictEqual(utils.isInteger(Math.PI), false);
+    assert.strictEqual(utils.isInteger(Infinity), false);
+    assert.strictEqual(utils.isInteger(-Infinity), false);
+    assert.strictEqual(utils.isInteger(true), false);
+    assert.strictEqual(utils.isInteger(false), false);
+    assert.strictEqual(utils.isInteger([1]), false);
+    assert.strictEqual(utils.isInteger(5.000000000000001), false);
+
+    // Valid values
+    assert.strictEqual(utils.isInteger(10), true);
+    assert.strictEqual(utils.isInteger(-10), true);
+    assert.strictEqual(utils.isInteger(0), true);
+    assert.strictEqual(utils.isInteger(-100000), true);
+    assert.strictEqual(utils.isInteger(99999999999999999999999), true);
+    assert.strictEqual(utils.isInteger(5.0), true);
+    assert.strictEqual(utils.isInteger(5.0000000000000001), true);
   });
 
   it("sanitizeApplicationId works correctly", function() {