Fix issue #893: Implement migration of the Event GraphQL layer to review-database rev 1285590 and ji by octoaide[bot] · Pull Request #917 · aicers/review-web

octoaide · 2026-08-28T16:17:03Z

Summary

Implement migration of the Event GraphQL layer to review-database rev 1285590 and jiff::Timestamp while preserving RFC3339 I/O and leaving non-event chrono usage unchanged.

Changed files

Cargo.toml
src/graphql/event.rs
src/graphql/event/bootp.rs
src/graphql/event/conn.rs
src/graphql/event/dcerpc.rs
src/graphql/event/dhcp.rs
src/graphql/event/dns.rs
src/graphql/event/ftp.rs
src/graphql/event/group.rs
src/graphql/event/http.rs
src/graphql/event/kerberos.rs
src/graphql/event/ldap.rs
src/graphql/event/log.rs
src/graphql/event/malformed_dns.rs
src/graphql/event/mqtt.rs
src/graphql/event/network.rs
src/graphql/event/nfs.rs
src/graphql/event/ntlm.rs
src/graphql/event/radius.rs
src/graphql/event/rdp.rs
src/graphql/event/smb.rs
src/graphql/event/smtp.rs
src/graphql/event/ssh.rs
src/graphql/event/sysmon.rs
src/graphql/event/tls.rs
src/graphql/event/unusual_destination_pattern.rs

Closes #893

This pull request was automatically created by octoaide.

codecov · 2026-08-28T16:28:08Z

Codecov Report

❌ Patch coverage is 99.34924% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 86.18%. Comparing base (5a46d42) to head (1668218).

Files with missing lines	Patch %	Lines
src/graphql/event.rs	99.53%	2 Missing ⚠️
src/graphql/event/group.rs	96.87%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #917      +/-   ##
==========================================
+ Coverage   85.96%   86.18%   +0.21%     
==========================================
  Files          77       77              
  Lines       31562    31899     +337     
==========================================
+ Hits        27133    27491     +358     
+ Misses       4429     4408      -21

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

sophie-cluml · 2026-08-31T10:27:29Z

-        let end = end - 1;
+        let end = end
+            .checked_sub(1)
+            .ok_or("invalid time `end`: no earlier event key exists")?;


end is an exclusive GraphQL time boundary, so both the Unix epoch and the lower i64 nanosecond bound are valid inputs rather than malformed timestamps. The current latest implementation rejects the epoch explicitly and rejects the lower bound through checked_sub(1). Please handle these as normal range boundaries: an epoch end should return matching pre-epoch events or an empty result, while end == i64::MIN should return an empty result because no representable event can precede it. Please apply this behavior consistently to event lists, totalCount, triage lists, aggregation queries, and frequency-series queries. Since this corrects observable GraphQL behavior for existing clients, please also document it under Fixed in CHANGELOG.md.

sophie-cluml · 2026-08-31T10:27:29Z

+                    "{{ eventList(filter: {{ start: \"{expected}\" }}, first: 1) {{ totalCount }} }}"
+                ))
+                .await;
+            assert!(input.errors.is_empty(), "{:?}", input.errors);


The new test covers successful parsing and serialization only for Jiff's canonical Z representation within the i64 nanosecond range. Please also cover the following boundary and compatibility cases:

A timestamp that is valid for Jiff but outside the event storage's i64 nanosecond range is rejected with a clear GraphQL error.

end == i64::MIN is accepted as a valid exclusive boundary and produces an empty result.

An RFC3339 input using the previous Chrono-style +00:00 offset remains accepted.

These cases exercise both the new failure boundary and compatibility with existing client input.

kimhanbeom

@octoaide

After reviewing the PR, I believe the following additional changes are needed. Please review and address each item.

The previous end key calculation in group.rs and load_triage_list() used 0 when end <= 0. Replacing it with latest() changes this behavior: end == 0 now returns an error, while negative values produce a different key. Since this issue is intended to migrate to jiff while preserving existing behavior, the end key semantics of these two paths should not change. Please leave latest(), which is used by the existing eventList and pagination paths, unchanged, and preserve the previous behavior specifically for group.rs and load_triage_list(). If appropriate, a dedicated helper shared by these two paths may be introduced. Please also add regression tests covering an omitted end, the Unix epoch, a negative end, and a positive end to prevent unintended behavior changes.
Although this change preserves external API compatibility, migrating the core time type in the Event GraphQL layer to jiff::Timestamp is a significant change and should be documented. Please add an appropriate entry to CHANGELOG.md.

Handle epoch and minimum nanosecond bounds as valid exclusive ranges, retain legacy group and triage key behavior, and document the GraphQL timestamp compatibility fixes.

octoaide · 2026-08-31T19:58:13Z

Pushed commit ae32878 with the verified changes requested in this pull request.

sophie-cluml · 2026-09-01T01:47:38Z

-        });
+        let start = earliest(filter.start, None)?;
+        let end_timestamp = filter.end;
+        let scan_end = legacy_latest(end_timestamp)?;


The current implementation mixes the legacy end <= 0 scan bound with the newly chosen [start, end) semantics. Since this PR now treats the Unix epoch and the lower i64 nanosecond bound as valid exclusive boundaries, please apply that decision consistently instead of retaining legacy scan limits. Remove legacy_latest and is_before_end, and use latest(end, None)? as the upper scan key in load_triage_list, event_frequency_series, count_events, and count_events_by_network after the existing empty_time_range short-circuit. This also avoids scanning and decoding events beyond a negative end and removes the per-event timestamp conversion performed by is_before_end.

Please add a GraphQL regression test using start = -2ns and end = -1ns, with events at both -2ns and exactly -1ns, and verify that the event at end is excluded consistently from the triage list, count aggregations, and frequency series.

