fix(daily-events): temporarly convert invalid timesamps - #680
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved cursor pagination and timestamp/count sanitization issues must be addressed before approval.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This PR temporarily sanitizes legacy timestamps and oversized counts in dailyEventsPortion while retaining the GraphQL Int schema.
Changes:
- Adds GraphQL-safe timestamp, count, ObjectId, and cursor helpers.
- Sanitizes daily-event responses and pagination cursors.
- Adds regression tests and bumps the version to
1.5.15.
File summaries
| File | Summary |
|---|---|
test/utils/graphqlIntSafe.test.ts |
Adds helper coverage. |
test/resolvers/project-daily-events-portion.test.ts |
Adds resolver regression coverage. |
src/utils/graphqlIntSafe.js |
Moderate (2 votes): Millisecond normalization can still be reported as safe. Moderate (3 votes): Count-based cursor boundaries may be misclassified as timestamps. |
src/resolvers/project.js |
Critical (3 votes): Converted cursors can become incomparable with raw Mongo values and skip rows. Moderate (1 vote): Raw millisecond repetition times can produce overflowing grouping timestamps. Moderate (1 vote): Sanitizing timestamp alone can make it inconsistent with originalTimestamp and alter valid Float values. |
package.json |
Bumps the version to 1.5.15. |
Review details
Suppressed comments (2)
src/resolvers/project.js:157
Event.timestampis declared asFloat!, so it does not need 32-bit Int sanitization. For an event without a repetition, the factory setsoriginalTimestampequal totimestamp; rewriting onlytimestamphere leaves the response internally inconsistent, and also changes valid Float timestamps outside the helper's 10-year window. Restrict this conversion to Int-backed fields or sanitize both timestamp fields consistently.
const safeEventTimestamp = toSafeUnixTimestampForGraphQLInt(
event.timestamp,
fallbackId,
nowSec
);
src/resolvers/project.js:133
- When
lastRepetitionTimeis stored in milliseconds,isUnsafeUnixTimestampnormalizes it and marks it safe, but this branch passes the raw millisecond value toutcMidnightUnix. That produces a grouping timestamp around 1e12 seconds, which is still outside GraphQLIntand can reproduce the serialization failure. UsesafeLastRepetitionTimefor the numeric branch so the grouping calculation uses the normalized seconds value.
: (typeof dailyEvent.lastRepetitionTime === 'number'
? dailyEvent.lastRepetitionTime
: safeGroupingTimestamp)
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
🔵 Needs a closer look
Moderate timestamp and pagination issues remain, along with resolver coverage gaps.
Review details
Suppressed comments (9)
Previously missed (1) — in code that hasn't changed since the last review.
src/utils/graphqlIntSafe.js:145
- The sanitizer returns
NaN/Infinityunchanged. Those values can still reachDailyEvent.groupingTimestamporevent.timestampand make GraphQL serialization fail, so non-finite numeric timestamps need the same ObjectId/current-time fallback as out-of-range timestamps.
src/resolvers/project.js:159
event.timestampis declared as GraphQLFloat!, notInt!, but this helper always truncates it throughnormalizeUnixSeconds. A valid fractional-seconds timestamp such as1700000000.5is therefore changed to1700000000even though it is representable by the schema; the same path also applies the 10-year/future fallback policy to a field that does not have the reported Int overflow. Restrict this conversion to the Int-backed fields, or preserve Float timestamps unless a separate, explicit millisecond-normalization contract is required.
const safeEventTimestamp = toSafeUnixTimestampForGraphQLInt(
event.timestamp,
fallbackId,
nowSec
);
src/resolvers/project.js:86
- Converting the cursor to ObjectId time (or Int max) changes the boundary that
findDailyEventsPortionuses against the raw Mongo fields. If a page ends on a legacy row, the next request will comparegroupingTimestamp/the sort field to the sanitized value, so remaining legacy rows with their original far-future timestamp or oversized count are filtered out and silently disappear from pagination. Preserve a raw continuation token or update the factory query to account for the sanitized boundary instead of returning a cursor with different ordering semantics.
return {
...cursor,
groupingTimestampBoundary: safeGrouping,
sortValueBoundary: safeSort,
src/resolvers/project.js:143
- The resolver-level tests only use ordinary
count/affectedUsersvalues, so this new daily-event sanitization path is not covered end to end. Add a fixture with values aboveGRAPHQL_INT_MAXand assert the resolver returns capped fields; otherwise a regression here can still surface the original GraphQLIntfailure despite the utility test.
const safeCount = typeof (dailyEvent && dailyEvent.count) === 'number'
? toSafeGraphQLInt(dailyEvent.count, 0)
: dailyEvent.count;
const safeAffectedUsers = typeof (dailyEvent && dailyEvent.affectedUsers) === 'number'
? toSafeGraphQLInt(dailyEvent.affectedUsers, 0)
src/resolvers/project.js:154
- The nested event count sanitization is likewise only exercised with normal values in the new resolver test (
totalCount: 13692,usersAffected: 0). Add an end-to-end case for oversizedtotalCount/usersAffectedso the GraphQL-facing mapping is protected, not justtoSafeGraphQLIntin isolation.
const safeTotalCount = typeof event.totalCount === 'number'
? toSafeGraphQLInt(event.totalCount, 0)
: event.totalCount;
const safeUsersAffected = typeof event.usersAffected === 'number'
? toSafeGraphQLInt(event.usersAffected, 0)
: event.usersAffected;
src/utils/graphqlIntSafe.js:18
- This makes every timestamp older than
nowSec - MAX_PAST_SECunsafe even when it is well within GraphQL's signed 32-bit range.dailyEventsPortionhas no date-retention filter, so a legitimate historical event (for example, from 2015) will have itslastRepetitionTime, event timestamp, and possibly day bucket rewritten to the ObjectId receive time. Please restrict this fallback to the actual overflow/invalid-future cases, or establish and enforce an explicit retention guarantee before applying this 10-year cutoff.
const MAX_PAST_SEC = 10 * 365.25 * 24 * 60 * 60;
src/utils/graphqlIntSafe.js:43
NaNandInfinityare not representable by GraphQLInt, but this branch reports both as in-range. Any caller relying on this predicate can therefore pass a non-finite value through to serialization; treat non-finite numbers as out of range.
if (typeof value !== 'number' || !Number.isFinite(value)) {
return false;
}
src/utils/graphqlIntSafe.js:205
- Cursor boundaries take this early return before reaching either
toSafeUnixTimestampForGraphQLIntortoSafeGraphQLInt, so a non-finitesortValueBoundaryis emitted unchanged and theInt!cursor field still throws during GraphQL serialization. Only bypass non-number values here; let non-finite numbers be normalized.
if (typeof value !== 'number' || !Number.isFinite(value)) {
return value;
}
src/utils/graphqlIntSafe.js:171
isUnsafeUnixTimestampalso classifiesNaN/Infinityas safe. InsanitizeDailyEvent, that leavesgroupingNeedsFixfalse, so a non-finite grouping timestamp is replaced with the fallback instant rather than being normalized to a UTC day boundary.
if (typeof value !== 'number' || !Number.isFinite(value)) {
return false;
}
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
dailyEventsPortionfailed on projects with legacy far-future Sentry timestamps (~2056) stored in Mongo: values exceeded GraphQLInt(32-bit) ongroupingTimestamp/ cursor boundaries.Int.Fixes: