fix(sentry): honor every offered watcher lookback period (#3828)

Co-authored-by: Carlos Florencio <carlosmflorencio@gmail.com>
This commit is contained in:
Clément Fiere
2026-09-20 01:04:28 +02:00
committed by GitHub
parent 2f44cd7b38
commit 1727435c14
13 changed files with 798 additions and 27 deletions
@@ -268,6 +268,20 @@ func TestHTTP_SearchIssues_RejectsMultipleProjectSlugs(t *testing.T) {
}
}
func TestHTTP_SearchIssues_RejectsInvalidStatsPeriod(t *testing.T) {
ctrl, router, client := newTestController(t)
inst := seedInstance(t, ctrl, "ws-1", "A", "tok")
client.searchIssuesFn = func(_ SearchFilter, _ string) (*SearchResult, error) {
t.Fatal("client must not be called for an invalid lookback")
return nil, nil
}
target := "/api/v1/sentry/issues?workspace_id=ws-1&instanceId=" + inst.ID +
"&orgSlug=acme&projectSlug=frontend&statsPeriod=nonsense"
if w := do(router, http.MethodGet, target, ""); w.Code != http.StatusBadRequest {
t.Fatalf("status = %d, body=%s", w.Code, w.Body.String())
}
}
func TestHTTP_GetIssue_ForwardsID(t *testing.T) {
ctrl, router, client := newTestController(t)
inst := seedInstance(t, ctrl, "ws-1", "A", "tok")
+36 -21
View File
@@ -393,6 +393,7 @@ func (c *RESTClient) searchIssues(
return nil, &APIError{StatusCode: http.StatusBadRequest, Message: "searchIssues: at most one project slug per request"}
}
q := url.Values{}
projectSlug := projectSlugForRequest(filter)
if limit > 0 {
q.Set("per_page", strconv.Itoa(limit))
}
@@ -403,7 +404,13 @@ func (c *RESTClient) searchIssues(
if filter.Environment != "" {
q.Set("environment", filter.Environment)
}
if filter.StatsPeriod != "" {
// statsPeriod is endpoint-specific: the project-scoped endpoint accepts
// only ''/24h/14d there, while the organization-scoped endpoint accepts any
// relative duration and uses it as its request time range. Send it only to
// the latter, and never empty — the org-scoped endpoint rejects an empty
// value. The `age:` term built in buildIssueQueryString is the age
// constraint both endpoints honor.
if filter.StatsPeriod != "" && projectSlug == "" {
q.Set("statsPeriod", filter.StatsPeriod)
}
if cursor != "" {
@@ -413,7 +420,7 @@ func (c *RESTClient) searchIssues(
q.Set("query", built)
}
var nodes []issueNode
resp, err := c.do(ctx, issuesSearchPath(filter.OrgSlug, projectSlugForRequest(filter)), q, &nodes)
resp, err := c.do(ctx, issuesSearchPath(filter.OrgSlug, projectSlug), q, &nodes)
if err != nil {
return nil, err
}
@@ -463,9 +470,9 @@ func projectSlugForRequest(f SearchFilter) string {
// watch filters are limited to a single status (enforced by
// validateFilterStatuses) because two `is:` tokens would AND-combine and match
// nothing. StatsPeriod is translated into an `age:` token — see
// statsPeriodAgeToken's doc comment for why that translation, not the
// `statsPeriod` query param set separately in searchIssues, is what actually
// restricts which issues come back.
// statsPeriodAgeToken's doc comment for why that term, rather than the
// endpoint-specific `statsPeriod` query param set separately in searchIssues,
// is the age constraint both issue endpoints honor.
//
// The free-text Query is parenthesized whenever it is combined with another
// token below. Sentry's search grammar ANDs implicitly-adjacent terms tighter
@@ -515,9 +522,10 @@ func buildIssueQueryString(f SearchFilter) string {
return strings.Join(parts, " ")
}
// statsPeriodPattern matches Sentry's relative-duration syntax: an integer
// followed by h(ours)/d(ays)/w(eeks) — the same units Sentry's `age:` search
// token and this integration's StatsPeriod values (1h, 24h, 7d, 14d, 30d) use.
// statsPeriodPattern matches the relative-duration syntax this integration
// accepts: an integer followed by h(ours)/d(ays)/w(eeks). Deliberately narrower
// than Sentry's own relative-duration grammar, which also accepts seconds and
// minutes — see parseStatsPeriodUnits.
var statsPeriodPattern = regexp.MustCompile(`^[1-9]\d*[hdw]$`)
// maxStatsPeriodUnits bounds the numeric component parseStatsPeriodUnits
@@ -531,12 +539,18 @@ var statsPeriodPattern = regexp.MustCompile(`^[1-9]\d*[hdw]$`)
// nonsensical in one path can't still pass as a valid filter in the other.
const maxStatsPeriodUnits = 3650
// parseStatsPeriodUnits validates period against Sentry's relative-duration
// syntax and the shared accepted range, returning the numeric component and
// unit byte ('h'/'d'/'w') on success. statsPeriodAgeToken below and
// mock_client.go's parseStatsPeriod both build on this so the real REST
// client and the mock that backs E2E tests accept or reject the exact same
// input.
// parseStatsPeriodUnits validates period against the shared accepted syntax
// and range, returning the numeric component and unit byte ('h'/'d'/'w') on
// success. It is the single accepted-value authority: the write sites reject a
// stored lookback it cannot express, statsPeriodAgeToken below derives the
// query term from it, and mock_client.go's parseStatsPeriod builds on it, so
// the real REST client and the mock that backs E2E tests accept or reject the
// exact same input.
//
// The syntax is narrower than Sentry's own relative-duration grammar, which
// also accepts seconds and minutes, so a "30m" lookback is rejected here even
// though an `age:-30m` term would be valid upstream: every Kandev surface
// offers only whole hours, days, or weeks.
func parseStatsPeriodUnits(period string) (n int, unit byte, ok bool) {
period = strings.TrimSpace(period)
if !statsPeriodPattern.MatchString(period) {
@@ -555,13 +569,14 @@ func parseStatsPeriodUnits(period string) (n int, unit byte, ok bool) {
// input, leaving the query unfiltered by age exactly as before this
// translation existed.
//
// This exists because Sentry's `statsPeriod` query param (set separately in
// searchIssues) does NOT filter which issues an issue search returns — it
// only sizes the per-issue event-count stats window returned alongside each
// result (https://github.com/getsentry/sentry/issues/36375). Both the issue
// browser and issue watches expose StatsPeriod as "how far back to look for
// matching issues"; without this translation a watch configured for e.g. the
// last 24h silently matches (and creates tasks for) issues of any age.
// Both the issue browser and issue watches expose StatsPeriod as "how far back
// to look for matching issues", and this token is the constraint that applies
// on both issue endpoints. The `statsPeriod` query param (set separately in
// searchIssues) cannot carry that meaning on its own: the project-scoped
// endpoint accepts only 24h and 14d there and rejects every other offered
// value with a 400, and only the organization-scoped endpoint turns it into a
// request time range. Without this translation a watch configured for the last
// 24h would silently match (and create tasks for) issues of any age.
func statsPeriodAgeToken(period string) string {
period = strings.TrimSpace(period)
if _, _, ok := parseStatsPeriodUnits(period); !ok {
@@ -4,6 +4,7 @@ import (
"context"
"net/http"
"net/http/httptest"
"net/url"
"strings"
"testing"
)
@@ -209,6 +210,86 @@ func TestRESTClient_SearchIssues_AllProjectsUsesOrgEndpoint(t *testing.T) {
}
}
// TestRESTClient_SearchIssues_ForwardsLookbackOnlyWhereAccepted locks in the
// fix for the "only 24h and 14d work" report. The two issue endpoints accept
// different statsPeriod sets: the project-scoped endpoint accepts only the
// empty value, 24h, and 14d there and answers 400 for every other offered
// lookback, while the organization-scoped endpoint takes any relative duration
// and uses it as its request time range. The lookback must therefore reach only
// the org-scoped request, while the `age:` term that actually limits issue
// eligibility is sent on both.
//
// @covers AC-INTEGRATIONS-SENTRY-WATCHER-LOOKBACK-PERIODS-001.1, AC-INTEGRATIONS-SENTRY-WATCHER-LOOKBACK-PERIODS-001.2, AC-INTEGRATIONS-SENTRY-WATCHER-LOOKBACK-PERIODS-001.5
func TestRESTClient_SearchIssues_ForwardsLookbackOnlyWhereAccepted(t *testing.T) {
type capture struct {
path string
query url.Values
}
requests := make([]capture, 0, 4)
ts := newMockServer(t, func(w http.ResponseWriter, r *http.Request) {
requests = append(requests, capture{path: r.URL.Path, query: r.URL.Query()})
_, _ = w.Write([]byte(`[]`))
})
c := pointTo(NewRESTClient(&SentryConfig{}, "tok"), ts.URL)
search := func(filter SearchFilter) capture {
t.Helper()
before := len(requests)
if _, err := c.SearchIssues(context.Background(), filter, ""); err != nil {
t.Fatalf("search %+v: %v", filter, err)
}
if len(requests) != before+1 {
t.Fatalf("expected one request per search, got %d", len(requests)-before)
}
return requests[before]
}
// Every offered lookback token must be expressible on both paths.
for _, token := range []string{"1h", "24h", "7d", "14d", "30d"} {
projectScoped := search(SearchFilter{
OrgSlug: "acme",
ProjectSlugs: []string{"frontend"},
StatsPeriod: token,
})
if projectScoped.path != "/projects/acme/frontend/issues/" {
t.Errorf("%s: project-scoped path = %q", token, projectScoped.path)
}
if projectScoped.query.Has("statsPeriod") {
t.Errorf("%s: project-scoped request must not carry statsPeriod, got %q",
token, projectScoped.query.Get("statsPeriod"))
}
if got := projectScoped.query.Get("query"); !strings.Contains(got, "age:-"+token) {
t.Errorf("%s: project-scoped query %q missing the age:-%s term", token, got, token)
}
orgScoped := search(SearchFilter{OrgSlug: "acme", StatsPeriod: token})
if orgScoped.path != "/organizations/acme/issues/" {
t.Errorf("%s: org-scoped path = %q", token, orgScoped.path)
}
if got := orgScoped.query.Get("statsPeriod"); got != token {
t.Errorf("%s: org-scoped statsPeriod = %q, want %q", token, got, token)
}
if got := orgScoped.query.Get("query"); !strings.Contains(got, "age:-"+token) {
t.Errorf("%s: org-scoped query %q missing the age:-%s term", token, got, token)
}
}
// An empty lookback carries neither the parameter nor an age term, on
// either path. The parameter key must be absent, not merely empty: the
// org-scoped endpoint rejects an empty statsPeriod.
for _, filter := range []SearchFilter{
{OrgSlug: "acme", ProjectSlugs: []string{"frontend"}},
{OrgSlug: "acme"},
} {
got := search(filter)
if got.query.Has("statsPeriod") {
t.Errorf("empty lookback must not send statsPeriod, got %q", got.query.Get("statsPeriod"))
}
if q := got.query.Get("query"); strings.Contains(q, "age:") {
t.Errorf("empty lookback must not send an age term, got %q", q)
}
}
}
// TestRESTClient_GetIssue_NumericID locks in that a numeric internal id hits
// the /issues/{id}/ endpoint directly (no org needed).
func TestRESTClient_GetIssue_NumericID(t *testing.T) {
@@ -311,11 +392,10 @@ func TestBuildIssueQueryString(t *testing.T) {
// TestBuildIssueQueryString_StatsPeriodBecomesAgeFilter locks in the fix for
// the "older issues get processed by a watch configured for the last 24h"
// bug: Sentry's statsPeriod query param does NOT filter which issues a
// search returns (it only sizes the per-issue stats window — see
// https://github.com/getsentry/sentry/issues/36375). The only way to
// actually restrict results to recently-first-seen issues is the `age:`
// search token, so StatsPeriod must be translated into one.
// bug: the `statsPeriod` request param cannot express that intent on the
// project-scoped issue endpoint, which accepts only 24h and 14d there. The
// `age:` search token is the age constraint both endpoints honor, so
// StatsPeriod must be translated into one.
func TestBuildIssueQueryString_StatsPeriodBecomesAgeFilter(t *testing.T) {
if got := buildIssueQueryString(SearchFilter{StatsPeriod: "24h"}); got != "age:-24h" {
t.Errorf("expected age:-24h, got %q", got)
+7 -1
View File
@@ -501,8 +501,14 @@ func (s *Service) ListProjects(ctx context.Context, workspaceID, instanceID stri
return client.ListProjects(ctx)
}
// SearchIssues runs a filtered search against one instance's client.
// SearchIssues runs a filtered search against one instance's client. The
// lookback is validated here because browse requests do not pass through the
// issue-watch write validation path.
func (s *Service) SearchIssues(ctx context.Context, workspaceID, instanceID string, filter SearchFilter, cursor string) (*SearchResult, error) {
filter.StatsPeriod = strings.TrimSpace(filter.StatsPeriod)
if err := validateFilterStatsPeriod(filter); err != nil {
return nil, err
}
client, err := s.browseClient(ctx, workspaceID, instanceID)
if err != nil {
return nil, err
@@ -143,6 +143,9 @@ func (s *Service) UpdateIssueWatch(ctx context.Context, id string, req *UpdateIs
if err := validateFilterStatuses(w.Filter); err != nil {
return nil, err
}
if err := validateFilterStatsPeriod(w.Filter); err != nil {
return nil, err
}
}
if w.WorkflowID == "" || w.WorkflowStepID == "" {
return nil, fmt.Errorf("%w: workflowId and workflowStepId cannot be empty", ErrInvalidConfig)
@@ -238,6 +241,19 @@ func (s *Service) CheckIssueWatch(ctx context.Context, w *IssueWatch) (string, [
s.stampWatchError(w.ID, err.Error())
return "", nil, err
}
if w.Filter.StatsPeriod != "" {
if _, _, ok := parseStatsPeriodUnits(w.Filter.StatsPeriod); !ok {
// A stored lookback outside the supported syntax has no `age:` term
// to constrain the search, so fail the poll closed instead of
// matching (and creating tasks for) issues of any age. Watch rows
// created before the write guard reject such a value, and they are
// never rewritten here.
err := fmt.Errorf("%w: watch lookback period %q is not a whole number of hours, days, or weeks",
ErrInvalidConfig, w.Filter.StatsPeriod)
s.stampWatchError(w.ID, err.Error())
return "", nil, err
}
}
instanceID, err := s.resolveWatchInstanceID(ctx, w)
if err != nil {
s.stampWatchError(w.ID, err.Error())
@@ -464,6 +480,9 @@ func validateIssueWatchCreate(req *CreateIssueWatchRequest) error {
if err := validateFilterStatuses(nf); err != nil {
return err
}
if err := validateFilterStatsPeriod(nf); err != nil {
return err
}
if err := validateMaxInflightTasks(req.MaxInflightTasks); err != nil {
return err
}
@@ -513,6 +532,21 @@ func validateFilterStatuses(f SearchFilter) error {
return nil
}
// validateFilterStatsPeriod rejects a non-empty lookback value that the shared
// accepted syntax (see parseStatsPeriodUnits) cannot express. The field is
// optional, so an empty value stays valid; like validateFilterStatuses this is
// applied only when a filter is created or changed, never against an unchanged
// stored one, so a legacy row cannot block an unrelated update.
func validateFilterStatsPeriod(f SearchFilter) error {
if f.StatsPeriod == "" {
return nil
}
if _, _, ok := parseStatsPeriodUnits(f.StatsPeriod); !ok {
return fmt.Errorf("%w: filter.statsPeriod must be a whole number of hours, days, or weeks such as 24h or 7d", ErrInvalidConfig)
}
return nil
}
func validatePollInterval(seconds int) error {
if seconds < MinIssueWatchPollInterval || seconds > MaxIssueWatchPollInterval {
return fmt.Errorf("%w: pollIntervalSeconds must be between %d and %d",
@@ -102,6 +102,121 @@ func TestService_UpdateIssueWatch_LegacyMultiStatusToggle(t *testing.T) {
}
}
// TestService_UpdateIssueWatch_PersistsLookbackPeriod covers the update half of
// the offered-token contract: a filter-carrying update stores the selected
// lookback unchanged, so a non-default choice survives the write path.
//
// @covers AC-INTEGRATIONS-SENTRY-WATCHER-LOOKBACK-PERIODS-001.1
func TestService_UpdateIssueWatch_PersistsLookbackPeriod(t *testing.T) {
f := newSvcFixture(t)
ctx := context.Background()
created, err := f.svc.CreateIssueWatch(ctx, &CreateIssueWatchRequest{
WorkspaceID: "ws-1", SentryInstanceID: f.ensureInstance(t, "ws-1"),
WorkflowID: "wf", WorkflowStepID: "step", Filter: validFilter(),
})
if err != nil {
t.Fatalf("create: %v", err)
}
filter := validFilter()
filter.StatsPeriod = "30d"
if _, err := f.svc.UpdateIssueWatch(ctx, created.ID, &UpdateIssueWatchRequest{Filter: &filter}); err != nil {
t.Fatalf("update: %v", err)
}
reloaded, err := f.store.GetIssueWatch(ctx, created.ID)
if err != nil {
t.Fatalf("reload: %v", err)
}
if reloaded.Filter.StatsPeriod != "30d" {
t.Errorf("stored lookback = %q, want %q", reloaded.Filter.StatsPeriod, "30d")
}
}
// TestService_IssueWatch_LookbackPeriodValidation covers the two guards that
// replace the project-scoped request parameter as the rejection point for a
// lookback outside Kandev's supported syntax (a whole number of hours, days,
// or weeks): the write sites reject it, an empty value stays acceptable, an
// unrelated partial update never re-validates a stored value, and a row that
// already stores one fails its poll closed without searching.
//
// @covers AC-INTEGRATIONS-SENTRY-WATCHER-LOOKBACK-PERIODS-001.4, AC-INTEGRATIONS-SENTRY-WATCHER-LOOKBACK-PERIODS-001.6
func TestService_IssueWatch_LookbackPeriodValidation(t *testing.T) {
f := newSvcFixture(t)
ctx := context.Background()
instID := f.ensureInstance(t, "ws-1")
createReq := func(filter SearchFilter) *CreateIssueWatchRequest {
return &CreateIssueWatchRequest{
WorkspaceID: "ws-1", SentryInstanceID: instID,
WorkflowID: "wf", WorkflowStepID: "step", Filter: filter,
}
}
// 30m is a valid Sentry relative duration but outside the accepted set. Both
// write sites must reject it: the shared parser derives nothing from it, so
// the stored search would carry no age term at all and the watch would match
// issues of any age.
unsupported := validFilter()
unsupported.StatsPeriod = "30m"
if _, err := f.svc.CreateIssueWatch(ctx, createReq(unsupported)); !errors.Is(err, ErrInvalidConfig) {
t.Errorf("create with a 30m lookback must be rejected, got %v", err)
}
created, err := f.svc.CreateIssueWatch(ctx, createReq(validFilter()))
if err != nil {
t.Fatalf("create: %v", err)
}
if _, err := f.svc.UpdateIssueWatch(ctx, created.ID, &UpdateIssueWatchRequest{
Filter: &unsupported,
}); !errors.Is(err, ErrInvalidConfig) {
t.Errorf("filter-carrying update with a 30m lookback must be rejected, got %v", err)
}
// An empty lookback stays accepted: the field is optional and a legacy row
// may store it.
for _, empty := range []string{"", " "} {
filter := validFilter()
filter.StatsPeriod = empty
if _, err := f.svc.CreateIssueWatch(ctx, createReq(filter)); err != nil {
t.Errorf("empty lookback %q must be accepted, got %v", empty, err)
}
}
// A row that already stores an unsupported value is neither rewritten nor
// re-validated by an update that omits the filter.
stored := newTestIssueWatch("ws-1")
stored.Filter.StatsPeriod = "30m"
if err := f.store.CreateIssueWatch(ctx, stored); err != nil {
t.Fatalf("seed watch: %v", err)
}
disabled := false
if _, err := f.svc.UpdateIssueWatch(ctx, stored.ID, &UpdateIssueWatchRequest{
Enabled: &disabled,
}); err != nil {
t.Errorf("unrelated partial update must not re-validate the stored lookback, got %v", err)
}
// That row's poll fails closed: the error is recorded and no search is
// issued, so an old issue cannot become a task through an age-less query.
searched := false
f.client.searchIssuesFn = func(_ SearchFilter, _ string) (*SearchResult, error) {
searched = true
return &SearchResult{IsLast: true}, nil
}
if _, _, err := f.svc.CheckIssueWatch(ctx, stored); !errors.Is(err, ErrInvalidConfig) {
t.Errorf("poll with a stored 30m lookback must fail closed, got %v", err)
}
if searched {
t.Error("poll must not search when the stored lookback is outside the supported syntax")
}
refreshed, err := f.store.GetIssueWatch(ctx, stored.ID)
if err != nil {
t.Fatalf("reload watch: %v", err)
}
if refreshed.LastError == "" {
t.Error("expected the poll failure recorded on the watch")
}
}
func TestService_UpdateIssueWatch_PartialPatch(t *testing.T) {
f := newSvcFixture(t)
ctx := context.Background()
@@ -339,6 +454,42 @@ func TestService_CheckIssueWatch_ClearsLastErrorAfterSuccess(t *testing.T) {
}
}
// TestService_CheckIssueWatch_PollsStoredLookbackPeriod proves an existing
// watch keeps polling with the lookback token it stored: the row round-trips
// through the store unaltered and the search client receives that token.
//
// @covers AC-INTEGRATIONS-SENTRY-WATCHER-LOOKBACK-PERIODS-001.3
func TestService_CheckIssueWatch_PollsStoredLookbackPeriod(t *testing.T) {
f := newSvcFixture(t)
ctx := context.Background()
f.seedInstance(t, "ws-1", "Primary", "tok")
w := newTestIssueWatch("ws-1")
w.Filter.StatsPeriod = "30d"
if err := f.store.CreateIssueWatch(ctx, w); err != nil {
t.Fatalf("seed watch: %v", err)
}
var polled []string
f.client.searchIssuesFn = func(filter SearchFilter, _ string) (*SearchResult, error) {
polled = append(polled, filter.StatsPeriod)
return &SearchResult{IsLast: true}, nil
}
reloaded, err := f.store.GetIssueWatch(ctx, w.ID)
if err != nil {
t.Fatalf("reload watch: %v", err)
}
if reloaded.Filter.StatsPeriod != "30d" {
t.Fatalf("stored lookback = %q, want %q", reloaded.Filter.StatsPeriod, "30d")
}
if _, _, err := f.svc.CheckIssueWatch(ctx, reloaded); err != nil {
t.Fatalf("check: %v", err)
}
if len(polled) != 1 || polled[0] != "30d" {
t.Errorf("search client received lookbacks %v, want [30d]", polled)
}
}
// TestService_CheckIssueWatch_ResolvesSoleInstance pins acceptance (b): an
// unbound (NULL-instance) watch resolves to its workspace's sole instance at
// poll time.
@@ -650,6 +650,32 @@ func TestService_Browse_RequireInstanceAndForwardsFilter(t *testing.T) {
}
}
func TestService_Browse_RejectsInvalidStatsPeriod(t *testing.T) {
f := newSvcFixture(t)
ctx := context.Background()
inst := f.seedInstance(t, "ws-1", "A", "t")
called := false
f.client.searchIssuesFn = func(_ SearchFilter, _ string) (*SearchResult, error) {
called = true
return &SearchResult{IsLast: true}, nil
}
for name, filter := range map[string]SearchFilter{
"project scoped": {OrgSlug: "acme", ProjectSlugs: []string{"frontend"}, StatsPeriod: "nonsense"},
"organization scoped": {OrgSlug: "acme", StatsPeriod: "nonsense"},
} {
t.Run(name, func(t *testing.T) {
called = false
if _, err := f.svc.SearchIssues(ctx, "ws-1", inst.ID, filter, ""); !errors.Is(err, ErrInvalidConfig) {
t.Fatalf("expected ErrInvalidConfig, got %v", err)
}
if called {
t.Fatal("invalid lookback must be rejected before the client search")
}
})
}
}
func TestService_GetIssue_NotConfiguredWhenNoSecret(t *testing.T) {
f := newSvcFixture(t)
ctx := context.Background()