GluuFederation / GluuFederation/flex

feat(jans-fido2): add unit tests for Fido2MetricsAggregation computed surface

Open
#2,864 0 comments 0 reactions 1 assignee Claimed by @imran-ishaq View on GitHub
enhancement kind-feature
Dominant language
TypeScript
Stars
31
Forks
21
Avg merge
2d 4h
Merged PRs (30d)
29

Description

**Is your feature request related to a problem? Please describe.**
`Fido2MetricsAggregation` (`jans-fido2/model/src/main/java/io/jans/fido2/model/metric/Fido2MetricsAggregation.java`) is the ORM entity that stores hourly/daily/weekly/monthly summary rows for the FIDO2 metrics feature. Most of its surface is plain getters/setters, but it has several small bits of computed behavior that matter and are completely untested today:

- A 4-arg constructor that builds the composite `id` as `aggregationType + "_" + period` and stamps `lastUpdated` in UTC.
- `getPeriod()` — derives the period string back out of `id` by splitting on the first `_`.
- `getLongMetric(key)` / `getDoubleMetric(key)` — null-safe `Number` coercion that returns `null` (not NaN, not NPE) when the underlying map entry is absent or non-numeric.
- `getDeviceTypes()` / `getErrorCounts()` — Map accessors that coerce `Integer` values to `Long` for JSON-serialization compatibility (exactly the shape Jackson hands back for JSON numbers).
- `incrementMetric(key, increment)` — null-safe accumulator (treats both nulls as `0`).
- A custom `equals`/`hashCode` based on (`id`, `aggregationType`, `startTime`, `endTime`) — ignoring `uniqueUsers` and `metricsData`.

Each method has explicit null-safety and type-coercion handling — silent failure modes would surface as wrong aggregation totals, dropped Integer→Long conversions, or NPE against partially-loaded ORM entries, all without compile-time signal. The class is small and self-contained — ideal for a quick test pass that locks the behaviors downstream aggregation/analytics code silently depends on.

This is step 7 of the FIDO2 metrics test rollout (steps 1–6 covered `Fido2MetricType`, `Fido2MetricsConstants`, `Fido2MetricsData`, `UserMetricsUpdateRequest`, `Fido2UserMetrics` rate calculations, and `Fido2UserMetrics` state-mutation methods).

**Describe the solution you'd like**
Add a new JUnit 5 test class:

- File: `jans-fido2/model/src/test/java/io/jans/fido2/model/metric/Fido2MetricsAggregationTest.java`

The test should cover, per behavior group (one `@Test` per group):

