Skip to content

Commit fe4e936

Browse files
mnoah1mnoah1
committed
fix(stovepipe): distinguish inconsistent history data
1 parent d99a91a commit fe4e936

3 files changed

Lines changed: 62 additions & 34 deletions

File tree

‎stovepipe/controller/read_errors.go‎

Lines changed: 21 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -41,22 +41,34 @@ func validateHistoryIdentifier(name, value string) error {
4141
return nil
4242
}
4343

44-
// RequestHistoryNotFoundError indicates that no retained history exists for a selector.
45-
type RequestHistoryNotFoundError struct {
44+
// RequestHistoryByIDNotFoundError indicates that no retained history exists for a request ID.
45+
type RequestHistoryByIDNotFoundError struct {
46+
// RequestID is the selected request identifier.
4647
RequestID string
47-
URI string
4848
}
4949

5050
// Error implements error.
51-
func (e *RequestHistoryNotFoundError) Error() string {
52-
if e.RequestID == "" {
53-
return fmt.Sprintf("request history not found for URI %q", e.URI)
54-
}
51+
func (e *RequestHistoryByIDNotFoundError) Error() string {
5552
return fmt.Sprintf("request history not found for request ID %q", e.RequestID)
5653
}
5754

55+
// RequestHistoryByURINotFoundError indicates that no retained history exists for a URI.
56+
type RequestHistoryByURINotFoundError struct {
57+
// URI is the selected commit URI.
58+
URI string
59+
}
60+
61+
// Error implements error.
62+
func (e *RequestHistoryByURINotFoundError) Error() string {
63+
return fmt.Sprintf("request history not found for URI %q", e.URI)
64+
}
65+
5866
// IsRequestHistoryNotFound reports whether err contains a retained-history absence.
5967
func IsRequestHistoryNotFound(err error) bool {
60-
var target *RequestHistoryNotFoundError
61-
return errors.As(err, &target)
68+
var byID *RequestHistoryByIDNotFoundError
69+
if errors.As(err, &byID) {
70+
return true
71+
}
72+
var byURI *RequestHistoryByURINotFoundError
73+
return errors.As(err, &byURI)
6274
}

‎stovepipe/controller/request_history.go‎

Lines changed: 11 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -79,8 +79,11 @@ func (c *requestHistoryController) readHistoryByID(ctx context.Context, req enti
7979
return nil, fmt.Errorf("GetRequestHistoryByID failed to resolve storage for queue %q: %w", req.Queue, err)
8080
}
8181

82-
logs, err := loadRequestLogs(ctx, stores.GetRequestLogStore(), req.ID, &RequestHistoryNotFoundError{RequestID: req.ID})
82+
logs, err := stores.GetRequestLogStore().List(ctx, req.ID)
8383
if err != nil {
84+
if storage.IsNotFound(err) {
85+
return nil, errs.NewUserError(&RequestHistoryByIDNotFoundError{RequestID: req.ID})
86+
}
8487
return nil, fmt.Errorf("GetRequestHistoryByID failed to list request logs request_id=%s: %w", req.ID, err)
8588
}
8689

@@ -107,10 +110,10 @@ func (c *requestHistoryController) GetRequestHistoryByURI(ctx context.Context, r
107110

108111
func (c *requestHistoryController) readHistoryByURI(ctx context.Context, req entity.GetRequestHistoryByURIRequest) (entity.RequestHistory, error) {
109112
if err := validateHistoryIdentifier("queue", req.Queue); err != nil {
110-
return entity.RequestHistory{}, fmt.Errorf("GetRequestHistoryByURI invalid queue: %w", err)
113+
return entity.RequestHistory{}, fmt.Errorf("GetRequestHistoryByURI invalid queue=%q: %w", req.Queue, err)
111114
}
112115
if err := validateHistoryIdentifier("URI", req.URI); err != nil {
113-
return entity.RequestHistory{}, fmt.Errorf("GetRequestHistoryByURI invalid request: %w", err)
116+
return entity.RequestHistory{}, fmt.Errorf("GetRequestHistoryByURI invalid uri=%q queue=%q: %w", req.URI, req.Queue, err)
114117
}
115118

116119
stores, err := c.stores.For(storage.Config{QueueName: req.Queue})
@@ -121,23 +124,18 @@ func (c *requestHistoryController) readHistoryByURI(ctx context.Context, req ent
121124
requestID, err := stores.GetRequestURIStore().GetIDByURI(ctx, req.URI)
122125
if err != nil {
123126
if storage.IsNotFound(err) {
124-
return entity.RequestHistory{}, errs.NewUserError(&RequestHistoryNotFoundError{URI: req.URI})
127+
return entity.RequestHistory{}, errs.NewUserError(&RequestHistoryByURINotFoundError{URI: req.URI})
125128
}
126129
return entity.RequestHistory{}, fmt.Errorf("GetRequestHistoryByURI failed to resolve request URI %s: %w", req.URI, err)
127130
}
128131

129-
logs, err := loadRequestLogs(ctx, stores.GetRequestLogStore(), requestID, &RequestHistoryNotFoundError{URI: req.URI})
132+
logs, err := stores.GetRequestLogStore().List(ctx, requestID)
130133
if err != nil {
134+
if storage.IsNotFound(err) {
135+
return entity.RequestHistory{}, fmt.Errorf("GetRequestHistoryByURI found URI mapping without retained request logs uri=%q request_id=%q: %w", req.URI, requestID, err)
136+
}
131137
return entity.RequestHistory{}, fmt.Errorf("GetRequestHistoryByURI failed to list request logs uri=%s request_id=%s: %w", req.URI, requestID, err)
132138
}
133139

134140
return entity.RequestHistory{RequestID: requestID, Events: logs}, nil
135141
}
136-
137-
func loadRequestLogs(ctx context.Context, store storage.RequestLogStore, requestID string, notFound *RequestHistoryNotFoundError) ([]entity.RequestLog, error) {
138-
logs, err := store.List(ctx, requestID)
139-
if storage.IsNotFound(err) {
140-
return nil, errs.NewUserError(notFound)
141-
}
142-
return logs, err
143-
}

‎stovepipe/controller/request_history_test.go‎

Lines changed: 30 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,11 @@ func TestGetRequestHistoryByID(t *testing.T) {
102102
}
103103
assert.Equal(t, tt.wantNotFound, IsRequestHistoryNotFound(err))
104104
assert.Equal(t, tt.wantUser, errs.IsUserError(err))
105+
if tt.wantNotFound {
106+
var notFound *RequestHistoryByIDNotFoundError
107+
require.ErrorAs(t, err, &notFound)
108+
assert.Equal(t, requestID, notFound.RequestID)
109+
}
105110
if tt.wantCause != nil {
106111
assert.ErrorIs(t, err, tt.wantCause)
107112
}
@@ -165,7 +170,7 @@ func TestGetRequestHistoryByURI(t *testing.T) {
165170
{name: "storage factory failure", req: entity.GetRequestHistoryByURIRequest{Queue: queue, URI: uri}, factoryErr: backendErr, wantCause: backendErr},
166171
{name: "URI mapping not found", req: entity.GetRequestHistoryByURIRequest{Queue: queue, URI: uri}, mappingErr: fmt.Errorf("lookup: %w", storage.ErrNotFound), wantNotFound: true},
167172
{name: "URI store failure", req: entity.GetRequestHistoryByURIRequest{Queue: queue, URI: uri}, mappingErr: backendErr, wantCause: backendErr},
168-
{name: "mapped history not found", req: entity.GetRequestHistoryByURIRequest{Queue: queue, URI: uri}, mappedID: requestID, listErr: fmt.Errorf("query: %w", storage.ErrNotFound), wantNotFound: true},
173+
{name: "mapped history absence is internal", req: entity.GetRequestHistoryByURIRequest{Queue: queue, URI: uri}, mappedID: requestID, listErr: fmt.Errorf("query: %w", storage.ErrNotFound), wantCause: storage.ErrNotFound},
169174
{name: "log store failure", req: entity.GetRequestHistoryByURIRequest{Queue: queue, URI: uri}, mappedID: requestID, listErr: backendErr, wantCause: backendErr},
170175
}
171176

@@ -210,9 +215,8 @@ func TestGetRequestHistoryByURI(t *testing.T) {
210215
require.Error(t, err)
211216
}
212217
if tt.wantNotFound {
213-
var notFound *RequestHistoryNotFoundError
218+
var notFound *RequestHistoryByURINotFoundError
214219
require.ErrorAs(t, err, &notFound)
215-
assert.Empty(t, notFound.RequestID)
216220
assert.Equal(t, uri, notFound.URI)
217221
}
218222

@@ -236,22 +240,36 @@ func TestGetRequestHistoryByURI(t *testing.T) {
236240
}
237241
}
238242

239-
func TestRequestHistoryNotFoundError(t *testing.T) {
243+
func TestRequestHistoryNotFoundErrors(t *testing.T) {
240244
tests := []struct {
241-
name string
242-
err error
243-
want RequestHistoryNotFoundError
245+
name string
246+
err error
247+
assert func(*testing.T, error)
244248
}{
245-
{name: "request ID", err: fmt.Errorf("lookup failed: %w", &RequestHistoryNotFoundError{RequestID: "request/queue/1"}), want: RequestHistoryNotFoundError{RequestID: "request/queue/1"}},
246-
{name: "URI", err: fmt.Errorf("lookup failed: %w", &RequestHistoryNotFoundError{URI: "git://repo/commit/1"}), want: RequestHistoryNotFoundError{URI: "git://repo/commit/1"}},
249+
{
250+
name: "request ID",
251+
err: fmt.Errorf("lookup failed: %w", &RequestHistoryByIDNotFoundError{RequestID: "request/queue/1"}),
252+
assert: func(t *testing.T, err error) {
253+
var notFound *RequestHistoryByIDNotFoundError
254+
require.ErrorAs(t, err, &notFound)
255+
assert.Equal(t, "request/queue/1", notFound.RequestID)
256+
},
257+
},
258+
{
259+
name: "URI",
260+
err: fmt.Errorf("lookup failed: %w", &RequestHistoryByURINotFoundError{URI: "git://repo/commit/1"}),
261+
assert: func(t *testing.T, err error) {
262+
var notFound *RequestHistoryByURINotFoundError
263+
require.ErrorAs(t, err, &notFound)
264+
assert.Equal(t, "git://repo/commit/1", notFound.URI)
265+
},
266+
},
247267
}
248268

249269
for _, tt := range tests {
250270
t.Run(tt.name, func(t *testing.T) {
251271
assert.True(t, IsRequestHistoryNotFound(tt.err))
252-
var notFound *RequestHistoryNotFoundError
253-
require.ErrorAs(t, tt.err, &notFound)
254-
assert.Equal(t, tt.want, *notFound)
272+
tt.assert(t, tt.err)
255273
})
256274
}
257275
assert.False(t, IsRequestHistoryNotFound(errors.New("other")))

0 commit comments

Comments
 (0)