[PM-38743] Honor a partially supplied event date range

[AlexRubik]

🎟️ Tracking

PM-38743

📔 Objective

Send only start or only end to an event log endpoint and both dates get ignored. You silently get the last 30 days instead, with no error.

The check responsible treats "one date given" the same as "no dates given":

if (!end.HasValue || !start.HasValue)

It lives in ApiHelpers.GetDateRange (11 endpoints) and in a copy inside EventFilterRequestModel.ToDateRange (GET /public/events). This fixes the shared one and points the copy at it, so all 12 endpoints are fixed at once.

Each date is now handled on its own:

Request	Range used
neither	last 30 days
start only	start through now
end only	the 30 days before end
both	unchanged

Callers sending one date now get different results. That is intended, but it hits all 12 endpoints, including GET /public/events (external callers) and GET /sm/events/service-accounts/{id} (Secrets Manager owns it, so a reviewer from that team would help).

Three notes for reading the diff:

• ToDateRange still copies the resolved dates back onto the model. Looks redundant, isn't: EventDiagnosticLogger reads them afterwards to log what was filtered. A test covers it.
• start alone more than 367 days back now returns 400 instead of 200. That is the existing range cap, which only applies now the date is used.
• A missing end resolves to now, not the end of the current day (review feedback). That also tightens the no filter default, which has run through end of day for years. Same rows in practice, since event dates are stamped server side.

🤖 Testing

• Full Api.Test suite: 2038 pass, 0 fail, including the 22 existing event tests, unchanged.
• GetDateRange had no tests despite 11 endpoints using it. Added 13, covering every branch and the DateTime min/max edges.
• Confirmed the new tests fail without the fix.
• To see the bug on main: ?start=<2 hours ago> returns the same count as no query string at all. After this change it returns fewer.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Re-reviewed after bfaaa0531, which corrects the Start/End property docs to match the DateTime.UtcNow default introduced in dacecc7ad. The one open finding from the previous round — the End doc still claiming "end of the current day" while the resolved default had moved to UtcNow — is resolved, and no new findings replace it. Re-traced each branch of GetDateRange (per-bound inference, the ThirtyDaysBefore clamp, the swap, the 367-day cap), the write-back in ToDateRange that EventDiagnosticLogger.LogAggregateData reads, and all 12 call sites; the 13 helper tests and 2 model tests match the behavior the code implements, including the MinValue + 30d boundary and the MaxValue swap that resolves to a 400 rather than overflowing.

Code Review Details

No findings.

Notes that did not rise to findings:

• A clamped DateTime.MinValue start is safe in both storage paths. Dapper binds @StartDate explicitly as DbType.DateTime2 with scale 7 (Infrastructure.Dapper/Dirt/Repositories/EventRepository.cs:197) against DATETIME2(7) stored-procedure parameters, so there is no SqlDateTime range problem; the EF path uses a LINQ range comparison. That value is also already reachable on main by supplying both bounds near MinValue, so this PR adds a route to existing behavior rather than a new failure mode.
• The behavior change reaches all 12 entry points, not just the Public API, and a start-only request older than 367 days now returns 400 instead of 200. Both are called out explicitly in the description with a request for a Secrets Manager reviewer, so they are surfaced product decisions rather than defects.
• With only start supplied and dated in the future, the swap makes the supplied value the end bound. ApiHelpers remarks cover this ("An inverted range is swapped") and a test pins it, though the Public API controller remark reads as if start is always the lower bound.
• The ApiHelpers cap message changed from "Range too large." to "Date range must be < 367 days.", unifying the two paths. No remaining references to the old string in this repo; low impact unless an external consumer matches on it.
• No checked-in OpenAPI artifacts exist in the repo, so the XML doc corrections need no regenerated spec committed alongside them.
• Two unresolved human threads remain on ApiHelpers.cs: the future-dated end question, which dacecc7ad answers in code by resolving a missing end to UtcNow, and a preference for rejecting inverted ranges with 400 rather than swapping them. The latter is a design decision on pre-existing behavior and is left to the participants.
• CI was mixed at review time — Lint, Analyze (csharp), Run tests, and Review reported pending, the rest passing — so nothing here is asserted about the build.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.04%. Comparing base (4c664a9) to head (bfaaa05).
⚠️ Report is 7 commits behind head on main.

❗ There is a different number of reports uploaded between BASE (4c664a9) and HEAD (bfaaa05). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (4c664a9)	HEAD (bfaaa05)
	3	1

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8265      +/-   ##
==========================================
- Coverage   69.56%   64.04%   -5.52%     
==========================================
  Files        2471     2473       +2     
  Lines      105932   106005      +73     
  Branches     9601     9613      +12     
==========================================
- Hits        73689    67896    -5793     
- Misses      29779    35750    +5971     
+ Partials     2464     2359     -105     

☔ 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.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[lastbestdev]

This functionality is interesting to me. I know it was existing so perhaps we leave it alone, but I would personally prefer our API to return a 400 Bad Request to the caller when incorrect date parameters are provided.

This makes it clear that the consumer of the API is responsible for providing a sensible date range, and doesn't let them build bad integrations with our API that mix up the start/end params

[AlexRubik]

I agree in principle but I think this might be out of scope/need a separate ticket because main is accounting for this backward date correction and so it could break existing code that is supplying the backwards dates. So everyone's code that is supplying bad dates would then break with no warning. What do you think? Do you think it's enough of an edge case to just implement it in this PR and maybe a couple people learn the hard way? @lastbestdev

[lastbestdev]

I agree this bit is out of scope, and it would be something to discuss with the team if we should remove it. To your point, doing this should include the due diligence of checking our clients references to the endpoints providing date range parameters and ensuring they don't pass the start/end backwards

[lastbestdev]

One last nit, not critical

[lastbestdev]

Nit: you could move the assignment of end to above this check, and then this line could be simplified to:

Suggested change

	start = end.HasValue ? ThirtyDaysBefore(end.Value) : DateTime.UtcNow.Date.AddDays(-30);
	start = ThirtyDaysBefore(end.Value);

[lastbestdev]

This wouldn't keep the same value, since .Date evaluates to the very beginning datetime for the date it is read from, but that should be no issue. It would be a true 30 days clock time back instead.