Skip to content

Commit

Permalink
Merge pull request DSpace#2731 from DSpace/backport-2670-to-main
Browse files Browse the repository at this point in the history
[Port main] Ensure HALEndpointService doesn't use stale responses
  • Loading branch information
tdonohue authored and milanmajchrak committed Feb 8, 2024
1 parent af6b4f5 commit daf4fb8
Show file tree
Hide file tree
Showing 11 changed files with 667 additions and 145 deletions.
98 changes: 54 additions & 44 deletions src/app/core/data/base/base-data.service.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ describe('BaseDataService', () => {
remoteDataMocks = {
RequestPending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.RequestPending, undefined, undefined, undefined),
ResponsePending: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePending, undefined, undefined, undefined),
ResponsePendingStale: new RemoteData(undefined, msToLive, timeStamp, RequestEntryState.ResponsePendingStale, undefined, undefined, undefined),
Success: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Success, undefined, payload, statusCodeSuccess),
SuccessStale: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.SuccessStale, undefined, payload, statusCodeSuccess),
Error: new RemoteData(timeStamp, msToLive, timeStamp, RequestEntryState.Error, errorMessage, undefined, statusCodeError),
Expand Down Expand Up @@ -303,19 +304,21 @@ describe('BaseDataService', () => {

it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
testScheduler.run(({ cold, expectObservable }) => {
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
a: remoteDataMocks.SuccessStale,
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e-f-g', {
a: remoteDataMocks.ResponsePendingStale,
b: remoteDataMocks.SuccessStale,
c: remoteDataMocks.ErrorStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
}));
const expected = '--b-c-d-e';
const expected = '------d-e-f-g';
const values = {
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
};

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

it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
testScheduler.run(({ cold, expectObservable }) => {
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e', {
a: remoteDataMocks.SuccessStale,
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
spyOn(rdbService, 'buildSingle').and.returnValue(cold('a-b-c-d-e-f-g', {
a: remoteDataMocks.ResponsePendingStale,
b: remoteDataMocks.SuccessStale,
c: remoteDataMocks.ErrorStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
}));
const expected = '--b-c-d-e';
const expected = '------d-e-f-g';
const values = {
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
};

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

it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
testScheduler.run(({ cold, expectObservable }) => {
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
a: remoteDataMocks.SuccessStale,
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e-f-g', {
a: remoteDataMocks.ResponsePendingStale,
b: remoteDataMocks.SuccessStale,
c: remoteDataMocks.ErrorStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
}));
const expected = '--b-c-d-e';
const expected = '------d-e-f-g';
const values = {
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
};

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

it(`should not emit a cached stale RemoteData, but only start emitting after the state first changes to RequestPending`, () => {
testScheduler.run(({ cold, expectObservable }) => {
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e', {
a: remoteDataMocks.SuccessStale,
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
spyOn(rdbService, 'buildList').and.returnValue(cold('a-b-c-d-e-f-g', {
a: remoteDataMocks.ResponsePendingStale,
b: remoteDataMocks.SuccessStale,
c: remoteDataMocks.ErrorStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
}));
const expected = '--b-c-d-e';
const expected = '------d-e-f-g';
const values = {
b: remoteDataMocks.RequestPending,
c: remoteDataMocks.ResponsePending,
d: remoteDataMocks.Success,
e: remoteDataMocks.SuccessStale,
d: remoteDataMocks.RequestPending,
e: remoteDataMocks.ResponsePending,
f: remoteDataMocks.Success,
g: remoteDataMocks.SuccessStale,
};


expectObservable(service.findListByHref(selfLink, findListOptions, false, true, ...linksToFollow)).toBe(expected, values);
});
});
Expand Down
4 changes: 2 additions & 2 deletions src/app/core/data/base/base-data.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -273,7 +273,7 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
// call it isn't immediately returned, but we wait until the remote data for the new request
// is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
// cached completed object
skipWhile((rd: RemoteData<T>) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted),
skipWhile((rd: RemoteData<T>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
this.reRequestStaleRemoteData(reRequestOnStale, () =>
this.findByHref(href$, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
);
Expand Down Expand Up @@ -307,7 +307,7 @@ export class BaseDataService<T extends CacheableObject> implements HALDataServic
// call it isn't immediately returned, but we wait until the remote data for the new request
// is created. If useCachedVersionIfAvailable is false it also ensures you don't get a
// cached completed object
skipWhile((rd: RemoteData<PaginatedList<T>>) => useCachedVersionIfAvailable ? rd.isStale : rd.hasCompleted),
skipWhile((rd: RemoteData<PaginatedList<T>>) => rd.isStale || (!useCachedVersionIfAvailable && rd.hasCompleted)),
this.reRequestStaleRemoteData(reRequestOnStale, () =>
this.findListByHref(href$, options, useCachedVersionIfAvailable, reRequestOnStale, ...linksToFollow)),
);
Expand Down
186 changes: 186 additions & 0 deletions src/app/core/data/request-entry-state.model.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,186 @@
import {
isRequestPending,
isError,
isSuccess,
isErrorStale,
isSuccessStale,
isResponsePending,
isResponsePendingStale,
isLoading,
isStale,
hasFailed,
hasSucceeded,
hasCompleted,
RequestEntryState
} from './request-entry-state.model';

describe(`isRequestPending`, () => {
it(`should only return true if the given state is RequestPending`, () => {
expect(isRequestPending(RequestEntryState.RequestPending)).toBeTrue();

expect(isRequestPending(RequestEntryState.ResponsePending)).toBeFalse();
expect(isRequestPending(RequestEntryState.Error)).toBeFalse();
expect(isRequestPending(RequestEntryState.Success)).toBeFalse();
expect(isRequestPending(RequestEntryState.ResponsePendingStale)).toBeFalse();
expect(isRequestPending(RequestEntryState.ErrorStale)).toBeFalse();
expect(isRequestPending(RequestEntryState.SuccessStale)).toBeFalse();
});
});

describe(`isError`, () => {
it(`should only return true if the given state is Error`, () => {
expect(isError(RequestEntryState.Error)).toBeTrue();

expect(isError(RequestEntryState.RequestPending)).toBeFalse();
expect(isError(RequestEntryState.ResponsePending)).toBeFalse();
expect(isError(RequestEntryState.Success)).toBeFalse();
expect(isError(RequestEntryState.ResponsePendingStale)).toBeFalse();
expect(isError(RequestEntryState.ErrorStale)).toBeFalse();
expect(isError(RequestEntryState.SuccessStale)).toBeFalse();
});
});

describe(`isSuccess`, () => {
it(`should only return true if the given state is Success`, () => {
expect(isSuccess(RequestEntryState.Success)).toBeTrue();

expect(isSuccess(RequestEntryState.RequestPending)).toBeFalse();
expect(isSuccess(RequestEntryState.ResponsePending)).toBeFalse();
expect(isSuccess(RequestEntryState.Error)).toBeFalse();
expect(isSuccess(RequestEntryState.ResponsePendingStale)).toBeFalse();
expect(isSuccess(RequestEntryState.ErrorStale)).toBeFalse();
expect(isSuccess(RequestEntryState.SuccessStale)).toBeFalse();
});
});

describe(`isErrorStale`, () => {
it(`should only return true if the given state is ErrorStale`, () => {
expect(isErrorStale(RequestEntryState.ErrorStale)).toBeTrue();

expect(isErrorStale(RequestEntryState.RequestPending)).toBeFalse();
expect(isErrorStale(RequestEntryState.ResponsePending)).toBeFalse();
expect(isErrorStale(RequestEntryState.Error)).toBeFalse();
expect(isErrorStale(RequestEntryState.Success)).toBeFalse();
expect(isErrorStale(RequestEntryState.ResponsePendingStale)).toBeFalse();
expect(isErrorStale(RequestEntryState.SuccessStale)).toBeFalse();
});
});

describe(`isSuccessStale`, () => {
it(`should only return true if the given state is SuccessStale`, () => {
expect(isSuccessStale(RequestEntryState.SuccessStale)).toBeTrue();

expect(isSuccessStale(RequestEntryState.RequestPending)).toBeFalse();
expect(isSuccessStale(RequestEntryState.ResponsePending)).toBeFalse();
expect(isSuccessStale(RequestEntryState.Error)).toBeFalse();
expect(isSuccessStale(RequestEntryState.Success)).toBeFalse();
expect(isSuccessStale(RequestEntryState.ResponsePendingStale)).toBeFalse();
expect(isSuccessStale(RequestEntryState.ErrorStale)).toBeFalse();
});
});

describe(`isResponsePending`, () => {
it(`should only return true if the given state is ResponsePending`, () => {
expect(isResponsePending(RequestEntryState.ResponsePending)).toBeTrue();

expect(isResponsePending(RequestEntryState.RequestPending)).toBeFalse();
expect(isResponsePending(RequestEntryState.Error)).toBeFalse();
expect(isResponsePending(RequestEntryState.Success)).toBeFalse();
expect(isResponsePending(RequestEntryState.ResponsePendingStale)).toBeFalse();
expect(isResponsePending(RequestEntryState.ErrorStale)).toBeFalse();
expect(isResponsePending(RequestEntryState.SuccessStale)).toBeFalse();
});
});

describe(`isResponsePendingStale`, () => {
it(`should only return true if the given state is requestPending`, () => {
expect(isResponsePendingStale(RequestEntryState.ResponsePendingStale)).toBeTrue();

expect(isResponsePendingStale(RequestEntryState.RequestPending)).toBeFalse();
expect(isResponsePendingStale(RequestEntryState.ResponsePending)).toBeFalse();
expect(isResponsePendingStale(RequestEntryState.Error)).toBeFalse();
expect(isResponsePendingStale(RequestEntryState.Success)).toBeFalse();
expect(isResponsePendingStale(RequestEntryState.ErrorStale)).toBeFalse();
expect(isResponsePendingStale(RequestEntryState.SuccessStale)).toBeFalse();
});
});

describe(`isLoading`, () => {
it(`should only return true if the given state is RequestPending, ResponsePending or ResponsePendingStale`, () => {
expect(isLoading(RequestEntryState.RequestPending)).toBeTrue();
expect(isLoading(RequestEntryState.ResponsePending)).toBeTrue();
expect(isLoading(RequestEntryState.ResponsePendingStale)).toBeTrue();

expect(isLoading(RequestEntryState.Error)).toBeFalse();
expect(isLoading(RequestEntryState.Success)).toBeFalse();
expect(isLoading(RequestEntryState.ErrorStale)).toBeFalse();
expect(isLoading(RequestEntryState.SuccessStale)).toBeFalse();
});
});

describe(`hasFailed`, () => {
describe(`when the state is loading`, () => {
it(`should return undefined`, () => {
expect(hasFailed(RequestEntryState.RequestPending)).toBeUndefined();
expect(hasFailed(RequestEntryState.ResponsePending)).toBeUndefined();
expect(hasFailed(RequestEntryState.ResponsePendingStale)).toBeUndefined();
});
});

describe(`when the state has completed`, () => {
it(`should only return true if the given state is Error or ErrorStale`, () => {
expect(hasFailed(RequestEntryState.Error)).toBeTrue();
expect(hasFailed(RequestEntryState.ErrorStale)).toBeTrue();

expect(hasFailed(RequestEntryState.Success)).toBeFalse();
expect(hasFailed(RequestEntryState.SuccessStale)).toBeFalse();
});
});
});

describe(`hasSucceeded`, () => {
describe(`when the state is loading`, () => {
it(`should return undefined`, () => {
expect(hasSucceeded(RequestEntryState.RequestPending)).toBeUndefined();
expect(hasSucceeded(RequestEntryState.ResponsePending)).toBeUndefined();
expect(hasSucceeded(RequestEntryState.ResponsePendingStale)).toBeUndefined();
});
});

describe(`when the state has completed`, () => {
it(`should only return true if the given state is Error or ErrorStale`, () => {
expect(hasSucceeded(RequestEntryState.Success)).toBeTrue();
expect(hasSucceeded(RequestEntryState.SuccessStale)).toBeTrue();

expect(hasSucceeded(RequestEntryState.Error)).toBeFalse();
expect(hasSucceeded(RequestEntryState.ErrorStale)).toBeFalse();
});
});
});


describe(`hasCompleted`, () => {
it(`should only return true if the given state is Error, Success, ErrorStale or SuccessStale`, () => {
expect(hasCompleted(RequestEntryState.Error)).toBeTrue();
expect(hasCompleted(RequestEntryState.Success)).toBeTrue();
expect(hasCompleted(RequestEntryState.ErrorStale)).toBeTrue();
expect(hasCompleted(RequestEntryState.SuccessStale)).toBeTrue();

expect(hasCompleted(RequestEntryState.RequestPending)).toBeFalse();
expect(hasCompleted(RequestEntryState.ResponsePending)).toBeFalse();
expect(hasCompleted(RequestEntryState.ResponsePendingStale)).toBeFalse();
});
});

describe(`isStale`, () => {
it(`should only return true if the given state is ResponsePendingStale, SuccessStale or ErrorStale`, () => {
expect(isStale(RequestEntryState.ResponsePendingStale)).toBeTrue();
expect(isStale(RequestEntryState.SuccessStale)).toBeTrue();
expect(isStale(RequestEntryState.ErrorStale)).toBeTrue();

expect(isStale(RequestEntryState.RequestPending)).toBeFalse();
expect(isStale(RequestEntryState.ResponsePending)).toBeFalse();
expect(isStale(RequestEntryState.Error)).toBeFalse();
expect(isStale(RequestEntryState.Success)).toBeFalse();
});
});
25 changes: 19 additions & 6 deletions src/app/core/data/request-entry-state.model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,9 @@ export enum RequestEntryState {
ResponsePending = 'ResponsePending',
Error = 'Error',
Success = 'Success',
ResponsePendingStale = 'ResponsePendingStale',
ErrorStale = 'ErrorStale',
SuccessStale = 'SuccessStale'
SuccessStale = 'SuccessStale',
}

/**
Expand Down Expand Up @@ -42,12 +43,21 @@ export const isSuccessStale = (state: RequestEntryState) =>
*/
export const isResponsePending = (state: RequestEntryState) =>
state === RequestEntryState.ResponsePending;

/**
* Returns true if the given state is RequestPending or ResponsePending,
* false otherwise
* Returns true if the given state is ResponsePendingStale, false otherwise
*/
export const isResponsePendingStale = (state: RequestEntryState) =>
state === RequestEntryState.ResponsePendingStale;

/**
* Returns true if the given state is RequestPending, RequestPendingStale, ResponsePending, or
* ResponsePendingStale, false otherwise
*/
export const isLoading = (state: RequestEntryState) =>
isRequestPending(state) || isResponsePending(state);
isRequestPending(state) ||
isResponsePending(state) ||
isResponsePendingStale(state);

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

/**
* Returns true if the given state is SuccessStale or ErrorStale, false otherwise
* Returns true if the given state is isRequestPendingStale, isResponsePendingStale, SuccessStale or
* ErrorStale, false otherwise
*/
export const isStale = (state: RequestEntryState) =>
isSuccessStale(state) || isErrorStale(state);
isResponsePendingStale(state) ||
isSuccessStale(state) ||
isErrorStale(state);
Loading

0 comments on commit daf4fb8

Please sign in to comment.