sophie-cluml · 2026-09-01T01:47:38Z


 ### Changed

+- Migrated Event GraphQL timestamps internally from `chrono::DateTime` to


The changelog requested in #917 (comment) was for the user-visible correction to event time-boundary behavior, not for the internal Rust timestamp type. Please remove the Changed entry describing the internal chrono::DateTime to jiff::Timestamp migration and retain the boundary correction under Fixed. If the +00:00 to Z canonicalization is intentionally considered user-visible, document that observable output change explicitly instead of the internal implementation.

Apply `latest(end, None)` across triage, aggregation, and frequency queries after empty-range checks, removing legacy scan limits and per-event timestamp filtering. Document the corrected boundary behavior and add regression coverage for negative nanosecond ranges.

octoaide · 2026-09-01T17:24:11Z

Pushed follow-up changes in 5efb8b4.

Updated event boundary handling to use the corrected exclusive-range semantics consistently.
Added regression coverage for negative timestamp boundaries and end-time exclusion.
Revised CHANGELOG.md to document the user-visible boundary behavior rather than the internal timestamp migration.

Make the polling interval and seconds unit explicit. Part of #893

Distinguish the timestamp prefix from a complete event key. Part of #893

Keep malformed-filter errors consistent across event queries. Valid empty ranges still avoid database scans. Part of #893

Use empty event ranges when exercising GraphQL DateTime inputs. This keeps parsing compatibility coverage independent of event-key iteration order and makes the expected empty result explicit. Part of #893

sophie-cluml · 2026-09-03T08:49:44Z

I pushed four focused follow-up commits identified during review, and CI is green. The rationale for each change is recorded in its commit message.

While validating the timestamp boundaries, I found a pre-existing correctness issue in review-database: the bytewise ordering of signed event keys does not match chronological order across the Unix epoch. The underlying ordering bug predates review-database commit 1285590, the revision adopted by this PR, so it is not caused by this dependency update and is intentionally out of scope here. The database correction is tracked in aicers/review-database#867. Updating review-web to consume that correction and aligning cross-epoch behavior across the affected Event GraphQL queries are tracked separately in #919.

sophie-cluml · 2026-09-03T08:51:31Z

@kimhanbeom Could you review the newly added commits please?

Implement migration of the Event GraphQL layer to

3d5a7de

sophie-cluml requested changes Aug 31, 2026

View reviewed changes

kimhanbeom requested changes Aug 31, 2026

View reviewed changes

Preserve event end boundary semantics

ae32878

Handle epoch and minimum nanosecond bounds as valid exclusive ranges, retain legacy group and triage key behavior, and document the GraphQL timestamp compatibility fixes.

sophie-cluml requested changes Sep 1, 2026

View reviewed changes

kimhanbeom previously approved these changes Sep 2, 2026

View reviewed changes

sophie-cluml added 4 commits September 3, 2026 08:09

Clarify event fetch interval names

0b96d75

Make the polling interval and seconds unit explicit. Part of #893

Rename event key prefix helper

4482ee5

Distinguish the timestamp prefix from a complete event key. Part of #893

Validate filters before empty ranges

fbfa3fc

Keep malformed-filter errors consistent across event queries. Valid empty ranges still avoid database scans. Part of #893

Isolate timestamp input parsing test

1668218

Use empty event ranges when exercising GraphQL DateTime inputs. This keeps parsing compatibility coverage independent of event-key iteration order and makes the expected empty result explicit. Part of #893

sophie-cluml dismissed kimhanbeom’s stale review via 1668218 September 3, 2026 05:32

sophie-cluml requested a review from kimhanbeom September 3, 2026 08:51

kimhanbeom approved these changes Sep 4, 2026

View reviewed changes

sophie-cluml approved these changes Sep 4, 2026

View reviewed changes

sophie-cluml merged commit ec7d498 into main Sep 4, 2026
14 checks passed

sophie-cluml deleted the octoaide/issue-893-1787932460191 branch September 4, 2026 02:27

Navigation Menu

Uh oh!

Fix issue #893: Implement migration of the Event GraphQL layer to review-database rev 1285590 and ji - #917

Fix issue #893: Implement migration of the Event GraphQL layer to review-database rev 1285590 and ji#917
sophie-cluml merged 7 commits into
mainfrom
octoaide/issue-893-1787932460191

octoaide Bot commented Aug 28, 2026

Uh oh!

codecov Bot commented Aug 28, 2026 •

edited

Loading

Uh oh!

sophie-cluml Aug 31, 2026

Uh oh!

sophie-cluml Aug 31, 2026

Uh oh!

kimhanbeom left a comment •

edited

Loading

Uh oh!

octoaide Bot commented Aug 31, 2026

Uh oh!

sophie-cluml Sep 1, 2026

Uh oh!

sophie-cluml Sep 1, 2026

Uh oh!

octoaide Bot commented Sep 1, 2026

Uh oh!

sophie-cluml commented Sep 3, 2026

Uh oh!

sophie-cluml commented Sep 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		### Changed

		- Migrated Event GraphQL timestamps internally from `chrono::DateTime` to

Uh oh!

Conversation

octoaide Bot commented Aug 28, 2026

Summary

Changed files

Uh oh!

codecov Bot commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sophie-cluml Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

sophie-cluml Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

kimhanbeom left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octoaide Bot commented Aug 31, 2026

Uh oh!

sophie-cluml Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

sophie-cluml Sep 1, 2026

Choose a reason for hiding this comment

Uh oh!

octoaide Bot commented Sep 1, 2026

Uh oh!

sophie-cluml commented Sep 3, 2026

Uh oh!

sophie-cluml commented Sep 3, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Aug 28, 2026 •

edited

Loading

kimhanbeom left a comment •

edited

Loading