Skip to content

Commit 610062b

Browse files
tdonohuetdonohue
authored
Merge pull request #2731 from DSpace/backport-2670-to-main
[Port main] Ensure HALEndpointService doesn't use stale responses
2 parents 92a10ce + 33b7c39 commit 610062b

11 files changed

Lines changed: 667 additions & 145 deletions

‎src/app/core/data/base/base-data.service.spec.ts‎

Lines changed: 54 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -95,6 +95,7 @@ describe('BaseDataService', () => {
9595
remoteDataMocks = {
9696
RequestPending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.RequestPending, undefined, undefined, undefined),
9797
ResponsePending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePending, undefined, undefined, undefined),
98+
ResponsePendingStale: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined),
9899
Success: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Success, undefined, payload, statusCodeSuccess),
99100
SuccessStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.SuccessStale, undefined, payload, statusCodeSuccess),
100101
Error: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError),
@@ -303,19 +304,21 @@ describe('BaseDataService', () => {
303304

304305
it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
305306
testScheduler.run(({ cold, expectObservable }) => {
306-
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
307-
a: remoteDataMocks.SuccessStale,
308-
b: remoteDataMocks.RequestPending,
309-
c: remoteDataMocks.ResponsePending,
310-
d: remoteDataMocks.Success,
311-
e: remoteDataMocks.SuccessStale,
307+
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e-f-g', {
308+
a: remoteDataMocks.ResponsePendingStale,
309+
b: remoteDataMocks.SuccessStale,
310+
c: remoteDataMocks.ErrorStale,
311+
d: remoteDataMocks.RequestPending,
312+
e: remoteDataMocks.ResponsePending,
313+
f: remoteDataMocks.Success,
314+
g: remoteDataMocks.SuccessStale,
312315
}));
313-
const expected = '--b-c-d-e';
316+
const expected = '------d-e-f-g';
314317
const values = {
315-
b: remoteDataMocks.RequestPending,
316-
c: remoteDataMocks.ResponsePending,
317-
d: remoteDataMocks.Success,
318-
e: remoteDataMocks.SuccessStale,
318+
d: remoteDataMocks.RequestPending,
319+
e: remoteDataMocks.ResponsePending,
320+
f: remoteDataMocks.Success,
321+
g: remoteDataMocks.SuccessStale,
319322
};
320323

321324
expectObservable(service.findByHref(selfLink, true, true, ...linksToFollow)).toBe(expected, values);
@@ -354,19 +357,21 @@ describe('BaseDataService', () => {
354357

355358
it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
356359
testScheduler.run(({ cold, expectObservable }) => {
357-
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
358-
a: remoteDataMocks.SuccessStale,
359-
b: remoteDataMocks.RequestPending,
360-
c: remoteDataMocks.ResponsePending,
361-
d: remoteDataMocks.Success,
362-
e: remoteDataMocks.SuccessStale,
360+
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e-f-g', {
361+
a: remoteDataMocks.ResponsePendingStale,
362+
b: remoteDataMocks.SuccessStale,
363+
c: remoteDataMocks.ErrorStale,
364+
d: remoteDataMocks.RequestPending,
365+
e: remoteDataMocks.ResponsePending,
366+
f: remoteDataMocks.Success,
367+
g: remoteDataMocks.SuccessStale,
363368
}));
364-
const expected = '--b-c-d-e';
369+
const expected = '------d-e-f-g';
365370
const values = {
366-
b: remoteDataMocks.RequestPending,
367-
c: remoteDataMocks.ResponsePending,
368-
d: remoteDataMocks.Success,
369-
e: remoteDataMocks.SuccessStale,
371+
d: remoteDataMocks.RequestPending,
372+
e: remoteDataMocks.ResponsePending,
373+
f: remoteDataMocks.Success,
374+
g: remoteDataMocks.SuccessStale,
370375
};
371376

372377
expectObservable(service.findByHref(selfLink, false, true, ...linksToFollow)).toBe(expected, values);
@@ -487,19 +492,21 @@ describe('BaseDataService', () => {
487492

488493
it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
489494
testScheduler.run(({ cold, expectObservable }) => {
490-
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
491-
a: remoteDataMocks.SuccessStale,
492-
b: remoteDataMocks.RequestPending,
493-
c: remoteDataMocks.ResponsePending,
494-
d: remoteDataMocks.Success,
495-
e: remoteDataMocks.SuccessStale,
495+
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e-f-g', {
496+
a: remoteDataMocks.ResponsePendingStale,
497+
b: remoteDataMocks.SuccessStale,
498+
c: remoteDataMocks.ErrorStale,
499+
d: remoteDataMocks.RequestPending,
500+
e: remoteDataMocks.ResponsePending,
501+
f: remoteDataMocks.Success,
502+
g: remoteDataMocks.SuccessStale,
496503
}));
497-
const expected = '--b-c-d-e';
504+
const expected = '------d-e-f-g';
498505
const values = {
499-
b: remoteDataMocks.RequestPending,
500-
c: remoteDataMocks.ResponsePending,
501-
d: remoteDataMocks.Success,
502-
e: remoteDataMocks.SuccessStale,
506+
d: remoteDataMocks.RequestPending,
507+
e: remoteDataMocks.ResponsePending,
508+
f: remoteDataMocks.Success,
509+
g: remoteDataMocks.SuccessStale,
503510
};
504511

505512
expectObservable(service.findListByHref(selfLink, findListOptions, true, true, ...linksToFollow)).toBe(expected, values);
@@ -538,21 +545,24 @@ describe('BaseDataService', () => {
538545

539546
it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
540547
testScheduler.run(({ cold, expectObservable }) => {
541-
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
542-
a: remoteDataMocks.SuccessStale,
543-
b: remoteDataMocks.RequestPending,
544-
c: remoteDataMocks.ResponsePending,
545-
d: remoteDataMocks.Success,
546-
e: remoteDataMocks.SuccessStale,
548+
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e-f-g', {
549+
a: remoteDataMocks.ResponsePendingStale,
550+
b: remoteDataMocks.SuccessStale,
551+
c: remoteDataMocks.ErrorStale,
552+
d: remoteDataMocks.RequestPending,
553+
e: remoteDataMocks.ResponsePending,
554+
f: remoteDataMocks.Success,
555+
g: remoteDataMocks.SuccessStale,
547556
}));
548-
const expected = '--b-c-d-e';
557+
const expected = '------d-e-f-g';
549558
const values = {
550-
b: remoteDataMocks.RequestPending,
551-
c: remoteDataMocks.ResponsePending,
552-
d: remoteDataMocks.Success,
553-
e: remoteDataMocks.SuccessStale,
559+
d: remoteDataMocks.RequestPending,
560+
e: remoteDataMocks.ResponsePending,
561+
f: remoteDataMocks.Success,
562+
g: remoteDataMocks.SuccessStale,
554563
};
555564

565+
556566
expectObservable(service.findListByHref(selfLink, findListOptions, false, true, ...linksToFollow)).toBe(expected, values);
557567
});
558568
});