- **Default constructor:** `metricsData` is a non-null empty map after `new Fido2MetricsAggregation()`; all other fields are `null`.
- **4-arg constructor composes the id and stamps `lastUpdated`:** `new Fido2MetricsAggregation("DAILY", "2026-05-22", start, end)` produces `id == "DAILY_2026-05-22"`, sets `aggregationType`, `startTime`, `endTime` to the passed values, and stamps `lastUpdated` to a non-null value within ±1s of `now`. The default-ctor invariant (`metricsData` non-null and empty) must still hold via `this()` delegation.
- **`getPeriod()`:** with `id == "DAILY_2026-05-22"`, returns `"2026-05-22"`; with `id == "HOURLY_2026-05-22_14"` (multi-`_`), returns `"2026-05-22_14"` to lock split-on-FIRST-`_`; with `id == "NOPERIOD"` (no underscore), returns `"NOPERIOD"`; with `id == null`, returns `null`.
- **`getLongMetric` / `getDoubleMetric` null-safety:** return `null` when `metricsData` itself is `null` (use `setMetricsData(null)`), and when the key is absent.
- **`getLongMetric` widens Integer/truncates Double:** when the underlying entry is a `Long`, returns the value; when it's an `Integer`, asserts it's widened to `Long`; when it's a `Double`, asserts `longValue()` truncates toward zero.
- **`getDoubleMetric` accepts Integer:** when the underlying entry is an `Integer`, returns the value as a `Double` (proves the `Number` path doesn't reject non-Double numerics).
- **`getLongMetric` rejects non-numbers:** stash `"not-a-number"` (a `String`) under a key, assert both `getLongMetric(key)` and `getDoubleMetric(key)` return `null` (not a `ClassCastException`).
- **Convenience setters/getters wire to the right constant keys:** call `setRegistrationAttempts(42L)` and assert (a) `getRegistrationAttempts() == 42L`, and (b) `getMetricsData().get(Fido2MetricsConstants.REGISTRATION_ATTEMPTS).equals(42L)`. Pick one more pair (`setAuthenticationSuccessRate(0.85)` / `getAuthenticationSuccessRate()`) to prove the `Double` path stores under `AUTHENTICATION_SUCCESS_RATE`. No need to test all 14 pairs.
- **`getDeviceTypes()` coerces Integer→Long:** populate the inner map with `Integer` values (the exact shape Jackson hands back for JSON numbers) and assert `getDeviceTypes()` returns a `Map` with widened values.
- **`getDeviceTypes()` / `getErrorCounts()` return empty (not null) when absent:** assert empty map, not null, when the key is missing from `metricsData`.
- **`incrementMetric` null-safety on both sides:** starting from an empty map, `incrementMetric("foo", 5L)` results in `getLongMetric("foo") == 5L`; calling it again with `3L` gives `8L`; calling it with `null` increment keeps it at `8L`; calling it on a fresh instance with `null` increment results in `0L` (current treated as 0, increment as 0).
- **`equals` / `hashCode` contract:** two instances with identical (`id`, `aggregationType`, `startTime`, `endTime`) are equal and share a hash; differing in any of the four breaks equality; differing in unrelated fields (`uniqueUsers`, `metricsData`) preserves equality. Include reflexive `x.equals(x)` and `x.equals(null)` / `x.equals("string")` cases.

Conventions to follow (same as `Fido2MetricTypeTest` through `Fido2UserMetricsRateCalculationsTest`):
- JUnit 5 (`org.junit.jupiter.api`).
- No Mockito, no async, no CDI — pure entity behavior.
- No additional dependencies — `junit-jupiter-api` and `junit-jupiter-engine` test-scope deps are already declared in `jans-fido2/model/pom.xml`.
- Floating-point assertions use `assertEquals(expected, actual, 1e-9)` to avoid spurious comparison failures.
- One `@Test` per behavior group with a descriptive name (e.g., `testFourArgConstructorComposesIdAndStampsLastUpdated`, `testGetLongMetricReturnsNullForNonNumberValue`, `testIncrementMetricIsNullSafeOnBothSides`).

Acceptance criteria:
- [ ] New test file exists at the path above.
- [ ] All behaviors listed are covered.
- [ ] `mvn -pl model -am test` from `jans-fido2/` passes locally.
- [ ] Test class runs in well under 1 second (no I/O, no `Thread.sleep`).
- [ ] No changes to production code under `src/main`.
- [ ] No pom changes required.

**Describe alternatives you've considered**
- Exhaustively testing every one of the ~14 convenience getter/setter pairs: rejected — they're mechanical wrappers around `getLongMetric` / `setMetric` against a constant key. Testing one `Long` pair + one `Double` pair proves the wiring; the rest follows.
- Splitting `equals`/`hashCode` into its own issue: rejected — it's a few assertions; bundling keeps the PR coherent.
- Adding Java native serialization roundtrip tests: rejected as overkill — no custom `readObject`/`writeObject`, plain JavaBeans.
- Testing `getPeriod()` only with a single happy-path id: rejected — the split-on-FIRST-`_` behavior is the load-bearing detail; a multi-`_` id (e.g., `HOURLY_2026-05-22_14`) is needed to catch a refactor that switches to `split("_")` or `lastIndexOf("_")`.
- Asserting `lastUpdated` is exactly `new Date()` from the ctor: rejected — clock skew between `before`/`after` reads makes that brittle. A ±1s window is tight enough to catch a missing stamp and loose enough to absorb scheduling jitter.

**Additional context**
Roadmap position: step 7 of the FIDO2 metrics test rollout, intentionally chosen as a small breather between the heavier server-side work. Next planned step is to start covering `Fido2MetricsService` itself — that work will be split across several focused issues by responsibility (storage/ingest, query, aggregation, analytics).

Same Maven module (`jans-fido2/model`) as issues #1–#6; no pom changes required.

Suggested label: `kind-feature`.

Contributor guide

No contributing guide indexed for this repository

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.