‎src/app/core/data/base/base-data.service.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,7 +273,7 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
273273
// call it isn't immediately returned, but we wait until the remote data for the new request
274274
// is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
275275
// cached completed object
276-
skipWhile((rd: RemoteData<T>) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted),
276+
skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
277277
this.reRequestStaleRemoteData(reRequestOnStale, () =>
278278
this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
279279
);
@@ -307,7 +307,7 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
307307
// call it isn't immediately returned, but we wait until the remote data for the new request
308308
// is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
309309
// cached completed object
310-
skipWhile((rd: RemoteData<PaginatedList<T>>) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted),
310+
skipWhile((rd: RemoteData<PaginatedList<T>>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
311311
this.reRequestStaleRemoteData(reRequestOnStale, () =>
312312
this.findListByHref(href$, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
313313
);

‎src/app/core/data/request-entry-state.model.spec.ts‎

Lines changed: 186 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,186 @@
1+
import {
2+
isRequestPending,
3+
isError,
4+
isSuccess,
5+
isErrorStale,
6+
isSuccessStale,
7+
isResponsePending,
8+
isResponsePendingStale,
9+
isLoading,
10+
isStale,
11+
hasFailed,
12+
hasSucceeded,
13+
hasCompleted,
14+
RequestEntryState
15+
} from './request-entry-state.model';
16+
17+
describe(`isRequestPending`, () => {
18+
it(`should only return true if the given state is RequestPending`, () => {
19+
expect(isRequestPending(RequestEntryState.RequestPending)).toBeTrue();
20+
21+
expect(isRequestPending(RequestEntryState.ResponsePending)).toBeFalse();
22+
expect(isRequestPending(RequestEntryState.Error)).toBeFalse();
23+
expect(isRequestPending(RequestEntryState.Success)).toBeFalse();
24+
expect(isRequestPending(RequestEntryState.ResponsePendingStale)).toBeFalse();
25+
expect(isRequestPending(RequestEntryState.ErrorStale)).toBeFalse();
26+
expect(isRequestPending(RequestEntryState.SuccessStale)).toBeFalse();
27+
});
28+
});
29+
30+
describe(`isError`, () => {
31+
it(`should only return true if the given state is Error`, () => {
32+
expect(isError(RequestEntryState.Error)).toBeTrue();
33+
34+
expect(isError(RequestEntryState.RequestPending)).toBeFalse();
35+
expect(isError(RequestEntryState.ResponsePending)).toBeFalse();
36+
expect(isError(RequestEntryState.Success)).toBeFalse();
37+
expect(isError(RequestEntryState.ResponsePendingStale)).toBeFalse();
38+
expect(isError(RequestEntryState.ErrorStale)).toBeFalse();
39+
expect(isError(RequestEntryState.SuccessStale)).toBeFalse();
40+
});
41+
});
42+
43+
describe(`isSuccess`, () => {
44+
it(`should only return true if the given state is Success`, () => {
45+
expect(isSuccess(RequestEntryState.Success)).toBeTrue();
46+
47+
expect(isSuccess(RequestEntryState.RequestPending)).toBeFalse();
48+
expect(isSuccess(RequestEntryState.ResponsePending)).toBeFalse();
49+
expect(isSuccess(RequestEntryState.Error)).toBeFalse();
50+
expect(isSuccess(RequestEntryState.ResponsePendingStale)).toBeFalse();
51+
expect(isSuccess(RequestEntryState.ErrorStale)).toBeFalse();
52+
expect(isSuccess(RequestEntryState.SuccessStale)).toBeFalse();
53+
});
54+
});
55+
56+
describe(`isErrorStale`, () => {
57+
it(`should only return true if the given state is ErrorStale`, () => {
58+
expect(isErrorStale(RequestEntryState.ErrorStale)).toBeTrue();
59+
60+
expect(isErrorStale(RequestEntryState.RequestPending)).toBeFalse();
61+
expect(isErrorStale(RequestEntryState.ResponsePending)).toBeFalse();
62+
expect(isErrorStale(RequestEntryState.Error)).toBeFalse();
63+
expect(isErrorStale(RequestEntryState.Success)).toBeFalse();
64+
expect(isErrorStale(RequestEntryState.ResponsePendingStale)).toBeFalse();
65+
expect(isErrorStale(RequestEntryState.SuccessStale)).toBeFalse();
66+
});
67+
});
68+
69+
describe(`isSuccessStale`, () => {
70+
it(`should only return true if the given state is SuccessStale`, () => {
71+
expect(isSuccessStale(RequestEntryState.SuccessStale)).toBeTrue();
72+
73+
expect(isSuccessStale(RequestEntryState.RequestPending)).toBeFalse();
74+
expect(isSuccessStale(RequestEntryState.ResponsePending)).toBeFalse();
75+
expect(isSuccessStale(RequestEntryState.Error)).toBeFalse();
76+
expect(isSuccessStale(RequestEntryState.Success)).toBeFalse();
77+
expect(isSuccessStale(RequestEntryState.ResponsePendingStale)).toBeFalse();
78+
expect(isSuccessStale(RequestEntryState.ErrorStale)).toBeFalse();
79+
});
80+
});
81+
82+
describe(`isResponsePending`, () => {
83+
it(`should only return true if the given state is ResponsePending`, () => {
84+
expect(isResponsePending(RequestEntryState.ResponsePending)).toBeTrue();
85+
86+
expect(isResponsePending(RequestEntryState.RequestPending)).toBeFalse();
87+
expect(isResponsePending(RequestEntryState.Error)).toBeFalse();
88+
expect(isResponsePending(RequestEntryState.Success)).toBeFalse();
89+
expect(isResponsePending(RequestEntryState.ResponsePendingStale)).toBeFalse();
90+
expect(isResponsePending(RequestEntryState.ErrorStale)).toBeFalse();
91+
expect(isResponsePending(RequestEntryState.SuccessStale)).toBeFalse();
92+
});
93+
});
94+
95+
describe(`isResponsePendingStale`, () => {
96+
it(`should only return true if the given state is requestPending`, () => {
97+
expect(isResponsePendingStale(RequestEntryState.ResponsePendingStale)).toBeTrue();
98+
99+
expect(isResponsePendingStale(RequestEntryState.RequestPending)).toBeFalse();
100+
expect(isResponsePendingStale(RequestEntryState.ResponsePending)).toBeFalse();
101+
expect(isResponsePendingStale(RequestEntryState.Error)).toBeFalse();
102+
expect(isResponsePendingStale(RequestEntryState.Success)).toBeFalse();
103+
expect(isResponsePendingStale(RequestEntryState.ErrorStale)).toBeFalse();
104+
expect(isResponsePendingStale(RequestEntryState.SuccessStale)).toBeFalse();
105+
});
106+
});
107+
108+
describe(`isLoading`, () => {
109+
it(`should only return true if the given state is RequestPending, ResponsePending or ResponsePendingStale`, () => {
110+
expect(isLoading(RequestEntryState.RequestPending)).toBeTrue();
111+
expect(isLoading(RequestEntryState.ResponsePending)).toBeTrue();
112+
expect(isLoading(RequestEntryState.ResponsePendingStale)).toBeTrue();
113+
114+
expect(isLoading(RequestEntryState.Error)).toBeFalse();
115+
expect(isLoading(RequestEntryState.Success)).toBeFalse();
116+
expect(isLoading(RequestEntryState.ErrorStale)).toBeFalse();
117+
expect(isLoading(RequestEntryState.SuccessStale)).toBeFalse();
118+
});
119+
});
120+
121+
describe(`hasFailed`, () => {
122+
describe(`when the state is loading`, () => {
123+
it(`should return undefined`, () => {
124+
expect(hasFailed(RequestEntryState.RequestPending)).toBeUndefined();
125+
expect(hasFailed(RequestEntryState.ResponsePending)).toBeUndefined();
126+
expect(hasFailed(RequestEntryState.ResponsePendingStale)).toBeUndefined();
127+
});
128+
});
129+
130+
describe(`when the state has completed`, () => {
131+
it(`should only return true if the given state is Error or ErrorStale`, () => {
132+
expect(hasFailed(RequestEntryState.Error)).toBeTrue();
133+
expect(hasFailed(RequestEntryState.ErrorStale)).toBeTrue();
134+
135+
expect(hasFailed(RequestEntryState.Success)).toBeFalse();
136+
expect(hasFailed(RequestEntryState.SuccessStale)).toBeFalse();
137+
});
138+
});
139+
});
140+
141+
describe(`hasSucceeded`, () => {
142+
describe(`when the state is loading`, () => {
143+
it(`should return undefined`, () => {
144+
expect(hasSucceeded(RequestEntryState.RequestPending)).toBeUndefined();
145+
expect(hasSucceeded(RequestEntryState.ResponsePending)).toBeUndefined();
146+
expect(hasSucceeded(RequestEntryState.ResponsePendingStale)).toBeUndefined();
147+
});
148+
});
149+
150+
describe(`when the state has completed`, () => {
151+
it(`should only return true if the given state is Error or ErrorStale`, () => {
152+
expect(hasSucceeded(RequestEntryState.Success)).toBeTrue();
153+
expect(hasSucceeded(RequestEntryState.SuccessStale)).toBeTrue();
154+
155+
expect(hasSucceeded(RequestEntryState.Error)).toBeFalse();
156+
expect(hasSucceeded(RequestEntryState.ErrorStale)).toBeFalse();
157+
});
158+
});
159+
});
160+
161+
162+
describe(`hasCompleted`, () => {
163+
it(`should only return true if the given state is Error, Success, ErrorStale or SuccessStale`, () => {
164+
expect(hasCompleted(RequestEntryState.Error)).toBeTrue();
165+
expect(hasCompleted(RequestEntryState.Success)).toBeTrue();
166+
expect(hasCompleted(RequestEntryState.ErrorStale)).toBeTrue();
167+
expect(hasCompleted(RequestEntryState.SuccessStale)).toBeTrue();
168+
169+
expect(hasCompleted(RequestEntryState.RequestPending)).toBeFalse();
170+
expect(hasCompleted(RequestEntryState.ResponsePending)).toBeFalse();
171+
expect(hasCompleted(RequestEntryState.ResponsePendingStale)).toBeFalse();
172+
});
173+
});
174+
175+
describe(`isStale`, () => {
176+
it(`should only return true if the given state is ResponsePendingStale, SuccessStale or ErrorStale`, () => {
177+
expect(isStale(RequestEntryState.ResponsePendingStale)).toBeTrue();
178+
expect(isStale(RequestEntryState.SuccessStale)).toBeTrue();
179+
expect(isStale(RequestEntryState.ErrorStale)).toBeTrue();
180+
181+
expect(isStale(RequestEntryState.RequestPending)).toBeFalse();
182+
expect(isStale(RequestEntryState.ResponsePending)).toBeFalse();
183+
expect(isStale(RequestEntryState.Error)).toBeFalse();
184+
expect(isStale(RequestEntryState.Success)).toBeFalse();
185+
});
186+
});

‎src/app/core/data/request-entry-state.model.ts‎

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,8 +3,9 @@ export enum RequestEntryState {
33
ResponsePending = 'ResponsePending',
44
Error = 'Error',
55
Success = 'Success',
6+
ResponsePendingStale = 'ResponsePendingStale',
67
ErrorStale = 'ErrorStale',
7-
SuccessStale = 'SuccessStale'
8+
SuccessStale = 'SuccessStale',
89
}
910

1011
/**
@@ -42,12 +43,21 @@ export const isSuccessStale = (state: RequestEntryState) =>
4243
*/
4344
export const isResponsePending = (state: RequestEntryState) =>
4445
state === RequestEntryState.ResponsePending;
46+
4547
/**
46-
* Returns true if the given state is RequestPending or ResponsePending,
47-
* false otherwise
48+
* Returns true if the given state is ResponsePendingStale, false otherwise
49+
*/
50+
export const isResponsePendingStale = (state: RequestEntryState) =>
51+
state === RequestEntryState.ResponsePendingStale;
52+
53+
/**
54+
* Returns true if the given state is RequestPending, RequestPendingStale, ResponsePending, or
55+
* ResponsePendingStale, false otherwise
4856
*/
4957
export const isLoading = (state: RequestEntryState) =>
50-
isRequestPending(state) || isResponsePending(state);
58+
isRequestPending(state) ||
59+
isResponsePending(state) ||
60+
isResponsePendingStale(state);
5161

5262
/**
5363
* If isLoading is true for the given state, this method returns undefined, we can't know yet.
@@ -82,7 +92,10 @@ export const hasCompleted = (state: RequestEntryState) =>
8292
!isLoading(state);
8393

8494
/**
85-
* Returns true if the given state is SuccessStale or ErrorStale, false otherwise
95+
* Returns true if the given state is isRequestPendingStale, isResponsePendingStale, SuccessStale or
96+
* ErrorStale, false otherwise
8697
*/
8798
export const isStale = (state: RequestEntryState) =>
88-
isSuccessStale(state) || isErrorStale(state);
99+
isResponsePendingStale(state) ||
100+
isSuccessStale(state) ||
101+
isErrorStale(state);

0 commit comments

Comments
 (0)